Bug 73545 - [GStreamer] FFTFrame implementation
Summary: [GStreamer] FFTFrame implementation
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 61355
  Show dependency treegraph
 
Reported: 2011-12-01 02:52 PST by Philippe Normand
Modified: 2012-02-02 00:04 PST (History)
8 users (show)

See Also:


Attachments
FFTFrameGStreamer implementation (13.15 KB, patch)
2012-01-10 10:17 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
FFTFrameGStreamer implementation (13.04 KB, patch)
2012-01-10 10:26 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
FFTFrameGStreamer implementation (14.81 KB, patch)
2012-01-16 06:56 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
FFTFrameGStreamer implementation (17.44 KB, patch)
2012-01-17 00:35 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
GstFFTFrame (17.15 KB, patch)
2012-01-23 04:33 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
GstFFTFrame (17.28 KB, patch)
2012-01-25 09:11 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
GstFFTFrame (17.28 KB, patch)
2012-01-25 09:48 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
GstFFTFrame (17.38 KB, patch)
2012-01-26 02:51 PST, Philippe Normand
gustavo: commit-queue-
Details | Formatted Diff | Diff
GstFFTFrame (17.15 KB, patch)
2012-01-31 03:17 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
FFTFrame (17.19 KB, patch)
2012-02-01 11:52 PST, Philippe Normand
crogers: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2011-12-01 02:52:34 PST
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 :)
Comment 1 Dongwoo Joshua Im (dwim) 2011-12-28 00:57:01 PST
I would be interested in this bug. 
Would you have any plan to implement it soon?
Comment 2 Philippe Normand 2011-12-28 01:27:19 PST
(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.
Comment 3 Philippe Normand 2012-01-10 10:17:17 PST
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.
Comment 4 Philippe Normand 2012-01-10 10:26:25 PST
Created attachment 121865 [details]
FFTFrameGStreamer implementation

Rebased against ToT
Comment 5 Chris Rogers 2012-01-10 14:40:52 PST
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.
Comment 6 Dongwoo Joshua Im (dwim) 2012-01-10 18:09:34 PST
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.
Comment 7 Philippe Normand 2012-01-11 00:03:23 PST
(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!
Comment 8 Philippe Normand 2012-01-11 00:11:13 PST
(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.
Comment 9 Philippe Normand 2012-01-11 03:26:52 PST
(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?
Comment 10 Chris Rogers 2012-01-11 11:27:51 PST
(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
Comment 11 Philippe Normand 2012-01-16 06:56:41 PST
Created attachment 122627 [details]
FFTFrameGStreamer implementation
Comment 12 Philippe Normand 2012-01-16 09:23:09 PST
Comment on attachment 122627 [details]
FFTFrameGStreamer implementation

I forgot to update some Makefiles. Will send a new version soon.
Comment 13 Philippe Normand 2012-01-17 00:35:19 PST
Created attachment 122727 [details]
FFTFrameGStreamer implementation
Comment 14 Philippe Normand 2012-01-18 04:03:42 PST
Hey Sebastian!
Would you mind having a look at this patch? When you find some time, please :)
Comment 15 Sebastian Dröge (slomo) 2012-01-18 04:32:25 PST
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.
Comment 16 Philippe Normand 2012-01-23 02:50:10 PST
(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 ;)
Comment 17 Sebastian Dröge (slomo) 2012-01-23 03:11:19 PST
(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?
Comment 18 Philippe Normand 2012-01-23 04:33:56 PST
Created attachment 123540 [details]
GstFFTFrame
Comment 19 Philippe Normand 2012-01-25 08:16:06 PST
Comment on attachment 123540 [details]
GstFFTFrame

Will send a new version replacing g_new with fastNewArray calls.
Comment 20 Philippe Normand 2012-01-25 09:11:20 PST
Created attachment 123952 [details]
GstFFTFrame
Comment 21 Philippe Normand 2012-01-25 09:12:10 PST
(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!
Comment 22 WebKit Review Bot 2012-01-25 09:13:49 PST
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.
Comment 23 Philippe Normand 2012-01-25 09:48:42 PST
Created attachment 123962 [details]
GstFFTFrame

Making style-bot happy.
Comment 24 Chris Rogers 2012-01-25 12:22:29 PST
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
Comment 25 Philippe Normand 2012-01-26 01:19:45 PST
(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.
Comment 26 Philippe Normand 2012-01-26 02:51:22 PST
Created attachment 124088 [details]
GstFFTFrame
Comment 27 Gustavo Noronha (kov) 2012-01-26 09:26:52 PST
Comment on attachment 124088 [details]
GstFFTFrame

Attachment 124088 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11191386
Comment 28 Chris Rogers 2012-01-26 12:23:44 PST
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<>
Comment 29 Philippe Normand 2012-01-31 03:17:20 PST
Created attachment 124698 [details]
GstFFTFrame
Comment 30 WebKit Review Bot 2012-01-31 03:18:41 PST
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.
Comment 31 Philippe Normand 2012-02-01 04:08:59 PST
Chris, you're officially a reviewer now, no? An official review of this patch would be very welcome! :)
Comment 32 Chris Rogers 2012-02-01 10:29:13 PST
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?
Comment 33 Philippe Normand 2012-02-01 10:31:12 PST
(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.
Comment 34 Philippe Normand 2012-02-01 11:52:06 PST
Created attachment 124981 [details]
FFTFrame

Same one as before but to go through now-sane style-bot.
Comment 35 Philippe Normand 2012-02-02 00:04:50 PST
Committed r106537: <http://trac.webkit.org/changeset/106537>