Bug 76516 - Dynamic allocate AudioBus with required number of channels for AudioNodeInput
Summary: Dynamic allocate AudioBus with required number of channels for AudioNodeInput
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Audio (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Raymond
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-17 22:39 PST by Raymond
Modified: 2012-01-31 19:23 PST (History)
3 users (show)

See Also:


Attachments
Patch (8.36 KB, patch)
2012-01-17 22:47 PST, Raymond
no flags Details | Formatted Diff | Diff
Patch (8.39 KB, patch)
2012-01-20 02:34 PST, Raymond
no flags Details | Formatted Diff | Diff
Patch (8.33 KB, patch)
2012-01-20 17:01 PST, Raymond
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Raymond 2012-01-17 22:39:12 PST
Dynamic allocate AudioBus with required number of channels for AudioNodeInput
Comment 1 Raymond 2012-01-17 22:47:42 PST
Created attachment 122876 [details]
Patch
Comment 2 Raymond 2012-01-17 22:50:14 PST
Hi

This patch with patch in #76417 complete the the dynamic allocation of AudioBus for Audio Node.
Comment 3 Raymond 2012-01-18 23:06:41 PST
Hi Chris

Since #76417 is already landed. Can you also help to review this patch? ;)
Comment 4 Chris Rogers 2012-01-19 12:43:20 PST
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
Comment 5 Chris Rogers 2012-01-19 12:48:39 PST
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...
Comment 6 Raymond 2012-01-19 17:42:33 PST
(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.
Comment 7 Raymond 2012-01-19 17:44:52 PST
(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? ;)
Comment 8 Chris Rogers 2012-01-19 18:07:38 PST
(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.
Comment 9 Raymond 2012-01-20 02:34:11 PST
Created attachment 123276 [details]
Patch
Comment 10 Raymond 2012-01-20 02:40:15 PST
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 11 Chris Rogers 2012-01-20 16:11:15 PST
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
Comment 12 Raymond 2012-01-20 17:01:33 PST
Created attachment 123415 [details]
Patch
Comment 13 Raymond 2012-01-20 17:05:00 PST
(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 ;)
Comment 14 Chris Rogers 2012-01-31 18:27:56 PST
Looks good.
Comment 15 Kenneth Russell 2012-01-31 18:32:10 PST
Comment on attachment 123415 [details]
Patch

rs=me
Comment 16 WebKit Review Bot 2012-01-31 19:23:26 PST
Comment on attachment 123415 [details]
Patch

Clearing flags on attachment: 123415

Committed r106423: <http://trac.webkit.org/changeset/106423>
Comment 17 WebKit Review Bot 2012-01-31 19:23:31 PST
All reviewed patches have been landed.  Closing bug.