Bug 47623

Summary: Add AudioResampler files
Product: WebKit Reporter: Chris Rogers <crogers>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cfleizach, 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 Flags
Patch
none
Patch none

Description Chris Rogers 2010-10-13 14:41:44 PDT 
Add AudioResampler files
Comment 1 Chris Rogers 2010-10-13 14:43:01 PDT 
Created attachment 70665 [details]
Patch
Comment 2 chris fleizach 2010-10-18 00:43:58 PDT 
Comment on attachment 70665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70665&action=review

> WebCore/platform/audio/AudioResampler.cpp:105
> +    for (unsigned i = 0; i < m_kernels.size(); ++i) {

dont' call a method within a loop. initialize the length outside the loop

> WebCore/platform/audio/AudioResampler.cpp:121
> +    for (unsigned i = 0; i < m_kernels.size(); ++i)

same here as above

> WebCore/platform/audio/AudioResampler.h:41
> +    AudioResampler(); // default to mono

This comment seems like it should be in the class description above, not inline.

> WebCore/platform/audio/AudioResampler.h:53
> +    // 0 < rate <= MaxRate

seems like MaxRate should be in the header so that clients could use the max rate if they so desired.

> WebCore/platform/audio/AudioResampler.h:59
> +protected:

should probably be private
Comment 3 Chris Rogers 2010-10-18 12:46:24 PDT 
Created attachment 71073 [details]
Patch
Comment 4 Chris Rogers 2010-10-18 12:49:10 PDT 
Comment on attachment 70665 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70665&action=review

>> WebCore/platform/audio/AudioResampler.cpp:105
>> +    for (unsigned i = 0; i < m_kernels.size(); ++i) {
> 
> dont' call a method within a loop. initialize the length outside the loop

FIXED

>> WebCore/platform/audio/AudioResampler.cpp:121
>> +    for (unsigned i = 0; i < m_kernels.size(); ++i)
> 
> same here as above

FIXED

>> WebCore/platform/audio/AudioResampler.h:41
>> +    AudioResampler(); // default to mono
> 
> This comment seems like it should be in the class description above, not inline.

FIXED: moved comment

>> WebCore/platform/audio/AudioResampler.h:53
>> +    // 0 < rate <= MaxRate
> 
> seems like MaxRate should be in the header so that clients could use the max rate if they so desired.

It *is* declared a few lines after this

>> WebCore/platform/audio/AudioResampler.h:59
>> +protected:
> 
> should probably be private

FIXED
Comment 5 chris fleizach 2010-10-18 17:45:14 PDT 
Comment on attachment 71073 [details]
Patch

r=me
Comment 6 WebKit Commit Bot 2010-10-18 18:59:29 PDT 
Comment on attachment 71073 [details]
Patch

Clearing flags on attachment: 71073

Committed r70015: <http://trac.webkit.org/changeset/70015>
Comment 7 WebKit Commit Bot 2010-10-18 18:59:35 PDT 
All reviewed patches have been landed.  Closing bug.