Bug 83250 – When create JavaScriptNode, do not ignore NumberOfOutputChannels parameter.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Raymond]

When create JavaScriptNode, do not ignore NumberOfOutputChannels parameter.

[Raymond]

Created attachment 135761 [details]
Patch

[Raymond]

Hi

This patch is aim to add multi-channel support for JavaScriptAudioNode. For the first step, honor the numberOfOutputChannels parameters when creating JavaScriptAudioNode.

For numberOfInputChannels, I don't touch it in this patch. And I think we probably don't need to have this input parameter. I suggest to remove it from the spec. Since the input channels actually depends on the upstream node that the JavaScriptNode connected to. It doesn't make much sense for the user to define it. And if the user of JavaScriptNode really want to change the input channels number, he should do it by himself for either upmix or downmix. And it can be done at the same time when he processing input data into output buffer.

[Raymond]

Created attachment 135764 [details]
Patch

[Raymond]

patch updated for compile warning on mac platform.

[Chris Rogers]

Comment on attachment 135764 [details]
Patch

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

Looking pretty good so far:

> Source/WebCore/Modules/webaudio/AudioContext.h:126
> +    PassRefPtr<JavaScriptAudioNode> createJavaScriptNode(size_t bufferSize, size_t numberOfInputChannels, size_t numberOfOutputChannels, ExceptionCode&);

We should add default arguments numberOfInputChannels==2 and numberOfOutputChannels==2

Although the spec should be updated to say this, there is code out there which assumes that it's OK to call createJavaScriptNode() with a single argument (the bufferSize)

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:61
> +    // FIXME: Maybe we shouldn't have the numberOfInputChannels exported as an input parameter, since it actually depends on upstream node.

I think it could be tricky to depend on upstream node (or nodes) connected to determine numberOfInputChannels because this could easily be changing during the life-time of the JavaScriptAudioNode.  For example if a mixture of mono and stereo AudioBufferSourceNodes are playing one-shot sounds, then sometimes the input would be mono and sometimes stereo, varying from callback-to-callback.  Even worse, the JS callback is not synchronized with these changes...

So, I think it's best to keep the numberOfInputChannels explicit and fixed.  Then any connections to this input will be up/down mixed as necessary.  Stereo is a good default.

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:64
> +    ASSERT(numberOfOutputChannels > 0 && numberOfOutputChannels <= AudioContext::maxNumberOfChannels());

WebKit style nit: "numberOfOutputChannels > 0" -> "numberOfOutputChannels"

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:65
> +    if (numberOfOutputChannels <= 0 || numberOfOutputChannels > AudioContext::maxNumberOfChannels())

WebKit style nit: "numberOfOutputChannels <= 0" -> "!numberOfOutputChannels"

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:83
> +    // FIXEME: We probably should remove numberOfInputChannels from input parameters since we actually doesn't use it.

typo: FIXEME -> FIXME

Also, comment should probably be that we "still" need to implement numberOfInputChannels (for the reasons I explain above)

[Raymond]

Created attachment 135964 [details]
Patch

[Raymond]

(In reply to comment #5)
Hi Chris
Thanks for the review. Patch updated.

> We should add default arguments numberOfInputChannels==2 and numberOfOutputChannels==2

Use [Optional] and overload functions to implement it. there is also a approaching of [Optional=defaultisUndefined] etc. But I think we do need to distiguish fn(buffersize) from fn(buffersize, 0, 0). thus I don't go that way.

> > Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:64
> > +    ASSERT(numberOfOutputChannels > 0 && numberOfOutputChannels <= AudioContext::maxNumberOfChannels());

Actually I remove this line, since we do want a dom exception been thrown by code below.

[Chris Rogers]

(In reply to comment #7)
> (In reply to comment #5)
> Hi Chris
> Thanks for the review. Patch updated.
> 
> > We should add default arguments numberOfInputChannels==2 and numberOfOutputChannels==2
> 
> Use [Optional] and overload functions to implement it. there is also a approaching of [Optional=defaultisUndefined] etc. But I think we do need to distiguish fn(buffersize) from fn(buffersize, 0, 0). thus I don't go that way.
> 
> > > Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:64
> > > +    ASSERT(numberOfOutputChannels > 0 && numberOfOutputChannels <= AudioContext::maxNumberOfChannels());
> 
> Actually I remove this line, since we do want a dom exception been thrown by code below.

Hi Raymond, this looks pretty good.  Have you tested this with:
http://chromium.googlecode.com/svn/trunk/samples/audio/javascript-processing.html

and this one:
http://www.oampo.co.uk/labs/technocrat/

?

[Raymond]

(In reply to comment #8)
> 
> Hi Raymond, this looks pretty good.  Have you tested this with:
> http://chromium.googlecode.com/svn/trunk/samples/audio/javascript-processing.html
> 
> and this one:
> http://www.oampo.co.uk/labs/technocrat/
> 
> ?

Hi Chris

I just tested them, yes, they works.

And actually, since there are no layout test for javascriptAudioNode, I also have written one locally to verify my code, say setting channels etc.

I would like to provide another patch for layout test case, after this patch is accepted. To have more time to think about the test case coverage.

So, if this patch is good, could you help to land it ;)

[Chris Rogers]

Comment on attachment 135964 [details]
Patch

Thanks Raymond.

[WebKit Review Bot]

Comment on attachment 135964 [details]
Patch

Clearing flags on attachment: 135964

Committed r113600: <http://trac.webkit.org/changeset/113600>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.