Bug 52989 – Web Audio API: port FFTFrame to FFTW

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

[Kenneth Russell]

For the Windows and Linux ports of the Web Audio API, a port of the FFTFrame class to the open source FFTW library is needed.

[Kenneth Russell]

Created attachment 79889 [details]
Patch

[James Robinson]

Comment on attachment 79889 [details]
Patch

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

r=me. before landing i think chris rogers should give this a once-over, and please make sure the EWS bots are happy with the gyp changes

> Source/WebCore/platform/audio/FFTFrame.h:123
>  #endif // !OS(DARWIN) && USE(WEBAUDIO_MKL)

nit: comment on this line should be // USE(WEBAUDIO_MKL)

> Source/WebCore/platform/audio/FFTFrame.h:148
> +#endif // !OS(DARWIN) && USE(WEBAUDIO_FFTW)

nit: // USE(WEBAUDIO_FFTW)

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

nit: here (and in other files) it's 2011 now :)

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:74
> +    float temporaryPointer[1];
> +    m_forwardPlan = fftwPlanForSize(fftSize, Forward,
> +                                    &temporaryPointer[0], realData(), imagData());
> +    m_backwardPlan = fftwPlanForSize(fftSize, Backward,
> +                                     realData(), imagData(), &temporaryPointer[0]);

nit: i'm not sure i fully grok this, but would 'float temp; fftwPlanForSize(..., &temp, ...);' do as well? 1-size arrays are odd

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:103
> +    unsigned nbytes = sizeof(float) * complexDataSize(m_FFTSize);

nit: number of bytes should be size_t

[Chris Rogers]

Hi Ken, overall this looks very good.  Thanks for running the unit tests I gave you to verify correctness as that counts the most!

Otherwise I have a few general comments:

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

> Source/WebCore/platform/audio/FFTFrame.h:139
> +    // overhead. For the time being, we just assume unaligned data.

For now just to get something working we can live with unaligned (no SIMD optimizations).  But, soon we'll have to think of improving this.  We can either require that the arrays passed in to doFFT() and doInverseFFT() be aligned (and check that they are with ASSERTs etc.) or do a memcpy into an aligned array.  I'm pretty sure that the overhead of the memcpy() is *very* much smaller than not having SIMD optimization.

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:45
> +int complexDataSize(int fftSize)

I think it would be better to have a more descriptive and specific name -- maybe "unpackedFFTWDataSize.

Also, fftSize should be "unsigned" to match the FFTFrame constructors

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:125
> +    // factor will need to change too.

Maybe fix this comment to be more generic instead of talking about vecLib on the Mac...

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:155
> +    int length = m_realData.size();

I think it would be more clear with something like:
int length = fftSize() / 2 + 1;

or maybe even better:

int length = complexDataSize();   // <---- or whatever the new function name turns out to be


then maybe even ASSERT(length == m_realData.size());

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:163
> +    m_imagData[0] = m_realData[length - 1];

I think it would be much clearer to explicitly reference the index in terms of the fftSize:

m_imagData[0] = m_realData[fftSize() / 2];

This is where the nyquist component is normally expected to live.  The way you have it now I had to look twice to verify that it's correct (looking at what size m_realData was allocated at, etc.)

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:170
> +    m_realData[length - 1] = m_imagData[0];

Same comment as above.  I think the following would be clearer and more explicit:

m_realData[fftSize() / 2] = m_imagData[0];

You could even define a variable called "halfSize" since this value is re-used later in this function:
int halfSize = fftSize() / 2;

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:186
> +    m_imagData[0] = m_realData[length - 1];

Similar comment to above:

m_imagData[0] = m_realData[fftSize() / 2];

or

m_imagData[0] = m_realData[halfSize];

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:243
> +        unsigned flags = FFTW_ESTIMATE | FFTW_PRESERVE_INPUT | FFTW_UNALIGNED;

please add FIXME about addressing the SIMD optimization issue and FFTW_UNALIGNED

[Kenneth Russell]

