Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(59)

Issue 808653010: Fixing input handling behavior of ChannelMergerNode (Closed)

Created:
5 years, 11 months ago by hongchan
Modified:
5 years, 9 months ago
Reviewers:
Raymond Toy
CC:
blink-reviews
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Target Ref:
refs/heads/master
Project:
blink
Visibility:
Public.

Description

Fixing input handling behavior of ChannelMergerNode The current implementation of ChannelMergerNode has two critical issues: 1. Impossible to specify input-output mapping due to dynamic channel re-arranging. https://github.com/WebAudio/web-audio-api/issues/304, crbug.com/441451 2. ChannelMergerNode connected with DelayNode in a cyclic audiograph causes a silent failure of audio rendering. https://github.com/WebAudio/web-audio-api/issues/459, crbug.com/442925 To address these issues, rtoy@, cwilso@ and hongchan@ proposed a new behavior of ChannelMergerNode: 1. The following properties are fixed for ChannelMergerNode and InvalidState error should be thrown when user changes them. - ChannelMergerNode.channelCount = 1 - ChannelMergerNode.channelCountMode = ‘explicit’ 2. Each input will be summed to mono based on the specified mixing rule. 3. The summed mono channel from input will be transferred to the corresponding output channel (input_1 -> output_channel_1). If an input is not connected (inactive or disconnected), it still counts as one silent channel in output. Note that the up/downmixing rule is only applied to each input when the channel layout is canonical (mono, stereo, quad, 5.0 and 5.1). For undefined channel layout, the mixing rule automatically changes to 'discrete' mode. That means the merger will just take the first channel and drop the rest. BUG=441451, 442925 TESTS=4 new tests audiochannelmerger-cycle.html: to cover the bug 442925 audiochannelmerger-disconnect.html: to cover the bug 441451 audiochannelmerger-input.html: to cover the bug 441451 audiochannelmerger-input-non-default.html: to test the merger with non-default setting Note that the following laytout test files were removed (or renamed) and merged into the new tests: channelmerger-basic.html => audiochannelmerger-basic.html channelmerger-input-handling.html => audiochannelmerger-input.html channelmerger-cycle.html => audiochannelmerger-cycle.html channelmerger-channel-limit.html => audiochannelmerger-basic.html audiochannelmerger-stereo.html => audiochannelmerger-input.html Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191723

Patch Set 1 #

Total comments: 32

Patch Set 2 : #

Total comments: 26

Patch Set 3 : #

Total comments: 10

Patch Set 4 : #

Patch Set 5 : Add layout tests #

Total comments: 63

Patch Set 6 : #

Patch Set 7 : Refactored input test #

Total comments: 4

Patch Set 8 : Fixed nits in layout tests. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+693 lines, -429 lines) Patch
M LayoutTests/webaudio/audiochannelmerger-basic.html View 1 2 3 4 5 6 7 1 chunk +68 lines, -55 lines 0 comments Download
M LayoutTests/webaudio/audiochannelmerger-basic-expected.txt View 1 2 3 4 5 6 7 1 chunk +10 lines, -6 lines 0 comments Download
A LayoutTests/webaudio/audiochannelmerger-cycle.html View 1 2 3 4 5 1 chunk +113 lines, -0 lines 0 comments Download
A + LayoutTests/webaudio/audiochannelmerger-cycle-expected.txt View 1 2 3 4 1 chunk +2 lines, -3 lines 0 comments Download
A LayoutTests/webaudio/audiochannelmerger-disconnect.html View 1 2 3 4 5 1 chunk +79 lines, -0 lines 0 comments Download
A LayoutTests/webaudio/audiochannelmerger-disconnect-expected.txt View 1 2 3 4 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/webaudio/audiochannelmerger-input.html View 1 2 3 4 5 6 7 1 chunk +127 lines, -0 lines 0 comments Download
A LayoutTests/webaudio/audiochannelmerger-input-expected.txt View 1 2 3 4 5 6 1 chunk +29 lines, -0 lines 0 comments Download
A LayoutTests/webaudio/audiochannelmerger-input-non-default.html View 1 2 3 4 5 6 7 1 chunk +92 lines, -0 lines 0 comments Download
A LayoutTests/webaudio/audiochannelmerger-input-non-default-expected.txt View 1 2 3 4 5 6 1 chunk +30 lines, -0 lines 0 comments Download
D LayoutTests/webaudio/audiochannelmerger-stereo.html View 1 2 3 4 1 chunk +0 lines, -115 lines 0 comments Download
D LayoutTests/webaudio/audiochannelmerger-stereo-expected.txt View 1 2 3 4 1 chunk +0 lines, -9 lines 0 comments Download
D LayoutTests/webaudio/channelmerger-channel-limit.html View 1 chunk +0 lines, -127 lines 0 comments Download
D LayoutTests/webaudio/channelmerger-channel-limit-expected.txt View 1 chunk +0 lines, -42 lines 0 comments Download
M LayoutTests/webaudio/resources/audio-testing.js View 1 2 3 4 5 6 8 chunks +42 lines, -9 lines 0 comments Download
A LayoutTests/webaudio/resources/merger-testing.js View 1 2 3 4 5 6 7 1 chunk +30 lines, -0 lines 0 comments Download
M Source/modules/webaudio/ChannelMergerNode.h View 1 chunk +3 lines, -7 lines 0 comments Download
M Source/modules/webaudio/ChannelMergerNode.cpp View 1 2 3 4 5 3 chunks +57 lines, -56 lines 0 comments Download

