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

[bugzilla-daemon at webkit.org]

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





--- Comment #3 from Chris Rogers <crogers at google.com>  2010-11-02 12:16:40 PST ---
(From update of attachment 72606)
View in context: https://bugs.webkit.org/attachment.cgi?id=72606&action=review

>> 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.

Actually jamesr thought the problem was the other way where MSVC didn't like the definition in the header, but required it in the .cpp

>> 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.

FIXED

>> 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.

FIXED

>> 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.

In this situation, compared with the cost of doing the windowing and the FFT, the cost of malloc() is extremely small.

>> WebCore/webaudio/RealtimeAnalyser.cpp:158
>> +    // FIXME : optimize with memcpy()
> 
> Add period at end of sentence.

FIXED

>> WebCore/webaudio/RealtimeAnalyser.cpp:215
>> +            double dbMag = !linearValue ? MinDecibels : 20.0 * log10(linearValue);
> 
> What's this 20.0 constant? Can it be defined somewhere?

Sorry, I forgot I had defined a function for this:
AudioUtilities::linearToDecibels()

FIXED

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

FIXED: switched to AudioUtilities::linearToDecibels()

>> 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.

FIXED

>> WebCore/webaudio/RealtimeAnalyser.cpp:288
>> +                scaledValue = 255.0;
> 
> These should use UCHAR_MAX.

FIXED

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