(In reply to comment #2)
> (From update of attachment 79889 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=79889&action=review
> 
> r=me. before landing i think chris rogers should give this a once-over, and please make sure the EWS bots are happy with the gyp changes

Done. EWS bots are happy; this source isn't being compiled in yet.

> > Source/WebCore/platform/audio/FFTFrame.h:123
> >  #endif // !OS(DARWIN) && USE(WEBAUDIO_MKL)
> 
> nit: comment on this line should be // USE(WEBAUDIO_MKL)

Done.

> > Source/WebCore/platform/audio/FFTFrame.h:148
> > +#endif // !OS(DARWIN) && USE(WEBAUDIO_FFTW)
> 
> nit: // USE(WEBAUDIO_FFTW)

Done.

> > Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:2
> > + * Copyright (C) 2010 Google Inc. All rights reserved.
> 
> nit: here (and in other files) it's 2011 now :)

Fixed.

> > Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:74
> > +    float temporaryPointer[1];
> > +    m_forwardPlan = fftwPlanForSize(fftSize, Forward,
> > +                                    &temporaryPointer[0], realData(), imagData());
> > +    m_backwardPlan = fftwPlanForSize(fftSize, Backward,
> > +                                     realData(), imagData(), &temporaryPointer[0]);
> 
> nit: i'm not sure i fully grok this, but would 'float temp; fftwPlanForSize(..., &temp, ...);' do as well? 1-size arrays are odd

Yes, that works; changed.

> > Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:103
> > +    unsigned nbytes = sizeof(float) * complexDataSize(m_FFTSize);
> 
> nit: number of bytes should be size_t

Done.

[Kenneth Russell]

(In reply to comment #3)
> Hi Ken, overall this looks very good.  Thanks for running the unit tests I gave you to verify correctness as that counts the most!
> 
> Otherwise I have a few general comments:
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=79889&action=review
> 
> > Source/WebCore/platform/audio/FFTFrame.h:139
> > +    // overhead. For the time being, we just assume unaligned data.
> 
> For now just to get something working we can live with unaligned (no SIMD optimizations).  But, soon we'll have to think of improving this.  We can either require that the arrays passed in to doFFT() and doInverseFFT() be aligned (and check that they are with ASSERTs etc.) or do a memcpy into an aligned array.  I'm pretty sure that the overhead of the memcpy() is *very* much smaller than not having SIMD optimization.

Understood. To consolidate the comments I've removed the comment from FFTFrame.h and moved it to FFTFrameFFTW.cpp, and added a FIXME.

> > Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:45
> > +int complexDataSize(int fftSize)
> 
> I think it would be better to have a more descriptive and specific name -- maybe "unpackedFFTWDataSize.

Done.

> Also, fftSize should be "unsigned" to match the FFTFrame constructors

Done.

> > Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:125
> > +    // factor will need to change too.
> 
> Maybe fix this comment to be more generic instead of talking about vecLib on the Mac...

I'm pretty sure we specifically referenced vecLib in the comments in the MKL port of the FFTFrame. I'm leaving this as is for now.

> > Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:155
> > +    int length = m_realData.size();
> 
> I think it would be more clear with something like:
> int length = fftSize() / 2 + 1;
> 
> or maybe even better:
> 
> int length = complexDataSize();   // <---- or whatever the new function name turns out to be
> 
> 
> then maybe even ASSERT(length == m_realData.size());

I've changed this to use unpackedFFTWDataSize and added the ASSERT you suggested.

> > Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:163
> > +    m_imagData[0] = m_realData[length - 1];
> 
> I think it would be much clearer to explicitly reference the index in terms of the fftSize:
> 
> m_imagData[0] = m_realData[fftSize() / 2];
> 
> This is where the nyquist component is normally expected to live.  The way you have it now I had to look twice to verify that it's correct (looking at what size m_realData was allocated at, etc.)
> 
> > Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:170
> > +    m_realData[length - 1] = m_imagData[0];
> 
> Same comment as above.  I think the following would be clearer and more explicit:
> 
> m_realData[fftSize() / 2] = m_imagData[0];
> 
> You could even define a variable called "halfSize" since this value is re-used later in this function:
> int halfSize = fftSize() / 2;

I've changed these to use unpackedFFTWDataSize(). I really don't want to replicate the math all over this file, especially because of the slightly different unpacked complex data length between the FFTW port and the MKL and vecLib ports.

> > Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:186
> > +    m_imagData[0] = m_realData[length - 1];
> 
> Similar comment to above:
> 
> m_imagData[0] = m_realData[fftSize() / 2];
> 
> or
> 
> m_imagData[0] = m_realData[halfSize];

See above.

> > Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:243
> > +        unsigned flags = FFTW_ESTIMATE | FFTW_PRESERVE_INPUT | FFTW_UNALIGNED;
> 
> please add FIXME about addressing the SIMD optimization issue and FFTW_UNALIGNED

Done.

All changes will be made in landed version.

[Kenneth Russell]

Committed r76562: <http://trac.webkit.org/changeset/76562>

[Soo-Hyun Choi]

I wonder how this patch (which uses FFTW library) could have been landed in the WebKit. I assume that FFTW should be compiled together with WebKit in order to use this patch (FFTFrame), which I suspect license mismatch between them. 

License violation, perhaps?

NB) WebKit is LGPL+BSD license where FFTW.org is GPL.
http://www.fftw.org/faq/section1.html#isfftwfree

Any comments?

[Adam Barth]

> Any comments?

Would you be willing to ask these questions on chromium-dev?  Hopefully that will make it easier to direct your question to the appropriate folks (who are better at reading email than at reading bug trackers).

Thanks.

[Soo-Hyun Choi]

(In reply to comment #8)
> > Any comments?
> 
> Would you be willing to ask these questions on chromium-dev?  Hopefully that will make it easier to direct your question to the appropriate folks (who are better at reading email than at reading bug trackers).
> 
> Thanks.

I am not certain if it is chromium issue or WebKit issue. I thought it is not a bad idea to ask WebKit first as this patch actually became a part of WebKit source through this bug report.

Anyway, I will direct my original message to the chromium-dev as you suggested.

Thanks,