Bug 84687 - Add multichannel support for input of JavaScriptAudioNode
Summary: Add multichannel support for input of JavaScriptAudioNode
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 84706 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-04-24 02:20 PDT by Xingnan Wang
Modified: 2012-05-10 23:21 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.19 KB, patch)
2012-04-24 02:27 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (14.22 KB, patch)
2012-04-27 00:04 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff
Patch (14.70 KB, patch)
2012-04-27 19:10 PDT, Xingnan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xingnan Wang 2012-04-24 02:20:54 PDT
Implement the numberOfInputChannels of JavaScriptAudioNode.
Comment 1 Xingnan Wang 2012-04-24 02:27:46 PDT
Created attachment 138525 [details]
Patch
Comment 2 Chris Rogers 2012-04-24 18:48:38 PDT
*** Bug 84706 has been marked as a duplicate of this bug. ***
Comment 3 Chris Rogers 2012-04-26 12:50:21 PDT
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.
Comment 4 Xingnan Wang 2012-04-26 19:11:09 PDT
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?
Comment 5 Chris Rogers 2012-04-26 19:26:37 PDT
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...
Comment 6 Xingnan Wang 2012-04-27 00:04:21 PDT
Created attachment 139139 [details]
Patch
Comment 7 Xingnan Wang 2012-04-27 00:05:02 PDT
(In reply to comment #6)
> Created an attachment (id=139139) [details]
> Patch

Updated the patch as the comments.
Comment 8 Chris Rogers 2012-04-27 12:25:27 PDT
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.
Comment 9 Xingnan Wang 2012-04-27 19:10:52 PDT
Created attachment 139317 [details]
Patch
Comment 10 Xingnan Wang 2012-04-27 19:12:53 PDT
(In reply to comment #9)
> Created an attachment (id=139317) [details]
> Patch

Chris, thanks for your comments, updated the patch.
Comment 11 Chris Rogers 2012-04-30 17:46:29 PDT
Comment on attachment 139317 [details]
Patch

Thanks Xingnan!
Comment 12 WebKit Review Bot 2012-04-30 18:49:54 PDT
Comment on attachment 139317 [details]
Patch

Clearing flags on attachment: 139317

Committed r115699: <http://trac.webkit.org/changeset/115699>
Comment 13 WebKit Review Bot 2012-04-30 18:50:07 PDT
All reviewed patches have been landed.  Closing bug.