Bug 36466 – audio engine: add Reverb class

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

audio engine: add Reverb class

[Chris Rogers]

Created attachment 51369 [details]
Patch

[Chris Rogers]

Created attachment 51370 [details]
image describing operation of Reverb class

Adding diagram describing operation of Reverb class

[Chris Rogers]

This is the top-level DSP object for multi-channel convolution reverb.  It uses several ReverbConvolver objects along with channel matrixing.  Please see attached diagram.

[Jeremy Orlow]

Comment on attachment 51369 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 1f2d874..5761d36 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,23 @@
> [phone]  Chris Rogers  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        audio engine: add Reverb class
> +        https://bugs.webkit.org/show_bug.cgi?id=36466
> +
> +        No tests since not yet connected to javascript API
> +
> +        * platform/audio/Reverb.cpp: Added.
> +        (WebCore::calculateNormalizationScale):
> +        (WebCore::normalizeResponse):
> +        (WebCore::Reverb::Reverb):
> +        (WebCore::Reverb::initialize):
> +        (WebCore::Reverb::process):
> +        (WebCore::Reverb::reset):
> +        (WebCore::Reverb::impulseResponseLength):
> +        * platform/audio/Reverb.h: Added.
> +        (WebCore::Reverb::):
> +
>  2010-03-05  Csaba Osztrogonác  <[e-mail]>
>  
>          Unreviewed buildfix after r55593. (To fix Qt --minimal build.)
> diff --git a/WebCore/platform/audio/Reverb.cpp b/WebCore/platform/audio/Reverb.cpp
> new file mode 100644
> index 0000000..2d163ba
> --- /dev/null
> +++ b/WebCore/platform/audio/Reverb.cpp
> @@ -0,0 +1,201 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include "config.h"
> +#include "Reverb.h"
> +
> +#include "AudioBus.h"
> +#include "AudioFileReader.h"
> +#include "ReverbConvolver.h"
> +#include <math.h>
> +#include <wtf/OwnPtr.h>
> +#include <wtf/PassOwnPtr.h>
> +
> +namespace WebCore {
> +
> +static double calculateNormalizationScale(AudioBus* response)
> +{
> +    // Normalize by RMS power
> +    size_t numberOfChannels = response->numberOfChannels();
> +    size_t frameSize = response->frameSize();
> +
> +    double power = 0.0;
> +
> +    for (size_t i = 0; i < numberOfChannels; ++i) {
> +        int n = frameSize;
> +        float* p = response->channel(i)->data();
> +
> +        while (n--) {
> +            float sample = *p++;
> +            power += sample * sample;
> +        }
> +    }
> +
> +    power = sqrt(power / (numberOfChannels * frameSize));
> +
> +    // Protect against accidental overload
> +    const double MinPower = 0.000125;
> +    if (std::isinf(power) || std::isnan(power) || power < MinPower)

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.

> +        power = MinPower;
> +
> +    double scale = 1.0 / power;
> +
> +    // Empirical gain calibration tested across many impulse responses to ensure perceived volume is same as dry (unprocessed) signal
> +    scale *= pow(10.0, -58.0 * 0.05); // -58dB gain reduction

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

> +
> +    // True-stereo compensation
> +    if (response->numberOfChannels() == 4)
> +        scale *= 0.5;
> +
> +    return scale;
> +}
> +
> +static void normalizeResponse(AudioBus* response)
> +{
> +    double scale = calculateNormalizationScale(response);
> +    response->scale(scale);
> +}
> +
> +Reverb::Reverb(const char* filePath, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels,  double sampleRate, bool useBackgroundThreads)
> +{
> +    OwnPtr<AudioBus> impulseResponse = createBusFromAudioFile(filePath, false, sampleRate);
> +    normalizeResponse(impulseResponse.get());
> +
> +    initialize(impulseResponse.get(), renderSliceSize, maxFFTSize, numberOfChannels, useBackgroundThreads);
> +}
> +
> +Reverb::Reverb(AudioBus* impulseResponse, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, bool useBackgroundThreads)
> +{
> +    double scale = calculateNormalizationScale(impulseResponse);
> +    if (scale)

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

> +        impulseResponse->scale(scale);
> +
> +    initialize(impulseResponse, renderSliceSize, maxFFTSize, numberOfChannels, useBackgroundThreads);
> +
> +    // Undo scaling since this shouldn't be a destructive operation on impulseResponse
> +    if (scale)
> +        impulseResponse->scale(1.0 / scale);
> +}
> +
> +void Reverb::initialize(AudioBus* impulseResponseBuffer, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, bool useBackgroundThreads)
> +{
> +    m_impulseResponseLength = impulseResponseBuffer->frameSize();
> +
> +    // The reverb can handle a mono impulse response and still do stereo processing
> +    size_t numResponseChannels = impulseResponseBuffer->numberOfChannels();
> +    m_convolvers.reserveCapacity(numberOfChannels);
> +
> +    int convolverRenderPhase = 0;
> +    for (size_t i = 0; i < numResponseChannels; ++i) {
> +        AudioChannel* channel = impulseResponseBuffer->channel(i);
> +
> +        ReverbConvolver* convolver = new ReverbConvolver(channel, renderSliceSize, maxFFTSize, convolverRenderPhase, useBackgroundThreads);
> +        m_convolvers.append(convolver);
> +
> +        convolverRenderPhase += renderSliceSize;
> +    }
> +
> +    // For "True" stereo processing

Can you add a bit more of a comment about why this is needed?  Is it just an optimization so you don't keep allocating and deallocating memory?

> +    if (numResponseChannels == 4)
> +        m_tempBuffer = new AudioBus(2, MaxFrameSize);
> +}
> +
> +void Reverb::process(AudioBus* sourceBus, AudioBus* destinationBus, size_t framesToProcess)
> +{
> +    bool isSafeToProcess = sourceBus && destinationBus && framesToProcess <= MaxFrameSize && framesToProcess <= sourceBus->frameSize() && framesToProcess <= destinationBus->frameSize(); 
> +    
> +    ASSERT(isSafeToProcess);
> +    if (!isSafeToProcess)
> +        return;
> +
> +    // For now only handle mono or stereo output
> +    if (destinationBus->numberOfChannels() > 2)
> +        return;
> +
> +    float* destinationL = destinationBus->channel(0)->data();
> +    float* sourceL = sourceBus->channel(0)->data();
> +
> +    // Handle input -> output matrixing...
> +    size_t numInputChannels = sourceBus->numberOfChannels();
> +    size_t numOutputChannels = destinationBus->numberOfChannels();
> +    size_t numReverbChannels = m_convolvers.size();
> +
> +    // 2 -> 2 -> 2
> +    if (numInputChannels == 2 && numReverbChannels == 2 && numOutputChannels == 2) {

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.

> +        float* sourceR = sourceBus->channel(1)->data();
> +        float* destinationR = destinationBus->channel(1)->data();
> +        m_convolvers[0]->process(sourceL, destinationL, framesToProcess);
> +        m_convolvers[1]->process(sourceR, destinationR, framesToProcess);
> +    } else  if (numInputChannels == 1 && numOutputChannels == 2 && numReverbChannels == 2) {
> +        // 1 -> 2 -> 2
> +        for (int i = 0; i < 2; ++i) {
> +            float* destinationP = destinationBus->channel(i)->data();
> +            m_convolvers[i]->process(sourceL, destinationP, framesToProcess);
> +        }
> +    } else if (numInputChannels == 1 && numReverbChannels == 1 && numOutputChannels == 2) {
> +        // 1 -> 1 -> 2
> +        m_convolvers[0]->process(sourceL, destinationL, framesToProcess);
> +
> +        // simply copy L -> R
> +        float* destinationR = destinationBus->channel(1)->data();
> +        memcpy(destinationR, destinationL, sizeof(float) * framesToProcess);
> +    } else if (numInputChannels == 1 && numReverbChannels == 1 && numOutputChannels == 1)

Since this is multiple lines (with the comment) I think it's proper to put {}'s around it.

> +        // 1 -> 1 -> 1
> +        m_convolvers[0]->process(sourceL, destinationL, framesToProcess);
> +    else if (numInputChannels == 2 && numReverbChannels == 4 && numOutputChannels == 2) {
> +        // 2 -> 4 -> 2 ("True" stereo)
> +        float* sourceR = sourceBus->channel(1)->data();
> +        float* destinationR = destinationBus->channel(1)->data();

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.

> +
> +        float* tempL = m_tempBuffer->channel(0)->data();
> +        float* tempR = m_tempBuffer->channel(1)->data();
> +
> +        // Process left virtual source
> +        m_convolvers[0]->process(sourceL, destinationL, framesToProcess);
> +        m_convolvers[1]->process(sourceL, destinationR, framesToProcess);
> +
> +        // Process right virtual source
> +        m_convolvers[2]->process(sourceR, tempL, framesToProcess);
> +        m_convolvers[3]->process(sourceR, tempR, framesToProcess);
> +
> +        destinationBus->sumFrom(*m_tempBuffer);
> +    }

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

> +}
> +
> +void Reverb::reset()
> +{
> +    for (size_t i = 0; i < m_convolvers.size(); ++i)
> +        m_convolvers[i]->reset();
> +}
> +
> +size_t Reverb::impulseResponseLength() const
> +{
> +    return m_impulseResponseLength;

Could have been done inline if you wanted.

> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/Reverb.h b/WebCore/platform/audio/Reverb.h
> new file mode 100644
> index 0000000..4cfa659
> --- /dev/null
> +++ b/WebCore/platform/audio/Reverb.h
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef Reverb_h
> +#define Reverb_h
> +
> +#include "AudioBus.h"
> +#include "ReverbConvolver.h"

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

> +#include <wtf/Vector.h>
> +
> +namespace WebCore {
> +
> +// Multi-channel convolution reverb with channel matrixing - one or more ReverbConvolver objects are used internally.
> +
> +class Reverb {
> +public:
> +    enum {MaxFrameSize = 256};

I believe a space after and before the { and } is the norm.  Actually, it seems like usually these are static const variables, but the enum method seems like a good one.

> +
> +    // renderSliceSize is a rendering hint, so the FFTs can be optimized to not all occur at the same time (very bad when rendering on a real-time thread).
> +    Reverb(AudioBus* impulseResponseBuffer, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, bool useBackgroundThreads);
> +    Reverb(const char* filePath, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, double sampleRate, bool useBackgroundThreads);

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

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

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

> +
> +    virtual void process(AudioBus* sourceBus, AudioBus* destinationBus, size_t framesToProcess);
> +    virtual void reset();

Why virtual?

> +
> +    size_t impulseResponseLength() const;
> +
> +private:
> +    void initialize(AudioBus* impulseResponseBuffer, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, bool useBackgroundThreads);
> +
> +    size_t m_impulseResponseLength;
> +
> +    Vector<OwnPtr<ReverbConvolver> > m_convolvers;
> +
> +    // For "True" stereo processing
> +    OwnPtr<AudioBus> m_tempBuffer;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // Reverb_h

[Chris Rogers]

Created attachment 51463 [details]
Patch

[Chris Rogers]

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.

[Jeremy Orlow]

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

[Jeremy Orlow]

Comment on attachment 51463 [details]
Patch

Please address my previous comments, but this looks really good.

[Chris Rogers]

Created attachment 51976 [details]
Patch

[Chris Rogers]

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

It is possible if the user un-intentionally (or intentionally in some cases) loads up an impulse response which represents "dead silence".  In this case, the code will work correctly (do the mathematically correct thing)  and generate silent output.  That said, there is the opportunity for an optimization in this case (avoid doing the convolution altogether and simply note this edge case and zero out the destination buffers directly).  But I think the case is rare enough that an optimization isn't really necessary and would actually make the code a little harder to read.

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

I did change ReverbConvolver::process() to take AudioChannels (instead of raw pointers).

[Jeremy Orlow]

(In reply to comment #10)
> > Is it possible for the normalization scale to ever ben 0?  If so, is there some
> > better default action we can take here?
> 
> It is possible if the user un-intentionally (or intentionally in some cases)
> loads up an impulse response which represents "dead silence".  In this case,
> the code will work correctly (do the mathematically correct thing)  and
> generate silent output.  That said, there is the opportunity for an
> optimization in this case (avoid doing the convolution altogether and simply
> note this edge case and zero out the destination buffers directly).  But I
> think the case is rare enough that an optimization isn't really necessary and
> would actually make the code a little harder to read.

Please put a concise comment about the divide by zero case and the fact that it needn't be normalized because the algorithm will generate silence anyway.

[Jeremy Orlow]

Comment on attachment 51976 [details]
Patch

r=me, but as with the other engine patches, this should go into the branch.


> +void Reverb::process(AudioBus* sourceBus, AudioBus* destinationBus, size_t framesToProcess)
> +{
> +    // Do a fairly comprehensive sanity check.
> +    // If these conditions are satisfied, all of the source and destination pointers will be valid for the various matrixing cases.
> +    bool isSafeToProcess = sourceBus && destinationBus && sourceBus->numberOfChannels() > 0 && destinationBus->numberOfChannels() > 0
> +        && framesToProcess <= MaxFrameSize && framesToProcess <= sourceBus->frameSize() && framesToProcess <= destinationBus->frameSize(); 

nit: probably better to line up on |sourceBus|

i.e.

    bool isSafeToProcess = sourceBus && destinationBus && sourceBus->numberOfChannels() > 0 && destinationBus->numberOfChannels() > 0
                           && framesToProcess <= MaxFrameSize && framesToProcess <= sourceBus->frameSize() && framesToProcess <= destinationBus->frameSize();

[Eric Seidel (no email)]

Was this landed in the big audio branch landing?  If so, please close so it's not in webkit.org/pending-commit anymore.

[Chris Rogers]

(In reply to comment #13)
> Was this landed in the big audio branch landing?  If so, please close so it's
> not in webkit.org/pending-commit anymore.

Hi Eric, I still need to land these bugs in trunk.  There may be some slight reorganization of directory layout as soon as other reviewers and I hash that out by looking at my branch...

[Adam Barth]

Comment on attachment 51976 [details]
Patch

Remove review while you sort out whether this is landing in trunk or branch.

[Chris Rogers]

Created attachment 65618 [details]
Patch

[Chris Rogers]

This was previously R+ by Jeremy Orlow.  This adds a WEB_AUDIO feature enable.  It also introduces a small amount of new code to handle the 1 -> 4 -> 2 case.

[Chris Marrin]

Comment on attachment 65618 [details]
Patch

This patch is ready to land

[WebKit Commit Bot]

Comment on attachment 65618 [details]
Patch

Rejecting patch 65618 from review queue.

[e-mail] does not have reviewer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have reviewer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have reviewer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your reviewer rights.

[Chris Rogers]

Thanks Chris, I can have somebody here do the formalities...

[Chris Marrin]

Comment on attachment 65618 [details]
Patch

Try r+ again after fixing committers.py

[Eric Seidel (no email)]

The cq probably hasn't done it's restart yet. :( (restarts about every 2 hours)  I need to add a manual restart button to queues.webkit.org.

[WebKit Commit Bot]

Comment on attachment 65618 [details]
Patch

Rejecting patch 65618 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=queues.webkit.org', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 65618, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 65618 from bug 36466.
Warning, attachment 65618 [details] on bug 36466 has invalid reviewer ([e-mail])
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Eric Seidel (no email)]

I restarted the cq by hand it should respect cmarrin now.

[Chris Marrin]

(In reply to comment #24)
> I restarted the cq by hand it should respect cmarrin now.

Is this where I start my Rodney Dangerfield routine?

[Chris Rogers]

Committed r66412: <http://trac.webkit.org/changeset/66412>

[Chris Rogers]

re-opening because patch did not actually land

[Chris Rogers]

Comment on attachment 65618 [details]
Patch

Clearing flags on attachment: 65618

Committed r66413: <http://trac.webkit.org/changeset/66413>

[Chris Rogers]

All reviewed patches have been landed.  Closing bug.