Messages

Total messages: 27 (3 generated)
hongchan
PTAL
5 years, 11 months ago (2015-01-13 19:05:44 UTC) #2
Raymond Toy
https://codereview.chromium.org/808653010/diff/1/LayoutTests/webaudio/channelmerger-cycle.html File LayoutTests/webaudio/channelmerger-cycle.html (right): https://codereview.chromium.org/808653010/diff/1/LayoutTests/webaudio/channelmerger-cycle.html#newcode19 LayoutTests/webaudio/channelmerger-cycle.html:19: var renderLength = 512; // 4x of rendering quantum ...
5 years, 11 months ago (2015-01-13 19:45:02 UTC) #3
hongchan
https://codereview.chromium.org/808653010/diff/1/LayoutTests/webaudio/channelmerger-cycle.html File LayoutTests/webaudio/channelmerger-cycle.html (right): https://codereview.chromium.org/808653010/diff/1/LayoutTests/webaudio/channelmerger-cycle.html#newcode19 LayoutTests/webaudio/channelmerger-cycle.html:19: var renderLength = 512; // 4x of rendering quantum ...
5 years, 11 months ago (2015-01-13 22:38:57 UTC) #4
Raymond Toy
Just a few more small comments. https://codereview.chromium.org/808653010/diff/1/Source/modules/webaudio/ChannelMergerNode.cpp File Source/modules/webaudio/ChannelMergerNode.cpp (right): https://codereview.chromium.org/808653010/diff/1/Source/modules/webaudio/ChannelMergerNode.cpp#newcode112 Source/modules/webaudio/ChannelMergerNode.cpp:112: "channelCount cannot be ...
5 years, 11 months ago (2015-01-13 23:20:37 UTC) #5
hongchan
https://codereview.chromium.org/808653010/diff/20001/LayoutTests/webaudio/channelmerger-cycle.html File LayoutTests/webaudio/channelmerger-cycle.html (right): https://codereview.chromium.org/808653010/diff/20001/LayoutTests/webaudio/channelmerger-cycle.html#newcode19 LayoutTests/webaudio/channelmerger-cycle.html:19: // in delay time. On 2015/01/13 23:20:36, Raymond Toy ...
5 years, 11 months ago (2015-01-14 17:49:49 UTC) #6
Raymond Toy
lgtm. https://codereview.chromium.org/808653010/diff/40001/LayoutTests/webaudio/channelmerger-cycle.html File LayoutTests/webaudio/channelmerger-cycle.html (right): https://codereview.chromium.org/808653010/diff/40001/LayoutTests/webaudio/channelmerger-cycle.html#newcode91 LayoutTests/webaudio/channelmerger-cycle.html:91: // zero because of 128 samples delay. Thus ...
5 years, 11 months ago (2015-01-14 18:11:45 UTC) #7
hongchan
Thank you for the review - and I apologize for small mistakes here and there. ...
5 years, 11 months ago (2015-01-14 18:18:16 UTC) #8
Raymond Toy
After this lands, I think we probably want to add an entry to the Chromium ...
5 years, 11 months ago (2015-01-14 18:44:04 UTC) #9
hongchan
On 2015/01/14 18:44:04, Raymond Toy wrote: > After this lands, I think we probably want ...
5 years, 11 months ago (2015-01-14 18:51:42 UTC) #10
hongchan
PTAL. We agreed to make several changes, so I think this CL needs one more ...
5 years, 9 months ago (2015-03-10 17:48:31 UTC) #11
Raymond Toy
First, a comment on the description. How is bug 448801 relevant to this CL? I ...
5 years, 9 months ago (2015-03-10 17:55:49 UTC) #12
blink-reviews
Actually I remember that we agreed to mention the bug 448801 as a side note ...
5 years, 9 months ago (2015-03-10 18:00:17 UTC) #13
Raymond Toy
On 2015/03/10 18:00:17, blink-reviews wrote: > Actually I remember that we agreed to mention the ...
5 years, 9 months ago (2015-03-10 18:06:04 UTC) #14
hongchan
On 2015/03/10 18:06:04, Raymond Toy wrote: > On 2015/03/10 18:00:17, blink-reviews wrote: > > Actually ...
5 years, 9 months ago (2015-03-10 18:33:35 UTC) #15
Raymond Toy
https://codereview.chromium.org/808653010/diff/80001/LayoutTests/webaudio/audiochannelmerger-basic-expected.txt File LayoutTests/webaudio/audiochannelmerger-basic-expected.txt (right): https://codereview.chromium.org/808653010/diff/80001/LayoutTests/webaudio/audiochannelmerger-basic-expected.txt#newcode10 LayoutTests/webaudio/audiochannelmerger-basic-expected.txt:10: PASS The evaluated target variable is equal to 1. ...
5 years, 9 months ago (2015-03-10 18:55:10 UTC) #16
Raymond Toy
On 2015/03/10 18:33:35, hoch wrote: > On 2015/03/10 18:06:04, Raymond Toy wrote: > > On ...
5 years, 9 months ago (2015-03-10 18:56:48 UTC) #17
hongchan
https://codereview.chromium.org/808653010/diff/80001/LayoutTests/webaudio/audiochannelmerger-basic-expected.txt File LayoutTests/webaudio/audiochannelmerger-basic-expected.txt (right): https://codereview.chromium.org/808653010/diff/80001/LayoutTests/webaudio/audiochannelmerger-basic-expected.txt#newcode10 LayoutTests/webaudio/audiochannelmerger-basic-expected.txt:10: PASS The evaluated target variable is equal to 1. ...
5 years, 9 months ago (2015-03-10 20:54:48 UTC) #18
Raymond Toy
https://codereview.chromium.org/808653010/diff/80001/LayoutTests/webaudio/audiochannelmerger-basic-expected.txt File LayoutTests/webaudio/audiochannelmerger-basic-expected.txt (right): https://codereview.chromium.org/808653010/diff/80001/LayoutTests/webaudio/audiochannelmerger-basic-expected.txt#newcode10 LayoutTests/webaudio/audiochannelmerger-basic-expected.txt:10: PASS The evaluated target variable is equal to 1. ...
5 years, 9 months ago (2015-03-10 21:09:28 UTC) #19
Raymond Toy
This CL doesn't fix issue 448801. I wouldn't put in the BUG list. I would ...
5 years, 9 months ago (2015-03-10 21:10:45 UTC) #20
Raymond Toy
Don't forget to mention in the description that you've deleted some tests, but they've been ...
5 years, 9 months ago (2015-03-10 21:11:44 UTC) #21
hongchan
- input and input-non-default tests are now refactored. - Should.beEqualTo() changed to print out the ...
5 years, 9 months ago (2015-03-10 22:48:30 UTC) #22
Raymond Toy
lgtm with a few nits. https://codereview.chromium.org/808653010/diff/120001/LayoutTests/webaudio/audiochannelmerger-basic-expected.txt File LayoutTests/webaudio/audiochannelmerger-basic-expected.txt (right): https://codereview.chromium.org/808653010/diff/120001/LayoutTests/webaudio/audiochannelmerger-basic-expected.txt#newcode10 LayoutTests/webaudio/audiochannelmerger-basic-expected.txt:10: PASS The merger channel ...
5 years, 9 months ago (2015-03-11 16:26:07 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/808653010/140001
5 years, 9 months ago (2015-03-11 17:24:13 UTC) #26
commit-bot: I haz the power
5 years, 9 months ago (2015-03-11 18:46:33 UTC) #27
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191723

Powered by Google App Engine
This is Rietveld 408576698