Bug 48012 - Add AudioBufferSourceNode files
Summary: Add AudioBufferSourceNode 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-20 15:01 PDT by Chris Rogers
Modified: 2010-11-02 19:36 PDT (History)
8 users (show)

See Also:


Attachments
Patch (28.08 KB, patch)
2010-10-20 15:03 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (28.37 KB, patch)
2010-11-02 14:13 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-20 15:01:23 PDT
Add AudioBufferSourceNode files
Comment 1 Chris Rogers 2010-10-20 15:03:14 PDT
Created attachment 71344 [details]
Patch
Comment 2 Chris Rogers 2010-10-20 15:06:57 PDT
This is the implementation for AudioBufferSourceNode as described in the audio specification:
http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#AudioBufferSourceNode-section
Comment 3 Kenneth Russell 2010-11-01 20:06:50 PDT
Comment on attachment 71344 [details]
Patch

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

Overall the code basically looks good to a layman not experienced in audio processing. r- for a few minor issues, and a couple of larger / higher-level comments.

> WebCore/webaudio/AudioBufferSourceNode.cpp:83
> +    // Careful - this is a tryLock() and not an autolocker, so we must unlock() before every return.

Please consider adding a TryLocker class and MutexTryLocker typedef to WTF. The TryLocker class could have a bool holdsLock() method, and would only unlock the mutex in its destructor if it successfully acquired it in its constructor. This would clean up this class and I'm pretty sure a few others. This can be a separate bug following this one.

> WebCore/webaudio/AudioBufferSourceNode.cpp:145
> +    bool channelCountGood = numberOfChannels == busNumberOfChannels && (numberOfChannels == 1 || numberOfChannels == 2);

How about a FIXME here as below? This is the first case of this test in this file reading from the top down.

> WebCore/webaudio/AudioBufferSourceNode.cpp:264
> +    // FIXME: we can add support for sources with more than two channels, but this is not a common case.

Is it expected that it may be more of a common case in the future? Is there any real problem with generalizing this code to arrays of channels, source pointers, destination pointers, etc.? It seems it might cut in half the amount of code in certain places.

> WebCore/webaudio/AudioBufferSourceNode.cpp:280
> +    sourceR += m_readIndex;

Please add assertions that you're not walking off the end of the source, and won't when accessing framesToProcess frames past these pointers.

> WebCore/webaudio/AudioBufferSourceNode.cpp:405
> +void AudioBufferSourceNode::noteOff(double /*when*/)

Just remove the commented-out variable name.

> WebCore/webaudio/AudioBufferSourceNode.cpp:411
> +    // FIXME: when is ignored.

Put quotes around "when". Confusing to read otherwise.

> WebCore/webaudio/AudioBufferSourceNode.h:48
> +    static PassRefPtr<AudioBufferSourceNode> create(AudioContext* context, double sampleRate)

Is there a particular reason to inline this creation method in the header? If not please move its body to the .cpp.
Comment 4 Chris Rogers 2010-11-02 14:10:48 PDT
Comment on attachment 71344 [details]
Patch

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

>> WebCore/webaudio/AudioBufferSourceNode.cpp:145
>> +    bool channelCountGood = numberOfChannels == busNumberOfChannels && (numberOfChannels == 1 || numberOfChannels == 2);
> 
> How about a FIXME here as below? This is the first case of this test in this file reading from the top down.

FIXED

>> WebCore/webaudio/AudioBufferSourceNode.cpp:264
>> +    // FIXME: we can add support for sources with more than two channels, but this is not a common case.
> 
> Is it expected that it may be more of a common case in the future? Is there any real problem with generalizing this code to arrays of channels, source pointers, destination pointers, etc.? It seems it might cut in half the amount of code in certain places.

Even in the future the mono and stereo cases will by far be the most common.  It's important to keep the structure of the code such that the inner-loops may be optimized, especially for stereo.  Fairly soon, we're anticipating adding automation curves and more generalized envelopes in here which can especially benefit from these optimizations.

>> WebCore/webaudio/AudioBufferSourceNode.cpp:280
>> +    sourceR += m_readIndex;
> 
> Please add assertions that you're not walking off the end of the source, and won't when accessing framesToProcess frames past these pointers.

FIXED: added this check in isSourceGood above

>> WebCore/webaudio/AudioBufferSourceNode.cpp:405
>> +void AudioBufferSourceNode::noteOff(double /*when*/)
> 
> Just remove the commented-out variable name.

FIXED

>> WebCore/webaudio/AudioBufferSourceNode.cpp:411
>> +    // FIXME: when is ignored.
> 
> Put quotes around "when". Confusing to read otherwise.

FIXED

>> WebCore/webaudio/AudioBufferSourceNode.h:48
>> +    static PassRefPtr<AudioBufferSourceNode> create(AudioContext* context, double sampleRate)
> 
> Is there a particular reason to inline this creation method in the header? If not please move its body to the .cpp.

FIXED
Comment 5 Chris Rogers 2010-11-02 14:13:08 PDT
Created attachment 72734 [details]
Patch
Comment 6 Kenneth Russell 2010-11-02 17:59:02 PDT
Comment on attachment 72734 [details]
Patch

Looks good to me.
Comment 7 WebKit Commit Bot 2010-11-02 19:35:57 PDT
Comment on attachment 72734 [details]
Patch

Clearing flags on attachment: 72734

Committed r71205: <http://trac.webkit.org/changeset/71205>
Comment 8 WebKit Commit Bot 2010-11-02 19:36:03 PDT
All reviewed patches have been landed.  Closing bug.