[webkit-reviews] review denied: [Bug 34912] audio engine: add ReverbConvolver class : [Attachment 50749] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Mar 17 09:47:24 PDT 2010


Jeremy Orlow <jorlow at chromium.org> has denied Chris Rogers
<crogers at google.com>'s request for review:
Bug 34912: audio engine: add ReverbConvolver class
https://bugs.webkit.org/show_bug.cgi?id=34912

Attachment 50749: Patch
https://bugs.webkit.org/attachment.cgi?id=50749&action=review

------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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


More information about the webkit-reviews mailing list