[webkit-reviews] review denied: [Bug 48810] Add RealtimeAnalyser files : [Attachment 72606] Patch

[bugzilla-daemon at webkit.org]

Kenneth Russell <kbr at google.com> has denied Chris Rogers <crogers at google.com>'s
request for review:
Bug 48810: Add RealtimeAnalyser files
https://bugs.webkit.org/show_bug.cgi?id=48810

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

------- Additional Comments from Kenneth Russell <kbr at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=72606&action=review

A few relatively minor issues.

> WebCore/webaudio/RealtimeAnalyser.cpp:53
> +const unsigned RealtimeAnalyser::InputBufferSize =
RealtimeAnalyser::MaxFFTSize * 2;

I recall that you may have compilation problems on Windows with static
constants. I think the MSVC compiler wants the definition in the header and
none in the .cpp file. However I guess we'll cross that bridge when we come to
it.

> WebCore/webaudio/RealtimeAnalyser.cpp:109
> +	   return;

This check is redundant with the second half of the isDestinationGood check
below, and has the disadvantage that it uses the InputBufferSize constant
instead of m_inputBuffer.size(). It should be removed and the isDestinationGood
check hoisted up to here.

> WebCore/webaudio/RealtimeAnalyser.cpp:129
> +static void ApplyWindow(float* p, size_t n)

WebKit style is lower case for first letter. Also, consider using an anonymous
namespace rather than a static function.

> WebCore/webaudio/RealtimeAnalyser.cpp:153
> +    AudioFloatArray temporaryBuffer(fftSize);

Isn't the reallocation of this array expensive? If so consider caching it and
only reallocating if fftSize changes.

> WebCore/webaudio/RealtimeAnalyser.cpp:158
> +    // FIXME : optimize with memcpy()

Add period at end of sentence.

> WebCore/webaudio/RealtimeAnalyser.cpp:215
> +	       double dbMag = !linearValue ? MinDecibels : 20.0 *
log10(linearValue);

What's this 20.0 constant? Can it be defined somewhere?

> WebCore/webaudio/RealtimeAnalyser.cpp:241
> +	       double dbMag = !linearValue ? m_minDecibels : 20.0 *
log10(linearValue);

Same comment about the 20.0 constant.

> WebCore/webaudio/RealtimeAnalyser.cpp:244
> +	       double scaledValue = 255.0 * (dbMag - m_minDecibels) *
RangeScaleFactor;

Please #include <limits.h> and use UCHAR_MAX rather than 255.0 sprinkled
everywhere.

> WebCore/webaudio/RealtimeAnalyser.cpp:282
> +	       double scaledValue = 128.0 * (value + 1.0);

Unfortunately SCHAR_MAX is 127 and not 128, but if there is a reasonable way to
express this scaling in terms of predefined constants please do so.

> WebCore/webaudio/RealtimeAnalyser.cpp:288
> +	       if (scaledValue > 255.0)
> +		   scaledValue = 255.0;

These should use UCHAR_MAX.