[webkit-reviews] review denied: [Bug 34912] audio engine: add ReverbConvolver class : [Attachment 48676] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Fri Mar 12 04:43:35 PST 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 48676: Patch
https://bugs.webkit.org/attachment.cgi?id=48676&action=review
------- Additional Comments from Jeremy Orlow <jorlow at chromium.org>
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
More information about the webkit-reviews
mailing list