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