Bug 83610

Summary: AudioParam must support fan-in (multiple audio connections)
Product: WebKit Reporter: Chris Rogers <crogers>
Component: Web AudioAssignee: Chris Rogers <crogers>
Status: RESOLVED FIXED    
Severity: Normal CC: crogers, eric.carlson, feature-media-reviews, kbr, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 86710    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch none

Description Chris Rogers 2012-04-10 13:09:31 PDT
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
Comment 1 Chris Rogers 2012-05-15 18:36:36 PDT
Created attachment 142120 [details]
Patch
Comment 2 Kenneth Russell 2012-05-15 20:41:05 PDT
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
Comment 3 Chris Rogers 2012-05-16 18:13:34 PDT
Committed r117372: <http://trac.webkit.org/changeset/117372>
Comment 4 Chris Rogers 2012-05-16 18:17:41 PDT
(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
Comment 5 WebKit Review Bot 2012-05-16 23:41:38 PDT
Re-opened since this is blocked by 86710
Comment 6 Chris Rogers 2012-05-23 11:47:25 PDT
Created attachment 143605 [details]
Patch
Comment 7 Chris Rogers 2012-05-23 11:49:21 PDT
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
Comment 8 Kenneth Russell 2012-05-23 13:51:58 PDT
Comment on attachment 143605 [details]
Patch

Looks fine as far as I can tell from code inspection.
Comment 9 WebKit Review Bot 2012-05-23 14:45:32 PDT
Comment on attachment 143605 [details]
Patch

Clearing flags on attachment: 143605

Committed r118247: <http://trac.webkit.org/changeset/118247>
Comment 10 WebKit Review Bot 2012-05-23 14:45:37 PDT
All reviewed patches have been landed.  Closing bug.