Bug 47624 - Add AudioResamplerKernel files
Summary: Add AudioResamplerKernel 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:43 PDT by Chris Rogers
Modified: 2010-10-27 15:29 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.75 KB, patch)
2010-10-13 14:45 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (9.97 KB, patch)
2010-10-27 12:12 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2010-10-13 14:43:42 PDT
Add AudioResamplerKernel files
Comment 1 Chris Rogers 2010-10-13 14:45:12 PDT
Created attachment 70667 [details]
Patch
Comment 2 Kenneth Russell 2010-10-26 18:51:03 PDT
Comment on attachment 70667 [details]
Patch

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

A few comments and questions.

> WebCore/platform/audio/AudioResamplerKernel.cpp:59
> +    int endIndex = static_cast<int>(nextFractionalIndex + 1.0); // round up to next integer index

Should this just use ceil()?

> WebCore/platform/audio/AudioResamplerKernel.cpp:102
> +        double sample2 = source[readIndex + 1];

It would be really nice to have some assertions (perhaps hoisted out of the loop) that m_virtualReadIndex + (framesToProcess * rate) + 1 is not going to walk off the end of the source buffer.

> WebCore/platform/audio/AudioResamplerKernel.h:57
> +protected:

Can these members be private rather than protected?
Comment 3 Chris Rogers 2010-10-27 12:07:02 PDT
Comment on attachment 70667 [details]
Patch

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

>> WebCore/platform/audio/AudioResamplerKernel.cpp:59
>> +    int endIndex = static_cast<int>(nextFractionalIndex + 1.0); // round up to next integer index
> 
> Should this just use ceil()?

ceil() doesn't have quite the same behavior.  For example ceil(1.0) == 1.0, but we need the value 2 since we need to have one sample *past* the rounded-down version.

>> WebCore/platform/audio/AudioResamplerKernel.cpp:102
>> +        double sample2 = source[readIndex + 1];
> 
> It would be really nice to have some assertions (perhaps hoisted out of the loop) that m_virtualReadIndex + (framesToProcess * rate) + 1 is not going to walk off the end of the source buffer.

FIXED: added some assertions before entering the loop.
Comment 4 Chris Rogers 2010-10-27 12:12:07 PDT
Created attachment 72068 [details]
Patch
Comment 5 Kenneth Russell 2010-10-27 13:44:38 PDT
Comment on attachment 72068 [details]
Patch

Looks good.
Comment 6 WebKit Commit Bot 2010-10-27 15:28:16 PDT
The commit-queue encountered the following flaky tests while processing attachment 72068 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  These tests were authored by dumi@chromium.org.  The commit-queue is continuing to process your patch.
Comment 7 WebKit Commit Bot 2010-10-27 15:29:08 PDT
Comment on attachment 72068 [details]
Patch

Clearing flags on attachment: 72068

Committed r70719: <http://trac.webkit.org/changeset/70719>
Comment 8 WebKit Commit Bot 2010-10-27 15:29:15 PDT
All reviewed patches have been landed.  Closing bug.