Summary: | AudioParam must support fan-in (multiple audio connections) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Rogers <crogers> | ||||||
Component: | Web Audio | Assignee: | 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
Chris Rogers
2012-04-10 13:09:31 PDT
Created attachment 142120 [details]
Patch
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 Committed r117372: <http://trac.webkit.org/changeset/117372> (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 Re-opened since this is blocked by 86710 Created attachment 143605 [details]
Patch
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 on attachment 143605 [details]
Patch
Looks fine as far as I can tell from code inspection.
Comment on attachment 143605 [details] Patch Clearing flags on attachment: 143605 Committed r118247: <http://trac.webkit.org/changeset/118247> All reviewed patches have been landed. Closing bug. |