Bug 46290 – Add AudioChannelSplitter files

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Chris Rogers]

Add AudioChannelSplitter files

[Chris Rogers]

Created attachment 68414 [details]
Patch

[Chris Rogers]

This is the implementation for AudioChannelSplitter as described in the web audio API:
http://chromium.googlecode.com/svn/trunk/samples/audio/specification/specification.html#AudioChannelSplitter-section

[Kenneth Russell]

Comment on attachment 68414 [details]
Patch

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

This basically looks good but I'm marking it r- because of the initialize() / uninitialize() refactoring issue. Couple of other minor details.

> WebCore/webaudio/AudioChannelSplitter.cpp:40
> +// This is considering that 5.1 (6 channels is the largest we'll ever deal with).

This isn't a complete sentence.

> WebCore/webaudio/AudioChannelSplitter.cpp:58
> +void AudioChannelSplitter::process(size_t /*framesToProcess*/)

In the .cpp, since the argument is completely unused, just leave the name off rather than commenting it out.

> WebCore/webaudio/AudioChannelSplitter.cpp:85
> +void AudioChannelSplitter::initialize()

It looks like every AudioNode subclass has this exact same initialize() and uninitialize() method. They should be refactored into the base class. At that point m_isInitialized could be private in AudioNode.

> WebCore/webaudio/AudioChannelSplitter.h:47
> +    virtual void process(size_t /*framesToProcess*/);

I think the argument name should be uncommented.

[Chris Rogers]

Created attachment 69123 [details]
Patch

[Chris Rogers]

Comment on attachment 68414 [details]
Patch

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

>> WebCore/webaudio/AudioChannelSplitter.cpp:40
>> +// This is considering that 5.1 (6 channels is the largest we'll ever deal with).
> 
> This isn't a complete sentence.

FIXED

>> WebCore/webaudio/AudioChannelSplitter.cpp:58
>> +void AudioChannelSplitter::process(size_t /*framesToProcess*/)
> 
> In the .cpp, since the argument is completely unused, just leave the name off rather than commenting it out.

I've added the following just for a little extra safety:
    ASSERT_UNUSED(framesToProcess, framesToProcess == source->length());

>> WebCore/webaudio/AudioChannelSplitter.cpp:85
>> +void AudioChannelSplitter::initialize()
> 
> It looks like every AudioNode subclass has this exact same initialize() and uninitialize() method. They should be refactored into the base class. At that point m_isInitialized could be private in AudioNode.

FIXED: I've changed AudioNode as you suggest (which I'll post as a separate patch)

>> WebCore/webaudio/AudioChannelSplitter.h:47
>> +    virtual void process(size_t /*framesToProcess*/);
> 
> I think the argument name should be uncommented.

FIXED

[Kenneth Russell]

Comment on attachment 69123 [details]
Patch

Looks good.

[WebKit Commit Bot]

Comment on attachment 69123 [details]
Patch

Clearing flags on attachment: 69123

Committed r68736: <http://trac.webkit.org/changeset/68736>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.