Implement the numberOfInputChannels of JavaScriptAudioNode.
Created attachment 138525 [details] Patch
*** Bug 84706 has been marked as a duplicate of this bug. ***
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 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 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...
Created attachment 139139 [details] Patch
(In reply to comment #6) > Created an attachment (id=139139) [details] > Patch Updated the patch as the comments.
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.
Created attachment 139317 [details] Patch
(In reply to comment #9) > Created an attachment (id=139317) [details] > Patch Chris, thanks for your comments, updated the patch.
Comment on attachment 139317 [details] Patch Thanks Xingnan!
Comment on attachment 139317 [details] Patch Clearing flags on attachment: 139317 Committed r115699: <http://trac.webkit.org/changeset/115699>
All reviewed patches have been landed. Closing bug.