[Webkit-unassigned] [Bug 48012] Add AudioBufferSourceNode files

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=48012





--- Comment #4 from Chris Rogers <crogers at google.com>  2010-11-02 14:10:48 PST ---
(From update of attachment 71344)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.