Dynamic allocate AudioBus with required number of channels for AudioNodeInput
Created attachment 122876 [details] Patch
Hi This patch with patch in #76417 complete the the dynamic allocation of AudioBus for Audio Node.
Hi Chris Since #76417 is already landed. Can you also help to review this patch? ;)
Comment on attachment 122876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122876&action=review Looks pretty good overall. I'd like to have a chance to apply your patch and test it out myself, since this is making pretty sensitive changes :) I assume you've done a debug build (to catch ASSERTs) and run some web audio examples? > Source/WebCore/webaudio/AudioChannelMerger.cpp:92 > +void AudioChannelMerger::checkNumberOfChannelsForInput(AudioNodeInput* theInput) nit: "theInput" -> "input" to be consistent with other versions of this method > Source/WebCore/webaudio/AudioNodeInput.cpp:45 > + m_internalSummingBus = adoptPtr(new AudioBus(2, AudioNode::ProcessingSizeInFrames)); Can you please check this. I looked through the logic and it seems like the current default behavior if there are no connections to this input is a "mono" bus. It seems that way by looking at the pull() method: if (!numberOfRenderingConnections()) { // At least, generate silence if we're not connected to anything. // FIXME: if we wanted to get fancy, we could propagate a 'silent hint' here to optimize the downstream graph processing. internalSummingBus->zero(); return internalSummingBus; } And numberOfRenderingChannels() is 1 (with no connections) so internalSummingBus() uses a mono bus > Source/WebCore/webaudio/AudioNodeInput.cpp:166 > + if (numberOfChannels() == m_internalSummingBus->numberOfChannels()) Please call numberOfChannels() a single time and cache in a local variable instead of calling numberOfChannels() twice in this method
By the way, as I was looking through your patch, I noticed something else we could simplify (not in this patch). If you look inside AudioBasicProcessorNode::checkNumberOfChannelsForInput() There's an ASSERT: ASSERT(context()->isAudioThread() && context()->isGraphOwner()); But later in the method we synchronize with the process() method: MutexLocker locker(m_processLock); It seems like this shouldn't be necessary, since we already know we're not in the middle of processing, but instead own the context lock and are either in the pre-rendering or post-rendering tasks... Seems like we have a similar thing happening also in AudioGainNode::checkNumberOfChannelsForInput() where we should probably add the ASSERT... I could be mistaken here, but it's something you could look at if you have the interest...
(In reply to comment #4) Hi Chris Thanks for your review. > Looks pretty good overall. I'd like to have a chance to apply your patch and test it out myself, since this is making pretty sensitive changes :) I assume you've done a debug build (to catch ASSERTs) and run some web audio examples? > Yes, I run some web audio examples on http://chromium.googlecode.com/svn/trunk/samples/audio/index.html like drum machine etc. And I do have a debug build, while test examples only on Release build. I will also test examples on Debug build in the future. > > Source/WebCore/webaudio/AudioChannelMerger.cpp:92 > > +void AudioChannelMerger::checkNumberOfChannelsForInput(AudioNodeInput* theInput) > > nit: "theInput" -> "input" to be consistent with other versions of this method This is due to there is a local variable named input already, though it's in for loop thus in different namespace. but I am a little bit worry about the confusion when reading the code. But anyway, I will modify "theInput" to "input". > > > Source/WebCore/webaudio/AudioNodeInput.cpp:45 > > + m_internalSummingBus = adoptPtr(new AudioBus(2, AudioNode::ProcessingSizeInFrames)); > > Can you please check this. I looked through the logic and it seems like the current default behavior if there are no connections to this input is a "mono" bus. > > It seems that way by looking at the pull() method: > > > if (!numberOfRenderingConnections()) { > // At least, generate silence if we're not connected to anything. > // FIXME: if we wanted to get fancy, we could propagate a 'silent hint' here to optimize the downstream graph processing. > internalSummingBus->zero(); > return internalSummingBus; > } > > And numberOfRenderingChannels() is 1 (with no connections) so internalSummingBus() uses a mono bus > Great catch! I have thought when doing connects, the rendering channels will be updated anyway. miss the case that it might not be connect to any input while been connect to output firstly. Will change it to mono by default. And btw, I did hesitate on the default setting here for a few minutes when I wrote the code. For it seems to me most audio source should be stereo now? I want to make the default value suit for the most case so I choose it. And what's more there are different default settings in different nodes at present, some set to mono, some set to stereo. I am wondering, maybe we can scan through the code and unify the default channels setting later in other patch? > > Source/WebCore/webaudio/AudioNodeInput.cpp:166 > > + if (numberOfChannels() == m_internalSummingBus->numberOfChannels()) > > Please call numberOfChannels() a single time and cache in a local variable instead of calling numberOfChannels() twice in this method ok, will fix it. I will check some more examples on Debug build and update the patch soon. Thx.
(In reply to comment #5) > By the way, as I was looking through your patch, I noticed something else we could simplify (not in this patch). > > If you look inside AudioBasicProcessorNode::checkNumberOfChannelsForInput() > > There's an ASSERT: > ASSERT(context()->isAudioThread() && context()->isGraphOwner()); > > But later in the method we synchronize with the process() method: > MutexLocker locker(m_processLock); > > It seems like this shouldn't be necessary, since we already know we're not in the middle of processing, but instead own the context lock and are either in the pre-rendering or post-rendering tasks... > > Seems like we have a similar thing happening also in AudioGainNode::checkNumberOfChannelsForInput() > where we should probably add the ASSERT... > > I could be mistaken here, but it's something you could look at if you have the interest... sure, will look into it. and if there do have issue, you prefer to fix it in another patch right? ;)
(In reply to comment #7) > (In reply to comment #5) > > By the way, as I was looking through your patch, I noticed something else we could simplify (not in this patch). > > > > If you look inside AudioBasicProcessorNode::checkNumberOfChannelsForInput() > > > > There's an ASSERT: > > ASSERT(context()->isAudioThread() && context()->isGraphOwner()); > > > > But later in the method we synchronize with the process() method: > > MutexLocker locker(m_processLock); > > > > It seems like this shouldn't be necessary, since we already know we're not in the middle of processing, but instead own the context lock and are either in the pre-rendering or post-rendering tasks... > > > > Seems like we have a similar thing happening also in AudioGainNode::checkNumberOfChannelsForInput() > > where we should probably add the ASSERT... > > > > I could be mistaken here, but it's something you could look at if you have the interest... > > sure, will look into it. and if there do have issue, you prefer to fix it in another patch right? ;) Yeah, that one would be a different patch.
Created attachment 123276 [details] Patch
well, Debug Build to test more example demos did find some issue, though not related to this patch ;) check https://bugs.webkit.org/show_bug.cgi?id=76685
Comment on attachment 123276 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=123276&action=review Thanks Raymond, I had a chance to build with this patch, and everything works well. Assuming the "nit" comments are addressed this looks good to me. > Source/WebCore/webaudio/AudioNodeInput.cpp:44 > + // set to mono by default nit: use full sentence for comments (capitalize first word - add period at end) // Set to mono by default. > Source/WebCore/webaudio/AudioNodeInput.cpp:216 > + // The m_internalSummingBus should have been allocated with right channels. nit: I might just remove the comment since it doesn't seem to add more information than the ASSERT itself. But, if you keep the comment, I'd change it to: // m_internalSummingBus should have been allocated with the correct number of channels. Since the word "right" is confusing here -- makes me think of left/right
Created attachment 123415 [details] Patch
(In reply to comment #11) > (From update of attachment 123276 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=123276&action=review > > Thanks Raymond, I had a chance to build with this patch, and everything works well. > > Assuming the "nit" comments are addressed this looks good to me. > Fixed ;)
Looks good.
Comment on attachment 123415 [details] Patch rs=me
Comment on attachment 123415 [details] Patch Clearing flags on attachment: 123415 Committed r106423: <http://trac.webkit.org/changeset/106423>
All reviewed patches have been landed. Closing bug.