| Summary: | Add RealtimeAnalyser files | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Rogers <crogers> | ||||||
| Component: | New Bugs | Assignee: | Nobody <webkit-unassigned> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cmarrin, commit-queue, dglazkov, eric.carlson, jamesr, jer.noble, kbr, simon.fraser | ||||||
| Priority: | P2 | ||||||||
| Version: | 528+ (Nightly build) | ||||||||
| Hardware: | Other | ||||||||
| OS: | OS X 10.5 | ||||||||
| Attachments: |
|
||||||||
|
Description
Chris Rogers
2010-11-01 17:24:00 PDT
Created attachment 72606 [details]
Patch
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. 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 Created attachment 72713 [details]
Patch
Comment on attachment 72713 [details]
Patch
Looks good.
Comment on attachment 72713 [details] Patch Clearing flags on attachment: 72713 Committed r71193: <http://trac.webkit.org/changeset/71193> All reviewed patches have been landed. Closing bug. |