[webkit-reviews] review granted: [Bug 52989] Web Audio API: port FFTFrame to FFTW : [Attachment 79889] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has granted Kenneth Russell
<kbr at google.com>'s request for review:
Bug 52989: Web Audio API: port FFTFrame to FFTW
https://bugs.webkit.org/show_bug.cgi?id=52989

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=79889&action=review

r=me. before landing i think chris rogers should give this a once-over, and
please make sure the EWS bots are happy with the gyp changes

> Source/WebCore/platform/audio/FFTFrame.h:123
>  #endif // !OS(DARWIN) && USE(WEBAUDIO_MKL)

nit: comment on this line should be // USE(WEBAUDIO_MKL)

> Source/WebCore/platform/audio/FFTFrame.h:148
> +#endif // !OS(DARWIN) && USE(WEBAUDIO_FFTW)

nit: // USE(WEBAUDIO_FFTW)

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

nit: here (and in other files) it's 2011 now :)

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:74
> +    float temporaryPointer[1];
> +    m_forwardPlan = fftwPlanForSize(fftSize, Forward,
> +				       &temporaryPointer[0], realData(),
imagData());
> +    m_backwardPlan = fftwPlanForSize(fftSize, Backward,
> +					realData(), imagData(),
&temporaryPointer[0]);

nit: i'm not sure i fully grok this, but would 'float temp;
fftwPlanForSize(..., &temp, ...);' do as well? 1-size arrays are odd

> Source/WebCore/platform/audio/fftw/FFTFrameFFTW.cpp:103
> +    unsigned nbytes = sizeof(float) * complexDataSize(m_FFTSize);

nit: number of bytes should be size_t