Bug 76417 – Only create AudioBus with required number of channels for AudioNodeOutput

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Audio

[Raymond]

Only create AudioBus with required number of channels for AudioNodeOutput

[Raymond]

Created attachment 122705 [details]
Patch

[Raymond]

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.

[Chris Rogers]

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)

[Raymond]

(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?

[Chris Rogers]

(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" :)

[Chris Rogers]

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"

[Raymond]

(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 ;)

[Raymond]

Created attachment 122852 [details]
Patch

[Chris Rogers]

Thanks - looks fine to me!

[Kenneth Russell]

Comment on attachment 122852 [details]
Patch

rs=me based on crogers' review.

[WebKit Review Bot]

Comment on attachment 122852 [details]
Patch

Clearing flags on attachment: 122852

Committed r105373: <http://trac.webkit.org/changeset/105373>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Roland Steiner]

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)

[Raymond]

(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.

[Roland Steiner]

I took the liberty and removed that part of the clause in r105390.

[Raymond]

(In reply to comment #15)
> I took the liberty and removed that part of the clause in r105390.

Thanks a lot!