[Webkit-unassigned] [Bug 36466] audio engine: add Reverb class

Tue Mar 23 16:22:36 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=36466

--- Comment #6 from Chris Rogers <crogers at google.com>  2010-03-23 16:22:36 PST ---
Addressed most of your comments.

> Is this a normal code path?  If a developer hit this, it seems like it might be
> worth asserting, though we should definitely still handle the case gracefully.

It's not really an ASSERT case (which I usually consider to be sanity checking
of the correctness of the code itself).  It *is* possible for a user to load up
an impulse response which is badly formed, like a completely silent audio file.
 In this case, I want to make sure that we don't have divide by zero errors
which can disrupt the audio output stream.  In other cases, it's trying to
protect from generating an extremely loud output.

> Constants like this should probably be put at the header of the file.

I put them at the top of the file.  Since the constants are really related to
internal implementation details I didn't want to expose them in the interface
(header) file.  I hope that's OK.

> Why if (scale)?  If anything shouldn't this check to see if it's 1.0?

It's basically to avoid a divide-by-zero, since this check works in tandem with
the similar check at the end of the method.  If you like, I could add the
optimization for the 1.0 case, but I think it will be extremely rare that the
scale value will be *exactly* 1.0 so maybe not worth it - it's up to you
though.

> I almost wonder if you should handle the various cases with just an enum... 
> I.e. have various modes.  I'm not sure how general purpose this code really is.

I wanted to make the processing code dynamically adaptable at runtime without
any required re-configuration when channel valences change which is now
possible with the new modular dynamic routing.  Also, I think an enum would
require somewhat more logic to configure the enum value (and reconfigure if it
changes) so I prefer this more direct approach.  Additional, if clauses can
easily be added to handle more cases such as 5.1 and 7.2 matrixing in the
future.

> All of this stuff scares me...does WebKit have an equivilent of a CHECK (a
> DCHECK that fires even in release)?  If not, can we add one?  And then whenever
> you do anything like this, can you check that what's happening is safe.  Also,
> ideally I'd like to pass around real objects (that can check their bounds and
> such) as much as possible and avoid this raw pointer stuff.

As far as CHECK, I'm not sure, but if there is one, I could change all my
ASSERTs.  If there's a precedent for that in WebKit I'm happy to do it.  I
added a comprehensive safety check at the top of the method which validates the
source and destination AudioBus objects.  This validation, along with the
checks for numberOfChannels should be sufficient to guarantee the pointer
values are valid and that the pointers point to buffers which are large enough.
 In general, I agree about passing real objects with bound-checking and that's
why I use the AudioBus objects so much (you'll see later in other parts of the
code).  But at a low enough level of the code, it becomes extremely difficult
to avoid the direct pointers, because there can be multiple sources for the
data: AudioFloatArray, AudioChannel, a pointer value provided from an OS-level
callback, a pointer to somewhere other than the beginning of an
AudioFloatArray, etc.  In this particular case, I could change
ReverbConvolver::process() to take AudioChannel arguments, which I think is
what you're looking for here (and I'm happy to do it :).

> Maybe have an else with an ASSERT(0) or something? (IIRC, there's a macro for
> this, but I can't remember what it is...maybe take a look for it?)

For now, I'm handling the un-handled case by just gracefully outputting
silence.  I'm a bit torn between this and just using ASSERT - so if you feel
strongly...

> These two could be just forward declared if you declared the destructor and
> implemented it in a .cpp file (that does include them).

I was able to do this for AudioBus, but ReverbConvolver (presumably since I'm
using a Vector of them) seems to require the header file.  I'm not sure what
you mean by "declaring the destructor"??

> How are file paths expressed elsewhere?  I doubt via const char* strings.

This was an old code-path used for testing/bring-up (I've removed this
constructor)

> Not sure size_t is the right way to express number of channels.

Should I use "unsigned"?

> Kinda weird that your sample rate is not an integer...but I don't really care.

This is how we designed the CoreAudio APIs and it has worked out well.  It
avoids a conversion from int -> float which is almost always required when
doing the math using it.  It also supports sample-rates which are non-integer,
which although rare can exist.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.