[Webkit-unassigned] [Bug 36466] audio engine: add Reverb class
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Mar 23 12:30:15 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=36466
Jeremy Orlow <jorlow at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #51369|review? |review-
Flag| |
--- Comment #4 from Jeremy Orlow <jorlow at chromium.org> 2010-03-23 12:30:15 PST ---
(From update of attachment 51369)
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 1f2d874..5761d36 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,23 @@
> +2010-03-22 Chris Rogers <crogers at google.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + audio engine: add Reverb class
> + https://bugs.webkit.org/show_bug.cgi?id=36466
> +
> + No tests since not yet connected to javascript API
> +
> + * platform/audio/Reverb.cpp: Added.
> + (WebCore::calculateNormalizationScale):
> + (WebCore::normalizeResponse):
> + (WebCore::Reverb::Reverb):
> + (WebCore::Reverb::initialize):
> + (WebCore::Reverb::process):
> + (WebCore::Reverb::reset):
> + (WebCore::Reverb::impulseResponseLength):
> + * platform/audio/Reverb.h: Added.
> + (WebCore::Reverb::):
> +
> 2010-03-05 Csaba Osztrogonác <ossy at webkit.org>
>
> Unreviewed buildfix after r55593. (To fix Qt --minimal build.)
> diff --git a/WebCore/platform/audio/Reverb.cpp b/WebCore/platform/audio/Reverb.cpp
> new file mode 100644
> index 0000000..2d163ba
> --- /dev/null
> +++ b/WebCore/platform/audio/Reverb.cpp
> @@ -0,0 +1,201 @@
> +/*
> + * 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 "Reverb.h"
> +
> +#include "AudioBus.h"
> +#include "AudioFileReader.h"
> +#include "ReverbConvolver.h"
> +#include <math.h>
> +#include <wtf/OwnPtr.h>
> +#include <wtf/PassOwnPtr.h>
> +
> +namespace WebCore {
> +
> +static double calculateNormalizationScale(AudioBus* response)
> +{
> + // Normalize by RMS power
> + size_t numberOfChannels = response->numberOfChannels();
> + size_t frameSize = response->frameSize();
> +
> + double power = 0.0;
> +
> + for (size_t i = 0; i < numberOfChannels; ++i) {
> + int n = frameSize;
> + float* p = response->channel(i)->data();
> +
> + while (n--) {
> + float sample = *p++;
> + power += sample * sample;
> + }
> + }
> +
> + power = sqrt(power / (numberOfChannels * frameSize));
> +
> + // Protect against accidental overload
> + const double MinPower = 0.000125;
> + if (std::isinf(power) || std::isnan(power) || power < MinPower)
Is this a normal code path? If a developer hit this, it seems like it might be
worth asserting, though we should definitely still handle the case gracefully.
> + power = MinPower;
> +
> + double scale = 1.0 / power;
> +
> + // Empirical gain calibration tested across many impulse responses to ensure perceived volume is same as dry (unprocessed) signal
> + scale *= pow(10.0, -58.0 * 0.05); // -58dB gain reduction
Constants like this should probably be put at the header of the file.
> +
> + // True-stereo compensation
> + if (response->numberOfChannels() == 4)
> + scale *= 0.5;
> +
> + return scale;
> +}
> +
> +static void normalizeResponse(AudioBus* response)
> +{
> + double scale = calculateNormalizationScale(response);
> + response->scale(scale);
> +}
> +
> +Reverb::Reverb(const char* filePath, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, double sampleRate, bool useBackgroundThreads)
> +{
> + OwnPtr<AudioBus> impulseResponse = createBusFromAudioFile(filePath, false, sampleRate);
> + normalizeResponse(impulseResponse.get());
> +
> + initialize(impulseResponse.get(), renderSliceSize, maxFFTSize, numberOfChannels, useBackgroundThreads);
> +}
> +
> +Reverb::Reverb(AudioBus* impulseResponse, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, bool useBackgroundThreads)
> +{
> + double scale = calculateNormalizationScale(impulseResponse);
> + if (scale)
Why if (scale)? If anything shouldn't this check to see if it's 1.0?
> + impulseResponse->scale(scale);
> +
> + initialize(impulseResponse, renderSliceSize, maxFFTSize, numberOfChannels, useBackgroundThreads);
> +
> + // Undo scaling since this shouldn't be a destructive operation on impulseResponse
> + if (scale)
> + impulseResponse->scale(1.0 / scale);
> +}
> +
> +void Reverb::initialize(AudioBus* impulseResponseBuffer, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, bool useBackgroundThreads)
> +{
> + m_impulseResponseLength = impulseResponseBuffer->frameSize();
> +
> + // The reverb can handle a mono impulse response and still do stereo processing
> + size_t numResponseChannels = impulseResponseBuffer->numberOfChannels();
> + m_convolvers.reserveCapacity(numberOfChannels);
> +
> + int convolverRenderPhase = 0;
> + for (size_t i = 0; i < numResponseChannels; ++i) {
> + AudioChannel* channel = impulseResponseBuffer->channel(i);
> +
> + ReverbConvolver* convolver = new ReverbConvolver(channel, renderSliceSize, maxFFTSize, convolverRenderPhase, useBackgroundThreads);
> + m_convolvers.append(convolver);
> +
> + convolverRenderPhase += renderSliceSize;
> + }
> +
> + // For "True" stereo processing
Can you add a bit more of a comment about why this is needed? Is it just an
optimization so you don't keep allocating and deallocating memory?
> + if (numResponseChannels == 4)
> + m_tempBuffer = new AudioBus(2, MaxFrameSize);
> +}
> +
> +void Reverb::process(AudioBus* sourceBus, AudioBus* destinationBus, size_t framesToProcess)
> +{
> + bool isSafeToProcess = sourceBus && destinationBus && framesToProcess <= MaxFrameSize && framesToProcess <= sourceBus->frameSize() && framesToProcess <= destinationBus->frameSize();
> +
> + ASSERT(isSafeToProcess);
> + if (!isSafeToProcess)
> + return;
> +
> + // For now only handle mono or stereo output
> + if (destinationBus->numberOfChannels() > 2)
> + return;
> +
> + float* destinationL = destinationBus->channel(0)->data();
> + float* sourceL = sourceBus->channel(0)->data();
> +
> + // Handle input -> output matrixing...
> + size_t numInputChannels = sourceBus->numberOfChannels();
> + size_t numOutputChannels = destinationBus->numberOfChannels();
> + size_t numReverbChannels = m_convolvers.size();
> +
> + // 2 -> 2 -> 2
> + if (numInputChannels == 2 && numReverbChannels == 2 && numOutputChannels == 2) {
I almost wonder if you should handle the various cases with just an enum...
I.e. have various modes. I'm not sure how general purpose this code really is.
> + float* sourceR = sourceBus->channel(1)->data();
> + float* destinationR = destinationBus->channel(1)->data();
> + m_convolvers[0]->process(sourceL, destinationL, framesToProcess);
> + m_convolvers[1]->process(sourceR, destinationR, framesToProcess);
> + } else if (numInputChannels == 1 && numOutputChannels == 2 && numReverbChannels == 2) {
> + // 1 -> 2 -> 2
> + for (int i = 0; i < 2; ++i) {
> + float* destinationP = destinationBus->channel(i)->data();
> + m_convolvers[i]->process(sourceL, destinationP, framesToProcess);
> + }
> + } else if (numInputChannels == 1 && numReverbChannels == 1 && numOutputChannels == 2) {
> + // 1 -> 1 -> 2
> + m_convolvers[0]->process(sourceL, destinationL, framesToProcess);
> +
> + // simply copy L -> R
> + float* destinationR = destinationBus->channel(1)->data();
> + memcpy(destinationR, destinationL, sizeof(float) * framesToProcess);
> + } else if (numInputChannels == 1 && numReverbChannels == 1 && numOutputChannels == 1)
Since this is multiple lines (with the comment) I think it's proper to put {}'s
around it.
> + // 1 -> 1 -> 1
> + m_convolvers[0]->process(sourceL, destinationL, framesToProcess);
> + else if (numInputChannels == 2 && numReverbChannels == 4 && numOutputChannels == 2) {
> + // 2 -> 4 -> 2 ("True" stereo)
> + float* sourceR = sourceBus->channel(1)->data();
> + float* destinationR = destinationBus->channel(1)->data();
All of this stuff scares me...does WebKit have an equivilent of a CHECK (a
DCHECK that fires even in release)? If not, can we add one? And then whenever
you do anything like this, can you check that what's happening is safe. Also,
ideally I'd like to pass around real objects (that can check their bounds and
such) as much as possible and avoid this raw pointer stuff.
> +
> + float* tempL = m_tempBuffer->channel(0)->data();
> + float* tempR = m_tempBuffer->channel(1)->data();
> +
> + // Process left virtual source
> + m_convolvers[0]->process(sourceL, destinationL, framesToProcess);
> + m_convolvers[1]->process(sourceL, destinationR, framesToProcess);
> +
> + // Process right virtual source
> + m_convolvers[2]->process(sourceR, tempL, framesToProcess);
> + m_convolvers[3]->process(sourceR, tempR, framesToProcess);
> +
> + destinationBus->sumFrom(*m_tempBuffer);
> + }
Maybe have an else with an ASSERT(0) or something? (IIRC, there's a macro for
this, but I can't remember what it is...maybe take a look for it?)
> +}
> +
> +void Reverb::reset()
> +{
> + for (size_t i = 0; i < m_convolvers.size(); ++i)
> + m_convolvers[i]->reset();
> +}
> +
> +size_t Reverb::impulseResponseLength() const
> +{
> + return m_impulseResponseLength;
Could have been done inline if you wanted.
> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/Reverb.h b/WebCore/platform/audio/Reverb.h
> new file mode 100644
> index 0000000..4cfa659
> --- /dev/null
> +++ b/WebCore/platform/audio/Reverb.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 Reverb_h
> +#define Reverb_h
> +
> +#include "AudioBus.h"
> +#include "ReverbConvolver.h"
These two could be just forward declared if you declared the destructor and
implemented it in a .cpp file (that does include them).
> +#include <wtf/Vector.h>
> +
> +namespace WebCore {
> +
> +// Multi-channel convolution reverb with channel matrixing - one or more ReverbConvolver objects are used internally.
> +
> +class Reverb {
> +public:
> + enum {MaxFrameSize = 256};
I believe a space after and before the { and } is the norm. Actually, it seems
like usually these are static const variables, but the enum method seems like a
good one.
> +
> + // renderSliceSize is 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).
> + Reverb(AudioBus* impulseResponseBuffer, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, bool useBackgroundThreads);
> + Reverb(const char* filePath, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, double sampleRate, bool useBackgroundThreads);
How are file paths expressed elsewhere? I doubt via const char* strings.
Not sure size_t is the right way to express number of channels.
Kinda weird that your sample rate is not an integer...but I don't really care.
> +
> + virtual void process(AudioBus* sourceBus, AudioBus* destinationBus, size_t framesToProcess);
> + virtual void reset();
Why virtual?
> +
> + size_t impulseResponseLength() const;
> +
> +private:
> + void initialize(AudioBus* impulseResponseBuffer, size_t renderSliceSize, size_t maxFFTSize, size_t numberOfChannels, bool useBackgroundThreads);
> +
> + size_t m_impulseResponseLength;
> +
> + Vector<OwnPtr<ReverbConvolver> > m_convolvers;
> +
> + // For "True" stereo processing
> + OwnPtr<AudioBus> m_tempBuffer;
> +};
> +
> +} // namespace WebCore
> +
> +#endif // Reverb_h
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list