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

[bugzilla-daemon at webkit.org]

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 50957: Patch
https://bugs.webkit.org/attachment.cgi?id=50957&action=review

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