Add support for using ARM FFT in WebAudio

[Raymond Toy]

Add support for using ARM FFT in WebAudio

[Raymond Toy]

Created attachment 188194 [details]
Patch

[Raymond Toy]

Blocked by https://codereview.chromium.org/12260023/ which creates the required use_arm_fft variable.

[Raymond Toy]

+ felipeg@

[felipe]

(In reply to comment #3)
> + felipeg@


LGTM

[Raymond Toy]

Created attachment 189788 [details]
Patch

[Raymond Toy]

Please review at your convenience.  This patch adds support for using the ARM FFT routines from OpenMAX DL.

As mentioned earlier, this cannot be checked in until https://codereview.chromium.org/12260023/ is reviewed and checked in.  That CL creates the necessary use_arm_fft variable to control whether the ARM FFT routines are used.

Also, use_arm_fft cannot be enabled until the new third party library in third_party/openmax_dl is made available.

[Peter Beverloo (cr-android ews)]

Comment on attachment 189788 [details]
Patch

Attachment 189788 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/16783347

[Kenneth Russell]

Comment on attachment 189788 [details]
Patch

You may need to roll forward the Chromium revision in Source/WebKit/chromium/DEPS in order to pick up the use_arm_fft gyp variable.

[Chris Rogers]

Comment on attachment 189788 [details]
Patch

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

The FFT code itself seems pretty straight-forward.  Have you been able to run your code against the Web Audio layout tests, especially "convolution-mono-mono.html"??    Somebody like Tony Chang should review the .gyp file changes, since I'm not the best reviewer for that part.

Seems like this shouldn't land until the actual chromium third_party library is in place, R- for right now

> Source/WebCore/ChangeLog:3
> +        Add support for using ARM FFT in WebAudio

Instead of using the term "ARM FFT" I think it's better to be more specific.  For example, in the different FFTFrame implementations, we don't have one called FFTFrameIntel, since there are several different library back-ends which target x86.

I'm not sure what the best name is.  Maybe it's "OpenMax"?  But it should be more specific since it's possible there could be different "ARM" libraries.

This name change should be taken into account for the ChangeLog comment, the file-name FFTFrameARM, WTF_USE_WEBAUDIO_ARMFFT, etc.

> Source/WebCore/platform/audio/android/FFTFrameARM.cpp:41
> +

nit: extra blank line

> Source/WebCore/platform/audio/android/FFTFrameARM.cpp:173
> +

nit: extra blank line

> Source/WebCore/platform/audio/android/FFTFrameARM.cpp:191
> +    int bufSize;

bufSize should be defined just before usage (line 195)

> Source/WebCore/platform/audio/android/FFTFrameARM.cpp:192
> +    OMXResult status;

nit: this line should be merged with line 195

> Source/WebCore/platform/audio/android/FFTFrameARM.cpp:193
> +    OMXFFTSpec_R_F32* context = 0;

context should be defined as late as possible (just before 197)

> Source/WebCore/platform/audio/android/FFTFrameARM.cpp:198
> +        context = (OMXFFTSpec_R_F32*) malloc(bufSize);

use static_cast<>

[Raymond Toy]

(In reply to comment #9)
> (From update of attachment 189788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189788&action=review
> 
> The FFT code itself seems pretty straight-forward.  Have you been able to run your code against the Web Audio layout tests, especially "convolution-mono-mono.html"??    Somebody like Tony Chang should review the .gyp file changes, since I'm not the best reviewer for that part.
> 
> Seems like this shouldn't land until the actual chromium third_party library is in place, R- for right now

When the library is in place, I'll update this code accordingly.
> 
> > Source/WebCore/ChangeLog:3
> > +        Add support for using ARM FFT in WebAudio
> 
> Instead of using the term "ARM FFT" I think it's better to be more specific.  For example, in the different FFTFrame implementations, we don't have one called FFTFrameIntel, since there are several different library back-ends which target x86.
> 
> I'm not sure what the best name is.  Maybe it's "OpenMax"?  But it should be more specific since it's possible there could be different "ARM" libraries.

How about "OpenMAX_DL", since that is the actual name of the OpenMAX spec/library that we are using.
> 
> This name change should be taken into account for the ChangeLog comment, the file-name FFTFrameARM, WTF_USE_WEBAUDIO_ARMFFT, etc.

Yes, I will update the names accordingly.
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:41
> > +
> 
> nit: extra blank line
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:173
> > +
> 
> nit: extra blank line
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:191
> > +    int bufSize;
> 
> bufSize should be defined just before usage (line 195)
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:192
> > +    OMXResult status;
> 
> nit: this line should be merged with line 195
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:193
> > +    OMXFFTSpec_R_F32* context = 0;
> 
> context should be defined as late as possible (just before 197)
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:198
> > +        context = (OMXFFTSpec_R_F32*) malloc(bufSize);
> 
> use static_cast<>

[Chris Rogers]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 189788 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=189788&action=review
> > 
> > The FFT code itself seems pretty straight-forward.  Have you been able to run your code against the Web Audio layout tests, especially "convolution-mono-mono.html"??    Somebody like Tony Chang should review the .gyp file changes, since I'm not the best reviewer for that part.
> > 
> > Seems like this shouldn't land until the actual chromium third_party library is in place, R- for right now
> 
> When the library is in place, I'll update this code accordingly.
> > 
> > > Source/WebCore/ChangeLog:3
> > > +        Add support for using ARM FFT in WebAudio
> > 
> > Instead of using the term "ARM FFT" I think it's better to be more specific.  For example, in the different FFTFrame implementations, we don't have one called FFTFrameIntel, since there are several different library back-ends which target x86.
> > 
> > I'm not sure what the best name is.  Maybe it's "OpenMax"?  But it should be more specific since it's possible there could be different "ARM" libraries.
> 
> How about "OpenMAX_DL", since that is the actual name of the OpenMAX spec/library that we are using.

Seems ok with me.  I'm not sure about the "_" as far as the style guide goes.  Maybe Ken Russell or others would have an opinion there.

> > 
> > This name change should be taken into account for the ChangeLog comment, the file-name FFTFrameARM, WTF_USE_WEBAUDIO_ARMFFT, etc.
> 
> Yes, I will update the names accordingly.
> > 
> > > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:41
> > > +
> > 
> > nit: extra blank line
> > 
> > > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:173
> > > +
> > 
> > nit: extra blank line
> > 
> > > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:191
> > > +    int bufSize;
> > 
> > bufSize should be defined just before usage (line 195)
> > 
> > > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:192
> > > +    OMXResult status;
> > 
> > nit: this line should be merged with line 195
> > 
> > > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:193
> > > +    OMXFFTSpec_R_F32* context = 0;
> > 
> > context should be defined as late as possible (just before 197)
> > 
> > > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:198
> > > +        context = (OMXFFTSpec_R_F32*) malloc(bufSize);
> > 
> > use static_cast<>

[Raymond Toy]

Created attachment 191568 [details]
Patch

[Raymond Toy]

Created attachment 191593 [details]
Patch

[Chris Rogers]

Is this ready for review?

[Raymond Toy]

(In reply to comment #14)
> Is this ready for review?

Almost.  I need to make one more update due to a change in the name of the FFT library.  I'll upload that shortly.

[Raymond Toy]

Created attachment 191793 [details]
Patch

[Raymond Toy]

(In reply to comment #9)
> (From update of attachment 189788 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189788&action=review
> 
> The FFT code itself seems pretty straight-forward.  Have you been able to run your code against the Web Audio layout tests, especially "convolution-mono-mono.html"??    Somebody like Tony Chang should review the .gyp file changes, since I'm not the best reviewer for that part.

I have not yet been able to run the webaudio layout tests, but I did run convolution-mono-mono.html which passes.

> 
> Seems like this shouldn't land until the actual chromium third_party library is in place, R- for right now
> 
> > Source/WebCore/ChangeLog:3
> > +        Add support for using ARM FFT in WebAudio
> 
> Instead of using the term "ARM FFT" I think it's better to be more specific.  For example, in the different FFTFrame implementations, we don't have one called FFTFrameIntel, since there are several different library back-ends which target x86.
> 
> I'm not sure what the best name is.  Maybe it's "OpenMax"?  But it should be more specific since it's possible there could be different "ARM" libraries.
> 
> This name change should be taken into account for the ChangeLog comment, the file-name FFTFrameARM, WTF_USE_WEBAUDIO_ARMFFT, etc.
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:41
> > +
> 
> nit: extra blank line
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:173
> > +
> 
> nit: extra blank line
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:191
> > +    int bufSize;
> 
> bufSize should be defined just before usage (line 195)
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:192
> > +    OMXResult status;
> 
> nit: this line should be merged with line 195
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:193
> > +    OMXFFTSpec_R_F32* context = 0;
> 
> context should be defined as late as possible (just before 197)
> 
> > Source/WebCore/platform/audio/android/FFTFrameARM.cpp:198
> > +        context = (OMXFFTSpec_R_F32*) malloc(bufSize);
> 
> use static_cast<>

[Peter Beverloo (cr-android ews)]

Comment on attachment 191793 [details]
Patch

Attachment 191793 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17057081

[Raymond Toy]

Comment on attachment 191793 [details]
Patch

Please review when you can.  I believe all review comments have been addressed.  Also, please note that this cannot be committed until https://codereview.chromium.org/12260023/  lands, which adds the required use_openmax_dl_fft gyp variable.  This is what is currently causing the cr-android bot to fail.

[Tony Chang]

Comment on attachment 191793 [details]
Patch

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

Please get a green cr-android before landing this patch (i.e., land the chromium side change, roll WebKit DEPS, reupload the patch).

> Source/WebCore/WebCore.gyp/WebCore.gyp:1941
> +            ['include', 'platform/audio/android/FFTFrameOpenMAXDL\\.cpp$'],

Nit: It's probably a bit better to name this file platform/audio/chromium/FFTFrameOpenMAXDLAndroid.cpp.  It looks like we don't have any other android subdirectories (since they are part of the chromium port) and we have a filename regex rule that already handles files that end in Android.cpp.  Which means you don't need this line if the filename ends in Android.cpp.

> Source/WebKit/chromium/features.gypi:229
> +      ['OS!="mac"', {
> +        'conditions' : [
> +          ['OS=="android"', {

Nit: The nesting here is confusing.  I would keep the OS!="android" and OS!="mac" section and add a section below OS=="android" that is OS=="android" and use_openmax_dl_fft!=0 for setting WTF_USE_WEBAUDIO_OPENMAX_DL_FFT=1.

[Raymond Toy]

Created attachment 192053 [details]
Patch

[Raymond Toy]

Comment on attachment 191793 [details]
Patch

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

>> Source/WebCore/WebCore.gyp/WebCore.gyp:1941
>> +            ['include', 'platform/audio/android/FFTFrameOpenMAXDL\\.cpp$'],
> 
> Nit: It's probably a bit better to name this file platform/audio/chromium/FFTFrameOpenMAXDLAndroid.cpp.  It looks like we don't have any other android subdirectories (since they are part of the chromium port) and we have a filename regex rule that already handles files that end in Android.cpp.  Which means you don't need this line if the filename ends in Android.cpp.

Renamed file as suggested.  Updated WebCore.gypi with the new name.

>> Source/WebKit/chromium/features.gypi:229
>> +          ['OS=="android"', {
> 
> Nit: The nesting here is confusing.  I would keep the OS!="android" and OS!="mac" section and add a section below OS=="android" that is OS=="android" and use_openmax_dl_fft!=0 for setting WTF_USE_WEBAUDIO_OPENMAX_DL_FFT=1.

Yes, this is clearer.  Fixed as sugggested.

[Peter Beverloo (cr-android ews)]

Comment on attachment 192053 [details]
Patch

Attachment 192053 [details] did not pass cr-android-ews (chromium-android):
Output: http://webkit-commit-queue.appspot.com/results/17013432

[Raymond Toy]

Created attachment 194868 [details]
Patch

[Peter Beverloo]

Comment on attachment 194868 [details]
Patch

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

> Source/WebCore/WebCore.gyp/WebCore.gyp:285
> +         '<(DEPTH)/third_party/openmax_dl'

Where does the openmax_dl directory come from? The directory doesn't seem to exist in the Chromium tree, nor is defined in the DEPS files..

> Source/WebKit/chromium/features.gypi:236
> +          'ENABLE_WEB_AUDIO=1',

So building Chromium with use_openmax_dl_fft=1 enables the Web Audio API for Android? I don't think this is clear from the variable's name, nor is it documented in common.gypi. Should a comment be added somewhere to clarify this?

[Raymond Toy]

Comment on attachment 194868 [details]
Patch

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

>> Source/WebCore/WebCore.gyp/WebCore.gyp:285
>> +         '<(DEPTH)/third_party/openmax_dl'
> 
> Where does the openmax_dl directory come from? The directory doesn't seem to exist in the Chromium tree, nor is defined in the DEPS files..

The DEPS change that adds the openmax_dl directory was reverted due to a license check failure.  I'm working on that.

>> Source/WebKit/chromium/features.gypi:236
>> +          'ENABLE_WEB_AUDIO=1',
> 
> So building Chromium with use_openmax_dl_fft=1 enables the Web Audio API for Android? I don't think this is clear from the variable's name, nor is it documented in common.gypi. Should a comment be added somewhere to clarify this?

Yes, I can add a comment.  

Setting use_openmax_dl_fft=1 is enough to get oscillators and most parts of webaudio working.  The other part (decoding audio files from memory) is not yet done, but it would always be compiled in since it only depends on existing functionality in chromium and Android.

[Raymond Toy]

Created attachment 194886 [details]
Patch

[Raymond Toy]

Comment on attachment 194868 [details]
Patch

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

>>> Source/WebKit/chromium/features.gypi:236
>>> +          'ENABLE_WEB_AUDIO=1',
>> 
>> So building Chromium with use_openmax_dl_fft=1 enables the Web Audio API for Android? I don't think this is clear from the variable's name, nor is it documented in common.gypi. Should a comment be added somewhere to clarify this?
> 
> Yes, I can add a comment.  
> 
> Setting use_openmax_dl_fft=1 is enough to get oscillators and most parts of webaudio working.  The other part (decoding audio files from memory) is not yet done, but it would always be compiled in since it only depends on existing functionality in chromium and Android.

Comment added.

[Chris Rogers]

Comment on attachment 194886 [details]
Patch

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

Looks ok if comments are addressed --- I'm trusting that the layout test "convolution-mono-mono.html" has been run and passes.

> Source/WebCore/platform/audio/chromium/FFTFrameOpenMAXDLAndroid.cpp:136
> +        for (unsigned k = 1; k < m_FFTSize / 2; ++k) {

It's better to calculate m_FFTSize / 2 outside of this loop and put in local var.

> Source/WebCore/platform/audio/chromium/FFTFrameOpenMAXDLAndroid.cpp:154
> +        for (unsigned k = 1; k < m_FFTSize / 2; ++k) {

It's better to calculate m_FFTSize / 2 outside of this loop and put in local var.

[Raymond Toy]

Created attachment 195099 [details]
Patch

[Raymond Toy]

Comment on attachment 194886 [details]
Patch

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

>> Source/WebCore/platform/audio/chromium/FFTFrameOpenMAXDLAndroid.cpp:136
>> +        for (unsigned k = 1; k < m_FFTSize / 2; ++k) {
> 
> It's better to calculate m_FFTSize / 2 outside of this loop and put in local var.

Done.

>> Source/WebCore/platform/audio/chromium/FFTFrameOpenMAXDLAndroid.cpp:154
>> +        for (unsigned k = 1; k < m_FFTSize / 2; ++k) {
> 
> It's better to calculate m_FFTSize / 2 outside of this loop and put in local var.

Done.

[Raymond Toy]

(In reply to comment #29)
> (From update of attachment 194886 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194886&action=review
> 
> Looks ok if comments are addressed --- I'm trusting that the layout test "convolution-mono-mono.html" has been run and passes.

Comments addressed.  Yes, the convolution-mono-mono test passes, except I ran a shorter version.  I will address this test and the remaining webaudio tests when the rest of the Android support is implemented.

> 
> > Source/WebCore/platform/audio/chromium/FFTFrameOpenMAXDLAndroid.cpp:136
> > +        for (unsigned k = 1; k < m_FFTSize / 2; ++k) {
> 
> It's better to calculate m_FFTSize / 2 outside of this loop and put in local var.
> 
> > Source/WebCore/platform/audio/chromium/FFTFrameOpenMAXDLAndroid.cpp:154
> > +        for (unsigned k = 1; k < m_FFTSize / 2; ++k) {
> 
> It's better to calculate m_FFTSize / 2 outside of this loop and put in local var.

[Raymond Toy]

Created attachment 195149 [details]
Patch

[Raymond Toy]

Created attachment 195630 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 195630 [details]
Patch for landing

Rejecting attachment 195630 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-03', 'validate-changelog', '--non-interactive', 195630, '--port=chromium-xvfb']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

[e-mail] found in /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog does not appear to be a valid reviewer according to committers.py.
/mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-commit-queue.appspot.com/results/17288733

[Raymond Toy]

Created attachment 196212 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 196212 [details]
Patch for landing

Clearing flags on attachment: 196212

Committed r147491: <http://trac.webkit.org/changeset/147491>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.