Bug 84687 – Add multichannel support for input of JavaScriptAudioNode

• Component: Web Audio
• Status: RESOLVED FIXED

[Xingnan Wang]

Implement the numberOfInputChannels of JavaScriptAudioNode.

[Xingnan Wang]

Created attachment 138525 [details]
Patch

[Chris Rogers]

*** Bug 84706 has been marked as a duplicate of this bug. ***

[Chris Rogers]

Comment on attachment 138525 [details]
Patch

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

Thanks Xingnan, this looks pretty good.  I like your approach of using the "m_internalInputBus" and think this should work much better than what we have now.
But we still need to handle the case where numberOfInputChannels==0 (if the JavaScriptAudioNode is only synthesizing audio and doesn't care about input).
And the test cases need to be split out into separate test files.

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:61
> +    if (!numberOfInputChannels || numberOfInputChannels > AudioContext::maxNumberOfChannels())

In the Web Audio API spec, it is allowed for *either* numberOfInputChannels or numberOfOutputChannels to be zero.  It is only illegal if *both* are zero at the same time.

So, we need to allow numberOfInputChannels==0

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:-108
> -        m_inputBuffers.append(AudioBuffer::create(2, bufferSize(), sampleRate));

Probably need to check m_internalInputBus.numberOfChannels() > 0 before appending to m_inputBuffers here, and instead append(0) (if there are no input channels)

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:171
> +

You need to check numberOfInputChannels > 0 before calling copyFrom below

> LayoutTests/webaudio/javascriptaudionode.html:54
>  

I think we should keep the original javascriptaudionode.html test-file the same, and create one or two additional tests using your new testing code here.

You can create a new .js helper test file in the "resources" directory called "javascriptaudionode-testing.js" with the fillData() and checkData() methods above.

Then you can create two new test files "javascriptaudionode-2channel-input.html" and "javascriptaudionode-6channel-input.html"

It looks like you're trying to test both cases (2-channel and 6-channel) in the same test which is a bit confusing and I'm not sure it can work properly.

[Xingnan Wang]

Comment on attachment 138525 [details]
Patch

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

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:65
>          return 0;

Hi Chris, as you said, whether should we handle numberOfOutputChannels == 0?
Now numberOfOutputChannels==0 is not allowed here.

> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:109
>          m_outputBuffers.append(AudioBuffer::create(this->output(0)->numberOfChannels(), bufferSize(), sampleRate));

Maybe need to check this->output(0)->numberOfChannels() > 0 here?

[Chris Rogers]

Comment on attachment 138525 [details]
Patch

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

>> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:65
>>          return 0;
> 
> Hi Chris, as you said, whether should we handle numberOfOutputChannels == 0?
> Now numberOfOutputChannels==0 is not allowed here.

I think for now we'll have to enforce that numberOfOutputChannels > 0.  But we might want to consider Raymond's work with the AudioBasicInspectorNode (later on - not important right now) where it might be possible to create a JavaScriptAudioNode with zero channels of output which gets automatically pulled.

So for now, I think we still need this check.

>> Source/WebCore/Modules/webaudio/JavaScriptAudioNode.cpp:109
>>          m_outputBuffers.append(AudioBuffer::create(this->output(0)->numberOfChannels(), bufferSize(), sampleRate));
> 
> Maybe need to check this->output(0)->numberOfChannels() > 0 here?

Yes, one way to handle this code would be:

RefPtr<AudioBuffer> inputBuffer = m_numberOfInputChannels ? AudioBuffer::create(m_numberOfInputChannels, bufferSize(), sampleRate) : 0;
RefPtr<AudioBuffer> outputBuffer = m_numberOfOutputChannels ? AudioBuffer::create(m_numberOfOutputChannels, bufferSize(), sampleRate) : 0;

m_inputBuffers.append(inputBuffer);
m_outputBuffers.append(outputBuffer);

*** please note I've created new instance variables m_numberOfInputChannels and m_numberOfOutputChannels because it seems like the code will be cleaner this way...

[Xingnan Wang]

Created attachment 139139 [details]
Patch

[Xingnan Wang]

(In reply to comment #6)
> Created an attachment (id=139139) [details]
> Patch

Updated the patch as the comments.

[Chris Rogers]

Comment on attachment 139139 [details]
Patch

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

Xingnan, this looks great.  I have some small changes for the test files to avoid using 6-channels, and instead use 8-channels, since 6-channels will later on be interpreted as 5.1 (see AudioBus::copyFrom() comment with bug link).  Also, I think it's better to rename these test files as I suggest since the names are a bit confusing now.

> LayoutTests/webaudio/javascriptaudionode-2channel-input-expected.txt:1
> +Tests JavaScriptAudioNode 2 channels input.

Tests upmixing a 2-channel source connected to a JavaScriptAudioNode with 8-channel input.

Please re-name test file to: javascriptaudionode-upmix2-8channel-input.html

> LayoutTests/webaudio/javascriptaudionode-2channel-input.html:15
> +description("Tests JavaScriptAudioNode 2 channels input.");

Tests upmixing a 2-channel source connected to a JavaScriptAudioNode with 8-channel input.

> LayoutTests/webaudio/javascriptaudionode-2channel-input.html:19
> +var inputChannels = 6;

Let's switch to 8-channels since 6 channels can be interpreted as 5.1 (and we have a comment with bug in AudioBus::copyFrom() about this mixdown case)

> LayoutTests/webaudio/javascriptaudionode-6channel-input-expected.txt:1
> +Tests JavaScriptAudioNode 6 channels input.

Tests downmixing an 8-channel source connected to a JavaScriptAudioNode with 2-channel input.

Please re-name test file to: javascriptaudionode-downmix8-2channel-input.html

> LayoutTests/webaudio/javascriptaudionode-6channel-input.html:15
> +description("Tests JavaScriptAudioNode 6 channels input.");

Tests downmixing an 8-channel source connected to a JavaScriptAudioNode with 2-channel input.

> LayoutTests/webaudio/javascriptaudionode-6channel-input.html:18
> +var sourceChannels = 6;

Let's switch to 8-channels since 6 channels can be interpreted as 5.1 (and we have a comment with bug in AudioBus::copyFrom() about this mixdown case)

> LayoutTests/webaudio/resources/javascriptaudionode-testing.js:47
> +function checkData(buffer, numberOfChannels, length) {

Please rename test function to checkStereoOnlyData, and add simple comment describing why this will be the case for 2 -> 8 upmix and 8 -> 2 downmix.

[Xingnan Wang]

Created attachment 139317 [details]
Patch

[Xingnan Wang]

(In reply to comment #9)
> Created an attachment (id=139317) [details]
> Patch

Chris, thanks for your comments, updated the patch.

[Chris Rogers]

Comment on attachment 139317 [details]
Patch

Thanks Xingnan!

[WebKit Review Bot]

Comment on attachment 139317 [details]
Patch

Clearing flags on attachment: 139317

Committed r115699: <http://trac.webkit.org/changeset/115699>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.