Bug 63586 - Simplify AudioBufferSourceNode rendering
Summary: Simplify AudioBufferSourceNode rendering
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Rogers
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-06-28 16:53 PDT by Chris Rogers
Modified: 2011-06-29 14:20 PDT (History)
1 user (show)

See Also:


Attachments
Patch (20.39 KB, patch)
2011-06-28 16:59 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (21.20 KB, patch)
2011-06-29 13:38 PDT, Chris Rogers
kbr: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2011-06-28 16:53:08 PDT
Simplify AudioBufferSourceNode rendering
Comment 1 Chris Rogers 2011-06-28 16:59:46 PDT
Created attachment 99003 [details]
Patch
Comment 2 Kenneth Russell 2011-06-28 19:17:33 PDT
Comment on attachment 99003 [details]
Patch

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

Thanks for walking me through the code offline. Few comments as we discussed.

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:121
> +        size_t bufferFramesToProcess = framesToProcess - quantumFrameOffset;

Missing zeroing from the beginning of the quantum to the quantumFrameOffset.

> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:246
> +        // Final sanity check on buffer access.

Should there be a FIXME to try to get rid of this check, and use assertions and guards outside the loop instead?

> Source/WebCore/webaudio/AudioBufferSourceNode.h:54
> +    void renderFromBuffer(AudioBus*, unsigned destinationFrameOffset, size_t numberOfFrames);

This should be private.
Comment 3 Chris Rogers 2011-06-29 13:38:59 PDT
Created attachment 99140 [details]
Patch
Comment 4 Chris Rogers 2011-06-29 13:43:37 PDT
Comment on attachment 99003 [details]
Patch

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

Also please note at line 250 of the new patch I've addressed the small clicks issue with the "pool" demo I discussed with you

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:121
>> +        size_t bufferFramesToProcess = framesToProcess - quantumFrameOffset;
> 
> Missing zeroing from the beginning of the quantum to the quantumFrameOffset.

FIXED: I've added the zeroing inside renderFromBuffer()

>> Source/WebCore/webaudio/AudioBufferSourceNode.cpp:246
>> +        // Final sanity check on buffer access.
> 
> Should there be a FIXME to try to get rid of this check, and use assertions and guards outside the loop instead?

Added FIXME

>> Source/WebCore/webaudio/AudioBufferSourceNode.h:54
>> +    void renderFromBuffer(AudioBus*, unsigned destinationFrameOffset, size_t numberOfFrames);
> 
> This should be private.

FIXED
Comment 5 Kenneth Russell 2011-06-29 14:08:54 PDT
Comment on attachment 99140 [details]
Patch

Looks great.
Comment 6 Chris Rogers 2011-06-29 14:20:20 PDT
Committed r90042: <http://trac.webkit.org/changeset/90042>