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 :)
I would be interested in this bug. Would you have any plan to implement it soon?
(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.
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.
Created attachment 121865 [details] FFTFrameGStreamer implementation Rebased against ToT
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.
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.
(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!
(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.
(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?
(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
Created attachment 122627 [details] FFTFrameGStreamer implementation
Comment on attachment 122627 [details] FFTFrameGStreamer implementation I forgot to update some Makefiles. Will send a new version soon.
Created attachment 122727 [details] FFTFrameGStreamer implementation
Hey Sebastian! Would you mind having a look at this patch? When you find some time, please :)
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.
(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 ;)
(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?
Created attachment 123540 [details] GstFFTFrame
Comment on attachment 123540 [details] GstFFTFrame Will send a new version replacing g_new with fastNewArray calls.
Created attachment 123952 [details] GstFFTFrame
(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!
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.
Created attachment 123962 [details] GstFFTFrame Making style-bot happy.
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
(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.
Created attachment 124088 [details] GstFFTFrame
Comment on attachment 124088 [details] GstFFTFrame Attachment 124088 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11191386
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<>
Created attachment 124698 [details] GstFFTFrame
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.
Chris, you're officially a reviewer now, no? An official review of this patch would be very welcome! :)
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?
(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.
Created attachment 124981 [details] FFTFrame Same one as before but to go through now-sane style-bot.
Committed r106537: <http://trac.webkit.org/changeset/106537>