Bug 34827 – audio engine: add FFTFrame files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

audio engine: add FFTFrame files

[Chris Rogers]

Created attachment 48540 [details]
Patch

[Chris Rogers]

Comment on attachment 48540 [details]
Patch

FFTFrame is one of the lowest level building blocks for the convolution engine and spatialized panning

[Chris Rogers]

Created attachment 48666 [details]
Patch

[Jeremy Orlow]

Comment on attachment 48666 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 866f6d8..a11dd7a 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,35 @@
> [phone]  Chris Rogers  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        audio engine: add FFTFrame files
> +        https://bugs.webkit.org/show_bug.cgi?id=34827
> +
> +        No tests - no javascript API

Add a "yet"


> diff --git a/WebCore/platform/audio/FFTFrame.cpp b/WebCore/platform/audio/FFTFrame.cpp
> new file mode 100644
> index 0000000..17f6c8e
> --- /dev/null
> +++ b/WebCore/platform/audio/FFTFrame.cpp
> @@ -0,0 +1,268 @@
> +/*
> + * 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.
> + */
> +
> +// Cross-platform FFTFrame implementation

This comment seems out of place and not particularly useful.

> +
> +#include "config.h"
> +#include "FFTFrame.h"
> +
> +#include <wtf/Complex.h>
> +#include <wtf/MathExtras.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, sizeof(float) * dataSize);
> +
> +    // Get the frequency-domain version of padded response
> +    doFFT(paddedResponse);
> +}
> +
> +FFTFrame* FFTFrame::createInterpolatedFrame(FFTFrame& frame1, FFTFrame& frame2, double x)

Use PassOwnPtr<>

