[Webkit-unassigned] [Bug 48810] Add RealtimeAnalyser files

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48810


Kenneth Russell <kbr at google.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #72606|review?                     |review-
               Flag|                            |




--- Comment #2 from Kenneth Russell <kbr at google.com>  2010-11-01 19:26:48 PST ---
(From update of attachment 72606)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.