Bug 83610 – AudioParam must support fan-in (multiple audio connections)

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

[Chris Rogers]

AudioNodes already support multiple connections from diverse sources (with implicit unity-gain summing).
When it becomes possible to connect an AudioNode to an AudioParam, then we should similarly support multiple connections with implicit summing.

This depends on:
https://bugs.webkit.org/show_bug.cgi?id=83524

[Chris Rogers]

Created attachment 142120 [details]
Patch

[Kenneth Russell]

Comment on attachment 142120 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142120&action=review

Looks good to me overall. Couple of minor comments.

> LayoutTests/webaudio/audioparam-connect-audioratesignal.html:92
> +    gainNode.gain.value = 0;

Is this change really related to this patch? If not, I suggest splitting it out. If so, will existing Web Audio apps need to be updated?

> LayoutTests/webaudio/audioparam-summingjunction.html:97
> +    // Create 2st buffer used to control gain (a simple sine wave tone).

2st -> 2nd

[Chris Rogers]

Committed r117372: <http://trac.webkit.org/changeset/117372>

[Chris Rogers]

(In reply to comment #2)
> (From update of attachment 142120 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142120&action=review
> 
> Looks good to me overall. Couple of minor comments.
> 
> > LayoutTests/webaudio/audioparam-connect-audioratesignal.html:92
> > +    gainNode.gain.value = 0;
> 
> Is this change really related to this patch? If not, I suggest splitting it out. If so, will existing Web Audio apps need to be updated?

Yes, the behavior needs to be changed to this -- it's the "correct" thing to do.

The audio-rate control of AudioParams is very new (within the last couple of weeks) and is a bit more specialized, so I'm not concerned about breaking existing content here.

> 
> > LayoutTests/webaudio/audioparam-summingjunction.html:97
> > +    // Create 2st buffer used to control gain (a simple sine wave tone).
> 
> 2st -> 2nd

FIXED

[WebKit Review Bot]

Re-opened since this is blocked by 86710

[Chris Rogers]

Created attachment 143605 [details]
Patch

[Chris Rogers]

Hi Ken, I'm hoping to reland the original patch (which was reverted) now that a fix has been commited in:
http://trac.webkit.org/changeset/118099

[Kenneth Russell]

Comment on attachment 143605 [details]
Patch

Looks fine as far as I can tell from code inspection.

[WebKit Review Bot]

Comment on attachment 143605 [details]
Patch

Clearing flags on attachment: 143605

Committed r118247: <http://trac.webkit.org/changeset/118247>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.