Bug 76516 – Dynamic allocate AudioBus with required number of channels for AudioNodeInput

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

[Raymond]

Dynamic allocate AudioBus with required number of channels for AudioNodeInput

[Raymond]

Created attachment 122876 [details]
Patch

[Raymond]

Hi

This patch with patch in #76417 complete the the dynamic allocation of AudioBus for Audio Node.

[Raymond]

Hi Chris

Since #76417 is already landed. Can you also help to review this patch? ;)

[Chris Rogers]

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

[Chris Rogers]

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

[Raymond]

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

[Raymond]

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

[Chris Rogers]

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

[Raymond]

Created attachment 123276 [details]
Patch

[Raymond]

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

[Chris Rogers]

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

[Raymond]

Created attachment 123415 [details]
Patch

[Raymond]

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

[Chris Rogers]

Looks good.

[Kenneth Russell]

Comment on attachment 123415 [details]
Patch

rs=me

[WebKit Review Bot]

Comment on attachment 123415 [details]
Patch

Clearing flags on attachment: 123415

Committed r106423: <http://trac.webkit.org/changeset/106423>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.