Bug 47623 - Add AudioResampler files
Summary: Add AudioResampler files
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-10-13 14:41 PDT by Chris Rogers
Modified: 2010-10-18 18:59 PDT (History)
9 users (show)

See Also:

Attachments
Patch (8.36 KB, patch)
2010-10-13 14:43 PDT, Chris Rogers 		no flags Details | Formatted Diff | Diff
Patch (8.45 KB, patch)
2010-10-18 12:46 PDT, Chris Rogers 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.