Bug 48810 – Add RealtimeAnalyser files

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Rogers]

Add RealtimeAnalyser files

[Chris Rogers]

Created attachment 72606 [details]
Patch

[Kenneth Russell]

Comment on attachment 72606 [details]
Patch

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.

[Chris Rogers]

Comment on attachment 72606 [details]
Patch

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

[Chris Rogers]

Created attachment 72713 [details]
Patch

[Kenneth Russell]

Comment on attachment 72713 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 72713 [details]
Patch

Clearing flags on attachment: 72713

Committed r71193: <http://trac.webkit.org/changeset/71193>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.