[Webkit-unassigned] [Bug 34827] audio engine: add FFTFrame files
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu Sep 2 18:29:19 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=34827
Kenneth Russell <kbr at google.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #66283|review? |review-
Flag| |
--- Comment #7 from Kenneth Russell <kbr at google.com> 2010-09-02 18:29:19 PST ---
(From update of attachment 66283)
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)
--
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