Bug 73545 – [GStreamer] FFTFrame implementation

• Component: Web Audio
• Status: RESOLVED FIXED

[Philippe Normand]

This is required for the WebAudio implementation. We currently use FFTW but it's to be removed from the tree for licensing issues. So a gstfft based implementation is needed :)

[Dongwoo Joshua Im (dwim)]

I would be interested in this bug. 
Would you have any plan to implement it soon?

[Philippe Normand]

(In reply to comment #1)
> I would be interested in this bug. 
> Would you have any plan to implement it soon?

Yes I started a patch. Will work on it in January.

[Philippe Normand]

Created attachment 121863 [details]
FFTFrameGStreamer implementation

Can you test this first iteration of the patch please? I'm also
interested by EWS status and some initial feedback.

[Philippe Normand]

Created attachment 121865 [details]
FFTFrameGStreamer implementation

Rebased against ToT

[Chris Rogers]

Wow, everyone is doing FFTs :) 

Intel also has one for an IPP backend:
https://bugs.webkit.org/show_bug.cgi?id=75522

Please see comments in that bug about the layout test which you will want to try as part of validation.

[Dongwoo Joshua Im (dwim)]

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

Great job!

Here's some minor comments.

> Source/WebCore/platform/audio/FFTFrame.h:53
> +#include <gst/fft/gstfft.h>

I guess this looks redundant as "gstfftf32.h" includes "gstfft.h" recursively.

> Source/WebCore/platform/audio/FFTFrameStub.cpp:32
> +#if !OS(DARWIN) && !USE(WEBAUDIO_MKL) && !USE(WEBAUDIO_FFMPEG) && !USE(GSTREAMER)

I think you'd better use "WEBAUDIO" prefix, such as WEBAUDIO_GSTREAMER.

[Philippe Normand]

(In reply to comment #5)
> Wow, everyone is doing FFTs :) 
> 
> Intel also has one for an IPP backend:
> https://bugs.webkit.org/show_bug.cgi?id=75522
> 
> Please see comments in that bug about the layout test which you will want to try as part of validation.

I actually saw that patch when writing this one :) Haven't yet checked the test though. Will do!

[Philippe Normand]

(In reply to comment #6)
> View in context: https://bugs.webkit.org/attachment.cgi?id=121865&action=review
> 
> Great job!
> 
> Here's some minor comments.
> 
> > Source/WebCore/platform/audio/FFTFrame.h:53
> > +#include <gst/fft/gstfft.h>
> 
> I guess this looks redundant as "gstfftf32.h" includes "gstfft.h" recursively.
> 

Hum, yes, probably :)

> > Source/WebCore/platform/audio/FFTFrameStub.cpp:32
> > +#if !OS(DARWIN) && !USE(WEBAUDIO_MKL) && !USE(WEBAUDIO_FFMPEG) && !USE(GSTREAMER)
> 
> I think you'd better use "WEBAUDIO" prefix, such as WEBAUDIO_GSTREAMER.

Right, I guess it'd be useful for ports using GStreamer and using a different lib for FFT processing.

[Philippe Normand]

(In reply to comment #5)
> Wow, everyone is doing FFTs :) 
> 
> Intel also has one for an IPP backend:
> https://bugs.webkit.org/show_bug.cgi?id=75522
> 
> Please see comments in that bug about the layout test which you will want to try as part of validation.

Hum it doesn't look good :/


PASS Initial latency of convolver is silent.
PASS Triangular portion of convolution is correct.
FAIL First part of tail of convolution is not sufficiently small: -129.7413645172823 dB
FAIL Test signal was not correctly convolved.
PASS successfullyParsed is true

One question, do I need to apply a function window to the timed data before performing the FFT?

[Chris Rogers]

(In reply to comment #9)
> (In reply to comment #5)
> > Wow, everyone is doing FFTs :) 
> > 
> > Intel also has one for an IPP backend:
> > https://bugs.webkit.org/show_bug.cgi?id=75522
> > 
> > Please see comments in that bug about the layout test which you will want to try as part of validation.
> 
> Hum it doesn't look good :/
> 
> 
> PASS Initial latency of convolver is silent.
> PASS Triangular portion of convolution is correct.
> FAIL First part of tail of convolution is not sufficiently small: -129.7413645172823 dB
> FAIL Test signal was not correctly convolved.
> PASS successfullyParsed is true
> 
> One question, do I need to apply a function window to the timed data before performing the FFT?

No, don't worry about a window.  The FFTConvolver doesn't want any window applied.

Also, don't worry about the failing test.  -129 dB is really quite good and we can adjust the threshold to be less stringent so the test passes.  In fact, the IPP FFT is also adjust this threshold in a similar way if you look at that patch:
https://bugs.webkit.org/show_bug.cgi?id=75522

You can feel free to adjust the layout test so that -129.7413645172823 dB passes

[Philippe Normand]

Created attachment 122627 [details]
FFTFrameGStreamer implementation

[Philippe Normand]

Comment on attachment 122627 [details]
FFTFrameGStreamer implementation

I forgot to update some Makefiles. Will send a new version soon.

[Philippe Normand]

Created attachment 122727 [details]
FFTFrameGStreamer implementation

[Philippe Normand]

Hey Sebastian!
Would you mind having a look at this patch? When you find some time, please :)

