Bug 83250

Summary: When create JavaScriptNode, do not ignore NumberOfOutputChannels parameter.
Product: WebKit Reporter: Raymond <rgbbones>
Component: Web AudioAssignee: Raymond <rgbbones>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, crogers, ojan, rtoy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Raymond 2012-04-04 23:09:46 PDT
When create JavaScriptNode, do not ignore NumberOfOutputChannels parameter.
Comment 1 Raymond 2012-04-04 23:11:48 PDT
Created attachment 135761 [details]
Patch
Comment 2 Raymond 2012-04-04 23:25:14 PDT
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.
Comment 3 Raymond 2012-04-04 23:38:20 PDT
Created attachment 135764 [details]
Patch
Comment 4 Raymond 2012-04-04 23:39:26 PDT
patch updated for compile warning on mac platform.
Comment 5 Chris Rogers 2012-04-05 12:09:26 PDT
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)
Comment 6 Raymond 2012-04-05 18:47:47 PDT
Created attachment 135964 [details]
Patch
Comment 7 Raymond 2012-04-05 19:17:26 PDT
(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.
Comment 8 Chris Rogers 2012-04-06 15:59:43 PDT
(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/

?
Comment 9 Raymond 2012-04-08 18:19:35 PDT
(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 ;)
Comment 10 Chris Rogers 2012-04-09 10:33:39 PDT
Comment on attachment 135964 [details]
Patch

Thanks Raymond.
Comment 11 WebKit Review Bot 2012-04-09 11:49:57 PDT
Comment on attachment 135964 [details]
Patch

Clearing flags on attachment: 135964

Committed r113600: <http://trac.webkit.org/changeset/113600>
Comment 12 WebKit Review Bot 2012-04-09 11:50:02 PDT
All reviewed patches have been landed.  Closing bug.