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

Wed Mar 24 03:26:13 PDT 2010

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

--- Comment #7 from Jeremy Orlow <jorlow at chromium.org>  2010-03-24 03:26:13 PST ---
(In reply to comment #6)
> > 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.

Is it possible for the normalization scale to ever ben 0?  If so, is there some
better default action we can take here?

> > 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.

Hm...that's fine...as long as you handle the cases that slip between the
cracks.

> > 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 :).

I think that would help.  And I'd encourage you to check as many assumptions as
are possible with some sort of fatal assert.

Speaking of which, I've asked for Jeremy to make one here:
https://bugs.webkit.org/show_bug.cgi?id=36426

> > 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...

I think an assert while debugging and silence during normal operation is a good
balance.

> > 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"??

If you ommit the destructor or put |~ClassName() { }| in the . h file, then
anything that might destroy ClassName needs to know how to destroy all the
Own/RefPtr's the class owns.  But if you do |~ClassName();| and put the
definition in the .cpp file, then only that file needs to know how to destroy
the class (and thus destroy anything you have Own/RefPtrs of).

I say this because having a RefPtr/OwnPtr<ClassName>, ClassName*, or ClassName&
in the header does not require you to include the .h, but
RefPtr/OwnPtr<ClassName>'s and a destructor that's in the .h file (whether
implicit or explicit) does require you to include the .h file rather than just
forward declaring.

> > Not sure size_t is the right way to express number of channels.
> 
> Should I use "unsigned"?

Yeah.  size_t is fine for the length of an array and such, but unsigned is
probably better for things like number of channels.

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