> +{
> +    FFTFrame* newFrame = new FFTFrame(frame1.fftSize());

Use OwnPtr<>

> +
> +    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);
> +
> +    memset(buffer.data() + fftSize / 2, 0, sizeof(float) * fftSize / 2);
> +
> +    newFrame->doFFT(buffer);
> +
> +    return newFrame;
> +}
> +
> +void FFTFrame::interpolateFrequencyComponents(FFTFrame& frame1,
> +                                              FFTFrame& frame2,
> +                                              double interp)
> +{
> +    // FIXME : with some work, this method could be optimized
> +
> +    float* realP = realData();
> +    float* imagP = imagData();
> +
> +    float* realP1 = frame1.realData();
> +    float* imagP1 = frame1.imagData();
> +    float* realP2 = frame2.realData();
> +    float* imagP2 = frame2.imagData();
> +
> +    m_FFTSize = frame1.fftSize();
> +    m_log2FFTSize = frame1.log2FFTSize();
> +
> +    double s1base = (1.0 - interp);
> +    double s2base = interp;
> +
> +    double phaseAccum = 0.0;
> +    double lastPhase1 = 0.0;
> +    double lastPhase2 = 0.0;
> +
> +    realP[0] = s1base * realP1[0] + s2base * realP2[0];
> +    imagP[0] = s1base * imagP1[0] + s2base * imagP2[0];
> +
> +    int n = m_FFTSize / 2;
> +
> +    for (int i = 1; i < n; ++i) {
> +        Complex c1(realP1[i], imagP1[i]);
> +        Complex c2(realP2[i], imagP2[i]);
> +
> +        double mag1 = abs(c1);
> +        double mag2 = abs(c2);
> +
> +        // Interpolate magnitudes in decibels
> +        double mag1db = 20.0 * log10(mag1);
> +        double mag2db = 20.0 * log10(mag2);
> +
> +        double s1 = s1base;
> +        double s2 = s2base;
> +
> +        double magdbdiff = mag1db - mag2db;
> +
> +        // Empirical tweak to retain higher-frequency zeroes
> +        double threshold =  (i > 16) ? 5.0 : 2.0;
> +
> +        if (magdbdiff < -threshold && mag1db < 0.0) {
> +            s1 = pow(s1, 0.75);
> +            s2 = 1.0 - s1;
> +        } else if (magdbdiff > threshold && mag2db < 0.0) {
> +            s2 = pow(s2, 0.75);
> +            s1 = 1.0 - s2;
> +        }
> +
> +        // Average magnitude by decibels instead of linearly
> +        double magdb = s1 * mag1db + s2 * mag2db;
> +        double mag = pow(10.0, 0.05 * magdb);
> +
> +        // Now, deal with phase
> +        double phase1 = arg(c1);
> +        double phase2 = arg(c2);
> +
> +        double deltaPhase1 = phase1 - lastPhase1;
> +        double deltaPhase2 = phase2 - lastPhase2;
> +        lastPhase1 = phase1;
> +        lastPhase2 = phase2;
> +
> +        // Unwrap phase deltas
> +        if (deltaPhase1 > M_PI)
> +            deltaPhase1 -= 2.0 * M_PI;
> +        if (deltaPhase1 < -M_PI)
> +            deltaPhase1 += 2.0 * M_PI;
> +        if (deltaPhase2 > M_PI)
> +            deltaPhase2 -= 2.0 * M_PI;
> +        if (deltaPhase2 < -M_PI)
> +            deltaPhase2 += 2.0 * M_PI;
> +
> +        // Blend group-delays
> +        double deltaPhaseBlend;
> +
> +        if (deltaPhase1 - deltaPhase2 > M_PI)
> +            deltaPhaseBlend = s1 * deltaPhase1 + s2 * (2.0 * M_PI + deltaPhase2);
> +        else if (deltaPhase2 - deltaPhase1 > M_PI)
> +            deltaPhaseBlend = s1 * (2.0 * M_PI + deltaPhase1) + s2 * deltaPhase2;
> +        else
> +            deltaPhaseBlend = s1 * deltaPhase1 + s2 * deltaPhase2;
> +
> +        phaseAccum += deltaPhaseBlend;
> +
> +        // Unwrap
> +        if (phaseAccum > M_PI)
> +            phaseAccum -= 2.0 * M_PI;
> +        if (phaseAccum < -M_PI)
> +            phaseAccum += 2.0 * M_PI;
> +
> +        Complex c = complexFromMagnitudePhase(mag, phaseAccum);
> +
> +        realP[i] = c.real();
> +        imagP[i] = c.imag();
> +    }
> +}
> +
> +double  FFTFrame::extractAverageGroupDelay()
> +{
> +    float* realP = realData();
> +    float* imagP = imagData();
> +
> +    double aveSum = 0.0;
> +    double weightSum = 0.0;
> +    double lastPhase = 0.0;
> +
> +    int halfSize = fftSize() / 2;
> +
> +    const double kSamplePhaseDelay = (2.0 * M_PI) / double(fftSize());
> +
> +    // Calculate weighted average group delay
> +    for (int i = 0; i < halfSize; i++) {
> +        Complex c(realP[i], imagP[i]);
> +        double mag = abs(c);
> +        double phase = arg(c);
> +
> +        double deltaPhase = phase - lastPhase;
> +        lastPhase = phase;
> +
> +        // Unwrap
> +        if (deltaPhase < -M_PI)
> +            deltaPhase += 2.0 * M_PI;
> +        if (deltaPhase > M_PI)
> +            deltaPhase -= 2.0 * M_PI;
> +
> +        aveSum += mag * deltaPhase;
> +        weightSum += mag;
> +    }
> +
> +    // Note how we invert the phase delta wrt frequency since this is how group delay is defined
> +    double ave = aveSum / weightSum;
> +    double aveSampleDelay = -ave / kSamplePhaseDelay;
> +
> +    // Leave 20 sample headroom (for leading edge of impulse)
> +    if (aveSampleDelay > 20.0)
> +        aveSampleDelay -= 20.0;
> +
> +    // Remove average group delay (minus 20 samples for headroom)
> +    addConstantGroupDelay(-aveSampleDelay);
> +
> +    // Remove DC offset
> +    realP[0] = 0.0;
> +
> +    return aveSampleDelay;
> +}
> +
> +void  FFTFrame::addConstantGroupDelay(double sampleFrameDelay)
> +{
> +    int halfSize = fftSize() / 2;
> +
> +    float* realP = realData();
> +    float* imagP = imagData();
> +
> +    const double kSamplePhaseDelay = (2.0 * M_PI) / double(fftSize());
> +
> +    double phaseAdj = -sampleFrameDelay * kSamplePhaseDelay;
> +
> +    // Add constant group delay
> +    for (int i = 1; i < halfSize; i++) {
> +        Complex c(realP[i], imagP[i]);
> +        double mag = abs(c);
> +        double phase = arg(c);
> +
> +        phase += i * phaseAdj;
> +
> +        Complex c2 = complexFromMagnitudePhase(mag, phase);
> +
> +        realP[i] = c2.real();
> +        imagP[i] = c2.imag();
> +    }
> +}
> +
> +// For debugging

