Only create AudioBus with required number of channels for AudioNodeOutput
Created attachment 122705 [details] Patch
Hi this patch is aim to create AudioBus on demand for AudioNodeOutput to : 1. Save memory 2. Help to support multi-channels for WebAudio later more easily. My goal is also apply this to AudioNodeInput, While since AudioNodeInput's case is slightly complicated than AudioNodeOutput due to the similar change might involve changes in AudioNode's checkNumberOfChannelsForInput and thus its subclass, So I would prefer to do it in a separate patch when this patch is accepted. One thing I am not quite sure is that it seems to me the original design is to pre-allocate all memories upon node creation, is that aim to help the render thread run more smoothly? However, since this memory allocation work is done at the beginning or end of a rendering quantum, and also in some audio processor class they also do memory allocations upon channels change, so I think this might not be a big deal, and after all, channels number rarely changes.
Raymond, thanks for the patch. As it stands right now the re-allocation of AudioBus is not thread-safe with the audio thread. For example, a mono bus might be deleted and a new stereo bus created, but meanwhile the audio thread is using the mono bus. Two solutions I can see: 1) Waste even more memory and allocate storage for up to six channels. This doesn't seem like a great solution 2) Make it thread-safe using a similar approach to what we now do for connections (see pre and post-render tasks in AudioContext)
(In reply to comment #3) > Raymond, thanks for the patch. > > As it stands right now the re-allocation of AudioBus is not thread-safe with the audio thread. For example, a mono bus might be deleted and a new stereo bus created, but meanwhile the audio thread is using the mono bus. > > Two solutions I can see: > > 1) Waste even more memory and allocate storage for up to six channels. This doesn't seem like a great solution > > 2) Make it thread-safe using a similar approach to what we now do for connections (see pre and post-render tasks in AudioContext) Hi Roger thanks for the review. I did pay attention to the thread-safe issue, while maybe I miss some code path? But as far as I can see at present: AudioNodeOutput::updateInternalBus is called by AudioNodeOutput::updateNumberOfChannels which is called by two function : AudioNodeOutput::updateRenderingState and AudioNodeOutput::setNumberOfChannels. In both case, AudioNodeOutput::updateNumberOfChannels will only be invoked at the very start or end of a rendering quantum, which is the same case as the connections handle dirty input/output node. so is there any other case I miss here?
(In reply to comment #4) > (In reply to comment #3) > > Raymond, thanks for the patch. > > > > As it stands right now the re-allocation of AudioBus is not thread-safe with the audio thread. For example, a mono bus might be deleted and a new stereo bus created, but meanwhile the audio thread is using the mono bus. > > > > Two solutions I can see: > > > > 1) Waste even more memory and allocate storage for up to six channels. This doesn't seem like a great solution > > > > 2) Make it thread-safe using a similar approach to what we now do for connections (see pre and post-render tasks in AudioContext) > > Hi Roger > > thanks for the review. I did pay attention to the thread-safe issue, while maybe I miss some code path? But as far as I can see at present: > > AudioNodeOutput::updateInternalBus is called by AudioNodeOutput::updateNumberOfChannels which is called by two function : AudioNodeOutput::updateRenderingState and AudioNodeOutput::setNumberOfChannels. > > In both case, AudioNodeOutput::updateNumberOfChannels will only be invoked at the very start or end of a rendering quantum, which is the same case as the connections handle dirty input/output node. > > so is there any other case I miss here? Sorry, you're right. I even see we have an ASSERT in updateNumberOfChannels()... This looks fine to me with one minor comment fix (but I'm not a reviewer). by the way, no worries, but my name is "Chris" :)
Comment on attachment 122705 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=122705&action=review > Source/WebCore/webaudio/AudioNodeOutput.h:113 > + // m_internalBus must only be changed in the audio thread (or constructor). It would be good to be exact here about where in the audio thread it's safe: "in the audio thread" -> "in the audio thread with the context's graph lock"
(In reply to comment #6) > (From update of attachment 122705 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=122705&action=review > > > Source/WebCore/webaudio/AudioNodeOutput.h:113 > > + // m_internalBus must only be changed in the audio thread (or constructor). > > It would be good to be exact here about where in the audio thread it's safe: > > "in the audio thread" -> "in the audio thread with the context's graph lock" Oh, right, exactly. I will update the patch. Thanks Chris ;)
Created attachment 122852 [details] Patch
Thanks - looks fine to me!
Comment on attachment 122852 [details] Patch rs=me based on crogers' review.
Comment on attachment 122852 [details] Patch Clearing flags on attachment: 122852 Committed r105373: <http://trac.webkit.org/changeset/105373>
All reviewed patches have been landed. Closing bug.
In line 50: ASSERT(numberOfChannels >= 0 && numberOfChannels <= MaxNumberOfChannels); numberOfChannels is 'unsigned', so it's always >= 0 (this causes compile issues when warnings are treated as errors)
(In reply to comment #13) > In line 50: ASSERT(numberOfChannels >= 0 && numberOfChannels <= MaxNumberOfChannels); > > numberOfChannels is 'unsigned', so it's always >= 0 (this causes compile issues when warnings are treated as errors) oops... will try to fix this issue together with other patch later.
I took the liberty and removed that part of the clause in r105390.
(In reply to comment #15) > I took the liberty and removed that part of the clause in r105390. Thanks a lot!