[webkit-reviews] review denied: [Bug 109755] Add support for using ARM FFT in WebAudio : [Attachment 189788] Patch

[bugzilla-daemon at webkit.org]

Chris Rogers <crogers at google.com> has denied Raymond Toy <rtoy at chromium.org>'s
request for review:
Bug 109755: Add support for using ARM FFT in WebAudio
https://bugs.webkit.org/show_bug.cgi?id=109755

Attachment 189788: Patch
https://bugs.webkit.org/attachment.cgi?id=189788&action=review

------- Additional Comments from Chris Rogers <crogers at google.com>
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<>