#ifndef NDEBUG then?

> +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");
> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/FFTFrame.h b/WebCore/platform/audio/FFTFrame.h
> new file mode 100644
> index 0000000..2130eb0
> --- /dev/null
> +++ b/WebCore/platform/audio/FFTFrame.h
> @@ -0,0 +1,52 @@
> +/*
> + * 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 <wtf/Platform.h>
> +
> +// 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
> +//
> +// The reason we don't create an abstract base class and subclass for Mac, MKL, etc.
> +// is because then it would not be possible to use the object in a cross-platform way
> +// as an ivarof a class or as a stack-based object, since we would have to resort to

s/ivar/instance variable/

> +// factory methods, etc.  Since it turns out to be quite useful to use these as ivars
> +// and stack based objects, and since the exact type *is* known at compile-time, we use
> +// this approach.

I don't understand.  You can just name all of the cross platform libraries FFTFrameBase or FFTFramePlatform or something and then have FFTFrame inherit from that.  The only reason not to do this is if there's a lot of 2 way dependencies between the halves (and thus you'd need to use virtual calls..which is not desirable).  You could also do this by having the platform specific parts be in their own class and have that class be owned by the cross platform bits or visa versa.  Or you can even split this into multiple layers.  I haven't looked closely 

The thing I dislike the most is that the .h file is 100% per platform but then pulls in some common CPP files.

> +
> +#if OS(DARWIN)
> +    #include "FFTFrameMac.h"
> +#elif OS(LINUX) || OS(WINDOWS)
> +    #include "FFTFrameMKL.h"
> +#else
> +    #error "OS not supported"
> +#endif

Don't indent.

> +
> +#endif // FFTFrame_h
> diff --git a/WebCore/platform/audio/mac/FFTFrameMac.cpp b/WebCore/platform/audio/mac/FFTFrameMac.cpp
> new file mode 100644
> index 0000000..e99b573
> --- /dev/null
> +++ b/WebCore/platform/audio/mac/FFTFrameMac.cpp
> @@ -0,0 +1,215 @@
> +/*
> + * 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"
> +#include "FFTFrameMac.h"
> +
> +namespace WebCore {
> +
> +const int kMaxFFTPow2Size = 24;
> +
> +FFTSetup* FFTFrame::fftSetups = 0;
> +
> +// Normal constructor: allocates for a given fftSize
> +FFTFrame::FFTFrame(size_t fftSize)
> +    : m_realData(fftSize)
> +    , m_imagData(fftSize)
> +{
> +    m_FFTSize = fftSize;
> +    m_log2FFTSize = log2(fftSize);
> +
> +    // We only allow power of two
> +    assert(pow(2.0, m_log2FFTSize) == m_FFTSize);

ASSERT not assert.

And I don't like that you're using a float function here.  Why not just use <<?

> +
> +    // Lazily create and share fftSetup with other frames
> +    m_FFTSetup = fftSetupForSize(fftSize);
> +
> +    // Setup frame data
> +    m_frame.realp = m_realData;
> +    m_frame.imagp = m_imagData;
> +}
> +
> +// 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)
> +{
> +    // Copy/setup frame data
> +    size_t nbytes = sizeof(float) * m_FFTSize;
> +    FFTFrame& safeFrame = const_cast<FFTFrame&>(frame);
> +    memcpy(realData(), safeFrame.realData(), nbytes);
> +    memcpy(imagData(), safeFrame.imagData(), nbytes);

You don't need to do this.  You can just do frame->realp and such.  In general, we should do everything we can to avoid const_cast.

> +
> +    m_frame.realp = realData();
> +    m_frame.imagp = imagData();
> +}
> +
> +FFTFrame::~FFTFrame()
> +{
> +}
> +
> +// Uses Accelerate.framework highly-optimized zvmul() function

Not sure if this comment adds any value.

> +void FFTFrame::multiply(const FFTFrame& frame)
> +{
> +    FFTFrame& frame1 = *this;
> +    FFTFrame& frame2 = const_cast<FFTFrame&>(frame);
> +
> +    float* realP1 = frame1.realData();
> +    float* imagP1 = frame1.imagData();
> +    float* realP2 = frame2.realData();
> +    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.5;
> +
> +    // 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++;
> +
> +    size_t halfSize = m_FFTSize / 2;
> +
> +    // Complex multiply
> +    zvmul(&sc1,
> +          1,
> +          &sc2,
> +          1,
> +          &sc1,
> +          1,
> +          halfSize - 1,
> +          1 /* normal multiplication */);
> +
> +    // We've previously scaled the packed part, now scale the rest.....
> +    vsmul(sc1.realp, 1, &scale, sc1.realp, 1, halfSize - 1);
> +    vsmul(sc1.imagp, 1, &scale, sc1.imagp, 1, halfSize - 1);
> +
> +    // Scalar code for reference