[Sebastian Dröge (slomo)]

Comment on attachment 122727 [details]
FFTFrameGStreamer implementation

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

> Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:29
> +unsigned unpackedFFTDataSize(unsigned fftSize)

Shouldn't this be static or something to prevent it from being exported?

> Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:48
> +    ASSERT(1UL << m_log2FFTSize == m_FFTSize);

GstFFT can handle FFT sizes that are multiples of only 2, 3, 5 fast (e.g. 30 too) but can also handle every other size (but might be slow)

> Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:52
> +    int fftLength = gst_fft_next_fast_length(m_FFTSize);

This doesn't make sense, you're forcing it to a power of two anyway ;) Either remove that assertion or this here.

> Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:154
> +    for (unsigned i = 0; i < halfSize; i++) {

Hm, aren't you dropping one value here? It's n/2 + 1, not n/2.

> Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:175
> +    const float scaleFactor = 1.0 / (2 * m_FFTSize);

You only have to divide by m_FFTSize IIRC. Or divide by 1/sqrt(m_FFTSize) after both transformations.

[Philippe Normand]

(In reply to comment #15)
> (From update of attachment 122727 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=122727&action=review
> 
> > Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:29
> > +unsigned unpackedFFTDataSize(unsigned fftSize)
> 
> Shouldn't this be static or something to prevent it from being exported?
> 

I think it's fine as it is. The anonymous namespace keeps this function private to this file, I think.

> > Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:48
> > +    ASSERT(1UL << m_log2FFTSize == m_FFTSize);
> 
> GstFFT can handle FFT sizes that are multiples of only 2, 3, 5 fast (e.g. 30 too) but can also handle every other size (but might be slow)
> 

The other 2 FFTFrame implementations I can see in WebKit trunk force a power-of-two size, I think I'll just remove this ASSERT and keep using gst_fft_next_fast_length :)

> > Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:52
> > +    int fftLength = gst_fft_next_fast_length(m_FFTSize);
> 
> This doesn't make sense, you're forcing it to a power of two anyway ;) Either remove that assertion or this here.
> 
> > Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:154
> > +    for (unsigned i = 0; i < halfSize; i++) {
> 
> Hm, aren't you dropping one value here? It's n/2 + 1, not n/2.
> 

I think it's n/2, it's the size of the m_realData and m_imagData arrays.

> > Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:175
> > +    const float scaleFactor = 1.0 / (2 * m_FFTSize);
> 
> You only have to divide by m_FFTSize IIRC. Or divide by 1/sqrt(m_FFTSize) after both transformations.

Ah Ok, I think I just did like in another FFTFrame implementation ;)

[Sebastian Dröge (slomo)]

(In reply to comment #16)
> (In reply to comment #15)
> > (From update of attachment 122727 [details] [details])

> > > Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:154
> > > +    for (unsigned i = 0; i < halfSize; i++) {
> > 
> > Hm, aren't you dropping one value here? It's n/2 + 1, not n/2.
> > 
> 
> I think it's n/2, it's the size of the m_realData and m_imagData arrays.

Where is the remaining one real and one imaginary value then?

[Philippe Normand]

Created attachment 123540 [details]
GstFFTFrame

[Philippe Normand]

Comment on attachment 123540 [details]
GstFFTFrame

Will send a new version replacing g_new with fastNewArray calls.

[Philippe Normand]

Created attachment 123952 [details]
GstFFTFrame

[Philippe Normand]

(In reply to comment #20)
> Created an attachment (id=123952) [details]
> GstFFTFrame

Hi Chris, can you please review this patch when you get time? Thanks!

[WebKit Review Bot]

Attachment 123952 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1

Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:25:  You should add a blank line after implementation file's own header.  [build/include_order] [4]
Total errors found: 1 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Created attachment 123962 [details]
GstFFTFrame

Making style-bot happy.

[Chris Rogers]

Comment on attachment 123962 [details]
GstFFTFrame

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

> Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:47
> +    m_complexData = WTF::fastNewArray<GstFFTF32Complex>(unpackedFFTDataSize(m_FFTSize));

It seems odd to use fastNewArray for the allocation.  I don't know if it's "wrong" or not, but FFTFrameFFMPEG and FFTFrameMKL simply use an AudioFloatArray

> Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:123
> +    for (size_t i = 0; i < unpackedFFTDataSize(m_FFTSize); ++i) {

Thanks to Intel, you should be able to optimize this loop using the new VectorMath::zvmul()  and VectorMath::vsmul() functions.  You can look at FFTFrameMac and FFTFrameFFMPEG as an example

> Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:145
> +    for (size_t i = 0; i < unpackedFFTDataSize(m_FFTSize); ++i) {

I think you could use VectorMath::vsmul() here

> Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:166
> +    for (unsigned i = 0; i < m_FFTSize; ++i)

could use VectorMath::vsmul() here

[Philippe Normand]

(In reply to comment #24)
> (From update of attachment 123962 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=123962&action=review
> 
> > Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:47
> > +    m_complexData = WTF::fastNewArray<GstFFTF32Complex>(unpackedFFTDataSize(m_FFTSize));
> 
> It seems odd to use fastNewArray for the allocation.  I don't know if it's "wrong" or not, but FFTFrameFFMPEG and FFTFrameMKL simply use an AudioFloatArray
> 

Well the GstFFT API stores the complex data in GstFFTF32Complex structures. Not sure I can do anything about that :)

> > Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:123
> > +    for (size_t i = 0; i < unpackedFFTDataSize(m_FFTSize); ++i) {
> 
> Thanks to Intel, you should be able to optimize this loop using the new VectorMath::zvmul()  and VectorMath::vsmul() functions.  You can look at FFTFrameMac and FFTFrameFFMPEG as an example
> 

Oh, nice!

> > Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:145
> > +    for (size_t i = 0; i < unpackedFFTDataSize(m_FFTSize); ++i) {
> 
> I think you could use VectorMath::vsmul() here
> 

Hum, really? How can I trick it to access the .i and .r fields of GstFFTF32Complex?

> > Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:166
> > +    for (unsigned i = 0; i < m_FFTSize; ++i)
> 
> could use VectorMath::vsmul() here

Ok.

[Philippe Normand]

Created attachment 124088 [details]
GstFFTFrame

[Gustavo Noronha (kov)]

Comment on attachment 124088 [details]
GstFFTFrame

Attachment 124088 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11191386

[Chris Rogers]

Comment on attachment 124088 [details]
GstFFTFrame

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

Philippe, the FFTFrame part looks good overall.  I added one comment there.  It's very encouraging that your code is passing the convolution layout test (with your slight threshold change).  That test really puts your code to work!

> Source/WebCore/platform/audio/gstreamer/FFTFrameGStreamer.cpp:151
> +    VectorMath::vsmul(processedRealData.data(), 1, &scaleFactor, realData, 1, size);

For lines 143:151 I'd recommend either reverting back to your previous approach (not using VectorMath::vsmul()) or calling VectorMath::vsmul() DIRECTLY on m_complexData,
since effectively it's a floating point vector (with alternating REAL/IMAG values).  To operate directly on m_complexData you'd have to use a static_cast<>

[Philippe Normand]

Created attachment 124698 [details]
GstFFTFrame

[WebKit Review Bot]

Attachment 124698 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit']" exit_code: 9

Updating OpenSource
First, rewinding head to replay your work on top of it...
Applying: Fix compilation errors on build-webkit --debug --no-workers on mac.
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging LayoutTests/ChangeLog
CONFLICT (content): Merge conflict in LayoutTests/ChangeLog
Auto-merging LayoutTests/platform/qt/Skipped
CONFLICT (content): Merge conflict in LayoutTests/platform/qt/Skipped
Auto-merging Source/WebCore/ChangeLog
CONFLICT (content): Merge conflict in Source/WebCore/ChangeLog
Failed to merge in the changes.
Patch failed at 0001 Fix compilation errors on build-webkit --debug --no-workers on mac.

When you have resolved this problem run "git rebase --continue".
If you would prefer to skip this patch, instead run "git rebase --skip".
To restore the original branch and stop rebasing run "git rebase --abort".

rebase refs/remotes/origin/master: command returned error: 1

Died at Tools/Scripts/update-webkit line 164.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Philippe Normand]

Chris, you're officially a reviewer now, no? An official review of this patch would be very welcome! :)

[Chris Rogers]

Hi Philippe, I'm not sure if my reviewer status has yet changed yet, since I haven't heard any official word.  But I can check...  Do you know what the "style" problem with the latest patch is?

[Philippe Normand]

(In reply to comment #32)
> Hi Philippe, I'm not sure if my reviewer status has yet changed yet, since I haven't heard any official word.  But I can check...  Do you know what the "style" problem with the latest patch is?

The style bot got crippled yesterday. I can attach a new patch just to make sure it's coding style is ok.

[Philippe Normand]

Created attachment 124981 [details]
FFTFrame

Same one as before but to go through now-sane style-bot.

[Philippe Normand]

Committed r106537: <http://trac.webkit.org/changeset/106537>