Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1773973002:
Complete the implementation of up/down-mixing rules for AudioNode (Closed)
|
Created:
4 years, 9 months ago by hongchan Modified:
4 years, 9 months ago Reviewers:
Raymond Toy CC:
chromium-reviews, blink-reviews, kinuko+watch, hongchan, Raymond Toy Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionComplete the implementation of up/down-mixing rules for AudioNode
The majority of up/down-mixing is not implemented in AudioBus.h/cpp.
This CL is to implement missing rules and improves an existing layout
test for various channel configurations to challenge the implementation
extensively.
The spec for the mixing rule is here:
http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing
BUG=460958, 465908
TEST=LayoutTests/webaudio/audionode-channel-rules.html
Committed: https://crrev.com/c5bf1a9a84aadcf927f94e806f4647d65e3da2c1
Cr-Commit-Position: refs/heads/master@{#380790}
Patch Set 1 : Initial Patch #
Total comments: 71
Patch Set 2 : Addressing Feedback (1) #
Total comments: 4
Patch Set 3 : Simplifying AudioBus::copyFrom #
Total comments: 4
Patch Set 4 : Clean up and addressing feedback #
Total comments: 7
Patch Set 5 : Cleaning up and removing casting (after l-g-t-m) #
Messages
Total messages: 37 (15 generated)
Patchset #1 (id:1) has been deleted
Description was changed from ========== Complete the implementation of up/down-mixing rules for AudioNode The majority of up/down-mixing is not implemented in AudioBus.h/cpp. This CL also improves an existing layout test for various channel configurations to challenge the implementation extensively. The spec for the mixing rule is here: http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing BUG=460958, 465908 TEST=LayoutTests/webaudio/audionode-channel-rules.html ========== to ========== Complete the implementation of up/down-mixing rules for AudioNode The majority of up/down-mixing is not implemented in AudioBus.h/cpp. This CL also improves an existing layout test for various channel configurations to challenge the implementation extensively. The spec for the mixing rule is here: http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing BUG=460958, 465908 TEST=LayoutTests/webaudio/audionode-channel-rules.html ==========
hongchan@chromium.org changed reviewers: + rtoy@chromium.org
Description was changed from ========== Complete the implementation of up/down-mixing rules for AudioNode The majority of up/down-mixing is not implemented in AudioBus.h/cpp. This CL also improves an existing layout test for various channel configurations to challenge the implementation extensively. The spec for the mixing rule is here: http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing BUG=460958, 465908 TEST=LayoutTests/webaudio/audionode-channel-rules.html ========== to ========== Complete the implementation of up/down-mixing rules for AudioNode The majority of up/down-mixing is not implemented in AudioBus.h/cpp. This CL is to implement missing rules and improves an existing layout test for various channel configurations to challenge the implementation extensively. The spec for the mixing rule is here: http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing BUG=460958, 465908 TEST=LayoutTests/webaudio/audionode-channel-rules.html ==========
Description was changed from ========== Complete the implementation of up/down-mixing rules for AudioNode The majority of up/down-mixing is not implemented in AudioBus.h/cpp. This CL is to implement missing rules and improves an existing layout test for various channel configurations to challenge the implementation extensively. The spec for the mixing rule is here: http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing BUG=460958, 465908 TEST=LayoutTests/webaudio/audionode-channel-rules.html ========== to ========== Complete the implementation of up/down-mixing rules for AudioNode The majority of up/down-mixing is not implemented in AudioBus.h/cpp. This CL is to implement missing rules and improves an existing layout test for various channel configurations to challenge the implementation extensively. The spec for the mixing rule is here: http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing BUG=460958, 465908 TEST=LayoutTests/webaudio/audionode-channel-rules.html ==========
PTAL. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/audionode-channel-rules-expected.txt (right): https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audionode-channel-rules-expected.txt:5: Not sure it's relevant, but I believe this line is added because I removed unnecessary DIVs.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773973002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773973002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/audionode-channel-rules-expected.txt (right): https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audionode-channel-rules-expected.txt:178: TEST COMPLETE Am I missing something? You added a bunch of new mixing rules, but the output didn't change at all. Does that mean the original test had all the tests but had the wrong reference output (because the rules were incomplete)? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js (right): https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:13: * @param {Number} numberOfChannels Number of channels of test buffer. If you're going to document the code this way, don't you need an entry for context and frameLength? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:27: * Stringify AudioBuffer content with options. Not obvious how you want to stringify the AudioBuffer. Describe better. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:28: * @param {AudioBuffer} audioBuffer AudioBuffer object to stringify. The other parameters/outputs have the description lined up in a column. Make this match? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:30: * @param {Number} frameOffset Offset frames to print. This description of frameOffset doesn't really say anything. Make it clearer. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:48: * Computer number of channels from the connection. Typo: "Computer" -> "Compute" https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:50: * string "128" means 3 connections, having Description doesn't line up with the rest of the descriptions. This also means that we can't have more than 9 channels. Is this a problem? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:54: * @return {Number} A computed number of channels. A link to the spec would be nice. I can never remember the rules and it always takes me a while to find it in the spec. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:82: throw "[mixing-rules.js] speakerSum(): buffer lengths mismatch"; Since you know the lengths, print them out in the message, somehow. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:95: if (source.numberOfChannels < destination.numberOfChannels) I find this style confusing. I think it's clearer just to do if (s.nc === d.nc) { ... } else if (s.nc < d.nc) { ... } else { ... } https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:104: * @param {AudioBuffer} destination Destination audio buffer. Since nothing is returned, I suggest saying that the destination is modified with the result. Otherwise, this looks like a nop. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:108: throw "[mixing-rules.js] discreteSum(): buffer lengths mismatch"; Give lengths in message. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:112: : destination.numberOfChannels; numberOfChannels = Math.min(source.numberOfChannels, destination.numberOfChannels)? Right? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:125: * @param {AudioBuffer} destination Destination audio buffer. Say that the destination is modified. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:136: // output.SR = 0 (in the case of 1 -> 4) Make comment notation match code notation more closely. output vs destination, input vs source. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:143: destinationChannel0[i] += sourceChannel[i]; This doesn't match the comment. You're summing the source into the destination, but the comment says destination = source. Update the code or update the comment appropriately. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:161: destinationChannel2[i] += sourceChannel[i]; Again, this doesn't match the comment. Update comment or code. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:181: destinationChannel1[i] += sourceChannel1[i]; Again, this doesn't match the comment. Update comment or code. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:234: destinationChannel0[i] += 0.5 * (sourceChannel0[i] + sourceChannel1[i]); Again, this doesn't match the comment. Update comment or code. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:248: destinationChannel0[i] += 0.25 * (sourceChannel0[i] + sourceChannel1[i] Again, this doesn't match the comment. Update comment or code. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:267: destinationChannel0[i] += This doesn't match the comment. Update comment or code. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:279: var sourceChannel0 = source.getChannelData(0); Probably want to say how sourceChannel<n> is assigned to input.<name>. Here and below. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:287: destinationChannel1[i] += 0.5 * (sourceChannel1[i] + sourceChannel3[i]); This doesn't match the comment. Update comment or code. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioBus.cpp (right): https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:235: zero(); Don't quite understand why you need to zero the bus here. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:245: default: This shouldn't be needed since channelInterpretation is a ChannelInterpretation object. I was expecting the compiler to warn that the default case doesn't happen. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:279: ASSERT_NOT_REACHED(); This switch statement is almost identical to the switch above. Can they be refactored into one function? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:298: return; Is it obvious that channels 2 and 3 are always zero in this case? What if the input node suddenly changed channel counts? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:311: return; Are the other channels always zero? Same question for the cases below. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:355: // output = 0.5 * (input.L + input.R) It would be nice if this comment and the code used the same names: input vs source; output vs destination. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:361: float* destination = channelByType(ChannelLeft)->mutableData(); Although you didn't change this, I wonder why destination = channel(0) doesn't work. There's only one channel in the destination, right? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:364: vsma(sourceL, 1, &scale, destination, 1, length()); Is there some implicit guarantee that destination is zero before you start summing in to it? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:378: float* destination = channelByType(ChannelLeft)->mutableData(); Is destination guaranteed to be zero? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:422: float* destinationR = channelByType(ChannelRight)->mutableData(); Again, are destinationL and destinationR guaranteed to contain 0? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:444: float* destinationR = channelByType(ChannelRight)->mutableData(); Are destinationL and destinationR guaranteed to be 0?
https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/audionode-channel-rules-expected.txt (right): https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/audionode-channel-rules-expected.txt:178: TEST COMPLETE On 2016/03/08 18:04:06, Raymond Toy wrote: > Am I missing something? You added a bunch of new mixing rules, but the output > didn't change at all. Does that mean the original test had all the tests but > had the wrong reference output (because the rules were incomplete)? Before this patch, this test was testing an incomplete implementation with the incomplete testing code. Now with the complete implementation, I had to complete the testing code as well. Does it make sense? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js (right): https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:13: * @param {Number} numberOfChannels Number of channels of test buffer. On 2016/03/08 18:04:06, Raymond Toy wrote: > If you're going to document the code this way, don't you need an entry for > context and frameLength? Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:27: * Stringify AudioBuffer content with options. On 2016/03/08 18:04:07, Raymond Toy wrote: > Not obvious how you want to stringify the AudioBuffer. Describe better. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:28: * @param {AudioBuffer} audioBuffer AudioBuffer object to stringify. On 2016/03/08 18:04:07, Raymond Toy wrote: > The other parameters/outputs have the description lined up in a column. Make > this match? Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:30: * @param {Number} frameOffset Offset frames to print. On 2016/03/08 18:04:07, Raymond Toy wrote: > This description of frameOffset doesn't really say anything. Make it clearer. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:48: * Computer number of channels from the connection. On 2016/03/08 18:04:06, Raymond Toy wrote: > Typo: "Computer" -> "Compute" Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:50: * string "128" means 3 connections, having On 2016/03/08 18:04:06, Raymond Toy wrote: > Description doesn't line up with the rest of the descriptions. > > This also means that we can't have more than 9 channels. Is this a problem? Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:54: * @return {Number} A computed number of channels. On 2016/03/08 18:04:06, Raymond Toy wrote: > A link to the spec would be nice. I can never remember the rules and it always > takes me a while to find it in the spec. URL is at the top of this file, and I don't think here is the place for the link. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:82: throw "[mixing-rules.js] speakerSum(): buffer lengths mismatch"; On 2016/03/08 18:04:06, Raymond Toy wrote: > Since you know the lengths, print them out in the message, somehow. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:95: if (source.numberOfChannels < destination.numberOfChannels) On 2016/03/08 18:04:07, Raymond Toy wrote: > I find this style confusing. I think it's clearer just to do > > if (s.nc === d.nc) { > ... > } else if (s.nc < d.nc) { > ... > } else { > ... > } Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:104: * @param {AudioBuffer} destination Destination audio buffer. On 2016/03/08 18:04:07, Raymond Toy wrote: > Since nothing is returned, I suggest saying that the destination is modified > with the result. Otherwise, this looks like a nop. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:108: throw "[mixing-rules.js] discreteSum(): buffer lengths mismatch"; On 2016/03/08 18:04:06, Raymond Toy wrote: > Give lengths in message. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:112: : destination.numberOfChannels; On 2016/03/08 18:04:07, Raymond Toy wrote: > numberOfChannels = Math.min(source.numberOfChannels, > destination.numberOfChannels)? Right? Done. I was following the original test code - but this is better. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:125: * @param {AudioBuffer} destination Destination audio buffer. On 2016/03/08 18:04:07, Raymond Toy wrote: > Say that the destination is modified. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:136: // output.SR = 0 (in the case of 1 -> 4) On 2016/03/08 18:04:06, Raymond Toy wrote: > Make comment notation match code notation more closely. output vs destination, > input vs source. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:143: destinationChannel0[i] += sourceChannel[i]; On 2016/03/08 18:04:06, Raymond Toy wrote: > This doesn't match the comment. You're summing the source into the destination, > but the comment says destination = source. > > Update the code or update the comment appropriately. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:161: destinationChannel2[i] += sourceChannel[i]; On 2016/03/08 18:04:06, Raymond Toy wrote: > Again, this doesn't match the comment. Update comment or code. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:181: destinationChannel1[i] += sourceChannel1[i]; On 2016/03/08 18:04:07, Raymond Toy wrote: > Again, this doesn't match the comment. Update comment or code. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:234: destinationChannel0[i] += 0.5 * (sourceChannel0[i] + sourceChannel1[i]); On 2016/03/08 18:04:07, Raymond Toy wrote: > Again, this doesn't match the comment. Update comment or code. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:248: destinationChannel0[i] += 0.25 * (sourceChannel0[i] + sourceChannel1[i] On 2016/03/08 18:04:07, Raymond Toy wrote: > Again, this doesn't match the comment. Update comment or code. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:267: destinationChannel0[i] += On 2016/03/08 18:04:07, Raymond Toy wrote: > This doesn't match the comment. Update comment or code. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:279: var sourceChannel0 = source.getChannelData(0); On 2016/03/08 18:04:07, Raymond Toy wrote: > Probably want to say how sourceChannel<n> is assigned to input.<name>. Here and > below. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:287: destinationChannel1[i] += 0.5 * (sourceChannel1[i] + sourceChannel3[i]); On 2016/03/08 18:04:07, Raymond Toy wrote: > This doesn't match the comment. Update comment or code. Done. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioBus.cpp (right): https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:235: zero(); On 2016/03/08 18:04:07, Raymond Toy wrote: > Don't quite understand why you need to zero the bus here. Because copying is basically zeroing-out and then summing into. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:245: default: On 2016/03/08 18:04:07, Raymond Toy wrote: > This shouldn't be needed since channelInterpretation is a ChannelInterpretation > object. > > I was expecting the compiler to warn that the default case doesn't happen. Then I will remove this. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:279: ASSERT_NOT_REACHED(); On 2016/03/08 18:04:07, Raymond Toy wrote: > This switch statement is almost identical to the switch above. Can they be > refactored into one function? We discussed this offline and decided to keep them as they are. Will revisit later when the optimization is needed. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:298: return; On 2016/03/08 18:04:07, Raymond Toy wrote: > Is it obvious that channels 2 and 3 are always zero in this case? What if the > input node suddenly changed channel counts? All up/down-mixing is in-place summing. The destination will be zero when 1) this gets called from 'copyFrom' or 2) the first connection is processed. By the way, I believe all AudioBus operation happens in the audio thread and will be safe from of 'audiograph change', which only happens in DeferredTaskHandler running on the main thread. Right? https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:311: return; On 2016/03/08 18:04:08, Raymond Toy wrote: > Are the other channels always zero? Same question for the cases below. Unlike the comment, all up/down-mixing is in-place summing. So leaving unused channels is okay. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:355: // output = 0.5 * (input.L + input.R) On 2016/03/08 18:04:08, Raymond Toy wrote: > It would be nice if this comment and the code used the same names: input vs > source; output vs destination. The situation is different with the test code - here I definitely want to keep the source/destination because that is our convention throughout the AudioBus and Channels. I can change the comment, but I believe leaving as it is on the spec is nice. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:361: float* destination = channelByType(ChannelLeft)->mutableData(); On 2016/03/08 18:04:07, Raymond Toy wrote: > Although you didn't change this, I wonder why destination = channel(0) doesn't > work. There's only one channel in the destination, right? I believe Chris did this for the readability, but channel(0) makes sense as well. I see this can go both ways. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:364: vsma(sourceL, 1, &scale, destination, 1, length()); On 2016/03/08 18:04:07, Raymond Toy wrote: > Is there some implicit guarantee that destination is zero before you start > summing in to it? All up/down-mixing is in-place summing. The destination will be zero when 1) this gets called from 'copyFrom' or 2) the first connection is processed. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:378: float* destination = channelByType(ChannelLeft)->mutableData(); On 2016/03/08 18:04:07, Raymond Toy wrote: > Is destination guaranteed to be zero? All up/down-mixing is in-place summing. The destination will be zero when 1) this gets called from 'copyFrom' or 2) the first connection is processed. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:422: float* destinationR = channelByType(ChannelRight)->mutableData(); On 2016/03/08 18:04:07, Raymond Toy wrote: > Again, are destinationL and destinationR guaranteed to contain 0? All up/down-mixing is in-place summing. The destination will be zero when 1) this gets called from 'copyFrom' or 2) the first connection is processed. https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:444: float* destinationR = channelByType(ChannelRight)->mutableData(); On 2016/03/08 18:04:07, Raymond Toy wrote: > Are destinationL and destinationR guaranteed to be 0? All up/down-mixing is in-place summing. The destination will be zero when 1) this gets called from 'copyFrom' or 2) the first connection is processed.
https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js (right): https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:54: * @return {Number} A computed number of channels. On 2016/03/08 23:53:35, hoch wrote: > On 2016/03/08 18:04:06, Raymond Toy wrote: > > A link to the spec would be nice. I can never remember the rules and it always > > takes me a while to find it in the spec. > > URL is at the top of this file, and I don't think here is the place for the > link. That link is for the mixing rules. This function is handling the computed number of channels. A link to that section would be nice.
Here are some numbers from the time-profiler: * Without simplification: (PS2) Total running time - 445.0ms (CPU 100%) AudioBus::sumFrom() - 55ms (CPU 12.3%) AudioBus::copyFrom() - 1ms (CPU 0.2%) * With simplification: (PS3) Total running time: 489.9ms (CPU 99.7%) AudioBus::sumFrom() - 62ms (CPU 12.6%) Even though there is the total running time is a bit different, I think the performance difference is almost negligible. WDYT https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js (right): https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:54: * @return {Number} A computed number of channels. On 2016/03/08 23:56:54, Raymond Toy wrote: > On 2016/03/08 23:53:35, hoch wrote: > > On 2016/03/08 18:04:06, Raymond Toy wrote: > > > A link to the spec would be nice. I can never remember the rules and it > always > > > takes me a while to find it in the spec. > > > > URL is at the top of this file, and I don't think here is the place for the > > link. > > That link is for the mixing rules. This function is handling the computed > number of channels. A link to that section would be nice. It does not have a separate section, but a definition. I added a URL to that.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773973002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773973002/60001
On Wed, Mar 9, 2016 at 11:34 AM, <hongchan@chromium.org> wrote: > Here are some numbers from the time-profiler: > > * Without simplification: (PS2) > Total running time - 445.0ms (CPU 100%) > AudioBus::sumFrom() - 55ms (CPU 12.3%) > AudioBus::copyFrom() - 1ms (CPU 0.2%) > > So, basically, the mixing cost is 12.5%? > * With simplification: (PS3) > Total running time: 489.9ms (CPU 99.7%) > AudioBus::sumFrom() - 62ms (CPU 12.6%) > > So 12.5% vs 12.6%? I'd say that's in the measurement noise. Thanks so much for doing the test! > Even though there is the total running time is a bit different, I think the > performance difference is almost negligible. > > WDYT > > > > https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js > (right): > > > https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:54: * > @return {Number} A computed number of channels. > On 2016/03/08 23:56:54, Raymond Toy wrote: > > On 2016/03/08 23:53:35, hoch wrote: > > > On 2016/03/08 18:04:06, Raymond Toy wrote: > > > > A link to the spec would be nice. I can never remember the rules > and it > > always > > > > takes me a while to find it in the spec. > > > > > > URL is at the top of this file, and I don't think here is the place > for the > > > link. > > > > That link is for the mixing rules. This function is handling the > computed > > number of channels. A link to that section would be nice. > > It does not have a separate section, but a definition. I added a URL to > that. > > https://codereview.chromium.org/1773973002/ > -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
On Wed, Mar 9, 2016 at 11:34 AM, <hongchan@chromium.org> wrote: > Here are some numbers from the time-profiler: > > * Without simplification: (PS2) > Total running time - 445.0ms (CPU 100%) > AudioBus::sumFrom() - 55ms (CPU 12.3%) > AudioBus::copyFrom() - 1ms (CPU 0.2%) > > So, basically, the mixing cost is 12.5%? > * With simplification: (PS3) > Total running time: 489.9ms (CPU 99.7%) > AudioBus::sumFrom() - 62ms (CPU 12.6%) > > So 12.5% vs 12.6%? I'd say that's in the measurement noise. Thanks so much for doing the test! > Even though there is the total running time is a bit different, I think the > performance difference is almost negligible. > > WDYT > > > > https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... > File third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js > (right): > > > https://codereview.chromium.org/1773973002/diff/20001/third_party/WebKit/Layo... > third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:54: * > @return {Number} A computed number of channels. > On 2016/03/08 23:56:54, Raymond Toy wrote: > > On 2016/03/08 23:53:35, hoch wrote: > > > On 2016/03/08 18:04:06, Raymond Toy wrote: > > > > A link to the spec would be nice. I can never remember the rules > and it > > always > > > > takes me a while to find it in the spec. > > > > > > URL is at the top of this file, and I don't think here is the place > for the > > > link. > > > > That link is for the mixing rules. This function is handling the > computed > > number of channels. A link to that section would be nice. > > It does not have a separate section, but a definition. I added a URL to > that. > > https://codereview.chromium.org/1773973002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1773973002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js (right): https://codereview.chromium.org/1773973002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:29: * Create a string displays the content of AudioBuffer. "string displays" -> "string that displays" https://codereview.chromium.org/1773973002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:125: * Perform up-mix by in-place summing to |output| buffer. For consistency, you probably want to add this comment to the following functions to emphasize in-place summing operation to the output. https://codereview.chromium.org/1773973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioBus.cpp (right): https://codereview.chromium.org/1773973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:247: processDownMix(sourceBus); To make it more obvious can we somehow include "sumFrom" (or similar) in the names processUpMix and processDownMix? Maybe processUpMixWithSumming? https://codereview.chromium.org/1773973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:343: if (numberOfSourceChannels == 4 && numberOfDestinationChannels == 1) { I find it clearer if this set of if's were written as a sequence like if (case1) { ... } else if (case2) { ... } else { defaultcase; } As it is currently, I have to notice that there's a return in every if. Same comment for processUpMix.
https://codereview.chromium.org/1773973002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js (right): https://codereview.chromium.org/1773973002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:29: * Create a string displays the content of AudioBuffer. On 2016/03/09 22:14:19, Raymond Toy wrote: > "string displays" -> "string that displays" Done. https://codereview.chromium.org/1773973002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/webaudio/resources/mixing-rules.js:125: * Perform up-mix by in-place summing to |output| buffer. On 2016/03/09 22:14:19, Raymond Toy wrote: > For consistency, you probably want to add this comment to the following > functions to emphasize in-place summing operation to the output. Done. https://codereview.chromium.org/1773973002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioBus.cpp (right): https://codereview.chromium.org/1773973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:247: processDownMix(sourceBus); On 2016/03/09 22:14:19, Raymond Toy wrote: > To make it more obvious can we somehow include "sumFrom" (or similar) in the > names processUpMix and processDownMix? Maybe processUpMixWithSumming? Done. https://codereview.chromium.org/1773973002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:343: if (numberOfSourceChannels == 4 && numberOfDestinationChannels == 1) { On 2016/03/09 22:14:19, Raymond Toy wrote: > I find it clearer if this set of if's were written as a sequence like > > if (case1) { > ... > } else if (case2) { > ... > } else { > defaultcase; > } > > As it is currently, I have to notice that there's a return in every if. > > Same comment for processUpMix. Done.
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773973002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773973002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm with some nits. You don't have to fix the nits. https://codereview.chromium.org/1773973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioBus.cpp (right): https://codereview.chromium.org/1773973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:276: if ((numberOfSourceChannels == 1 && numberOfDestinationChannels == 2) || (numberOfSourceChannels == 1 && numberOfDestinationChannels == 4)) { Not something to fix here, but we should probably start limiting the line length since blink style will become chromium someday. (Gotta remember that myself.) https://codereview.chromium.org/1773973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:282: const AudioChannel* sourceChannel = sourceBus.channel(0); Style nit: In sumFromByDownMixing, you access channels using channelByType() method. Should we do the same here? https://codereview.chromium.org/1773973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:360: AudioBus& sourceBusConst = const_cast<AudioBus&>(sourceBus); Seems weird that you have to do a const cast. I'm guessing data() doesn't return a const float*. Which seems strange since for the destination you call mutableData(). A bug in AudioBus?
The CQ bit was checked by hongchan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773973002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773973002/100001
Did some cleaning up in AudioBus.cpp. PTAL. https://codereview.chromium.org/1773973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioBus.cpp (right): https://codereview.chromium.org/1773973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:276: if ((numberOfSourceChannels == 1 && numberOfDestinationChannels == 2) || (numberOfSourceChannels == 1 && numberOfDestinationChannels == 4)) { On 2016/03/10 18:56:47, Raymond Toy wrote: > Not something to fix here, but we should probably start limiting the line length > since blink style will become chromium someday. (Gotta remember that myself.) Acknowledged, but "git cl format" always reverts my effort of wrapping codes in 80 columns. https://codereview.chromium.org/1773973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:282: const AudioChannel* sourceChannel = sourceBus.channel(0); On 2016/03/10 18:56:47, Raymond Toy wrote: > Style nit: In sumFromByDownMixing, you access channels using channelByType() > method. Should we do the same here? Done, but what about the sources? Should we change everything to maintain the consistency? https://codereview.chromium.org/1773973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:360: AudioBus& sourceBusConst = const_cast<AudioBus&>(sourceBus); On 2016/03/10 18:56:47, Raymond Toy wrote: > Seems weird that you have to do a const cast. I'm guessing data() doesn't return > a const float*. Which seems strange since for the destination you call > mutableData(). A bug in AudioBus? After the discussion we had, I removed the all const_cast in this method. It compiles and works correctly.
lgtm. https://codereview.chromium.org/1773973002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/audio/AudioBus.cpp (right): https://codereview.chromium.org/1773973002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/audio/AudioBus.cpp:276: if ((numberOfSourceChannels == 1 && numberOfDestinationChannels == 2) || (numberOfSourceChannels == 1 && numberOfDestinationChannels == 4)) { On 2016/03/11 19:48:06, hoch wrote: > On 2016/03/10 18:56:47, Raymond Toy wrote: > > Not something to fix here, but we should probably start limiting the line > length > > since blink style will become chromium someday. (Gotta remember that myself.) > > Acknowledged, but "git cl format" always reverts my effort of wrapping codes in > 80 columns. Oh well. We'll just have to wait for the Great Reformatting to Come.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hongchan@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1773973002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1773973002/100001
Message was sent while issue was closed.
Description was changed from ========== Complete the implementation of up/down-mixing rules for AudioNode The majority of up/down-mixing is not implemented in AudioBus.h/cpp. This CL is to implement missing rules and improves an existing layout test for various channel configurations to challenge the implementation extensively. The spec for the mixing rule is here: http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing BUG=460958, 465908 TEST=LayoutTests/webaudio/audionode-channel-rules.html ========== to ========== Complete the implementation of up/down-mixing rules for AudioNode The majority of up/down-mixing is not implemented in AudioBus.h/cpp. This CL is to implement missing rules and improves an existing layout test for various channel configurations to challenge the implementation extensively. The spec for the mixing rule is here: http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing BUG=460958, 465908 TEST=LayoutTests/webaudio/audionode-channel-rules.html ==========
Message was sent while issue was closed.
Committed patchset #5 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Complete the implementation of up/down-mixing rules for AudioNode The majority of up/down-mixing is not implemented in AudioBus.h/cpp. This CL is to implement missing rules and improves an existing layout test for various channel configurations to challenge the implementation extensively. The spec for the mixing rule is here: http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing BUG=460958, 465908 TEST=LayoutTests/webaudio/audionode-channel-rules.html ========== to ========== Complete the implementation of up/down-mixing rules for AudioNode The majority of up/down-mixing is not implemented in AudioBus.h/cpp. This CL is to implement missing rules and improves an existing layout test for various channel configurations to challenge the implementation extensively. The spec for the mixing rule is here: http://webaudio.github.io/web-audio-api/#channel-up-mixing-and-down-mixing BUG=460958, 465908 TEST=LayoutTests/webaudio/audionode-channel-rules.html Committed: https://crrev.com/c5bf1a9a84aadcf927f94e806f4647d65e3da2c1 Cr-Commit-Position: refs/heads/master@{#380790} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c5bf1a9a84aadcf927f94e806f4647d65e3da2c1 Cr-Commit-Position: refs/heads/master@{#380790} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