I'm leaning towards saying this should removed so it won't go stale.  Would you mind terribly?

> +#if 0
> +    float scale = 0.5;
> +
> +    // multiply packed DC/nyquist component
> +    realP2[0] *= scale *realP1[0];
> +    imagP2[0] *= scale *imagP1[0];
> +
> +    // scalar code
> +    for (int i = 1; i < halfSize; i++) {
> +        // scale by 1/2 since we're not handling the scaling in the FFT -> IFFT
> +        // (we're handling it all in the IFFT stage)
> +
> +
> +        float r1 = realP1[i];
> +        float c1 = imagP1[i];
> +
> +        float r2 = realP2[i];
> +        float c2 = imagP2[i];
> +
> +        // multiply in the frequency domain
> +        float r3 = r1*r2 - c1*c2;
> +        float c3 = r1*c2 + r2*c1;
> +
> +        realP2[i] = scale * r3;
> +        imagP2[i] = scale * c3;
> +    }
> +#endif
> +}
> +
> +void FFTFrame::doFFT(float* data)
> +{
> +    ctoz((DSPComplex*)data, 2, &m_frame, 1, m_FFTSize / 2);
> +    fft_zrip(m_FFTSetup, &m_frame, 1, m_log2FFTSize, FFT_FORWARD);
> +}
> +
> +void FFTFrame::doInverseFFT(float* data)
> +{
> +    fft_zrip(m_FFTSetup, &m_frame, 1, m_log2FFTSize, FFT_INVERSE);
> +    ztoc(&m_frame, 1, (DSPComplex*)data, 2, m_FFTSize / 2);
> +
> +    // Do final scaling so that x == IFFT(FFT(x))
> +    float scale = 0.5 / m_FFTSize;
> +    vsmul(data, 1, &scale, data, 1, m_FFTSize);
> +}
> +
> +FFTSetup FFTFrame::fftSetupForSize(size_t fftSize)
> +{
> +    if (!fftSetups) {
> +        fftSetups = (FFTSetup*)malloc(sizeof(FFTSetup) * kMaxFFTPow2Size);
> +        memset(fftSetups, 0, sizeof(FFTSetup) * kMaxFFTPow2Size);
> +    }
> +
> +    int pow2size = log2(fftSize);
> +
> +    assert(pow2size < kMaxFFTPow2Size);
> +
> +    if (!fftSetups[pow2size])
> +        fftSetups[pow2size] = create_fftsetup(pow2size, FFT_RADIX2);
> +
> +    return fftSetups[pow2size];
> +}

