Bug 50986 – Web Audio API: port FFTFrame to MKL

• Component: New Bugs
• Status: RESOLVED FIXED

[Kenneth Russell]

For the Windows and Linux ports of the Web Audio API, a port of the FFTFrame class to Intel's Math Kernel Library is needed.

[Kenneth Russell]

Created attachment 76457 [details]
Patch

[James Robinson]

Comment on attachment 76457 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=76457&action=review

How can we tell if this math is right?

> JavaScriptCore/wtf/MathExtras.h:149
> -inline bool log2(double num)
> +inline double log2(double num)

Woah

> WebCore/platform/audio/FFTFrame.h:38
> +#if !OS(DARWIN) && USE(WEBAUDIO_MKL)

The OS(DARWIN) things seems redundant, shouldn't ports simply not define WEBAUDIO_MKL on platforms that do not have it?

[Kenneth Russell]

(In reply to comment #2)
> (From update of attachment 76457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=76457&action=review
> 
> How can we tell if this math is right?

Right now it's difficult to check in any unit test for this code. I've verified by hand with a unit test from Chris Rogers, which prints its output to stdout, that the three main routines are working correctly.

> > JavaScriptCore/wtf/MathExtras.h:149
> > -inline bool log2(double num)
> > +inline double log2(double num)
> 
> Woah

Yeah, I added that function for the Windows port of the Web Audio stuff and obviously didn't test it. This patch has verified that this fix works.

> > WebCore/platform/audio/FFTFrame.h:38
> > +#if !OS(DARWIN) && USE(WEBAUDIO_MKL)
> 
> The OS(DARWIN) things seems redundant, shouldn't ports simply not define WEBAUDIO_MKL on platforms that do not have it?

I spent a lot of time wrestling with GYP to conditionally define WTF_USE_WEBAUDIO_MKL in features_override.gypi and test its presence in feature_defines in WebCore.gyp, but it didn't work. I'll have to ask for help from a GYP expert to figure out why not. For the time being the conditional guard works and allows us to make forward progress on the Windows port.

[Chris Rogers]

Ken, thanks for the patch!  It looks very good except for a few comments.  It's unfortunate that we need the getUpToDateComplexData() method, but it's more important to get a reasonable working version before we spend too much time with further optimizations.  Thanks for running the unit tests I sent, they should provide a high level of confidence that the code is correct.

> WebCore/platform/audio/FFTFrame.h:103
> +    float* getUpToDateComplexData();

Could you please add a comment explaining what this method does.  Also, although you have FIXMEs elsewhere related to this, could you please add one here describing that we'd like to eventually remove this if we can find a way in MKL to work in the planar format.

> WebCore/platform/audio/mkl/FFTFrameMKL.cpp:57
> +    // Set the forward scale factor to 2 to match Accelerate.framework's.

Please add a FIXME explaining that FFTFrameMac's scaling factor of 2.0 can be fixed to be 1.0 (and this implementation as well).
In the future, this will be cleaner and more correct.  But also note that this change can only occur if all clients of FFTFrame are
adjusted to account for the scaling difference.

> WebCore/platform/audio/mkl/FFTFrameMKL.cpp:62
> +    double scale = 1.0 / (2.0 * fftSize);

FIXME that this scaling factor also needs to be adjusted if the FIXME above is made.

> WebCore/platform/audio/mkl/FFTFrameMKL.cpp:139
> +    float scale = 0.5f;

FIXME: that this will be 1.0f if the FIXME about scaling factor (above) from 2.0 -> 1.0 is made.

> WebCore/platform/audio/mkl/FFTFrameMKL.cpp:158
> +    // FIXME: find an MKL routine to do at least the scaling more efficiently.

I would not use the word "unpack", but would favor "de-interleave", "split", or "make planar", since "packed" format has a special meaning related to DC/nyquist components in FFT terminology.

> WebCore/platform/audio/mkl/FFTFrameMKL.cpp:172
> +    // Unpack to separate real and complex arrays. FIXME: figure out

Same comment about "unpack" as above.

> WebCore/platform/audio/mkl/FFTFrameMKL.cpp:178
> +        m_realData[i] = m_complexData[baseComplexIndex];

Maybe a comment here about m_realData[0] being DC component and m_imagData[0] being nyquist component since it is "packed"

> WebCore/platform/audio/mkl/FFTFrameMKL.cpp:224
> +    for (int i = 0; i < len; ++i) {

Ideally, this method can be removed if we can figure out how to use MKL with planar data.  But if we must keep it, since this gets called a lot, a FIXME might be good indicating that SSE optimization can be considered if profiling shows this as a hot-spot.

> WebCore/platform/audio/mkl/FFTFrameMKL.cpp:240
> +    int pow2size = static_cast<int>(log2(fftSize));

Maybe an ASSERT(fftSize) before the call to log2()

[Kenneth Russell]

(In reply to comment #4)
> Ken, thanks for the patch!  It looks very good except for a few comments.  It's unfortunate that we need the getUpToDateComplexData() method, but it's more important to get a reasonable working version before we spend too much time with further optimizations.  Thanks for running the unit tests I sent, they should provide a high level of confidence that the code is correct.

Thanks for your review. Revised patch to follow.

> > WebCore/platform/audio/FFTFrame.h:103
> > +    float* getUpToDateComplexData();
> 
> Could you please add a comment explaining what this method does.  Also, although you have FIXMEs elsewhere related to this, could you please add one here describing that we'd like to eventually remove this if we can find a way in MKL to work in the planar format.

Done.

> > WebCore/platform/audio/mkl/FFTFrameMKL.cpp:57
> > +    // Set the forward scale factor to 2 to match Accelerate.framework's.
> 
> Please add a FIXME explaining that FFTFrameMac's scaling factor of 2.0 can be fixed to be 1.0 (and this implementation as well).
> In the future, this will be cleaner and more correct.  But also note that this change can only occur if all clients of FFTFrame are
> adjusted to account for the scaling difference.

Done.

> > WebCore/platform/audio/mkl/FFTFrameMKL.cpp:62
> > +    double scale = 1.0 / (2.0 * fftSize);
> 
> FIXME that this scaling factor also needs to be adjusted if the FIXME above is made.

Done.

> > WebCore/platform/audio/mkl/FFTFrameMKL.cpp:139
> > +    float scale = 0.5f;
> 
> FIXME: that this will be 1.0f if the FIXME about scaling factor (above) from 2.0 -> 1.0 is made.

Done.

> > WebCore/platform/audio/mkl/FFTFrameMKL.cpp:158
> > +    // FIXME: find an MKL routine to do at least the scaling more efficiently.
> 
> I would not use the word "unpack", but would favor "de-interleave", "split", or "make planar", since "packed" format has a special meaning related to DC/nyquist components in FFT terminology.

Done.

> > WebCore/platform/audio/mkl/FFTFrameMKL.cpp:172
> > +    // Unpack to separate real and complex arrays. FIXME: figure out
> 
> Same comment about "unpack" as above.

Done.

> > WebCore/platform/audio/mkl/FFTFrameMKL.cpp:178
> > +        m_realData[i] = m_complexData[baseComplexIndex];
> 
> Maybe a comment here about m_realData[0] being DC component and m_imagData[0] being nyquist component since it is "packed"

Done.

> > WebCore/platform/audio/mkl/FFTFrameMKL.cpp:224
> > +    for (int i = 0; i < len; ++i) {
> 
> Ideally, this method can be removed if we can figure out how to use MKL with planar data.  But if we must keep it, since this gets called a lot, a FIXME might be good indicating that SSE optimization can be considered if profiling shows this as a hot-spot.

Done.

> > WebCore/platform/audio/mkl/FFTFrameMKL.cpp:240
> > +    int pow2size = static_cast<int>(log2(fftSize));
> 
> Maybe an ASSERT(fftSize) before the call to log2()

Done.

[Kenneth Russell]

Created attachment 76494 [details]
Patch

[Chris Rogers]

Ken, thanks for the changes.  LGTM

[James Robinson]

Comment on attachment 76494 [details]
Patch

OK

[Kenneth Russell]

Committed r74147: <http://trac.webkit.org/changeset/74147>