[webkit-reviews] review denied: [Bug 34827] audio engine: add FFTFrame files : [Attachment 66283] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 2 18:29:18 PDT 2010
Kenneth Russell <kbr at google.com> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 34827: audio engine: add FFTFrame files
https://bugs.webkit.org/show_bug.cgi?id=34827
Attachment 66283: Patch
https://bugs.webkit.org/attachment.cgi?id=66283&action=review
------- Additional Comments from Kenneth Russell <kbr at google.com>
I reorganized the files below to put the header first.
General comment: as in the other files, you really should settle on either
floats or doubles throughout. Since it looks like your audio streams are all
floats, it seems natural that everything should stick to floats except if there
is some precision or range problem with doing so.
> Index: WebCore/platform/audio/FFTFrame.h
> ===================================================================
> --- WebCore/platform/audio/FFTFrame.h (revision 0)
> +++ WebCore/platform/audio/FFTFrame.h (revision 0)
> @@ -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.
> + */
> +
> +#ifndef FFTFrame_h
> +#define FFTFrame_h
> +
> +#include "AudioFloatArray.h"
> +
> +#if OS(DARWIN)
> +#include <Accelerate/Accelerate.h>
> +#endif
> +
> +#include <wtf/PassOwnPtr.h>
> +#include <wtf/Platform.h>
> +
> +namespace WebCore {
> +
> +// Defines the interface for an "FFT frame", an object which is able to
perform a forward
> +// and reverse FFT, internally storing the resultant frequency-domain data.
> +
> +class FFTFrame {
> +public:
It's hard to figure out which functions are implemented by cross-platform code
in platform/audio/FFTFrame.cpp and which require platform-dependent
implementations. Please add a comment to the class indicating that all methods
must be implemented with platform-dependent code except where indicated. Then
doPaddedFFT, createInterpolatedFrame, interpolateFrequencyComponents,
extractAverageGroupDelay, addConstantGroupDelay, and print can be marked as
being platform-independent in the header.
> + FFTFrame(unsigned fftSize);
> + FFTFrame(); // creates a blank/empty frame for later use with
createInterpolatedFrame()
> + FFTFrame(const FFTFrame& frame);
> + ~FFTFrame();
> +
> + static void cleanup();
There's a better way to handle the global cleanup problem. See below.
> +
> + void doFFT(float* data);
> + void doPaddedFFT(float* data, size_t dataSize); // zero-padding with
dataSize <= fftSize
> + void doInverseFFT(float* data);
> +
> + // Multiplies ourself with frame : effectively operator*=()
> + void multiply(const FFTFrame& frame);
> +
> + // Interpolates from frame1 -> frame2 as x goes from 0.0 -> 1.0
> + static PassOwnPtr<FFTFrame> createInterpolatedFrame(const FFTFrame&
frame1, const FFTFrame& frame2, double x);
> +
> + double extractAverageGroupDelay();
> + void addConstantGroupDelay(double sampleFrameDelay);
> +
> + unsigned fftSize() const { return m_FFTSize; }
> + unsigned log2FFTSize() const { return m_log2FFTSize; }
> +
> + float* realData() const;
> + float* imagData() const;
> +
> + // For debugging
> + void print();
> +
> +private:
> + unsigned m_FFTSize;
> + unsigned m_log2FFTSize;
> +
> + void interpolateFrequencyComponents(const FFTFrame& frame1, const
FFTFrame& frame2, double x);
> +
> +#if OS(DARWIN)
> + DSPSplitComplex& dspSplitComplex() { return m_frame; }
> + DSPSplitComplex dspSplitComplex() const { return m_frame; }
> +
> + static FFTSetup fftSetupForSize(unsigned fftSize);
> +
> + static FFTSetup* fftSetups;
See below for a suggestion that will allow you to get rid of fftSetupForSize
and fftSetups here.
> +
> + FFTSetup m_FFTSetup;
> +
> + DSPSplitComplex m_frame;
> + AudioFloatArray m_realData;
> + AudioFloatArray m_imagData;
> +#endif // OS(DARWIN)
> +};
> +
> +} // namespace WebCore
> +
> +#endif // FFTFrame_h
> Index: WebCore/platform/audio/FFTFrame.cpp
> ===================================================================
> --- WebCore/platform/audio/FFTFrame.cpp (revision 0)
> +++ WebCore/platform/audio/FFTFrame.cpp (revision 0)
> @@ -0,0 +1,271 @@
> +/*
> + * 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"
> +
> +#if ENABLE(WEB_AUDIO)
> +
> +#include "FFTFrame.h"
> +
> +#include <wtf/Complex.h>
> +#include <wtf/MathExtras.h>
> +#include <wtf/OwnPtr.h>
> +
> +namespace WebCore {
> +
> +void FFTFrame::doPaddedFFT(float* data, size_t dataSize)
> +{
> + // Zero-pad the impulse response
> + AudioFloatArray paddedResponse(fftSize());
> +
> + ASSERT(dataSize <= fftSize());
> + memcpy(paddedResponse.data(), data, sizeof(float) * dataSize);
AudioFloatArray should handle this; you shouldn't need to call memcpy manually
here.
> +
> + // Get the frequency-domain version of padded response
> + doFFT(paddedResponse.data());
> +}
> +
> +PassOwnPtr<FFTFrame> FFTFrame::createInterpolatedFrame(const FFTFrame&
frame1, const FFTFrame& frame2, double x)
> +{
> + OwnPtr<FFTFrame> newFrame = adoptPtr(new FFTFrame(frame1.fftSize()));
> +
> + newFrame->interpolateFrequencyComponents(frame1, frame2, x);
> +
> + // In the time-domain, the 2nd half of the response must be zero, to
avoid circular convolution aliasing...
> + int fftSize = newFrame->fftSize();
> + AudioFloatArray buffer(fftSize);
> + newFrame->doInverseFFT(buffer.data());
> +
> + memset(buffer.data() + fftSize / 2, 0, sizeof(float) * fftSize / 2);
How about an AudioFloatArray::clearRange() or similar to avoid manual memset?
> +
> + newFrame->doFFT(buffer.data());
> +
> + return newFrame.release();
> +}
> +
The other routines in this file look fine to me.
> +#ifndef NDEBUG
> +void FFTFrame::print()
> +{
> + FFTFrame& frame = *this;
> + float* realP = frame.realData();
> + float* imagP = frame.imagData();
> + printf("**** \n");
> + printf("DC = %f : nyquist = %f\n", realP[0], imagP[0]);
> +
> + int n = m_FFTSize / 2;
> +
> + for (int i = 1; i < n; i++) {
> + double mag = sqrt(realP[i] * realP[i] + imagP[i] * imagP[i]);
> + double phase = atan2(realP[i], imagP[i]);
> +
> + printf("[%d] (%f %f)\n", i, mag, phase);
> + }
> + printf("****\n");
> +}
> +#endif // NDEBUG
I don't think this matters too much, but consider using the LOG macros in
wtf/Assertions.h for your printing. Otherwise, #ifndef NDEBUG #include
<stdio.h>.
> +
> +} // namespace WebCore
> +
> +#endif // ENABLE(WEB_AUDIO)
> Index: WebCore/platform/audio/mac/FFTFrameMac.cpp
> ===================================================================
> --- WebCore/platform/audio/mac/FFTFrameMac.cpp (revision 0)
> +++ WebCore/platform/audio/mac/FFTFrameMac.cpp (revision 0)
> @@ -0,0 +1,191 @@
> +/*
> + * 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.
> + */
> +
> +// Mac OS X - specific FFTFrame implementation
> +
> +#include "config.h"
> +
> +#if ENABLE(WEB_AUDIO)
> +
> +#include "FFTFrame.h"
> +
> +namespace WebCore {
> +
> +const int kMaxFFTPow2Size = 24;
> +
> +FFTSetup* FFTFrame::fftSetups = 0;
> +
> +// Normal constructor: allocates for a given fftSize
> +FFTFrame::FFTFrame(unsigned fftSize)
> + : m_realData(fftSize)
> + , m_imagData(fftSize)
> +{
> + m_FFTSize = fftSize;
> + m_log2FFTSize = static_cast<unsigned>(log2(fftSize));
> +
> + // We only allow power of two
> + ASSERT(1UL << m_log2FFTSize == m_FFTSize);
> +
> + // Lazily create and share fftSetup with other frames
> + m_FFTSetup = fftSetupForSize(fftSize);
See below for suggestion on this.
> +
> + // Setup frame data
> + m_frame.realp = m_realData.data();
> + m_frame.imagp = m_imagData.data();
> +}
> +
> +// Creates a blank/empty frame (interpolate() must later be called)
> +FFTFrame::FFTFrame()
> + : m_realData(0)
> + , m_imagData(0)
> +{
> + // Later will be set to correct values when interpolate() is called
> + m_frame.realp = 0;
> + m_frame.imagp = 0;
> +
> + m_FFTSize = 0;
> + m_log2FFTSize = 0;
> +}
> +
> +// Copy constructor
> +FFTFrame::FFTFrame(const FFTFrame& frame)
> + : m_FFTSize(frame.m_FFTSize)
> + , m_log2FFTSize(frame.m_log2FFTSize)
> + , m_FFTSetup(frame.m_FFTSetup)
> + , m_realData(frame.m_FFTSize)
> + , m_imagData(frame.m_FFTSize)
> +{
> + // Setup frame data
> + m_frame.realp = m_realData.data();
> + m_frame.imagp = m_imagData.data();
> +
> + // Copy/setup frame data
> + unsigned nbytes = sizeof(float) * m_FFTSize;
> + memcpy(realData(), frame.m_frame.realp, nbytes);
> + memcpy(imagData(), frame.m_frame.imagp, nbytes);
Helper setters on AudioFloatArray would be
> +}
> +
> +FFTFrame::~FFTFrame()
> +{
> +}
> +
> +void FFTFrame::multiply(const FFTFrame& frame)
> +{
> + FFTFrame& frame1 = *this;
> + const FFTFrame& frame2 = frame;
> +
> + float* realP1 = frame1.realData();
> + float* imagP1 = frame1.imagData();
> + const float* realP2 = frame2.realData();
> + const float* imagP2 = frame2.imagData();
> +
> + // Scale accounts for vecLib's peculiar scaling
> + // This ensures the right scaling all the way back to inverse FFT
> + float scale = 0.5f;
> +
> + // Multiply packed DC/nyquist component
> + realP1[0] *= scale * realP2[0];
> + imagP1[0] *= scale * imagP2[0];
> +
> + // Multiply the rest, skipping packed DC/Nyquist components
> + DSPSplitComplex sc1 = frame1.dspSplitComplex();
> + sc1.realp++;
> + sc1.imagp++;
> +
> + DSPSplitComplex sc2 = frame2.dspSplitComplex();
> + sc2.realp++;
> + sc2.imagp++;
> +
> + unsigned halfSize = m_FFTSize / 2;
> +
> + // Complex multiply
> + vDSP_zvmul(&sc1, 1, &sc2, 1, &sc1, 1, halfSize - 1, 1 /* normal
multiplication */);
> +
> + // We've previously scaled the packed part, now scale the rest.....
> + vDSP_vsmul(sc1.realp, 1, &scale, sc1.realp, 1, halfSize - 1);
> + vDSP_vsmul(sc1.imagp, 1, &scale, sc1.imagp, 1, halfSize - 1);
> +}
> +
> +void FFTFrame::doFFT(float* data)
> +{
> + vDSP_ctoz((DSPComplex*)data, 2, &m_frame, 1, m_FFTSize / 2);
> + vDSP_fft_zrip(m_FFTSetup, &m_frame, 1, m_log2FFTSize, FFT_FORWARD);
> +}
> +
> +void FFTFrame::doInverseFFT(float* data)
> +{
> + vDSP_fft_zrip(m_FFTSetup, &m_frame, 1, m_log2FFTSize, FFT_INVERSE);
> + vDSP_ztoc(&m_frame, 1, (DSPComplex*)data, 2, m_FFTSize / 2);
> +
> + // Do final scaling so that x == IFFT(FFT(x))
> + float scale = 0.5f / m_FFTSize;
> + vDSP_vsmul(data, 1, &scale, data, 1, m_FFTSize);
> +}
> +
> +FFTSetup FFTFrame::fftSetupForSize(unsigned fftSize)
> +{
> + if (!fftSetups) {
> + fftSetups = (FFTSetup*)malloc(sizeof(FFTSetup) * kMaxFFTPow2Size);
> + memset(fftSetups, 0, sizeof(FFTSetup) * kMaxFFTPow2Size);
> + }
> +
> + int pow2size = static_cast<int>(log2(fftSize));
> + ASSERT(pow2size < kMaxFFTPow2Size);
> + if (!fftSetups[pow2size])
> + fftSetups[pow2size] = vDSP_create_fftsetup(pow2size, FFT_RADIX2);
> +
> + return fftSetups[pow2size];
> +}
> +
> +void FFTFrame::cleanup()
> +{
> + if (!fftSetups)
> + return;
> +
> + for (int i = 0; i < kMaxFFTPow2Size; ++i) {
> + if (fftSetups[i])
> + vDSP_destroy_fftsetup(fftSetups[i]);
> + }
> +
> + free(fftSetups);
> + fftSetups = 0;
> +}
> +
Here's how you can get rid of FFTFrame::cleanup. Make a class private to this
implementation file which holds and lazily initializes the fftSetups. Create a
static instance of that class in this file. All its constructor should do is
zero its internal fftSetups pointer. You can do the same work in
fftSetupForSize (you can just make it a method on that new class). Its
destructor should do the work currently in FFTFrame::cleanup. Potentially you
could also just make the static variable in this file an OwnPtr of that new
class. I'm not sure which would be preferred in WebKit style.
> +float* FFTFrame::realData() const
> +{
> + return m_frame.realp;
> +}
> +
> +float* FFTFrame::imagData() const
> +{
> + return m_frame.imagp;
> +}
> +
> +} // namespace WebCore
> +
> +#endif // ENABLE(WEB_AUDIO)
More information about the webkit-reviews
mailing list