Nit, but it seems like vertical whitespace could be condensed a bit here without making it much harder to read.

> +
> +void FFTFrame::cleanupFFTSetups()
> +{
> +    if (!fftSetups)
> +        return;
> +
> +    for (int i = 0; i < kMaxFFTPow2Size; ++i) {
> +        if (fftSetups[i])
> +            destroy_fftsetup(fftSetups[i]);
> +    }
> +
> +    free(fftSetups);
> +    fftSetups = 0;
> +}
> +
> +} // namespace WebCore
> diff --git a/WebCore/platform/audio/mac/FFTFrameMac.h b/WebCore/platform/audio/mac/FFTFrameMac.h
> new file mode 100644
> index 0000000..40f976b
> --- /dev/null
> +++ b/WebCore/platform/audio/mac/FFTFrameMac.h
> @@ -0,0 +1,91 @@
> +/*
> + * 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.
> + */
> +
> +// FFT abstraction implemented using Mac OS X Accelerate.framework (vecLib)
> +//
> +
> +#ifndef FFTFrameMac_h
> +#define FFTFrameMac_h
> +
> +#include "AudioFloatArray.h"
> +#include <Accelerate/Accelerate.h>
> +
> +namespace WebCore {
> +
> +class FFTFrame {
> +public:
> +    FFTFrame(size_t fftSize);
> +    FFTFrame(); // creates a blank/empty frame for later use with interpolate()

You mean createInterpolatedFrame or something in some other class?  If the former, correct.  If the latter, make it ClassName::interpolate()

Btw, all of these should be private and you should expose constructors for only what needs to be created outside of this class.

> +    FFTFrame(const FFTFrame& frame);
> +    virtual ~FFTFrame();

Why does this need to be virtual?

> +
> +    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 FFTFrame* createInterpolatedFrame(FFTFrame& frame1, FFTFrame& frame2, double x);

Can the inputs be const?

This should return a PassOwnPtr<FFTFrame>.

[Chris Rogers]

Created attachment 66283 [details]
Patch

[Chris Rogers]

Addressed all of Jeremy Orlow's comments.

[Kenneth Russell]

Comment on attachment 66283 [details]
Patch

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)

[Chris Rogers]

Hi Ken, thanks for your comments.  I'll fix everything, but I had a question about your suggestion to create a static instance variable to handle the fft setups.  I'm pretty sure that we're not allowed to have code run at static initialization time since one of the build steps checks for this.  So I don't think we can have a static instance of the class, but instead will need a pointer to the class.  Then we'll still need a cleanup method to delete it later on...

[Kenneth Russell]

(In reply to comment #8)
> Hi Ken, thanks for your comments.  I'll fix everything, but I had a question about your suggestion to create a static instance variable to handle the fft setups.  I'm pretty sure that we're not allowed to have code run at static initialization time since one of the build steps checks for this.  So I don't think we can have a static instance of the class, but instead will need a pointer to the class.  Then we'll still need a cleanup method to delete it later on...

I see. I didn't realize that. In that case don't worry about that part of my review.

[Chris Rogers]

Created attachment 66544 [details]
Patch

[Chris Rogers]

Hi Ken, I think I've addressed all of your comments.

[Kenneth Russell]

Comment on attachment 66544 [details]
Patch

Revised code looks good to me.

[Chris Rogers]

Comment on attachment 66544 [details]
Patch

Clearing flags on attachment: 66544

Committed r66941: <http://trac.webkit.org/changeset/66941>

[Chris Rogers]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66941 might have broken SnowLeopard Intel Release (Tests)