Bug 85818 – JavaScriptAudioNode should not ASSERT if number of input channels is 0

• Component: Web Audio
• Status: RESOLVED FIXED

[Raymond Toy]

It is valid to have 0 input channels for a JavaScriptAudioNode, so we shouldn't ASSERT in that case.

[Raymond Toy]

Created attachment 140592 [details]
Patch

[Chris Rogers]

Comment on attachment 140592 [details]
Patch

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

Hi Ray, thanks very much for the fix and layout test.  Just a few nits:

> LayoutTests/webaudio/javascriptaudionode-zero-input-channels.html:7
> +<title>JavaScriptAudioNode</title>

<title> line should be removed

> LayoutTests/webaudio/javascriptaudionode-zero-input-channels.html:35
> +    var context = new webkitAudioContext(1, 44100, 44100);

Best not to use hard-coded values here, and instead use the exact same pattern as the other JavaScriptAudioNode tests such as "javascriptaudionode.html"

> LayoutTests/webaudio/javascriptaudionode-zero-input-channels.html:36
> +    var node = context.createJavaScriptNode(4096, 0, 2);

Please put this creation in a try/catch block to check for exceptions here

> LayoutTests/webaudio/javascriptaudionode-zero-input-channels.html:40
> +

Please add short comment explaining that the onaudioprocess function is doing nothing since we simply want to make sure that the processing logic in the implementation can handle 0 input channels and don't care about doing any processing.
You might want to add a FIXME explaining that we could check the .inputBuffer attribute of the AudioProcessingEvent appropriately (don't need to add that part right now).

[Raymond Toy]

Created attachment 140753 [details]
Patch

[Raymond Toy]

Comment on attachment 140592 [details]
Patch

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

>> LayoutTests/webaudio/javascriptaudionode-zero-input-channels.html:7
>> +<title>JavaScriptAudioNode</title>
> 
> <title> line should be removed

Done.

>> LayoutTests/webaudio/javascriptaudionode-zero-input-channels.html:35
>> +    var context = new webkitAudioContext(1, 44100, 44100);
> 
> Best not to use hard-coded values here, and instead use the exact same pattern as the other JavaScriptAudioNode tests such as "javascriptaudionode.html"

Done.

>> LayoutTests/webaudio/javascriptaudionode-zero-input-channels.html:40
>> +
> 
> Please add short comment explaining that the onaudioprocess function is doing nothing since we simply want to make sure that the processing logic in the implementation can handle 0 input channels and don't care about doing any processing.
> You might want to add a FIXME explaining that we could check the .inputBuffer attribute of the AudioProcessingEvent appropriately (don't need to add that part right now).

Comment and FIXME added.

[Eric Seidel (no email)]

Comment on attachment 140753 [details]
Patch

Seems reasonable.

[WebKit Review Bot]

Comment on attachment 140753 [details]
Patch

Clearing flags on attachment: 140753

Committed r116465: <http://trac.webkit.org/changeset/116465>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.