Bug 34912 – audio engine: add ReverbConvolver class

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

audio engine: add ReverbConvolver class

[Chris Rogers]

Created attachment 48675 [details]
diagram describing operation of ReverbConvolver

The ReverbConvolver is able to perform extremely long real-time convolutions on a single audio channel. It uses multiple FFTConvolver objects as well as an input buffer and an accumulation buffer. Note that it's possible to get a multi-threaded implementation by exploiting the parallelism. Also note that the leading sections of the long impulse response are processed in the real-time thread for minimum latency. In theory it's possible to get zero latency if the very first FFTConvolver is replaced with a DirectConvolver (not using a FFT).

[Chris Rogers]

Created attachment 48676 [details]
Patch

[Chris Rogers]

One thing I know I'll have to change here is how I deal with threading.
Right now, I'm explicitly using pthreads, but this won't be cross-platform.
Do we have a cross-platform threading abstraction I can use?

[Jeremy Orlow]

Comment on attachment 48676 [details]
Patch

I'll take a closer look once these comments are addressed.


> diff --git a/WebCore/platform/audio/ReverbConvolver.cpp b/WebCore/platform/audio/ReverbConvolver.cpp
> new file mode 100644
> index 0000000..0594c8c
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolver.cpp
> @@ -0,0 +1,476 @@
> +/*
> + * 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 "ReverbConvolver.h"
> +
> +#include "Accelerate.h"
> +#include "AudioBus.h"
> +
> +namespace WebCore {
> +
> +const int kInputBufferSize = 8 * 16384;
> +
> +// We only process the leading portion of the impulse response in the real-time thread.  We don't exceed this length.
> +// It turns out then, that the background thread has about 278msec of scheduling slop
> +const size_t kRealtimeFrameLimit = 8192  + 4096; // ~278msec @ 44.1KHz

How was this measured?  On what platforms?  How will we make sure that this stays close to reality?  (I.e. can we make a layout test that verifies this or something?)

> +
> +const size_t kMinFFTSize = 256;
> +const size_t kMaxRealtimeFFTSize = 2048;

I believe the preferred style is LikeThis.  I.e. no k prefix.  It's unfortunate that there are so many examples of the k prefix (and other styles, IIRC).

> +
> +static void* BackgroundThreadDispatch(void* p)

p is not a very good variable name.

> +{
> +    ReverbConvolver* This = static_cast<ReverbConvolver*>(p);
> +    This->backgroundThreadEntry();
> +    return 0;
> +}
> +
> +ReverbConvolver::ReverbConvolver(AudioChannel* impulseResponse,
> +                                 size_t renderSliceSize,
> +                                 size_t maxFFTSize,
> +                                 size_t convolverRenderPhase,
> +                                 bool useBackgroundThreads)
> +    : m_impulseResponseLength(impulseResponse->frameSize())
> +    , m_accumulationBuffer(impulseResponse->frameSize() + renderSliceSize)
> +    , m_inputBuffer(kInputBufferSize)
> +    , m_renderSliceSize(renderSliceSize)
> +    , m_useBackgroundThreads(useBackgroundThreads)
> +    , m_wantsToExit(false)
> +{
> +    m_minFFTSize = kMinFFTSize; // First stage will have this size
> +                                // successive stages will double in size each time

This comment could have all just gone on the same line.  WebKit does not share Chromium's 80 col limit.

> +
> +    m_maxFFTSize = maxFFTSize; // until we hit |m_maxFFTSize|
> +
> +    // If we are using background threads then don't exceed this FFT size for the
> +    // stages which run in the real-time thread.  This avoids having only one or two
> +    // large stages (size 16384 or so) at the end which take a lot of time every several
> +    // processing slices.  This way we amortize the cost over more processing slices.
> +    m_maxRealtimeFFTSize = kMaxRealtimeFFTSize;
> +
> +    // For the moment, a good way to know if we have real-time constraint
> +    // is to check if we're using background threads.  Otherwise, assume we're
> +    // being run from a command-line tool.
> +    bool hasRealtimeConstraint = useBackgroundThreads;

Is any such "command line tool" intended to be upstreamed?  If not, this support should be removed.

> +    float* response = impulseResponse->data();
> +    size_t totalResponseLength = impulseResponse->frameSize();
> +
> +    ReverbAccumulationBuffer* accumBufferP = 0;
> +

This is an example of where you bleed vertical space when you probably don't need to.

> +    accumBufferP = &m_accumulationBuffer;
> +
> +    // Because we're not using direct-convolution first the leading portion, the reverb
> +    // has an overall latency of half the first-stage FFT size
> +    size_t reverbTotalLatency = m_minFFTSize / 2;
> +
> +    size_t stageOffset = 0;
> +    int i = 0;
> +    size_t fftSize = m_minFFTSize;
> +

Ditto.

> +    while (stageOffset < totalResponseLength) {
> +        size_t stageSize = fftSize / 2;
> +
> +        // For the last stage, it's possible that |stageOffset| is such that we're straddling the end
> +        // of the impulse response buffer (if we use |stageSize|), so reduce the last stage's
> +        // length...
> +
> +        // May have to reduce length of last stage (clip to very end of response)
> +        if (stageSize + stageOffset > totalResponseLength)
> +            stageSize = totalResponseLength - stageOffset;
> +
> +        // This "staggers" the time when each FFT happens so they don't all happen at the same time
> +        int renderPhase = convolverRenderPhase + i * renderSliceSize;
> +
> +        ReverbConvolverStage* stage = new ReverbConvolverStage(response,
> +                                                               totalResponseLength,
> +                                                               reverbTotalLatency,
> +                                                               stageOffset,
> +                                                               stageSize,
> +                                                               fftSize,
> +                                                               renderPhase,
> +                                                               renderSliceSize,
> +                                                               accumBufferP);

This should all be on one line.  It should use a ::create function.  The ::create function should return a PassOwnPtr<>.  Avoid manual memory management at all costs.

> +
> +        bool isBackgroundStage = false;
> +
> +        if (stageOffset <= kRealtimeFrameLimit)
> +            m_stages.append(stage);
> +        else {
> +            if (this->useBackgroundThreads()) {
> +                m_backgroundStages.append(stage);
> +                isBackgroundStage = true;
> +            } else
> +                m_stages.append(stage);
> +        }
> +
> +        stageOffset += stageSize;
> +        i++;
> +
> +        // Figure out next FFT size
> +        fftSize *= 2;
> +        if (hasRealtimeConstraint && !isBackgroundStage && fftSize > m_maxRealtimeFFTSize)
> +            fftSize = m_maxRealtimeFFTSize;
> +        if (fftSize > m_maxFFTSize)
> +            fftSize = m_maxFFTSize;
> +    }
> +
> +    // Start up background thread
> +    // FIXME: would be better to up the thread priority here.  It doesn't need to be real-time, but higher than the default...
> +    if (this->useBackgroundThreads() && m_backgroundStages.size() > 0)
> +        pthread_create(&m_backgroundThread, 0, BackgroundThreadDispatch, this);
> +    else
> +        m_backgroundThread = 0;
> +}
> +
> +ReverbConvolver::~ReverbConvolver()
> +{
> +    // Wait for background thread to stop
> +    if (useBackgroundThreads() && m_backgroundThread) {
> +        m_wantsToExit = true;
> +        pthread_join(m_backgroundThread, 0);
> +    }
> +
> +    for (size_t i = 0; i < m_stages.size(); ++i)
> +        delete m_stages[i];
> +
> +    for (size_t i = 0; i < m_backgroundStages.size(); ++i)
> +        delete m_backgroundStages[i];
> +}
> +
> +void ReverbConvolver::backgroundThreadEntry()
> +{
> +    while (!m_wantsToExit) {
> +        // Check to see if there's any more input to consume
> +        int writeIndex = m_inputBuffer.writeIndex();
> +
> +        // Even though it doesn't seem like every stage needs to maintain its own version of |readIndex| 
> +        // we do this in case we want to run in more than one background thread

Add a period to the end of this.

> +        int readIndex;
> +
> +        while ((readIndex = m_backgroundStages[0]->inputReadIndex()) != writeIndex) { // FIXME : do better to detect buffer overrun...
> +            // FIXME : remove hard-coded value
> +            const int kSliceSize = 128;
> +
> +            // Accumulate contributions from each stage
> +            for (size_t i = 0; i < m_backgroundStages.size(); ++i)
> +                m_backgroundStages[i]->processInBackground(this, kSliceSize);
> +        }
> +
> +        // Sleep 10ms
> +        usleep(10000); // FIXME : this really isn't ideal - could use wait/signal
> +    }
> +}
> +
> +size_t ReverbConvolver::impulseResponseLength()
> +{
> +    return m_impulseResponseLength;
> +}
> +
> +void ReverbConvolver::process(float* sourceP,
> +                              float* destP,
> +                              size_t framesToProcess)

All on the same line.

> +{
> +    // Feed input buffer (read by all threads)
> +    m_inputBuffer.write(sourceP, framesToProcess);
> +
> +    // Accumulate contributions from each stage
> +    for (size_t i = 0; i < m_stages.size(); ++i)
> +        m_stages[i]->process(sourceP, framesToProcess);
> +
> +    // Finally read from accumulation buffer
> +    m_accumulationBuffer.readAndClear(destP, framesToProcess);
> +}
> +
> +void ReverbConvolver::reset()
> +{
> +    for (size_t i = 0; i < m_stages.size(); ++i)
> +        m_stages[i]->reset();
> +
> +    for (size_t i = 0; i < m_backgroundStages.size(); ++i)
> +        m_backgroundStages[i]->reset();
> +
> +    m_accumulationBuffer.reset();
> +    m_inputBuffer.reset();
> +}
> +
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +ReverbConvolverStage::ReverbConvolverStage(float* impulseResponse,
> +                                           size_t responseLength,
> +                                           size_t reverbTotalLatency,
> +                                           size_t stageOffset,
> +                                           size_t stageLength,
> +                                           size_t fftSize,
> +                                           size_t renderPhase,
> +                                           size_t renderSliceSize,
> +                                           ReverbAccumulationBuffer* accumulationBuffer)

I'm not sure, but I believe this should be on the same line...even though the initializing list below is supposed to be split.

> +    : m_fftKernel(fftSize)
> +    , m_accumulationBuffer(accumulationBuffer)
> +    , m_accumulationReadIndex(0)
> +    , m_inputReadIndex(0)
> +    , m_accumulationReadTimeFrame(0)
> +    , m_impulseResponseLength(responseLength)
> +{
> +    m_fftKernel.doPaddedFFT(impulseResponse + stageOffset, stageLength);
> +
> +    m_convolver = new FFTConvolver(fftSize);
> +
> +    m_tempBuffer.allocate(renderSliceSize);
> +
> +    // The convolution stage at offset |stageOffset| needs to have a corresponding delay to cancel out the offset
> +    size_t totalDelay = stageOffset + reverbTotalLatency;
> +
> +    // But, the FFT convolution itself incurs |fftSize| / 2 latency, so subtract this out...
> +    size_t halfSize = fftSize / 2;
> +    if (totalDelay >= halfSize)
> +        totalDelay -= halfSize;
> +
> +    // FIXME : DEAL with case when total delay is less than fftSize/2....
> +
> +    // We divide up the total delay, into pre and post delay sections so that we can
> +    // schedule at exactly the moment when the FFT will happen.  This is coordinated
> +    // with the other stages, so they don't all do their FFTs at the same time...
> +
> +    int m = (halfSize <= totalDelay) ? halfSize : totalDelay;
> +    m_preDelayLength = (totalDelay > 0) ? (renderPhase % m) : 0;
> +
> +    if (m_preDelayLength > totalDelay)
> +        m_preDelayLength = 0;
> +
> +    m_postDelayLength = totalDelay - m_preDelayLength;
> +    m_preReadWriteIndex = 0;
> +    m_framesProcessed = 0; // total frames processed so far
> +
> +    m_preDelayBuffer.allocate(m_preDelayLength < fftSize ? fftSize : m_preDelayLength);
> +}
> +
> +ReverbConvolverStage::~ReverbConvolverStage()
> +{
> +    delete m_convolver;

Avoid manual memory management.  Use stuff like OwnPtrs.

> +}
> +
> +void ReverbConvolverStage::processInBackground(ReverbConvolver* convolver,
> +                                               size_t framesToProcess)
> +{
> +    ReverbInputBuffer& inputBuffer = convolver->inputBuffer();
> +
> +    float* sourceP = inputBuffer.directReadFrom(&m_inputReadIndex, framesToProcess);
> +
> +    process(sourceP, framesToProcess);
> +}
> +
> +void  ReverbConvolverStage::process(float* sourceP,
> +                                    size_t framesToProcess)
> +{
> +    //
> +    // FIXME : do sanity check on framesToProcess versus delay buffer size
> +    //
> +
> +    // Get pointer to pre-delay stream : note special handling of zero delay
> +    float* preDelayedSourceP = sourceP;
> +    float* preDelayBufferP = m_preDelayBuffer;
> +    float* tempP = preDelayBufferP;
> +    if (m_preDelayLength > 0) {
> +        preDelayedSourceP = preDelayBufferP + m_preReadWriteIndex;
> +        tempP = m_tempBuffer;
> +    }
> +
> +    int writeIndex = 0;
> +
> +    if (m_framesProcessed < m_preDelayLength) {
> +        // For the first |m_preDelayLength| frames don't process the convolver, instead simply buffer in the pre-delay
> +
> +        // While buffering the pre-delay, we still need to update our index
> +        m_accumulationBuffer->updateReadIndex(&m_accumulationReadIndex, framesToProcess);
> +    } else {
> +        // Now, run the convolution (into the delay buffer)
> +        // An expensive FFT will happen every (fftSize/2) frames
> +        // We process in-place here...
> +        m_convolver->process(&m_fftKernel,
> +                             preDelayedSourceP,
> +                             tempP,
> +                             framesToProcess);
> +
> +        // Now accumulate into reverb's accumulation buffer
> +        // FIXME : really need to have locking mechanism here!!
> +        writeIndex = m_accumulationBuffer->accumulate(tempP,
> +                                                      framesToProcess,
> +                                                      &m_accumulationReadIndex,
> +                                                      m_postDelayLength);
> +    }
> +
> +    // Finally copy input to pre-delay
> +    if (m_preDelayLength > 0) {
> +        memcpy(preDelayedSourceP, sourceP, sizeof(float) * framesToProcess);
> +        m_preReadWriteIndex += framesToProcess;
> +
> +        if (m_preReadWriteIndex >= m_preDelayLength)
> +            m_preReadWriteIndex = 0; // should only be <=
> +    }
> +
> +#if 0

Typically we like to avoid dead code in the tree like this.  Has this perhaps served its purpose, so we can get rid of it?

> +    // TESTING - sanity check
> +    int timelineReadFrame = m_accumulationBuffer->readTimeFrame();
> +    int timelineWriteFrame = m_accumulationReadTimeFrame + m_postDelayLength;
> +
> +    // printf("%p %p: ReverbConvolverStage::process(%d) : (%d \t %d)\n", pthread_self(), this, m_fftKernel.fftSize(), timelineWriteFrame, timelineReadFrame);
> +
> +    if (timelineReadFrame > timelineWriteFrame)
> +        printf("%p %p: ReverbConvolverStage::process(%d) : (%d \t %d)\n", pthread_self(), this, m_fftKernel.fftSize(), timelineWriteFrame, timelineReadFrame);
> +
> +    if (timelineWriteFrame > timelineReadFrame + m_impulseResponseLength)
> +        printf("%p %p: ReverbConvolverStage::process(%d) : (%d \t %d)\n", pthread_self(), this, m_fftKernel.fftSize(), timelineWriteFrame, timelineReadFrame);
> +#endif
> +
> +    m_accumulationReadTimeFrame += framesToProcess;
> +    m_framesProcessed += framesToProcess;
> +}
> +
> +void ReverbConvolverStage::reset()
> +{
> +    m_convolver->reset();
> +    m_preDelayBuffer.zero();
> +    m_accumulationReadIndex = 0;
> +    m_inputReadIndex = 0;
> +    m_framesProcessed = 0;
> +    m_accumulationReadTimeFrame = 0;
> +}
> +
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +ReverbAccumulationBuffer::ReverbAccumulationBuffer(size_t length)
> +    : m_buffer(length)
> +    , m_length(length)
> +    , m_readIndex(0)
> +    , m_readTimeFrame(0)
> +{
> +}
> +
> +void ReverbAccumulationBuffer::readAndClear(float* destP, size_t nframes)
> +{
> +    assert(m_readIndex <= m_length);
> +    size_t framesAvailable = m_length - m_readIndex;
> +    size_t nframes1 = (nframes <= framesAvailable) ? nframes : framesAvailable;
> +    size_t nframes2 = nframes - nframes1;
> +
> +    float* sourceP = m_buffer;
> +    memcpy(destP, sourceP + m_readIndex, sizeof(float) * nframes1);
> +    memset(sourceP + m_readIndex, 0, sizeof(float) * nframes1);
> +
> +    // Handle wrap-around if necessary
> +    if (nframes2 > 0) {
> +        memcpy(destP + nframes1, sourceP, sizeof(float) * nframes2);
> +        memset(sourceP, 0, sizeof(float) * nframes2);
> +    }
> +
> +    m_readIndex = (m_readIndex + nframes) % m_length;
> +    m_readTimeFrame += nframes;
> +}
> +
> +void ReverbAccumulationBuffer::updateReadIndex(int* readIndex, size_t nframes)
> +{
> +    // Update caller's |readIndex|
> +    *readIndex = (*readIndex + nframes) % m_length;
> +}
> +
> +int ReverbAccumulationBuffer::accumulate(float* sourceP,
> +                                         size_t nframes,
> +                                         int* readIndex,
> +                                         size_t delayFrames)
> +{
> +    size_t writeIndex = (*readIndex + delayFrames) % m_length;
> +
> +    // Update caller's |readIndex|
> +    *readIndex = (*readIndex + nframes) % m_length;
> +
> +    assert(writeIndex <= m_length);
> +    size_t framesAvailable = m_length - writeIndex;
> +    size_t nframes1 = (nframes <= framesAvailable) ? nframes : framesAvailable;
> +    size_t nframes2 = nframes - nframes1;
> +
> +    float* destP = m_buffer;
> +
> +    vadd(sourceP,
> +         1,
> +         destP + writeIndex,
> +         1,
> +         destP + writeIndex,
> +         1,
> +         nframes1);
> +
> +    // Handle wrap-around if necessary
> +    if (nframes2 > 0) {
> +        vadd(sourceP + nframes1,
> +             1,
> +             destP,
> +             1,
> +             destP,
> +             1,
> +             nframes2);

Don't split

> +    }
> +
> +    return writeIndex;
> +}
> +
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This isn't really WebKit's style.

> +
> +ReverbInputBuffer::ReverbInputBuffer(size_t length)
> +    : m_buffer(length)
> +    , m_length(length)
> +    , m_writeIndex(0)
> +{
> +}
> +
> +void ReverbInputBuffer::write(float* sourceP, size_t nframes)
> +{
> +    memcpy(m_buffer.data() + m_writeIndex, sourceP, sizeof(float) * nframes);
> +
> +    m_writeIndex += nframes;
> +    assert(m_writeIndex <= m_length);
> +
> +    if (m_writeIndex >= m_length)
> +        m_writeIndex = 0;
> +}
> +
> +float* ReverbInputBuffer::directReadFrom(int* index, size_t nframes)
> +{
> +    assert(*index >= 0 && *index + nframes <= m_length);
> +    float* sourceP = m_buffer;
> +    float* p = sourceP + *index;
> +
> +    // Update index
> +    *index = (*index + nframes) % m_length;
> +
> +    return p;
> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/ReverbConvolver.h b/WebCore/platform/audio/ReverbConvolver.h
> new file mode 100644
> index 0000000..e37ccf9
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolver.h
> @@ -0,0 +1,226 @@
> +/*
> + * 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 ReverbConvolver_h
> +#define ReverbConvolver_h
> +
> +#include "AudioFloatArray.h"
> +#include "FFTConvolver.h"
> +#include <pthread.h>

There's thread abstractions in wtf.  Please use those.

> +#include <wtf/Vector.h>
> +
> +namespace WebCore {
> +
> +class AudioChannel;
> +class ReverbConvolver;
> +
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +// ReverbAccumulationBuffer is a circular delay buffer with one client reading
> +// from it and multiple clients writing/accumulating to it at different delay
> +// offsets from the read position.  The read operation will zero the memory
> +// just read from the buffer, so it will be ready for accumulation the next
> +// time around.
> +//
> +class ReverbAccumulationBuffer {

Each class should have its own .h file and .cpp file.

> +public:
> +    ReverbAccumulationBuffer(size_t length);
> +
> +    // This will read from, then clear-out |nframes|
> +    void readAndClear(float* destP, size_t nframes);
> +
> +    // Each ReverbConvolverStage will accumulate its output at the appropriate delay from the read position.
> +    // We need to pass in and update |readIndex| here, since each ReverbConvolverStage may be running in
> +    // a different thread than the realtime thread calling ReadAndClear() and maintaining |m_readIndex|
> +    // Returns the |writeIndex| where the accumulation took place
> +    int accumulate(float* sourceP,
> +                   size_t nframes,
> +                   int* readIndex,
> +                   size_t delayFrames);

Don't split things onto multiple lines like this unless you have a _really_ good reason.

> +
> +    size_t readIndex() const { return m_readIndex; }
> +    size_t readTimeFrame() const { return m_readTimeFrame; }
> +
> +    void updateReadIndex(int* readIndex, size_t nframes);
> +
> +    void reset()
> +    {
> +        m_buffer.zero();
> +        m_readIndex = 0;
> +        m_readTimeFrame = 0;
> +    }
> +
> +private:
> +    AudioFloatArray m_buffer;
> +    size_t m_length;
> +    size_t m_readIndex;
> +    size_t m_readTimeFrame; // for debugging (frame on continuous timeline)
> +};
> +
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +// ReverbInputBuffer is used to buffer input samples for deferred processing by the background threads
> +class ReverbInputBuffer {
> +public:
> +    ReverbInputBuffer(size_t length);
> +
> +    // The realtime audio thread keeps writing samples here.
> +    // The assumption is that the buffer's length is evenly divisible by |nframes|  (for nearly all cases this will be fine)
> +    // FIXME : remove |nframes| restriction...

Use FIXME: not FIXME :.  Here and lots of places.

> +    void write(float* sourceP, size_t nframes);
> +
> +    // Background threads can call this to check if there's anything to read...
> +    // FIXME : create better system to check for buffer overruns / error conditions...
> +    size_t writeIndex() const { return m_writeIndex; }
> +
> +    // The individual background threads read here (and hope that they can keep up with the buffer writing)
> +    // |index| is updated with the next index to read from...
> +    // The assumption is that the buffer's length is evenly divisible by |nframes|
> +    // FIXME : remove |nframes| restriction...
> +    float* directReadFrom(int* index, size_t nframes);
> +
> +    void reset()
> +    {
> +        m_buffer.zero();
> +        m_writeIndex = 0;
> +    }
> +
> +private:
> +    AudioFloatArray m_buffer;
> +    size_t m_length;
> +    size_t m_writeIndex;
> +};
> +
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +// A ReverbConvolverStage represents the convolution associated with a sub-section of a large impulse response.
> +// It incorporates a delay line to account for the offset of the sub-section within the larger impulse response.
> +class ReverbConvolverStage {
> +public:
> +    // |renderPhase| is useful to know so that we can manipulate the
> +    // pre versus post delay so that stages will perform their heavy work
> +    // (FFT processing) on different slices to balance the load in a real-time thread
> +    ReverbConvolverStage(float* impulseResponse,
> +                         size_t responseLength,
> +                         size_t reverbTotalLatency,
> +                         size_t stageOffset,
> +                         size_t stageLength,
> +                         size_t fftSize,
> +                         size_t renderPhase,
> +                         size_t renderSliceSize,
> +                         ReverbAccumulationBuffer* accumulationBuffer);
> +
> +    virtual ~ReverbConvolverStage();
> +
> +    // WARNING: |framesToProcess| must be such that it evenly divides the delay buffer size (stage_offset)
> +    void process(float* sourceP, size_t framesToProcess);
> +
> +    void processInBackground(ReverbConvolver* convolver, size_t framesToProcess);
> +
> +    void reset();
> +
> +    // Useful for background processing
> +    int inputReadIndex() const { return m_inputReadIndex; }
> +
> +private:
> +    FFTFrame m_fftKernel;
> +    FFTConvolver* m_convolver;
> +
> +    AudioFloatArray m_preDelayBuffer;
> +
> +    ReverbAccumulationBuffer* m_accumulationBuffer;
> +    int m_accumulationReadIndex;
> +    int m_inputReadIndex;
> +
> +    int m_accumulationReadTimeFrame; // for testing (frame on continuous timeline)
> +
> +    size_t m_preDelayLength;
> +    size_t m_postDelayLength;
> +    size_t m_preReadWriteIndex;
> +    size_t m_framesProcessed;
> +
> +    AudioFloatArray m_tempBuffer;
> +
> +    size_t m_impulseResponseLength;
> +};
> +
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +class ReverbConvolver {
> +public:
> +    // |maxFFTSize| can be adjusted (from say 2048 to 32768) depending on how much precision is necessary.
> +    // For certain tweaky de-convolving applications the phase errors add up quickly and lead to non-sensical results with
> +    // larger FFT sizes and single-precision floats.  In these cases 2048 is a good size.
> +    // If not doing multi-threaded convolution, then should not go > 8192.
> +    ReverbConvolver(AudioChannel* impulseResponse,
> +                    size_t renderSliceSize,
> +                    size_t maxFFTSize,
> +                    size_t convolverRenderPhase,
> +                    bool useBackgroundThreads);
> +
> +    virtual ~ReverbConvolver();
> +
> +    void process(float* sourceP,
> +                 float* destP,
> +                 size_t framesToProcess);
> +
> +    void reset();
> +
> +    size_t impulseResponseLength();
> +
> +    void backgroundThreadEntry();
> +    ReverbInputBuffer& inputBuffer() { return m_inputBuffer; }
> +
> +    bool useBackgroundThreads() const { return m_useBackgroundThreads; }
> +
> +private:
> +    Vector<ReverbConvolverStage*> m_stages;
> +    Vector<ReverbConvolverStage*> m_backgroundStages;
> +    size_t m_impulseResponseLength;
> +
> +    ReverbAccumulationBuffer m_accumulationBuffer;
> +
> +    // For multithreading
> +    ReverbInputBuffer m_inputBuffer;
> +
> +    // We're given 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)
> +    size_t m_renderSliceSize;
> +
> +    // First stage will be of size |m_minFFTSize|.  Each next stage will be twice as big until we hit |m_maxFFTSize|
> +    size_t m_minFFTSize;
> +    size_t m_maxFFTSize;
> +
> +    // But don't exceed this size in the real-time thread (if we're doing background processing)
> +    size_t m_maxRealtimeFFTSize;
> +
> +    // FIXME : use thread abstraction (assuming pthreads here)
> +    bool m_useBackgroundThreads;
> +    pthread_t m_backgroundThread;
> +    bool m_wantsToExit;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // ReverbConvolver_h

[Chris Rogers]

Created attachment 50748 [details]
Patch

[Chris Rogers]

Created attachment 50749 [details]
Patch

[Chris Rogers]

Hi Jeremy,

[Chris Rogers]

Hi Jeremy, thanks very much for your review!

I tried carefully to address your style comments, but you can check and maybe you'll find a few more :)

You had two more complex questions which I'll try to address:

> How was this measured?  On what platforms?  How will we make sure that this
> stays close to reality?  (I.e. can we make a layout test that verifies this or
> something?)

I added some more comments here.  Later on in the code I had a block which
was #if 0 for testing which could at some point be part of a test to make sure the background thread is working properly.  For now I've removed this "dead" code.  For a layout test, we would have to create a "special" testing javascript API which could report any problems back.  Or maybe there's a simpler way...


> Is any such "command line tool" intended to be upstreamed?  If not, this
> support should be removed.

I believe that we should, since the tools are very important for any javascript developers wanting to create their own custom impulse responses.

[Jeremy Orlow]

(In reply to comment #8)
> Hi Jeremy, thanks very much for your review!
> 
> I tried carefully to address your style comments, but you can check and maybe
> you'll find a few more :)
> 
> You had two more complex questions which I'll try to address:
> 
> > How was this measured?  On what platforms?  How will we make sure that this
> > stays close to reality?  (I.e. can we make a layout test that verifies this or
> > something?)
> 
> I added some more comments here.  Later on in the code I had a block which
> was #if 0 for testing which could at some point be part of a test to make sure
> the background thread is working properly.  For now I've removed this "dead"
> code.  For a layout test, we would have to create a "special" testing
> javascript API which could report any problems back.  Or maybe there's a
> simpler way...

Yeah, normally you add special methods to the layoutTestController.  Your code might be a good candidate for unit testing, though.  I believe there's some unit testing being done now on WebCore code.  If so, it'd be great if we could plug into whatever frameworks are available.  Ping fishd/dglazkov about this. 
 
> > Is any such "command line tool" intended to be upstreamed?  If not, this
> > support should be removed.
> 
> I believe that we should, since the tools are very important for any javascript
> developers wanting to create their own custom impulse responses.

Sounds good.  Lets try to keep the footprint of extra code required for this to a minimum though.

[Jeremy Orlow]

Comment on attachment 50749 [details]
Patch

More comments.  Still haven't really looked at the semantics of the code...just the syntax.


> diff --git a/WebCore/platform/audio/ReverbAccumulationBuffer.cpp b/WebCore/platform/audio/ReverbAccumulationBuffer.cpp
> new file mode 100644
> index 0000000..a0e0dc5
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbAccumulationBuffer.cpp
> @@ -0,0 +1,101 @@
> +/*
> + * 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 "ReverbAccumulationBuffer.h"
> +
> +#include "Accelerate.h"

Is this file mac specific?

> +
> +namespace WebCore {
> +
> +ReverbAccumulationBuffer::ReverbAccumulationBuffer(size_t length)
> +    : m_buffer(length)
> +    , m_length(length)
> +    , m_readIndex(0)
> +    , m_readTimeFrame(0)
> +{
> +}
> +
> +void ReverbAccumulationBuffer::readAndClear(float* destP, size_t nframes)
> +{
> +    ASSERT(m_readIndex <= m_length);
> +    size_t framesAvailable = m_length - m_readIndex;
> +    size_t nframes1 = (nframes <= framesAvailable) ? nframes : framesAvailable;
> +    size_t nframes2 = nframes - nframes1;
> +
> +    float* sourceP = m_buffer;
> +    memcpy(destP, sourceP + m_readIndex, sizeof(float) * nframes1);
> +    memset(sourceP + m_readIndex, 0, sizeof(float) * nframes1);
> +
> +    // Handle wrap-around if necessary
> +    if (nframes2 > 0) {
> +        memcpy(destP + nframes1, sourceP, sizeof(float) * nframes2);
> +        memset(sourceP, 0, sizeof(float) * nframes2);
> +    }
> +
> +    m_readIndex = (m_readIndex + nframes) % m_length;
> +    m_readTimeFrame += nframes;
> +}
> +
> +void ReverbAccumulationBuffer::updateReadIndex(int* readIndex, size_t nframes)
> +{
> +    // Update caller's |readIndex|
> +    *readIndex = (*readIndex + nframes) % m_length;
> +}
> +
> +int ReverbAccumulationBuffer::accumulate(float* sourceP, size_t nframes, int* readIndex, size_t delayFrames)
> +{
> +    size_t writeIndex = (*readIndex + delayFrames) % m_length;
> +
> +    // Update caller's |readIndex|
> +    *readIndex = (*readIndex + nframes) % m_length;
> +
> +    ASSERT(writeIndex <= m_length);
> +    size_t framesAvailable = m_length - writeIndex;
> +    size_t nframes1 = (nframes <= framesAvailable) ? nframes : framesAvailable;
> +    size_t nframes2 = nframes - nframes1;
> +
> +    float* destP = m_buffer;
> +
> +    vadd(sourceP, 1, destP + writeIndex, 1, destP + writeIndex, 1, nframes1);
> +
> +    // Handle wrap-around if necessary
> +    if (nframes2 > 0)
> +        vadd(sourceP + nframes1, 1, destP, 1, destP, 1, nframes2);
> +
> +    return writeIndex;
> +}
> +
> +void ReverbAccumulationBuffer::reset()
> +{
> +    m_buffer.zero();
> +    m_readIndex = 0;
> +    m_readTimeFrame = 0;
> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/ReverbAccumulationBuffer.h b/WebCore/platform/audio/ReverbAccumulationBuffer.h
> new file mode 100644
> index 0000000..6a5a65c
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbAccumulationBuffer.h
> @@ -0,0 +1,68 @@
> +/*
> + * 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 ReverbAccumulationBuffer_h
> +#define ReverbAccumulationBuffer_h
> +
> +#include "AudioFloatArray.h"
> +
> +namespace WebCore {
> +
> +// ReverbAccumulationBuffer is a circular delay buffer with one client reading from it and multiple clients
> +// writing/accumulating to it at different delay offsets from the read position.  The read operation will zero the memory
> +// just read from the buffer, so it will be ready for accumulation the next time around.
> +class ReverbAccumulationBuffer {
> +public:
> +    ReverbAccumulationBuffer(size_t length);

This should be private and you should have a ::create() factory (static function) that's public.

> +
> +    // This will read from, then clear-out |nframes|
> +    void readAndClear(float* destP, size_t nframes);
> +
> +    // Each ReverbConvolverStage will accumulate its output at the appropriate delay from the read position.
> +    // We need to pass in and update |readIndex| here, since each ReverbConvolverStage may be running in
> +    // a different thread than the realtime thread calling ReadAndClear() and maintaining |m_readIndex|
> +    // Returns the |writeIndex| where the accumulation took place
> +    int accumulate(float* sourceP, size_t nframes, int* readIndex, size_t delayFrames);
> +
> +    size_t readIndex() const { return m_readIndex; }
> +    void updateReadIndex(int* readIndex, size_t nframes);
> +
> +    size_t readTimeFrame() const { return m_readTimeFrame; }
> +
> +    void reset();
> +
> +private:
> +    AudioFloatArray m_buffer;
> +    size_t m_length;
> +    size_t m_readIndex;
> +    size_t m_readTimeFrame; // for debugging (frame on continuous timeline)
> +};
> +
> +} // namespace WebCore
> +
> +#endif // ReverbAccumulationBuffer_h
> diff --git a/WebCore/platform/audio/ReverbConvolver.cpp b/WebCore/platform/audio/ReverbConvolver.cpp
> new file mode 100644
> index 0000000..d1bf559
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolver.cpp
> @@ -0,0 +1,196 @@
> +/*
> + * 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 "ReverbConvolver.h"
> +
> +#include "Accelerate.h"
> +#include "AudioBus.h"
> +
> +namespace WebCore {
> +
> +const int InputBufferSize = 8 * 16384;
> +
> +// We only process the leading portion of the impulse response in the real-time thread.  We don't exceed this length.
> +// It turns out then, that the background thread has about 278msec of scheduling slop.
> +// Empirically, this has been found to be a good compromise between giving enough time for scheduling slop,
> +// while still minimizing the amount of processing done in the primary (high-priority) thread.
> +// This was found to be a good value on Mac OS X, and may work well on other platforms as well, assuming
> +// the very rough scheduling latencies are similar on these time-scales.  Of course, this code may need to be
> +// tuned for individual platforms if this assumption is found to be incorrect.
> +const size_t RealtimeFrameLimit = 8192  + 4096; // ~278msec @ 44.1KHz

Still not super happy about this solution, but I guess we should wait until it actually needs per-platform tuning to add in such code.

> +const size_t MinFFTSize = 256;
> +const size_t MaxRealtimeFFTSize = 2048;
> +
> +static void* BackgroundThreadDispatch(void* threadData)
> +{
> +    ReverbConvolver* reverbConvolver = static_cast<ReverbConvolver*>(threadData);
> +    reverbConvolver->backgroundThreadEntry();
> +    return 0;
> +}
> +
> +ReverbConvolver::ReverbConvolver(AudioChannel* impulseResponse, size_t renderSliceSize, size_t maxFFTSize, size_t convolverRenderPhase, bool useBackgroundThreads)
> +    : m_impulseResponseLength(impulseResponse->frameSize())
> +    , m_accumulationBuffer(impulseResponse->frameSize() + renderSliceSize)
> +    , m_inputBuffer(InputBufferSize)
> +    , m_renderSliceSize(renderSliceSize)
> +    , m_useBackgroundThreads(useBackgroundThreads)
> +    , m_wantsToExit(false)
> +{
> +    m_minFFTSize = MinFFTSize; // First stage will have this size - successive stages will double in size each time
> +    m_maxFFTSize = maxFFTSize; // until we hit |m_maxFFTSize|

nit: I'm not aware of any other part of the code that uses |'s and I think it's pretty clear m_maxFFTSize is a variable.  If you can find other places in the code that use them or if there are individual cases that are hard to read without them, go ahead and leave them in.  Otherwise, please remove.

And why aren't these initialized up above?

> +
> +    // If we are using background threads then don't exceed this FFT size for the
> +    // stages which run in the real-time thread.  This avoids having only one or two
> +    // large stages (size 16384 or so) at the end which take a lot of time every several
> +    // processing slices.  This way we amortize the cost over more processing slices.
> +    m_maxRealtimeFFTSize = MaxRealtimeFFTSize;

Even these probably should be initialized in the first part of the constructor, but since these comments are long, it kind of makes sense why you did this, I suppose.

> +
> +    // For the moment, a good way to know if we have real-time constraint is to check if we're using background threads.
> +    // Otherwise, assume we're being run from a command-line tool.
> +    bool hasRealtimeConstraint = useBackgroundThreads;
> +
> +    float* response = impulseResponse->data();
> +    size_t totalResponseLength = impulseResponse->frameSize();
> +    ReverbAccumulationBuffer* accumBufferP = &m_accumulationBuffer;
> +
> +    // Because we're not using direct-convolution in the leading portion, the reverb has an overall latency of half the first-stage FFT size
> +    size_t reverbTotalLatency = m_minFFTSize / 2;
> +
> +    size_t stageOffset = 0;
> +    int i = 0;
> +    size_t fftSize = m_minFFTSize;
> +    while (stageOffset < totalResponseLength) {
> +        size_t stageSize = fftSize / 2;
> +
> +        // For the last stage, it's possible that |stageOffset| is such that we're straddling the end
> +        // of the impulse response buffer (if we use |stageSize|), so reduce the last stage's length...
> +        if (stageSize + stageOffset > totalResponseLength)
> +            stageSize = totalResponseLength - stageOffset;
> +
> +        // This "staggers" the time when each FFT happens so they don't all happen at the same time
> +        int renderPhase = convolverRenderPhase + i * renderSliceSize;
> +
> +        ReverbConvolverStage* stage = new ReverbConvolverStage(response, totalResponseLength, reverbTotalLatency, stageOffset, stageSize, fftSize, renderPhase, renderSliceSize, accumBufferP);

This should be an OwnPtr.

> +
> +        bool isBackgroundStage = false;
> +
> +        if (stageOffset <= RealtimeFrameLimit)
> +            m_stages.append(stage);
> +        else {
> +            if (this->useBackgroundThreads()) {
> +                m_backgroundStages.append(stage);
> +                isBackgroundStage = true;
> +            } else
> +                m_stages.append(stage);
> +        }
> +
> +        stageOffset += stageSize;
> +        i++;

++i.

> +
> +        // Figure out next FFT size
> +        fftSize *= 2;
> +        if (hasRealtimeConstraint && !isBackgroundStage && fftSize > m_maxRealtimeFFTSize)
> +            fftSize = m_maxRealtimeFFTSize;
> +        if (fftSize > m_maxFFTSize)
> +            fftSize = m_maxFFTSize;
> +    }
> +
> +    // Start up background thread
> +    // FIXME: would be better to up the thread priority here.  It doesn't need to be real-time, but higher than the default...
> +    if (this->useBackgroundThreads() && m_backgroundStages.size() > 0)
> +        m_backgroundThread = createThread(BackgroundThreadDispatch, this, "convolution background thread");
> +    else
> +        m_backgroundThread = 0;
> +}
> +
> +ReverbConvolver::~ReverbConvolver()
> +{
> +    // Wait for background thread to stop
> +    if (useBackgroundThreads() && m_backgroundThread) {
> +        m_wantsToExit = true;
> +        waitForThreadCompletion(m_backgroundThread, 0);
> +    }
> +}
> +
> +void ReverbConvolver::backgroundThreadEntry()
> +{
> +    while (!m_wantsToExit) {
> +        // Check to see if there's any more input to consume
> +        int writeIndex = m_inputBuffer.writeIndex();
> +
> +        // Even though it doesn't seem like every stage needs to maintain its own version of |readIndex| 
> +        // we do this in case we want to run in more than one background thread.
> +        int readIndex;
> +
> +        while ((readIndex = m_backgroundStages[0]->inputReadIndex()) != writeIndex) { // FIXME: do better to detect buffer overrun...
> +            // FIXME: remove hard-coded value
> +            const int kSliceSize = 128;
> +
> +            // Accumulate contributions from each stage
> +            for (size_t i = 0; i < m_backgroundStages.size(); ++i)
> +                m_backgroundStages[i]->processInBackground(this, kSliceSize);
> +        }
> +
> +        // Sleep 10ms
> +        usleep(10000); // FIXME: although it works well in practice, this really isn't ideal - could use wait/signal

There's probably something in wtf that'll do this for you now.  Please take a look and consider fixing this now.


> diff --git a/WebCore/platform/audio/ReverbConvolver.h b/WebCore/platform/audio/ReverbConvolver.h
> new file mode 100644
> index 0000000..8cc1d9e
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolver.h
> @@ -0,0 +1,96 @@
> +/*
> + * 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 ReverbConvolver_h
> +#define ReverbConvolver_h
> +
> +#include "AudioFloatArray.h"
> +#include "FFTConvolver.h"
> +#include "ReverbAccumulationBuffer.h"
> +#include "ReverbConvolverStage.h"
> +#include "ReverbInputBuffer.h"
> +#include <wtf/OwnPtr.h>
> +#include <wtf/RefCounted.h>
> +#include <wtf/Threading.h>
> +#include <wtf/Vector.h>
> +
> +namespace WebCore {
> +
> +class AudioChannel;
> +
> +class ReverbConvolver {
> +public:
> +    // |maxFFTSize| can be adjusted (from say 2048 to 32768) depending on how much precision is necessary.
> +    // For certain tweaky de-convolving applications the phase errors add up quickly and lead to non-sensical results with
> +    // larger FFT sizes and single-precision floats.  In these cases 2048 is a good size.
> +    // If not doing multi-threaded convolution, then should not go > 8192.
> +    ReverbConvolver(AudioChannel* impulseResponse, size_t renderSliceSize, size_t maxFFTSize, size_t convolverRenderPhase, bool useBackgroundThreads);

Here and in all of your classes, the constructors should be private and you should have factory functions instead.  The factory functions should return either PassOwnPtr<> or PassRefPtr<>.  This minimizes the number of places where memory leaks can start and makes it clear whether it's ref counted or not.

> +
> +    virtual ~ReverbConvolver();

Doesn't seem like this needs to be virtual.  Only make it virtual if it's subclassed and needs it.

> +
> +    void process(float* sourceP, float* destP, size_t framesToProcess);
> +
> +    void reset();
> +
> +    size_t impulseResponseLength();
> +
> +    ReverbInputBuffer& inputBuffer() { return m_inputBuffer; }

This should return a pointer.  Do not pass around references unless they're const.

> +
> +    bool useBackgroundThreads() const { return m_useBackgroundThreads; }
> +
> +    void backgroundThreadEntry();
> +
> +private:
> +    Vector<OwnPtr<ReverbConvolverStage> > m_stages;
> +    Vector<OwnPtr<ReverbConvolverStage> > m_backgroundStages;

Be careful with this stuff.  I checked and there are other cases of Vector<OwnPtr<>>'s, but it seems a tad dangerous (in terms of accidentally claiming ownership and thus making the OwnPtr in the vector point to 0.  But, like I said, it's done elsewhere and you can certainly do it safely if you're just a little careful.

> +    size_t m_impulseResponseLength;
> +
> +    ReverbAccumulationBuffer m_accumulationBuffer;
> +
> +    // For multithreading
> +    ReverbInputBuffer m_inputBuffer;

The comment isn't very clear.

> +
> +    // We're given 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)
> +    size_t m_renderSliceSize;
> +
> +    // First stage will be of size |m_minFFTSize|.  Each next stage will be twice as big until we hit |m_maxFFTSize|
> +    size_t m_minFFTSize;
> +    size_t m_maxFFTSize;
> +
> +    // But don't exceed this size in the real-time thread (if we're doing background processing)

In general, I'd rather you used punctuation at the end of all sentences, but there's plenty who don't.  So it's up to you, I guess.

> +    size_t m_maxRealtimeFFTSize;
> +
> +    bool m_useBackgroundThreads;
> +    ThreadIdentifier m_backgroundThread;
> +    bool m_wantsToExit;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // ReverbConvolver_h
> diff --git a/WebCore/platform/audio/ReverbConvolverStage.cpp b/WebCore/platform/audio/ReverbConvolverStage.cpp
> new file mode 100644
> index 0000000..cb6956d
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolverStage.cpp
> @@ -0,0 +1,145 @@
> +/*
> + * 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 "ReverbConvolverStage.h"
> +
> +#include "Accelerate.h"
> +#include "ReverbAccumulationBuffer.h"
> +#include "ReverbConvolver.h"
> +#include "ReverbInputBuffer.h"
> +
> +namespace WebCore {
> +
> +ReverbConvolverStage::ReverbConvolverStage(float* impulseResponse, size_t responseLength, size_t reverbTotalLatency, size_t stageOffset, size_t stageLength,
> +                                           size_t fftSize, size_t renderPhase, size_t renderSliceSize, ReverbAccumulationBuffer* accumulationBuffer)
> +    : m_fftKernel(fftSize)
> +    , m_accumulationBuffer(accumulationBuffer)
> +    , m_accumulationReadIndex(0)
> +    , m_inputReadIndex(0)
> +    , m_accumulationReadTimeFrame(0)
> +    , m_impulseResponseLength(responseLength)
> +{
> +    m_fftKernel.doPaddedFFT(impulseResponse + stageOffset, stageLength);
> +
> +    m_convolver = new FFTConvolver(fftSize);
> +
> +    m_tempBuffer.allocate(renderSliceSize);
> +
> +    // The convolution stage at offset |stageOffset| needs to have a corresponding delay to cancel out the offset
> +    size_t totalDelay = stageOffset + reverbTotalLatency;
> +
> +    // But, the FFT convolution itself incurs |fftSize| / 2 latency, so subtract this out...
> +    size_t halfSize = fftSize / 2;
> +    if (totalDelay >= halfSize)
> +        totalDelay -= halfSize;
> +
> +    // FIXME: DEAL with case when total delay is less than fftSize/2....
> +
> +    // We divide up the total delay, into pre and post delay sections so that we can
> +    // schedule at exactly the moment when the FFT will happen.  This is coordinated
> +    // with the other stages, so they don't all do their FFTs at the same time...
> +
> +    int m = (halfSize <= totalDelay) ? halfSize : totalDelay;
> +    m_preDelayLength = (totalDelay > 0) ? (renderPhase % m) : 0;
> +
> +    if (m_preDelayLength > totalDelay)
> +        m_preDelayLength = 0;
> +
> +    m_postDelayLength = totalDelay - m_preDelayLength;
> +    m_preReadWriteIndex = 0;
> +    m_framesProcessed = 0; // total frames processed so far
> +
> +    m_preDelayBuffer.allocate(m_preDelayLength < fftSize ? fftSize : m_preDelayLength);
> +}
> +
> +void ReverbConvolverStage::processInBackground(ReverbConvolver* convolver,
> +                                               size_t framesToProcess)

no need to split lines

> +{
> +    ReverbInputBuffer& inputBuffer = convolver->inputBuffer();
> +    float* sourceP = inputBuffer.directReadFrom(&m_inputReadIndex, framesToProcess);
> +    process(sourceP, framesToProcess);
> +}
> +
> +void  ReverbConvolverStage::process(float* sourceP,
> +                                    size_t framesToProcess)

no need to split lines

[Chris Rogers]

Created attachment 50945 [details]
Patch

[Chris Rogers]

Hi Jeremy, I addressed a bunch of your comments.  Once again, I really appreciate your help.  Here are some comments on the remaining issues:

> Is this file mac specific?

No, it's a cross-platform abstraction which declares the necessary DSP functions
It's in another patch:
https://bugs.webkit.org/show_bug.cgi?id=34452

>This should be private and you should have a ::create() factory (static
>function) that's public.

This is in regards to ReverbAccumulationBuffer which is a helper class only used by ReverbConvolver.  It's used directly as a composed member object there, so the object lifetime is taken care of automatically.  Dimitri Glazkov told me it's definitely OK to have helper classes which can be used as stack-based objects and directly as composed members of other classes with no need for ::create() and private constructor.

> Still not super happy about this solution, but I guess we should wait until it
> actually needs per-platform tuning to add in such code.

Yeah, I understand your feeling.  Unfortunately, this is the messy world of real-time systems where scheduler performance of different OS kernel's may come into play.  In this particular case, I think it will be OK.  But it's one of several things to be tested and tuned (if necessary) per-platform.

> This should be an OwnPtr.

The pointer is being put either into m_backgroundStages or m_stages which are both Vector<OwnPtr<ReverbConvolverStage> >
I *did* make the local pointer into a PassOwnPtr<ReverbConvolverStage>
I found that the compiler was not happy if I used OwnPtr in the local variable in addition to the Vector...

> There's probably something in wtf that'll do this for you now.  Please take a
> look and consider fixing this now.

I hunted all over for a sleep(), usleep(), nanosleep() abstraction in wtf, but did not find it.  I think a good place to put one would be in <wtf/Threading.h>
But I'm not sure how to implement that for all platforms yet.

> Here and in all of your classes, the constructors should be private and you
> should have factory functions instead.  The factory functions should return
> either PassOwnPtr<> or PassRefPtr<>.  This minimizes the number of places > where memory leaks can start and makes it clear whether it's ref counted or 
> not.

Dimitri assured me that it's OK to not use ::create() methods / private-constructors (in order to be able to use as stack-based objects, composed member variables, etc.).  The case where it is required is when it will be ref-counted (RefPtr<>) which is not the case here.

> Doesn't seem like this needs to be virtual.  Only make it virtual if it's
> subclassed and needs it.

I *did* remove the virtual, but I tend to *always* use virtual for destructors unless I know for a fact that I need the object to occupy an exact number of bytes (to overlay a struct, for example) or if there are other clear performance reasons not to.  The reason is that it's very easy to create difficult to track down bugs if you later sub-class and forget the virtual.

> Be careful with this stuff.  I checked and there are other cases of
> Vector<OwnPtr<>>'s, but it seems a tad dangerous (in terms of accidentally
> claiming ownership and thus making the OwnPtr in the vector point to 0.  But,
> like I said, it's done elsewhere and you can certainly do it safely if you're
> just a little careful.

Yeah I'm trying to be pretty paranoid when using this kind of stuff.  But, I'm not sure what the alternative is here if you want to use OwnPtr, since it is indeed a Vector of them that I'm keeping track of...

[Chris Rogers]

Created attachment 50957 [details]
Patch

[Jeremy Orlow]

> I *did* remove the virtual, but I tend to *always* use virtual for destructors
> unless I know for a fact that I need the object to occupy an exact number of
> bytes (to overlay a struct, for example) or if there are other clear
> performance reasons not to.  The reason is that it's very easy to create
> difficult to track down bugs if you later sub-class and forget the virtual.

Understood, but in general we try hard to avoid making things virtual in WebKit.  I also think it's a good habit to always double check whenever you subclass something that the parent has a virtual destructor.

[Jeremy Orlow]

Comment on attachment 50957 [details]
Patch

> +ReverbConvolver::ReverbConvolver(AudioChannel* impulseResponse, size_t renderSliceSize, size_t maxFFTSize, size_t convolverRenderPhase, bool useBackgroundThreads)
> +    : m_impulseResponseLength(impulseResponse->frameSize())
> +    , m_accumulationBuffer(impulseResponse->frameSize() + renderSliceSize)
> +    , m_inputBuffer(InputBufferSize)
> +    , m_renderSliceSize(renderSliceSize)
> +    , m_minFFTSize(MinFFTSize) // First stage will have this size - successive stages will double in size each time
> +    , m_maxFFTSize(maxFFTSize) // until we hit m_maxFFTSize
> +    , m_useBackgroundThreads(useBackgroundThreads)
> +    , m_wantsToExit(false)
> +{
> +    // If we are using background threads then don't exceed this FFT size for the
> +    // stages which run in the real-time thread.  This avoids having only one or two
> +    // large stages (size 16384 or so) at the end which take a lot of time every several
> +    // processing slices.  This way we amortize the cost over more processing slices.
> +    m_maxRealtimeFFTSize = MaxRealtimeFFTSize;
> +
> +    // For the moment, a good way to know if we have real-time constraint is to check if we're using background threads.
> +    // Otherwise, assume we're being run from a command-line tool.
> +    bool hasRealtimeConstraint = useBackgroundThreads;
> +
> +    float* response = impulseResponse->data();
> +    size_t totalResponseLength = impulseResponse->frameSize();
> +    ReverbAccumulationBuffer* accumBufferP = &m_accumulationBuffer;
> +
> +    // Because we're not using direct-convolution in the leading portion, the reverb has an overall latency of half the first-stage FFT size
> +    size_t reverbTotalLatency = m_minFFTSize / 2;
> +
> +    size_t stageOffset = 0;
> +    int i = 0;
> +    size_t fftSize = m_minFFTSize;
> +    while (stageOffset < totalResponseLength) {
> +        size_t stageSize = fftSize / 2;
> +
> +        // For the last stage, it's possible that stageOffset is such that we're straddling the end
> +        // of the impulse response buffer (if we use stageSize), so reduce the last stage's length...
> +        if (stageSize + stageOffset > totalResponseLength)
> +            stageSize = totalResponseLength - stageOffset;
> +
> +        // This "staggers" the time when each FFT happens so they don't all happen at the same time
> +        int renderPhase = convolverRenderPhase + i * renderSliceSize;
> +
> +        PassOwnPtr<ReverbConvolverStage> stage(new ReverbConvolverStage(response, totalResponseLength, reverbTotalLatency, stageOffset, stageSize, fftSize, renderPhase, renderSliceSize, accumBufferP));

Always use ___Ptr's as member and automatic/local variables.  Use Pass___Ptr's as method parameters and return values.  You might want to re-read the PassRefPtr doc (just google that word) if you don't understand why.


> +void ReverbConvolver::backgroundThreadEntry()
> +{
> +    while (!m_wantsToExit) {
> +        // Check to see if there's any more input to consume
> +        int writeIndex = m_inputBuffer.writeIndex();
> +
> +        // Even though it doesn't seem like every stage needs to maintain its own version of readIndex 
> +        // we do this in case we want to run in more than one background thread.
> +        int readIndex;
> +
> +        while ((readIndex = m_backgroundStages[0]->inputReadIndex()) != writeIndex) { // FIXME: do better to detect buffer overrun...
> +            // FIXME: remove hard-coded value
> +            const int kSliceSize = 128;
> +
> +            // Accumulate contributions from each stage
> +            for (size_t i = 0; i < m_backgroundStages.size(); ++i)
> +                m_backgroundStages[i]->processInBackground(this, kSliceSize);
> +        }
> +
> +        // Sleep 10ms
> +        usleep(10000); // FIXME: although it works well in practice, this really isn't ideal - could use wait/signal

Sorry, what I was suggesting you do is make this use condition variables now rather than leaving it a FIXME.  WebCore/storage/StorageAreaSync does something like this IIRC.

[Jeremy Orlow]

Comment on attachment 50957 [details]
Patch

> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbAccumulationBuffer.cpp
> @@ -0,0 +1,101 @@
> +/*
> + * 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 "ReverbAccumulationBuffer.h"
> +
> +#include "Accelerate.h"
> +
> +namespace WebCore {
> +
> +ReverbAccumulationBuffer::ReverbAccumulationBuffer(size_t length)
> +    : m_buffer(length)
> +    , m_length(length)
> +    , m_readIndex(0)
> +    , m_readTimeFrame(0)
> +{
> +}
> +
> +void ReverbAccumulationBuffer::readAndClear(float* destP, size_t nframes)

destP and nframes are not the best variables names.  Same goes for nframes1 and 2.  I don't think it gets too much in the way of readability here, but it's a good practice to get into...even if it means a bit more typing and thinking about what to name stuff.

> +{
> +    ASSERT(m_readIndex <= m_length);
> +    size_t framesAvailable = m_length - m_readIndex;
> +    size_t nframes1 = (nframes <= framesAvailable) ? nframes : framesAvailable;

min(nframes, framseAvailable)

> +    size_t nframes2 = nframes - nframes1;
> +
> +    float* sourceP = m_buffer;
> +    memcpy(destP, sourceP + m_readIndex, sizeof(float) * nframes1);
> +    memset(sourceP + m_readIndex, 0, sizeof(float) * nframes1);
> +
> +    // Handle wrap-around if necessary
> +    if (nframes2 > 0) {
> +        memcpy(destP + nframes1, sourceP, sizeof(float) * nframes2);
> +        memset(sourceP, 0, sizeof(float) * nframes2);
> +    }
> +
> +    m_readIndex = (m_readIndex + nframes) % m_length;

You could avoid the division by adding |m_readIndex = nframes2| in the if and adding an else to it that sets |m_readIndex += nframes1|.  Its probably over optimizing, but...  (Your choice.)

> +    m_readTimeFrame += nframes;
> +}
> +
> +void ReverbAccumulationBuffer::updateReadIndex(int* readIndex, size_t nframes)
> +{

First of all, this could be const.

Seconly, I'm not sure whether this is better or it's better to do this logic in the caller.  If you had a length getter method, then you could just do this up one level.  On the other hand, I see this is a weird case of a more general pattern that you follow.  If you still think this is best, it's OK with me though.

> +    // Update caller's readIndex
> +    *readIndex = (*readIndex + nframes) % m_length;
> +}
> +
> +int ReverbAccumulationBuffer::accumulate(float* sourceP, size_t nframes, int* readIndex, size_t delayFrames)
> +{
> +    size_t writeIndex = (*readIndex + delayFrames) % m_length;
> +
> +    // Update caller's readIndex
> +    *readIndex = (*readIndex + nframes) % m_length;

If you change above, change this.




> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbInputBuffer.cpp
> @@ -0,0 +1,70 @@
> +/*
> + * 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 "ReverbInputBuffer.h"
> +
> +namespace WebCore {
> +
> +ReverbInputBuffer::ReverbInputBuffer(size_t length)
> +    : m_buffer(length)
> +    , m_length(length)
> +    , m_writeIndex(0)
> +{
> +}
> +
> +void ReverbInputBuffer::write(float* sourceP, size_t nframes)
> +{
> +    memcpy(m_buffer.data() + m_writeIndex, sourceP, sizeof(float) * nframes);
> +
> +    m_writeIndex += nframes;
> +    ASSERT(m_writeIndex <= m_length);

Do this check before we potentially corrupt memory.  If it's over, that's really bad...right?  Maybe even use CRASH() here?  Otherwise you probably should handle this case more gracefully.

> +
> +    if (m_writeIndex >= m_length)
> +        m_writeIndex = 0;
> +}
> +
> +float* ReverbInputBuffer::directReadFrom(int* index, size_t nframes)
> +{
> +    ASSERT(*index >= 0 && *index + nframes <= m_length);

ditto

> +    float* sourceP = m_buffer;
> +    float* p = sourceP + *index;
> +
> +    // Update index
> +    *index = (*index + nframes) % m_length;
> +
> +    return p;
> +}
> +
> +void ReverbInputBuffer::reset()
> +{
> +    m_buffer.zero();
> +    m_writeIndex = 0;
> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/ReverbInputBuffer.h b/WebCore/platform/audio/ReverbInputBuffer.h
> new file mode 100644
> index 0000000..0b80f42
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbInputBuffer.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 ReverbInputBuffer_h
> +#define ReverbInputBuffer_h
> +
> +#include "AudioFloatArray.h"
> +
> +namespace WebCore {
> +
> +// ReverbInputBuffer is used to buffer input samples for deferred processing by the background threads

Period.  Here and elsewhere please.

> +class ReverbInputBuffer {
> +public:
> +    ReverbInputBuffer(size_t length);
> +
> +    // The realtime audio thread keeps writing samples here.
> +    // The assumption is that the buffer's length is evenly divisible by nframes  (for nearly all cases this will be fine)
> +    // FIXME: remove nframes restriction...
> +    void write(float* sourceP, size_t nframes);
> +
> +    // Background threads can call this to check if there's anything to read...
> +    // FIXME: create better system to check for buffer overruns / error conditions...

I'm not sure if this fixme is actually useful.

> +    size_t writeIndex() const { return m_writeIndex; }
> +
> +    // The individual background threads read here (and hope that they can keep up with the buffer writing)
> +    // index is updated with the next index to read from...
> +    // The assumption is that the buffer's length is evenly divisible by nframes
> +    // FIXME: remove nframes restriction...
> +    float* directReadFrom(int* index, size_t nframes);
> +
> +    void reset();
> +
> +private:
> +    AudioFloatArray m_buffer;
> +    size_t m_length;

Doesn't the buffer itself know its length?

> +    size_t m_writeIndex;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // ReverbInputBuffer_h

[Jeremy Orlow]

Comment on attachment 50957 [details]
Patch

Some more...

> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolverStage.h
> @@ -0,0 +1,85 @@
> +/*
> + * 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 ReverbConvolverStage_h
> +#define ReverbConvolverStage_h
> +
> +#include "AudioFloatArray.h"
> +#include "FFTConvolver.h"

Forward declare rather than including since it's only an OwnPtr.

> +#include <wtf/OwnPtr.h>
> +#include <wtf/PassOwnPtr.h>

This isn't used.


Try to avoid adding includes to .h files whenever possible.

[Chris Rogers]

Created attachment 51354 [details]
Patch

[Chris Rogers]

Hi Jeremy, I've addressed most of your comments.  Here are my comments about the others:

>Always use ___Ptr's as member and automatic/local variables.  Use Pass___Ptr's
>as method parameters and return values.  You might want to re-read the
>PassRefPtr doc (just google that word) if you don't understand why.

I've found that this can work with RefPtr, but not with OwnPtr (won't compile).  It looks like in the case of OwnPtr, it's not reference counted and you can't assign from one OwnPtr to another.  The current code does the right thing, but another option is to just forget the PassOwnPtr and use a straight pointer as the local variable.  It gets appended to a Vector<OwnPtr<> > so the memory management is handled.

> Forward declare rather than including since it's only an OwnPtr.
In general I always try to follow that principle.  I was able to get rid of
#include "FFTConvolver.h"
but then had to add in
#include "FFTFrame.h"
since I'm using object composition.
I was able to get rid of OwnPtr.h and PassOwnPtr.h though - thanks...

[Chris Rogers]

Created attachment 51358 [details]
Patch

[Jeremy Orlow]

(In reply to comment #19)
> Hi Jeremy, I've addressed most of your comments.  Here are my comments about
> the others:
> 
> >Always use ___Ptr's as member and automatic/local variables.  Use Pass___Ptr's
> >as method parameters and return values.  You might want to re-read the
> >PassRefPtr doc (just google that word) if you don't understand why.
> 
> I've found that this can work with RefPtr, but not with OwnPtr (won't compile).
>  It looks like in the case of OwnPtr, it's not reference counted and you can't
> assign from one OwnPtr to another.  The current code does the right thing, but
> another option is to just forget the PassOwnPtr and use a straight pointer as
> the local variable.  It gets appended to a Vector<OwnPtr<> > so the memory
> management is handled.

The point of the OwnPtr is that it'll get deleted if you go out of scope.  I don't know what you're doing wrong here, but there are a LOT of examples in the code of how to do this properly.  Make sure you have PassOwnPtr.h included in the file.  It should know how to convert stuff from an OwnPtr to a PassOwnPtr.

[Jeremy Orlow]

Comment on attachment 51358 [details]
Patch

Getting really close on RevertAccumulationBuffer and ReverbInputBuffer.  You might want to split them into their own patch to make reviewing easier.  I'll look at the other half soon.


> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbAccumulationBuffer.cpp
> @@ -0,0 +1,108 @@
> +/*
> + * 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 "ReverbAccumulationBuffer.h"
> +
> +#include "Accelerate.h"
> +
> +namespace WebCore {
> +
> +ReverbAccumulationBuffer::ReverbAccumulationBuffer(size_t length)
> +    : m_buffer(length)
> +    , m_readIndex(0)
> +    , m_readTimeFrame(0)
> +{
> +}
> +
> +void ReverbAccumulationBuffer::readAndClear(float* destination, size_t numberOfFrames)

Thanks for making these variable names better...but I think the same can be said for a lot of the variable names in these files.  Can you please do a pass?  Spell things out and err on the side of variables being too long to make thing readable.

> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolverStage.h
> @@ -0,0 +1,84 @@
> +/*
> + * 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 ReverbConvolverStage_h
> +#define ReverbConvolverStage_h
> +
> +#include "AudioFloatArray.h"
> +#include "FFTFrame.h"

You should include wtf/OwnPtr since it's used below.

> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbInputBuffer.cpp
> @@ -0,0 +1,77 @@
> +/*
> + * 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 "ReverbInputBuffer.h"
> +
> +namespace WebCore {
> +
> +ReverbInputBuffer::ReverbInputBuffer(size_t length)
> +    : m_buffer(length)
> +    , m_writeIndex(0)
> +{
> +}
> +
> +void ReverbInputBuffer::write(float* sourceP, size_t numberOfFrames)
> +{
> +    size_t bufferLength = m_buffer.size();
> +    bool isCopyGood = m_writeIndex + numberOfFrames <= bufferLength;
> +    ASSERT(isCopyGood);
> +    if (!isCopyGood)

isCopySafe seems like a better name

> +        return;
> +        
> +    memcpy(m_buffer.data() + m_writeIndex, sourceP, sizeof(float) * numberOfFrames);
> +
> +    m_writeIndex += numberOfFrames;
> +    ASSERT(m_writeIndex <= bufferLength);
> +
> +    if (m_writeIndex >= bufferLength)
> +        m_writeIndex = 0;
> +}
> +
> +float* ReverbInputBuffer::directReadFrom(int* index, size_t numberOfFrames)
> +{
> +    size_t bufferLength = m_buffer.size();
> +    bool isPtrGood = *index >= 0 && *index + numberOfFrames <= bufferLength;
> +    ASSERT(isPtrGood);
> +    float* sourceP = m_buffer;
> +    float* p = isPtrGood ? sourceP + *index : 0; // pointer should always be good - force read from 0 to expose bug otherwise

Usually you spell out words like Pointer instead of using Ptr.

If your intention is to crash the program, using CRASH() is better.

[Chris Rogers]

(In reply to comment #21)
> (In reply to comment #19)
> > Hi Jeremy, I've addressed most of your comments.  Here are my comments about
> > the others:
> > 
> > >Always use ___Ptr's as member and automatic/local variables.  Use Pass___Ptr's
> > >as method parameters and return values.  You might want to re-read the
> > >PassRefPtr doc (just google that word) if you don't understand why.
> > 
> > I've found that this can work with RefPtr, but not with OwnPtr (won't compile).
> >  It looks like in the case of OwnPtr, it's not reference counted and you can't
> > assign from one OwnPtr to another.  The current code does the right thing, but
> > another option is to just forget the PassOwnPtr and use a straight pointer as
> > the local variable.  It gets appended to a Vector<OwnPtr<> > so the memory
> > management is handled.
> 
> The point of the OwnPtr is that it'll get deleted if you go out of scope.  I
> don't know what you're doing wrong here, but there are a LOT of examples in the
> code of how to do this properly.  Make sure you have PassOwnPtr.h included in
> the file.  It should know how to convert stuff from an OwnPtr to a PassOwnPtr.

I had Dimitri take a look and he's pretty sure it's not possible to do this in this particular case.  I've created a simple test case to illustrate how this won't compile:

//~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

class Test {
public:
    Test() { printf("%p: Test::Test()\n", this); }
    ~Test() { printf("%p: Test::~Test()\n", this); }
};

Vector<OwnPtr<Test> > testList;

void test()
{
    OwnPtr<Test> p(new Test());  // <------------ WON'T COMPILE
    testList.append(p);
}

//~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

If you change the test() function to be:

void test()
{
    Test* p = new Test(); // < ---- WILL COMPILE
    testList.append(p);
}

Then it *will* compile.  The issue is that you cannot assign from one OwnPtr to another OwnPtr, and this is what is happening when you call the append() method.  Please have a close look at the exact usage in my code - thanks.

[Chris Rogers]

Created attachment 51461 [details]
Patch

[Chris Rogers]

Hi Jeremy, I addressed your last comments (see my other comment about the OwnPtr).

I took some time to scan through the files and improve all the variable names I could find.  Also, I added a few more safety checks / ASSERTs.

[Jeremy Orlow]

(In reply to comment #23)
> (In reply to comment #21)
> > (In reply to comment #19)
> > > Hi Jeremy, I've addressed most of your comments.  Here are my comments about
> > > the others:
> > > 
> > > >Always use ___Ptr's as member and automatic/local variables.  Use Pass___Ptr's
> > > >as method parameters and return values.  You might want to re-read the
> > > >PassRefPtr doc (just google that word) if you don't understand why.
> > > 
> > > I've found that this can work with RefPtr, but not with OwnPtr (won't compile).
> > >  It looks like in the case of OwnPtr, it's not reference counted and you can't
> > > assign from one OwnPtr to another.  The current code does the right thing, but
> > > another option is to just forget the PassOwnPtr and use a straight pointer as
> > > the local variable.  It gets appended to a Vector<OwnPtr<> > so the memory
> > > management is handled.
> > 
> > The point of the OwnPtr is that it'll get deleted if you go out of scope.  I
> > don't know what you're doing wrong here, but there are a LOT of examples in the
> > code of how to do this properly.  Make sure you have PassOwnPtr.h included in
> > the file.  It should know how to convert stuff from an OwnPtr to a PassOwnPtr.
> 
> I had Dimitri take a look and he's pretty sure it's not possible to do this in
> this particular case.  I've created a simple test case to illustrate how this
> won't compile:
> 
> //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> class Test {
> public:
>     Test() { printf("%p: Test::Test()\n", this); }
>     ~Test() { printf("%p: Test::~Test()\n", this); }
> };
> 
> Vector<OwnPtr<Test> > testList;
> 
> void test()
> {
>     OwnPtr<Test> p(new Test());  // <------------ WON'T COMPILE
>     testList.append(p);
> }
> 
> //~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> If you change the test() function to be:
> 
> void test()
> {
>     Test* p = new Test(); // < ---- WILL COMPILE
>     testList.append(p);
> }
> 
> Then it *will* compile.  The issue is that you cannot assign from one OwnPtr to
> another OwnPtr, and this is what is happening when you call the append()
> method.  Please have a close look at the exact usage in my code - thanks.

I see.  I guess you'd have to convert to a PassOwnPtr in the middle.

Ok, well I guess the original way you did it is good then.  Sorry for sending you on a run-around like this.

[Jeremy Orlow]

Comment on attachment 51461 [details]
Patch

Few more comments.  (Didn't have time to do the full review.  Sorry.  Hopefully I can tomorrow and we can get this finished with.


> diff --git a/WebCore/platform/audio/ReverbConvolver.cpp b/WebCore/platform/audio/ReverbConvolver.cpp
> new file mode 100644
> index 0000000..7ce7df8
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolver.cpp
> @@ -0,0 +1,223 @@
> +/*
> + * 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 "ReverbConvolver.h"
> +
> +#include "Accelerate.h"
> +#include "AudioBus.h"
> +
> +namespace WebCore {
> +
> +const int InputBufferSize = 8 * 16384;
> +
> +// We only process the leading portion of the impulse response in the real-time thread.  We don't exceed this length.
> +// It turns out then, that the background thread has about 278msec of scheduling slop.
> +// Empirically, this has been found to be a good compromise between giving enough time for scheduling slop,
> +// while still minimizing the amount of processing done in the primary (high-priority) thread.
> +// This was found to be a good value on Mac OS X, and may work well on other platforms as well, assuming
> +// the very rough scheduling latencies are similar on these time-scales.  Of course, this code may need to be
> +// tuned for individual platforms if this assumption is found to be incorrect.
> +const size_t RealtimeFrameLimit = 8192  + 4096; // ~278msec @ 44.1KHz
> +
> +const size_t MinFFTSize = 256;
> +const size_t MaxRealtimeFFTSize = 2048;
> +
> +static void* BackgroundThreadDispatch(void* threadData)

Lower case first letter.  Also I'd call it backgroundThreadEntry or backgroundThreadEntryHelper.

> +{
> +    ReverbConvolver* reverbConvolver = static_cast<ReverbConvolver*>(threadData);
> +    reverbConvolver->backgroundThreadEntry();
> +    return 0;
> +}
> +
> +ReverbConvolver::ReverbConvolver(AudioChannel* impulseResponse, size_t renderSliceSize, size_t maxFFTSize, size_t convolverRenderPhase, bool useBackgroundThreads)
> +    : m_impulseResponseLength(impulseResponse->frameSize())
> +    , m_accumulationBuffer(impulseResponse->frameSize() + renderSliceSize)
> +    , m_inputBuffer(InputBufferSize)
> +    , m_renderSliceSize(renderSliceSize)
> +    , m_minFFTSize(MinFFTSize) // First stage will have this size - successive stages will double in size each time
> +    , m_maxFFTSize(maxFFTSize) // until we hit m_maxFFTSize
> +    , m_useBackgroundThreads(useBackgroundThreads)
> +    , m_wantsToExit(false)
> +    , m_moreInputBuffered(false)
> +{
> +    // If we are using background threads then don't exceed this FFT size for the
> +    // stages which run in the real-time thread.  This avoids having only one or two
> +    // large stages (size 16384 or so) at the end which take a lot of time every several
> +    // processing slices.  This way we amortize the cost over more processing slices.
> +    m_maxRealtimeFFTSize = MaxRealtimeFFTSize;
> +
> +    // For the moment, a good way to know if we have real-time constraint is to check if we're using background threads.
> +    // Otherwise, assume we're being run from a command-line tool.
> +    bool hasRealtimeConstraint = useBackgroundThreads;
> +
> +    float* response = impulseResponse->data();
> +    size_t totalResponseLength = impulseResponse->frameSize();
> +
> +    // Because we're not using direct-convolution in the leading portion, the reverb has an overall latency of half the first-stage FFT size
> +    size_t reverbTotalLatency = m_minFFTSize / 2;
> +
> +    size_t stageOffset = 0;
> +    int i = 0;
> +    size_t fftSize = m_minFFTSize;
> +    while (stageOffset < totalResponseLength) {
> +        size_t stageSize = fftSize / 2;
> +
> +        // For the last stage, it's possible that stageOffset is such that we're straddling the end
> +        // of the impulse response buffer (if we use stageSize), so reduce the last stage's length...
> +        if (stageSize + stageOffset > totalResponseLength)
> +            stageSize = totalResponseLength - stageOffset;
> +
> +        // This "staggers" the time when each FFT happens so they don't all happen at the same time
> +        int renderPhase = convolverRenderPhase + i * renderSliceSize;
> +
> +        ReverbConvolverStage* stage = new ReverbConvolverStage(response, totalResponseLength, reverbTotalLatency, stageOffset, stageSize, fftSize, renderPhase, renderSliceSize, &m_accumulationBuffer);
> +
> +        bool isBackgroundStage = false;
> +
> +        if (stageOffset <= RealtimeFrameLimit)
> +            m_stages.append(stage);
> +        else {
> +            if (this->useBackgroundThreads()) {
> +                m_backgroundStages.append(stage);
> +                isBackgroundStage = true;
> +            } else
> +                m_stages.append(stage);
> +        }

This seems nicer:

if (this->useBackgroundThreads() && stageOffset > RealtimeFrameLimit) {
    m_backgroundStages.append(stage);
    isBackgroundStage = true;
} else
    m_stages.append(stage);

> +
> +        stageOffset += stageSize;
> +        ++i;
> +
> +        // Figure out next FFT size
> +        fftSize *= 2;
> +        if (hasRealtimeConstraint && !isBackgroundStage && fftSize > m_maxRealtimeFFTSize)
> +            fftSize = m_maxRealtimeFFTSize;
> +        if (fftSize > m_maxFFTSize)
> +            fftSize = m_maxFFTSize;
> +    }
> +
> +    // Start up background thread
> +    // FIXME: would be better to up the thread priority here.  It doesn't need to be real-time, but higher than the default...
> +    if (this->useBackgroundThreads() && m_backgroundStages.size() > 0)
> +        m_backgroundThread = createThread(BackgroundThreadDispatch, this, "convolution background thread");

Hmm...so each impulse response has its own background thread?  That seems a bit excessive, but no need to fix now I suppose.

> +    else
> +        m_backgroundThread = 0;

Don't do this here.  Instead initialize it to 0 at the beginning.  It'll then be overridden if the thread is create.

> +}
> +
> +ReverbConvolver::~ReverbConvolver()
> +{
> +    // Wait for background thread to stop
> +    if (useBackgroundThreads() && m_backgroundThread) {
> +        m_wantsToExit = true;
> +
> +        // Wake up thread so it can return - don't use MutexLocker since lock must be unlocked before we call waitForThreadCompletion().
> +        m_backgroundThreadLock.lock();
> +        m_moreInputBuffered = true;

Kill this.

Not sure if you actually need to do the locking here.  Verify what I'm saying tho...last time I tried to do anything complex with condition variables was a couple years ago.  :-)

> +        m_backgroundThreadCondition.signal();
> +        m_backgroundThreadLock.unlock();

IIRC, it's better to signal outside of the lock so that it doesn't wake up, try to get the lock, and then sleep again.

Also, use MutextLocker with {}'s to scope it.

> +
> +        waitForThreadCompletion(m_backgroundThread, 0);
> +    }
> +}
> +
> +void ReverbConvolver::backgroundThreadEntry()
> +{
> +    while (!m_wantsToExit) {
> +        // Check to see if there's any more input to consume
> +        int writeIndex = m_inputBuffer.writeIndex();
> +
> +        // Even though it doesn't seem like every stage needs to maintain its own version of readIndex 
> +        // we do this in case we want to run in more than one background thread.
> +        int readIndex;
> +
> +        while ((readIndex = m_backgroundStages[0]->inputReadIndex()) != writeIndex) { // FIXME: do better to detect buffer overrun...
> +            // FIXME: remove hard-coded value
> +            const int SliceSize = 128;
> +
> +            // Accumulate contributions from each stage
> +            for (size_t i = 0; i < m_backgroundStages.size(); ++i)
> +                m_backgroundStages[i]->processInBackground(this, SliceSize);
> +        }
> +
> +        // Wait for realtime thread to give us more input
> +        m_moreInputBuffered = false;        
> +        MutexLocker locker(m_backgroundThreadLock);
> +        while (!m_moreInputBuffered)

&& !m_wantsToExit)

> +            m_backgroundThreadCondition.wait(m_backgroundThreadLock);
> +    }
> +}
> +
> +size_t ReverbConvolver::impulseResponseLength()
> +{
> +    return m_impulseResponseLength;
> +}
> +
> +void ReverbConvolver::process(float* source, float* destination, size_t framesToProcess)
> +{
> +    bool isSafe = source && destination;
> +    ASSERT(isSafe);
> +    if (!isSafe)
> +        return;
> +
> +    // Feed input buffer (read by all threads)
> +    m_inputBuffer.write(source, framesToProcess);
> +
> +    // Accumulate contributions from each stage
> +    for (size_t i = 0; i < m_stages.size(); ++i)
> +        m_stages[i]->process(source, framesToProcess);
> +
> +    // Finally read from accumulation buffer
> +    m_accumulationBuffer.readAndClear(destination, framesToProcess);
> +        
> +    // Now that we've buffered more input, wake up our background thread.
> +    
> +    // Not using a MutexLocker looks strange, but we use a tryLock() instead because this is run on the real-time
> +    // thread where it is a disaster for the lock to be contended (causes audio glitching).  It's OK if we fail to
> +    // signal from time to time, since we'll get to it the next time we're called.  We're called repeatedly
> +    // and frequently (around every 3ms).  The background thread is processing well into the future and has a considerable amount of 
> +    // leeway here...
> +    if (m_backgroundThreadLock.tryLock()) {
> +        m_moreInputBuffered = true;
> +        m_backgroundThreadCondition.signal();
> +        m_backgroundThreadLock.unlock();

I'm pretty sure you can simply set the variable to try and send the signal (without the try lock here).  As long as the consumer has something that causes a read barrier (which I believe the mutex will) you should be OK.  It might be a good idea to write a simple app to verify all this threading stuff (whether you go with your model or my suggestion) works as expected though.  :-)

> +    }
> +}
> +
> +void ReverbConvolver::reset()
> +{
> +    for (size_t i = 0; i < m_stages.size(); ++i)
> +        m_stages[i]->reset();
> +
> +    for (size_t i = 0; i < m_backgroundStages.size(); ++i)
> +        m_backgroundStages[i]->reset();
> +
> +    m_accumulationBuffer.reset();
> +    m_inputBuffer.reset();
> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/ReverbConvolver.h b/WebCore/platform/audio/ReverbConvolver.h
> new file mode 100644
> index 0000000..0f7aeb7
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolver.h
> @@ -0,0 +1,100 @@
> +/*
> + * 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 ReverbConvolver_h
> +#define ReverbConvolver_h
> +
> +#include "AudioFloatArray.h"
> +#include "FFTConvolver.h"
> +#include "ReverbAccumulationBuffer.h"
> +#include "ReverbConvolverStage.h"
> +#include "ReverbInputBuffer.h"
> +#include <wtf/OwnPtr.h>
> +#include <wtf/RefCounted.h>
> +#include <wtf/Threading.h>
> +#include <wtf/Vector.h>
> +
> +namespace WebCore {
> +
> +class AudioChannel;
> +
> +class ReverbConvolver {
> +public:
> +    // maxFFTSize can be adjusted (from say 2048 to 32768) depending on how much precision is necessary.
> +    // For certain tweaky de-convolving applications the phase errors add up quickly and lead to non-sensical results with
> +    // larger FFT sizes and single-precision floats.  In these cases 2048 is a good size.
> +    // If not doing multi-threaded convolution, then should not go > 8192.
> +    ReverbConvolver(AudioChannel* impulseResponse, size_t renderSliceSize, size_t maxFFTSize, size_t convolverRenderPhase, bool useBackgroundThreads);
> +
> +    ~ReverbConvolver();
> +
> +    void process(float* source, float* destination, size_t framesToProcess);

nit: spaces between all of these methods seems a bit excessive.  Maybe group them together a bit?

> +    void reset();
> +
> +    size_t impulseResponseLength();
> +
> +    ReverbInputBuffer* inputBuffer() { return &m_inputBuffer; }
> +
> +    bool useBackgroundThreads() const { return m_useBackgroundThreads; }
> +
> +    void backgroundThreadEntry();
> +
> +private:
> +    Vector<OwnPtr<ReverbConvolverStage> > m_stages;
> +    Vector<OwnPtr<ReverbConvolverStage> > m_backgroundStages;
> +    size_t m_impulseResponseLength;
> +
> +    ReverbAccumulationBuffer m_accumulationBuffer;
> +
> +    // One or more background threads read from this input buffer which is fed from the realtime thread.
> +    ReverbInputBuffer m_inputBuffer;
> +
> +    // We're given 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).
> +    size_t m_renderSliceSize;
> +
> +    // First stage will be of size m_minFFTSize.  Each next stage will be twice as big until we hit m_maxFFTSize.
> +    size_t m_minFFTSize;
> +    size_t m_maxFFTSize;
> +
> +    // But don't exceed this size in the real-time thread (if we're doing background processing).
> +    size_t m_maxRealtimeFFTSize;
> +
> +    // Background thread and synchronization
> +    bool m_useBackgroundThreads;
> +    ThreadIdentifier m_backgroundThread;
> +    bool m_wantsToExit;
> +    bool m_moreInputBuffered;
> +    mutable Mutex m_backgroundThreadLock;
> +    mutable ThreadCondition m_backgroundThreadCondition;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // ReverbConvolver_h

[Jeremy Orlow]

Comment on attachment 51461 [details]
Patch

This + my other comments and this should be about ready for an r+.  Sorry it took so long.


> diff --git a/WebCore/platform/audio/ReverbConvolver.cpp b/WebCore/platform/audio/ReverbConvolver.cpp
> new file mode 100644
> index 0000000..7ce7df8
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolver.cpp
> @@ -0,0 +1,223 @@
> +/*
> + * 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 "ReverbConvolver.h"
> +
> +#include "Accelerate.h"
> +#include "AudioBus.h"
> +
> +namespace WebCore {
> +
> +const int InputBufferSize = 8 * 16384;
> +
> +// We only process the leading portion of the impulse response in the real-time thread.  We don't exceed this length.
> +// It turns out then, that the background thread has about 278msec of scheduling slop.
> +// Empirically, this has been found to be a good compromise between giving enough time for scheduling slop,
> +// while still minimizing the amount of processing done in the primary (high-priority) thread.
> +// This was found to be a good value on Mac OS X, and may work well on other platforms as well, assuming
> +// the very rough scheduling latencies are similar on these time-scales.  Of course, this code may need to be
> +// tuned for individual platforms if this assumption is found to be incorrect.
> +const size_t RealtimeFrameLimit = 8192  + 4096; // ~278msec @ 44.1KHz
> +
> +const size_t MinFFTSize = 256;
> +const size_t MaxRealtimeFFTSize = 2048;
> +
> +static void* BackgroundThreadDispatch(void* threadData)
> +{
> +    ReverbConvolver* reverbConvolver = static_cast<ReverbConvolver*>(threadData);
> +    reverbConvolver->backgroundThreadEntry();
> +    return 0;
> +}
> +
> +ReverbConvolver::ReverbConvolver(AudioChannel* impulseResponse, size_t renderSliceSize, size_t maxFFTSize, size_t convolverRenderPhase, bool useBackgroundThreads)
> +    : m_impulseResponseLength(impulseResponse->frameSize())
> +    , m_accumulationBuffer(impulseResponse->frameSize() + renderSliceSize)
> +    , m_inputBuffer(InputBufferSize)
> +    , m_renderSliceSize(renderSliceSize)
> +    , m_minFFTSize(MinFFTSize) // First stage will have this size - successive stages will double in size each time
> +    , m_maxFFTSize(maxFFTSize) // until we hit m_maxFFTSize
> +    , m_useBackgroundThreads(useBackgroundThreads)
> +    , m_wantsToExit(false)
> +    , m_moreInputBuffered(false)
> +{
> +    // If we are using background threads then don't exceed this FFT size for the
> +    // stages which run in the real-time thread.  This avoids having only one or two
> +    // large stages (size 16384 or so) at the end which take a lot of time every several
> +    // processing slices.  This way we amortize the cost over more processing slices.
> +    m_maxRealtimeFFTSize = MaxRealtimeFFTSize;
> +
> +    // For the moment, a good way to know if we have real-time constraint is to check if we're using background threads.
> +    // Otherwise, assume we're being run from a command-line tool.
> +    bool hasRealtimeConstraint = useBackgroundThreads;
> +
> +    float* response = impulseResponse->data();
> +    size_t totalResponseLength = impulseResponse->frameSize();
> +
> +    // Because we're not using direct-convolution in the leading portion, the reverb has an overall latency of half the first-stage FFT size
> +    size_t reverbTotalLatency = m_minFFTSize / 2;
> +
> +    size_t stageOffset = 0;
> +    int i = 0;
> +    size_t fftSize = m_minFFTSize;
> +    while (stageOffset < totalResponseLength) {
> +        size_t stageSize = fftSize / 2;
> +
> +        // For the last stage, it's possible that stageOffset is such that we're straddling the end
> +        // of the impulse response buffer (if we use stageSize), so reduce the last stage's length...
> +        if (stageSize + stageOffset > totalResponseLength)
> +            stageSize = totalResponseLength - stageOffset;
> +
> +        // This "staggers" the time when each FFT happens so they don't all happen at the same time
> +        int renderPhase = convolverRenderPhase + i * renderSliceSize;
> +
> +        ReverbConvolverStage* stage = new ReverbConvolverStage(response, totalResponseLength, reverbTotalLatency, stageOffset, stageSize, fftSize, renderPhase, renderSliceSize, &m_accumulationBuffer);

If you do keep this a raw pointer keep it as close as possible to where it gets assigned to an OwnPtr.

And, not to beat a dead horse, but did you try making this an OwnPtr and then call .release() on that when passing it to the array?  I think that'd work and I think it'd be the best.

> +
> +        bool isBackgroundStage = false;
> +
> +        if (stageOffset <= RealtimeFrameLimit)
> +            m_stages.append(stage);
> +        else {
> +            if (this->useBackgroundThreads()) {
> +                m_backgroundStages.append(stage);
> +                isBackgroundStage = true;
> +            } else
> +                m_stages.append(stage);
> +        }
> +
> +        stageOffset += stageSize;
> +        ++i;
> +
> +        // Figure out next FFT size
> +        fftSize *= 2;
> +        if (hasRealtimeConstraint && !isBackgroundStage && fftSize > m_maxRealtimeFFTSize)
> +            fftSize = m_maxRealtimeFFTSize;
> +        if (fftSize > m_maxFFTSize)
> +            fftSize = m_maxFFTSize;
> +    }
> +
> +    // Start up background thread
> +    // FIXME: would be better to up the thread priority here.  It doesn't need to be real-time, but higher than the default...
> +    if (this->useBackgroundThreads() && m_backgroundStages.size() > 0)
> +        m_backgroundThread = createThread(BackgroundThreadDispatch, this, "convolution background thread");
> +    else
> +        m_backgroundThread = 0;
> +}
> +
> +ReverbConvolver::~ReverbConvolver()
> +{
> +    // Wait for background thread to stop
> +    if (useBackgroundThreads() && m_backgroundThread) {
> +        m_wantsToExit = true;
> +
> +        // Wake up thread so it can return - don't use MutexLocker since lock must be unlocked before we call waitForThreadCompletion().
> +        m_backgroundThreadLock.lock();
> +        m_moreInputBuffered = true;
> +        m_backgroundThreadCondition.signal();
> +        m_backgroundThreadLock.unlock();
> +
> +        waitForThreadCompletion(m_backgroundThread, 0);
> +    }
> +}
> +
> +void ReverbConvolver::backgroundThreadEntry()
> +{
> +    while (!m_wantsToExit) {
> +        // Check to see if there's any more input to consume
> +        int writeIndex = m_inputBuffer.writeIndex();
> +
> +        // Even though it doesn't seem like every stage needs to maintain its own version of readIndex 
> +        // we do this in case we want to run in more than one background thread.
> +        int readIndex;
> +
> +        while ((readIndex = m_backgroundStages[0]->inputReadIndex()) != writeIndex) { // FIXME: do better to detect buffer overrun...
> +            // FIXME: remove hard-coded value

Why is this hard coded?  Is removing it an optimization (fine to leave for now, but describe more) or code cleanup (probably best to just do now if possible...otherwise describe why not done now in fixme.)

> +            const int SliceSize = 128;
> +
> +            // Accumulate contributions from each stage
> +            for (size_t i = 0; i < m_backgroundStages.size(); ++i)
> +                m_backgroundStages[i]->processInBackground(this, SliceSize);
> +        }
> +
> +        // Wait for realtime thread to give us more input
> +        m_moreInputBuffered = false;        
> +        MutexLocker locker(m_backgroundThreadLock);
> +        while (!m_moreInputBuffered)
> +            m_backgroundThreadCondition.wait(m_backgroundThreadLock);

Shouldn't this while loop go at the beginning??  You can scope the MutexLocker with {}'s.

> +    }
> +}
> +
> +size_t ReverbConvolver::impulseResponseLength()
> +{
> +    return m_impulseResponseLength;

Can probably be const and inlined.

> +}
> +
> +void ReverbConvolver::process(float* source, float* destination, size_t framesToProcess)
> +{
> +    bool isSafe = source && destination;
> +    ASSERT(isSafe);
> +    if (!isSafe)
> +        return;
> +
> +    // Feed input buffer (read by all threads)
> +    m_inputBuffer.write(source, framesToProcess);
> +
> +    // Accumulate contributions from each stage
> +    for (size_t i = 0; i < m_stages.size(); ++i)
> +        m_stages[i]->process(source, framesToProcess);
> +
> +    // Finally read from accumulation buffer
> +    m_accumulationBuffer.readAndClear(destination, framesToProcess);
> +        
> +    // Now that we've buffered more input, wake up our background thread.
> +    
> +    // Not using a MutexLocker looks strange, but we use a tryLock() instead because this is run on the real-time
> +    // thread where it is a disaster for the lock to be contended (causes audio glitching).  It's OK if we fail to
> +    // signal from time to time, since we'll get to it the next time we're called.  We're called repeatedly
> +    // and frequently (around every 3ms).  The background thread is processing well into the future and has a considerable amount of 
> +    // leeway here...
> +    if (m_backgroundThreadLock.tryLock()) {
> +        m_moreInputBuffered = true;
> +        m_backgroundThreadCondition.signal();
> +        m_backgroundThreadLock.unlock();
> +    }
> +}
> +
> +void ReverbConvolver::reset()
> +{
> +    for (size_t i = 0; i < m_stages.size(); ++i)
> +        m_stages[i]->reset();
> +
> +    for (size_t i = 0; i < m_backgroundStages.size(); ++i)
> +        m_backgroundStages[i]->reset();
> +
> +    m_accumulationBuffer.reset();
> +    m_inputBuffer.reset();
> +}
> +
> +} // namespace WebCore

> diff --git a/WebCore/platform/audio/ReverbConvolverStage.cpp b/WebCore/platform/audio/ReverbConvolverStage.cpp
> new file mode 100644
> index 0000000..f687753
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolverStage.cpp
> @@ -0,0 +1,164 @@
> +/*
> + * 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 "ReverbConvolverStage.h"
> +
> +#include "Accelerate.h"
> +#include "ReverbAccumulationBuffer.h"
> +#include "ReverbConvolver.h"
> +#include "ReverbInputBuffer.h"
> +#include <wtf/OwnPtr.h>
> +#include <wtf/PassOwnPtr.h>
> +
> +namespace WebCore {
> +
> +ReverbConvolverStage::ReverbConvolverStage(float* impulseResponse, size_t responseLength, size_t reverbTotalLatency, size_t stageOffset, size_t stageLength,
> +                                           size_t fftSize, size_t renderPhase, size_t renderSliceSize, ReverbAccumulationBuffer* accumulationBuffer)
> +    : m_fftKernel(fftSize)
> +    , m_accumulationBuffer(accumulationBuffer)
> +    , m_accumulationReadIndex(0)
> +    , m_inputReadIndex(0)
> +    , m_impulseResponseLength(responseLength)
> +{
> +    ASSERT(impulseResponse);
> +    ASSERT(accumulationBuffer);
> +    
> +    m_fftKernel.doPaddedFFT(impulseResponse + stageOffset, stageLength);
> +
> +    m_convolver = new FFTConvolver(fftSize);
> +
> +    m_temporaryBuffer.allocate(renderSliceSize);
> +
> +    // The convolution stage at offset stageOffset needs to have a corresponding delay to cancel out the offset.
> +    size_t totalDelay = stageOffset + reverbTotalLatency;
> +
> +

Tidy up whitespace a bit....no 2 lines here...some of the lines above can maybe be condensed a bit...etc

> +    // But, the FFT convolution itself incurs fftSize / 2 latency, so subtract this out...
> +    size_t halfSize = fftSize / 2;
> +    ASSERT(totalDelay >= halfSize);
> +    if (totalDelay >= halfSize)
> +        totalDelay -= halfSize;
> +
> +    // We divide up the total delay, into pre and post delay sections so that we can schedule at exactly the moment when the FFT will happen.
> +    // This is coordinated with the other stages, so they don't all do their FFTs at the same time...
> +

Maybe delete this newline.

> +    int maxPreDelayLength = std::min(halfSize, totalDelay);
> +    m_preDelayLength = totalDelay > 0 ? renderPhase % maxPreDelayLength : 0;
> +
> +    if (m_preDelayLength > totalDelay)
> +        m_preDelayLength = 0;
> +
> +    m_postDelayLength = totalDelay - m_preDelayLength;
> +    m_preReadWriteIndex = 0;
> +    m_framesProcessed = 0; // total frames processed so far
> +
> +    m_preDelayBuffer.allocate(m_preDelayLength < fftSize ? fftSize : m_preDelayLength);
> +}
> +
> +void ReverbConvolverStage::processInBackground(ReverbConvolver* convolver, size_t framesToProcess)
> +{
> +    ReverbInputBuffer* inputBuffer = convolver->inputBuffer();
> +    float* source = inputBuffer->directReadFrom(&m_inputReadIndex, framesToProcess);
> +    process(source, framesToProcess);
> +}
> +
> +void ReverbConvolverStage::process(float* source, size_t framesToProcess)
> +{
> +    ASSERT(source);
> +    if (!source)
> +        return;
> +    
> +    // Deal with pre-delay stream : note special handling of zero delay.
> +
> +    float* preDelayedSource;
> +    float* temporaryBuffer;
> +    if (m_preDelayLength > 0) {
> +        // Handles both the read case (call to process() ) and the write case (memcpy() )
> +        bool isPreDelaySafe = m_preReadWriteIndex + framesToProcess <= m_preDelayBuffer.size();
> +        ASSERT(isPreDelaySafe);
> +        if (!isPreDelaySafe)
> +            return;
> +
> +        bool isTemporaryBufferSafe = framesToProcess <= m_temporaryBuffer.size();
> +        ASSERT(isTemporaryBufferSafe);
> +        if (!isTemporaryBufferSafe)
> +            return;

This can be pulled out of the if statement and put below.

> +
> +        preDelayedSource = m_preDelayBuffer.data() + m_preReadWriteIndex;
> +        temporaryBuffer = m_temporaryBuffer;        
> +    } else {
> +        // Zero delay
> +        preDelayedSource = source;
> +        temporaryBuffer = m_preDelayBuffer.data();
> +        
> +        bool isTemporaryBufferSafe = framesToProcess <= m_preDelayBuffer.size();
> +        ASSERT(isTemporaryBufferSafe);
> +        if (!isTemporaryBufferSafe)
> +            return;
> +    }
> +
> +    int writeIndex = 0;
> +
> +    if (m_framesProcessed < m_preDelayLength) {
> +        // For the first m_preDelayLength frames don't process the convolver, instead simply buffer in the pre-delay.
> +        // But while buffering the pre-delay, we still need to update our index.
> +        m_accumulationBuffer->updateReadIndex(&m_accumulationReadIndex, framesToProcess);
> +    } else {
> +        // Now, run the convolution (into the delay buffer).
> +        // An expensive FFT will happen every fftSize / 2 frames.
> +        // We process in-place here...
> +        m_convolver->process(&m_fftKernel, preDelayedSource, temporaryBuffer, framesToProcess);
> +
> +        // Now accumulate into reverb's accumulation buffer.
> +        writeIndex = m_accumulationBuffer->accumulate(temporaryBuffer, framesToProcess, &m_accumulationReadIndex, m_postDelayLength);
> +    }
> +
> +    // Finally copy input to pre-delay.
> +    if (m_preDelayLength > 0) {
> +        memcpy(preDelayedSource, source, sizeof(float) * framesToProcess);
> +        m_preReadWriteIndex += framesToProcess;
> +
> +        ASSERT(m_preReadWriteIndex <= m_preDelayLength);
> +        if (m_preReadWriteIndex >= m_preDelayLength)
> +            m_preReadWriteIndex = 0;
> +    }
> +
> +    m_framesProcessed += framesToProcess;
> +}
> +
> +void ReverbConvolverStage::reset()
> +{
> +    m_convolver->reset();
> +    m_preDelayBuffer.zero();
> +    m_accumulationReadIndex = 0;
> +    m_inputReadIndex = 0;
> +    m_framesProcessed = 0;
> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/ReverbConvolverStage.h b/WebCore/platform/audio/ReverbConvolverStage.h
> new file mode 100644
> index 0000000..88351af
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbConvolverStage.h
> @@ -0,0 +1,83 @@
> +/*
> + * 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 ReverbConvolverStage_h
> +#define ReverbConvolverStage_h
> +
> +#include "AudioFloatArray.h"
> +#include "FFTFrame.h"
> +#include <wtf/OwnPtr.h>
> +
> +namespace WebCore {
> +
> +class ReverbAccumulationBuffer;
> +class ReverbConvolver;
> +class FFTConvolver;
> +    
> +// A ReverbConvolverStage represents the convolution associated with a sub-section of a large impulse response.
> +// It incorporates a delay line to account for the offset of the sub-section within the larger impulse response.
> +class ReverbConvolverStage {
> +public:
> +    // renderPhase is useful to know so that we can manipulate the pre versus post delay so that stages will perform
> +    // their heavy work (FFT processing) on different slices to balance the load in a real-time thread.
> +    ReverbConvolverStage(float* impulseResponse, size_t responseLength, size_t reverbTotalLatency, size_t stageOffset, size_t stageLength,
> +                         size_t fftSize, size_t renderPhase, size_t renderSliceSize, ReverbAccumulationBuffer* accumulationBuffer);
> +
> +    // WARNING: framesToProcess must be such that it evenly divides the delay buffer size (stage_offset).
> +    void process(float* source, size_t framesToProcess);
> +
> +    void processInBackground(ReverbConvolver* convolver, size_t framesToProcess);
> +
> +    void reset();
> +
> +    // Useful for background processing
> +    int inputReadIndex() const { return m_inputReadIndex; }
> +
> +private:
> +    FFTFrame m_fftKernel;
> +    OwnPtr<FFTConvolver> m_convolver;
> +
> +    AudioFloatArray m_preDelayBuffer;
> +
> +    ReverbAccumulationBuffer* m_accumulationBuffer;

Gotta be careful about lifetime issues with stuff like this, but what you've got looks good.  It's possible the destructor would kill the RevertAccumulationBuffer first, but there should never be any ReverbConvolverStage code running at that time, and this class should be deleted in the same destructor.

> +    int m_accumulationReadIndex;
> +    int m_inputReadIndex;
> +
> +    size_t m_preDelayLength;
> +    size_t m_postDelayLength;
> +    size_t m_preReadWriteIndex;
> +    size_t m_framesProcessed;
> +
> +    AudioFloatArray m_temporaryBuffer;
> +
> +    size_t m_impulseResponseLength;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // ReverbConvolverStage_h
> diff --git a/WebCore/platform/audio/ReverbInputBuffer.cpp b/WebCore/platform/audio/ReverbInputBuffer.cpp
> new file mode 100644
> index 0000000..1c4fb71
> --- /dev/null
> +++ b/WebCore/platform/audio/ReverbInputBuffer.cpp
> @@ -0,0 +1,79 @@
> +/*
> + * 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 "ReverbInputBuffer.h"
> +
> +namespace WebCore {
> +
> +ReverbInputBuffer::ReverbInputBuffer(size_t length)
> +    : m_buffer(length)
> +    , m_writeIndex(0)
> +{
> +}
> +
> +void ReverbInputBuffer::write(float* sourceP, size_t numberOfFrames)
> +{
> +    size_t bufferLength = m_buffer.size();
> +    bool isCopySafe = m_writeIndex + numberOfFrames <= bufferLength;
> +    ASSERT(isCopySafe);
> +    if (!isCopySafe)
> +        return;
> +        
> +    memcpy(m_buffer.data() + m_writeIndex, sourceP, sizeof(float) * numberOfFrames);
> +
> +    m_writeIndex += numberOfFrames;
> +    ASSERT(m_writeIndex <= bufferLength);
> +
> +    if (m_writeIndex >= bufferLength)
> +        m_writeIndex = 0;
> +}
> +
> +float* ReverbInputBuffer::directReadFrom(int* readIndex, size_t numberOfFrames)
> +{
> +    size_t bufferLength = m_buffer.size();
> +    bool isPointerGood = *readIndex >= 0 && *readIndex + numberOfFrames <= bufferLength;
> +    if (!isPointerGood)
> +        CRASH();

What would happen if you just returned here?  Maybe you could just fill in zeros after asserting?

> +        
> +    float* sourceP = m_buffer;
> +    float* p = sourceP + *readIndex;
> +
> +    // Update readIndex
> +    *readIndex = (*readIndex + numberOfFrames) % bufferLength;
> +
> +    return p;
> +}
> +
> +void ReverbInputBuffer::reset()
> +{
> +    m_buffer.zero();
> +    m_writeIndex = 0;
> +}
> +
> +} // namespace WebCore

[Chris Rogers]

Created attachment 51964 [details]
Patch

[Chris Rogers]

Hi Jeremy, hopefully my latest changes address the last of your comments.

> Hmm...so each impulse response has its own background thread?  That seems a bit excessive, but no need to fix now I suppose.

Good observation.  It's actually a good thing if the machine has enough cores to really get the advantage of the other threads.  On the Mac on Snow Leopard, it might be possible to take advantage of "Grand Central Dispatch", but otherwise it would be possible to check the number of cores on the machine and try to optimally farm out worker threads to best match the number of cores.  But this is an optimization which isn't critical to the fairly good performance we're already seeing...

> IIRC, it's better to signal outside of the lock so that it doesn't wake up, try
> to get the lock, and then sleep again.
> Also, use MutextLocker with {}'s to scope it.

I'm pretty sure it has to be signaled inside the lock which is the pattern I've seen in other code like this, and also seems to be the pattern used elsewhere in WebKit.

> I'm pretty sure you can simply set the variable to try and send the signal
> (without the try lock here).  As long as the consumer has something that causes
> a read barrier (which I believe the mutex will) you should be OK.  It might be
> a good idea to write a simple app to verify all this threading stuff (whether
> you go with your model or my suggestion) works as expected though.  :-)

I think there are some race conditions which can happen if you don't actually get the lock here.

> If you do keep this a raw pointer keep it as close as possible to where it gets
> assigned to an OwnPtr.
> And, not to beat a dead horse, but did you try making this an OwnPtr and then
> call .release() on that when passing it to the array?  I think that'd work and
> I think it'd be the best.

Good call - this seems to work and be the best solution...

[Chris Rogers]

Created attachment 51975 [details]
Patch

[Jeremy Orlow]

> > IIRC, it's better to signal outside of the lock so that it doesn't wake up, try
> > to get the lock, and then sleep again.
> > Also, use MutextLocker with {}'s to scope it.
> 
> I'm pretty sure it has to be signaled inside the lock which is the pattern I've
> seen in other code like this, and also seems to be the pattern used elsewhere
> in WebKit.

Doing so seems always safe, so I'm fine leaving things as is and not trying to over-optimize.

> 
> > I'm pretty sure you can simply set the variable to try and send the signal
> > (without the try lock here).  As long as the consumer has something that causes
> > a read barrier (which I believe the mutex will) you should be OK.  It might be
> > a good idea to write a simple app to verify all this threading stuff (whether
> > you go with your model or my suggestion) works as expected though.  :-)
> 
> I think there are some race conditions which can happen if you don't actually
> get the lock here.

Sure, but what you have right now is a race condition as well.  That said, I guess I don't think making a race condition more subtle is much help.

Are you sure you're not being overly paranoid about the lock?  I suppose it's possible that one of the background threads could take the lock and then do a context switch in the couple instructions before the lock is given up, but that seems fairly unlikely.  And modern OSs have heuristics to try and avoid stuff like that, typically.

[Jeremy Orlow]

Comment on attachment 51975 [details]
Patch

r=me but this should go to the branch not trunk

[Eric Seidel (no email)]

Any update?  Should this still be r+ in the pending-commit list?

[Jeremy Orlow]

Chris is on vacation.

[Chris Rogers]

(In reply to comment #35)
> Chris is on vacation.

just got back - will soon check lots of stuff into a branch, then we can sort out landing this

[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 #37)
> Was this landed in the big audio branch landing?  If so, please close so it's
> not in webkit.org/pending-commit anymore.

This still needs to land in trunk.

[Adam Barth]

Comment on attachment 51975 [details]
Patch

Removing the r+ until you all sort out whether this is landing on trunk or on branch.

[Chris Rogers]

Created attachment 65615 [details]
Patch

[Chris Rogers]

This has already been R+ by Jeremy Orlow.  This new patch introduces the WEB_AUDIO feature enable.

[Kenneth Russell]

Comment on attachment 65615 [details]
Patch

I skimmed briefly through this but am r+'ing based on earlier review.

[Chris Rogers]

Comment on attachment 65615 [details]
Patch

Clearing flags on attachment: 65615

Committed r66419: <http://trac.webkit.org/changeset/66419>

[Chris Rogers]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66419 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/66418
http://trac.webkit.org/changeset/66419
http://trac.webkit.org/changeset/66420
http://trac.webkit.org/changeset/66421