Bug 46290 - Add AudioChannelSplitter files
Summary: Add AudioChannelSplitter 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-09-22 12:13 PDT by Chris Rogers
Modified: 2010-09-29 18:27 PDT (History)
8 users (show)

See Also:


Attachments
Patch (9.20 KB, patch)
2010-09-22 12:14 PDT, Chris Rogers
no flags Details | Formatted Diff | Diff
Patch (8.93 KB, patch)
2010-09-28 16:01 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-09-22 12:13:37 PDT
Add AudioChannelSplitter files
Comment 1 Chris Rogers 2010-09-22 12:14:46 PDT
Created attachment 68414 [details]
Patch
Comment 2 Chris Rogers 2010-09-22 12:18:01 PDT
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
Comment 3 Kenneth Russell 2010-09-28 10:33:53 PDT
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.
Comment 4 Chris Rogers 2010-09-28 16:01:20 PDT
Created attachment 69123 [details]
Patch
Comment 5 Chris Rogers 2010-09-28 16:04:51 PDT
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
Comment 6 Kenneth Russell 2010-09-29 15:12:40 PDT
Comment on attachment 69123 [details]
Patch

Looks good.
Comment 7 WebKit Commit Bot 2010-09-29 18:27:18 PDT
Comment on attachment 69123 [details]
Patch

Clearing flags on attachment: 69123

Committed r68736: <http://trac.webkit.org/changeset/68736>
Comment 8 WebKit Commit Bot 2010-09-29 18:27:24 PDT
All reviewed patches have been landed.  Closing bug.