Bug 83648 – AudioContext createChannelSplitter() method should have optional argument for number of outputs

• Component: Web Audio
• Status: RESOLVED FIXED

[Chris Rogers]

createChannelSplitter() currently creates a hard-coded number of six outputs.  The default should be six, but an optional argument should allow more (up to a certain limit) and will throw an exception if it exceeds this limit.

The limit can be AudioContext::maxNumberOfChannels()

[Raymond]

Created attachment 136605 [details]
Patch

[Raymond]

Hi Chris

Patch for this issue. seems channel-splitter doesn't have a layout test yet also. Is Raymond Toy working on it?

[Chris Rogers]

Comment on attachment 136605 [details]
Patch

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

Thanks Raymond - seems pretty good.  I know it's a pain to create the layout tests, but now would be a good time to write one :)

If you're interested, we should also create a similar patch for AudioChannelMerger to make numberOfInputs be an optional argument...

> Source/WebCore/Modules/webaudio/AudioContext.cpp:463
> +    // Set output to 5.1 by default.

I know this file made reference to 5.1 before, but since the AudioChannelSplitter doesn't interpret the channel layout, I think we should be more general and have the comment say:

// Set output to default number of channels.

But thinking more about this, I would just remove the comment, since you can use the constant (as explained below)

> Source/WebCore/Modules/webaudio/AudioContext.cpp:464
> +    return createChannelSplitter(6, ec);

And instead of using a hard-coded constant 6 here, keep the constant NumberOfOutputs at the top of the file (but rename it DefaultNumberOfChannels)

[Raymond]

Created attachment 136631 [details]
Patch

[Raymond]

(In reply to comment #3)
Hi Chris 

Thanks for the review. Patch updated.

> 
> > Source/WebCore/Modules/webaudio/AudioContext.cpp:464
> > +    return createChannelSplitter(6, ec);
> 
> And instead of using a hard-coded constant 6 here, keep the constant NumberOfOutputs at the top of the file (but rename it DefaultNumberOfChannels)

Since this constant var is now defined in audioContext.cpp. Thus I have name it ChannelSplitterDefaultNumberOfOutputs instead. And put it near the function instead of file head.

[Raymond]

(In reply to comment #3)
> (From update of attachment 136605 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136605&action=review
> 
> Thanks Raymond - seems pretty good.  I know it's a pain to create the layout tests, but now would be a good time to write one :)

And I open another bug at : https://bugs.webkit.org/show_bug.cgi?id=83667 for layout test case, since it involves test cases cover codes that other than this patch is changing. Please take a review there also ;)

[Kentaro Hara]

Comment on attachment 136631 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        AudioContext createChannelSplitter() method should have optional argument for number of outputs

Do you have any spec that supports this change?

It seems the spec (https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html) does not have such an optional argument.

[Chris Rogers]

(In reply to comment #7)
> (From update of attachment 136631 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136631&action=review
> 
> > Source/WebCore/ChangeLog:3
> > +        AudioContext createChannelSplitter() method should have optional argument for number of outputs
> 
> Do you have any spec that supports this change?
> 
> It seems the spec (https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html) does not have such an optional argument.

I am in the process of updating the Web Audio editor's draft.  I should be able to get this in today.

[Kentaro Hara]

Comment on attachment 136631 [details]
Patch

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

The change looks OK. r- due to missing tests.

>>> Source/WebCore/ChangeLog:3
>>> +        AudioContext createChannelSplitter() method should have optional argument for number of outputs
>> 
>> Do you have any spec that supports this change?
>> 
>> It seems the spec (https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html) does not have such an optional argument.
> 
> I am in the process of updating the Web Audio editor's draft.  I should be able to get this in today.

OK. It might be good to include the link to the spec in this ChangeLog.

> Source/WebCore/ChangeLog:7
> +

Does any existing test cover this change? If so, you can write "Test: xxx.html (No change in test results)". Otherwise, please add a test.

[Raymond]

(In reply to comment #9)
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> Does any existing test cover this change? If so, you can write "Test: xxx.html (No change in test results)". Otherwise, please add a test.

I have a test case written at https://bugs.webkit.org/show_bug.cgi?id=83667, so I should merge that patch with this one?

[Chris Rogers]

(In reply to comment #10)
> (In reply to comment #9)
> > 
> > > Source/WebCore/ChangeLog:7
> > > +
> > 
> > Does any existing test cover this change? If so, you can write "Test: xxx.html (No change in test results)". Otherwise, please add a test.
> 
> I have a test case written at https://bugs.webkit.org/show_bug.cgi?id=83667, so I should merge that patch with this one?

Hi Raymond, yes it's probably best to merge that patch into this bug.  But first please address my review comments there.

By the way, the latest editor's draft now includes the spec update for createChannelSplitter() and createChannelMerger():
https://dvcs.w3.org/hg/audio/raw-file/tip/webaudio/specification.html

[Raymond]

Created attachment 136804 [details]
Patch

[Raymond]

Hi

Patch updated. Layout test case added/merged from https://bugs.webkit.org/show_bug.cgi?id=83667. With the comments there addressed.

[Chris Rogers]

Comment on attachment 136804 [details]
Patch

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

Looks very good - just a couple final nits:

> Source/WebCore/Modules/webaudio/AudioContext.cpp:461
> +const unsigned ChannelSplitterDefaultNumberOfOutputs = 6;

I would move this constant inside of the AudioContext::createChannelSplitter() method below (after line 464)

> LayoutTests/webaudio/audiochannelsplitter.html:4
> +Tests that AudioChannelSplitter working correctly.

working -> works

> LayoutTests/webaudio/audiochannelsplitter.html:31
> +

please remove extra blank line

[Raymond]

Created attachment 136805 [details]
Patch

[Raymond]

(In reply to comment #14)
> (From update of attachment 136804 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=136804&action=review
> 
> Looks very good - just a couple final nits:
> 

Done ;)

[Chris Rogers]

Comment on attachment 136805 [details]
Patch

Thanks Raymond!

[WebKit Review Bot]

Comment on attachment 136805 [details]
Patch

Clearing flags on attachment: 136805

Committed r113940: <http://trac.webkit.org/changeset/113940>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.