Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11150034:
Add support for channel transforms. (Closed)
|
Created:
8 years, 2 months ago by DaleCurtis Modified:
8 years, 2 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd support for channel transforms.
Introduces the ChannelMixer class for upmixing and downmixing
AudioBus objects from one channel layout to another.
Matrix building code is based on libavresample and libswresample
from ffmpeg.
Full decision listing: http://pastebin.com/KfeFzC0X
BUG=138762
TEST=unit tests.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=163158
Patch Set 1 #Patch Set 2 : Fixes. Mix LFE. #Patch Set 3 : Rebase. #
Total comments: 42
Patch Set 4 : Comments. Unit tests. #
Total comments: 13
Patch Set 5 : Fix try failures. Comments. #
Total comments: 4
Patch Set 6 : Visual Studio fixes. #
Total comments: 3
Patch Set 7 : Rename to Transform() #
Total comments: 14
Patch Set 8 : Comments. #Patch Set 9 : Add TODO. #
Messages
Total messages: 27 (0 generated)
The unit test just runs through all possible combinations right now (~0ms w/ DEBUG, will run valgrind, etc later). I'll see if I can come up with something better tomorrow. Suggestions welcome. crogers: Please check my math and logic! scherkus: General review.
Hi Dale, nice work! I've had an initial look and added comments. The main point is that we should handle mono <-> stereo conversions in a more classical/traditional way rather than try to over-generalize with equal-power mix-down coefficients which are used with more "multi-channel" layouts such as 5.1 which have been mastered differently to account for possible clipping on down-mix. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:41: DCHECK((ChannelOrder(layout, LEFT) >= 0 && Where's the ChannelOrder() function defined? http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:74: extra blank line http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:99: } I think this is the wrong approach for stereo -> mono downmix which will clip mixing down a full-scale stereo mix. Clipping is bad and will often happen. I've never seen any commercial audio sound editors do this. Instead I recommend 0.5 I know it seems inconsistent as compared with, for example, the ITU-R approach for 5.1 down-mixing. But 5.1 (and higher) mixes usually are mastered to account for the possibility that they might often be down-mixed, and thus won't have such *hot* signals in L/R, etc. Stereo mixes, on the other hand are almost always mastered "to the max", and have no extra safety margin to be using a 1/sqrt(2) approach. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:105: } This is odd and very much different from what audio systems usually do for the mono -> stereo up-mix case. A full-range mono signal will not be full-range when up-mixed to stereo which is not how it's normally handled by an OS, audio editors, etc. Instead I recommend copying mono to both left and right. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:206: void ChannelMixer::Rematrix(const AudioBus* input, AudioBus* output) { What is our plan for simple channel swizzling to account for differences between Chrome/FFmpeg channel orders, and the orders used by each OS? I'm thinking of our plan for replacing PCMQueueOutAudioOutputStream::SwizzleLayout(), and similar... It seems like we should have a highly efficient AudioBus method to simply swap the internal pointers around to avoid any memcpy's or touching the memory itself. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:214: output->frames() * sizeof(*output->channel(output_ch))); This cries out for a AudioBus::ZeroChannel() method http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:222: output->channel(output_ch)); What if the scale is 1, or does FMAC optimize for this case? http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.h File media/base/channel_mixer.h (right): http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.h#... media/base/channel_mixer.h:57: std::vector< std::vector<float> > matrix_; In general you want to avoid the pattern of vector< vector<> > since pushing new elements requires copying the whole vector. In this case, it's probably not a problem since the vector size is small and you're setting it up once in the constructor. But FYI, in other cases when dealing with larger vectors it's better to use other techniques, to avoid this kind of inefficiency.
http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:41: DCHECK((ChannelOrder(layout, LEFT) >= 0 && On 2012/10/17 19:46:50, Chris Rogers wrote: > Where's the ChannelOrder() function defined? http://codereview.chromium.org/11198018/ Will land soon. Just replaces the previous direct access to kChannelOrderings[][] http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:99: } On 2012/10/17 19:46:50, Chris Rogers wrote: > I think this is the wrong approach for stereo -> mono downmix which will clip > mixing down a full-scale stereo mix. Clipping is bad and will often happen. > > I've never seen any commercial audio sound editors do this. Instead I recommend > 0.5 > > I know it seems inconsistent as compared with, for example, the ITU-R approach > for 5.1 down-mixing. But 5.1 (and higher) mixes usually are mastered to account > for the possibility that they might often be down-mixed, and thus won't have > such *hot* signals in L/R, etc. Stereo mixes, on the other hand are almost > always mastered "to the max", and have no extra safety margin to be using a > 1/sqrt(2) approach. Okay, sounds like we just need two special case blocks for stereo -> mono and mono -> stereo. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:105: } On 2012/10/17 19:46:50, Chris Rogers wrote: > This is odd and very much different from what audio systems usually do for the > mono -> stereo up-mix case. A full-range mono signal will not be full-range > when up-mixed to stereo which is not how it's normally handled by an OS, audio > editors, etc. Instead I recommend copying mono to both left and right. Ditto. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:206: void ChannelMixer::Rematrix(const AudioBus* input, AudioBus* output) { On 2012/10/17 19:46:50, Chris Rogers wrote: > What is our plan for simple channel swizzling to account for differences between > Chrome/FFmpeg channel orders, and the orders used by each OS? I'm thinking of > our plan for replacing PCMQueueOutAudioOutputStream::SwizzleLayout(), and > similar... > > It seems like we should have a highly efficient AudioBus method to simply swap > the internal pointers around to avoid any memcpy's or touching the memory > itself. Yes I plan to add an AudioBus::Remap(int ch, int ch) for non-wrapped AudioBus objects. Each output stream will be responsible for swizzling as necessary. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:214: output->frames() * sizeof(*output->channel(output_ch))); On 2012/10/17 19:46:50, Chris Rogers wrote: > This cries out for a AudioBus::ZeroChannel() method Good idea. Will add. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:222: output->channel(output_ch)); On 2012/10/17 19:46:50, Chris Rogers wrote: > What if the scale is 1, or does FMAC optimize for this case? FMAC doesn't optimize for this case. I could add a Float-Accumulate method which skips the multiply step, but I wonder how much that would really save. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.h File media/base/channel_mixer.h (right): http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.h#... media/base/channel_mixer.h:57: std::vector< std::vector<float> > matrix_; On 2012/10/17 19:46:50, Chris Rogers wrote: > In general you want to avoid the pattern of vector< vector<> > since pushing new > elements requires copying the whole vector. In this case, it's probably not a > problem since the vector size is small and you're setting it up once in the > constructor. But FYI, in other cases when dealing with larger vectors it's > better to use other techniques, to avoid this kind of inefficiency. Sure, I chose this for readability and portability vs trying to manage a float[][] or vector<float*> of variable size. As you say given it's one time and of small size it'll be fine here.
neato! https://codereview.chromium.org/11150034/diff/9002/media/base/channel_layout.cc File media/base/channel_layout.cc (right): https://codereview.chromium.org/11150034/diff/9002/media/base/channel_layout.... media/base/channel_layout.cc:37: static const int kChannelOrderings[CHANNEL_LAYOUT_MAX][CHANNELS_MAX] = { random thought: I don't suspect we'll have to change this very often, but I wonder if a lot of this channel map/layout/etc business could get generated by a script for example, my brain hurts looking at this struct but I don't think the resulting script would be any better! https://codereview.chromium.org/11150034/diff/9002/media/base/channel_layout.... media/base/channel_layout.cc:89: { 0 , 1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 }, is this a bug fix that didn't happen when DOWNMIX was added? can we document the difference between FL/FR and StL/StR on the Channels enum? https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.c... media/base/channel_mixer.cc:6: #define _USE_MATH_DEFINES nit: move closer to where we need it (i.e., after channel_mixer.h include) https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.c... media/base/channel_mixer.cc:40: if (channel_count > 1) { is it worth asserting that channel_count == 1 iff CHANNEL_LAYOUT_MONO? https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.c... media/base/channel_mixer.cc:212: // Zero initialize |output| so we're accumulating from zero. silly optimization question... (I think chris asked this in a different way) for cases where we don't need to accumulate and the scale is 1 can't we get by w/ a single memcpy? we can leave it as a follow up TODO https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.h File media/base/channel_mixer.h (right): https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.h... media/base/channel_mixer.h:27: virtual ~ChannelMixer(); drop virtual https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer_u... File media/base/channel_mixer_unittest.cc (right): https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer_u... media/base/channel_mixer_unittest.cc:19: ChannelMixer mixer(input_layout, output_layout); should consider using SCOPED_TRACE() to document the combination of input+output in case of a failure either that or parameterized tests, but that seems like a lot of work for little benefit https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer_u... media/base/channel_mixer_unittest.cc:27: mixer.Rematrix(input_bus.get(), output_bus.get()); what exactly are we testing?
https://codereview.chromium.org/11150034/diff/9002/media/base/channel_layout.cc File media/base/channel_layout.cc (right): https://codereview.chromium.org/11150034/diff/9002/media/base/channel_layout.... media/base/channel_layout.cc:37: static const int kChannelOrderings[CHANNEL_LAYOUT_MAX][CHANNELS_MAX] = { On 2012/10/18 01:30:39, scherkus wrote: > random thought: I don't suspect we'll have to change this very often, but I > wonder if a lot of this channel map/layout/etc business could get generated by a > script > > for example, my brain hurts looking at this struct but I don't think the > resulting script would be any better! I think a better approach would be the bitmask method that ffmpeg uses now. E.g. CHANNEL_LAYOUT_STEREO = LEFT|RIGHT, etc. Then using a compile assert to keep these in sync with the ffmpeg ones. https://codereview.chromium.org/11150034/diff/9002/media/base/channel_layout.... media/base/channel_layout.cc:89: { 0 , 1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 }, On 2012/10/18 01:30:39, scherkus wrote: > is this a bug fix that didn't happen when DOWNMIX was added? > > can we document the difference between FL/FR and StL/StR on the Channels enum? I'm not sure what those channels actually mean and no one is using them. Do you have some historical context on this? Internally in FFmpeg's rematrixer they just set output = STEREO when STEREO_DOWNMIX is encountered. Barring any other information, I'll probably just delete those last two. https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.c... media/base/channel_mixer.cc:6: #define _USE_MATH_DEFINES On 2012/10/18 01:30:39, scherkus wrote: > nit: move closer to where we need it (i.e., after channel_mixer.h include) Can't do that, since channel_mixer.h and headers it includes might end up including <math.h>/<cmath> which will break this. https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.c... media/base/channel_mixer.cc:99: } On 2012/10/17 20:06:47, DaleCurtis wrote: > On 2012/10/17 19:46:50, Chris Rogers wrote: > > I think this is the wrong approach for stereo -> mono downmix which will clip > > mixing down a full-scale stereo mix. Clipping is bad and will often happen. > > > > I've never seen any commercial audio sound editors do this. Instead I > recommend > > 0.5 > > > > I know it seems inconsistent as compared with, for example, the ITU-R approach > > for 5.1 down-mixing. But 5.1 (and higher) mixes usually are mastered to > account > > for the possibility that they might often be down-mixed, and thus won't have > > such *hot* signals in L/R, etc. Stereo mixes, on the other hand are almost > > always mastered "to the max", and have no extra safety margin to be using a > > 1/sqrt(2) approach. > > Okay, sounds like we just need two special case blocks for stereo -> mono and > mono -> stereo. Done. https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.c... media/base/channel_mixer.cc:105: } On 2012/10/17 20:06:47, DaleCurtis wrote: > On 2012/10/17 19:46:50, Chris Rogers wrote: > > This is odd and very much different from what audio systems usually do for the > > mono -> stereo up-mix case. A full-range mono signal will not be full-range > > when up-mixed to stereo which is not how it's normally handled by an OS, audio > > editors, etc. Instead I recommend copying mono to both left and right. > > Ditto. Done. https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.c... media/base/channel_mixer.cc:212: // Zero initialize |output| so we're accumulating from zero. On 2012/10/18 01:30:39, scherkus wrote: > silly optimization question... (I think chris asked this in a different way) > > for cases where we don't need to accumulate and the scale is 1 can't we get by > w/ a single memcpy? > > we can leave it as a follow up TODO Hmm, we don't need to accumulate when the output layout is a remapping of the input layout. Are there any other common cases? For that case, I can set a bool during construction if there are no unaccounted channels to handle, then simply memcpy. https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer_u... File media/base/channel_mixer_unittest.cc (right): https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer_u... media/base/channel_mixer_unittest.cc:27: mixer.Rematrix(input_bus.get(), output_bus.get()); On 2012/10/18 01:30:39, scherkus wrote: > what exactly are we testing? Just that construction of all layouts is possible (DCHECKS will explode otherwise) at the moment. I haven't thought of a good automated test for this yet, other than maybe that some input exists in the output? Ideas welcome.
https://codereview.chromium.org/11150034/diff/9002/media/base/channel_layout.cc File media/base/channel_layout.cc (right): https://codereview.chromium.org/11150034/diff/9002/media/base/channel_layout.... media/base/channel_layout.cc:89: { 0 , 1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 }, On 2012/10/18 01:56:44, DaleCurtis wrote: > On 2012/10/18 01:30:39, scherkus wrote: > > is this a bug fix that didn't happen when DOWNMIX was added? > > > > can we document the difference between FL/FR and StL/StR on the Channels enum? > > I'm not sure what those channels actually mean and no one is using them. Do you > have some historical context on this? Internally in FFmpeg's rematrixer they > just set output = STEREO when STEREO_DOWNMIX is encountered. > > Barring any other information, I'll probably just delete those last two. No clue. You'll have to dig through commit logs + code reviews. https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.c... media/base/channel_mixer.cc:6: #define _USE_MATH_DEFINES On 2012/10/18 01:56:44, DaleCurtis wrote: > On 2012/10/18 01:30:39, scherkus wrote: > > nit: move closer to where we need it (i.e., after channel_mixer.h include) > > Can't do that, since channel_mixer.h and headers it includes might end up > including <math.h>/<cmath> which will break this. Gah. https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.c... media/base/channel_mixer.cc:212: // Zero initialize |output| so we're accumulating from zero. On 2012/10/18 01:56:44, DaleCurtis wrote: > On 2012/10/18 01:30:39, scherkus wrote: > > silly optimization question... (I think chris asked this in a different way) > > > > for cases where we don't need to accumulate and the scale is 1 can't we get by > > w/ a single memcpy? > > > > we can leave it as a follow up TODO > > Hmm, we don't need to accumulate when the output layout is a remapping of the > input layout. Are there any other common cases? > > For that case, I can set a bool during construction if there are no unaccounted > channels to handle, then simply memcpy. Not sure. I was thinking of a situation (for whatever silly reason) where a ChannelMixer was instantiated for stereo -> stereo, in which case it seems like that'd be a memcpy() instead of FMAC()... but TBH I'm not sure how often this would come up. https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer_u... File media/base/channel_mixer_unittest.cc (right): https://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer_u... media/base/channel_mixer_unittest.cc:27: mixer.Rematrix(input_bus.get(), output_bus.get()); On 2012/10/18 01:56:44, DaleCurtis wrote: > On 2012/10/18 01:30:39, scherkus wrote: > > what exactly are we testing? > > Just that construction of all layouts is possible (DCHECKS will explode > otherwise) at the moment. I haven't thought of a good automated test for this > yet, other than maybe that some input exists in the output? Ideas welcome. How about naming this "AllLayoutCombinations" instead of "Construction"? The only idea I have is 3 canned sanity checking tests that cover the three different mixing cases chris pointed out (mono -> stereo mix using scale=1, stereo -> mono using scale=1/2, surround mix using scale=1/sqrt(2)). They don't have to be fancy.
http://codereview.chromium.org/11150034/diff/9002/media/base/channel_layout.cc File media/base/channel_layout.cc (right): http://codereview.chromium.org/11150034/diff/9002/media/base/channel_layout.c... media/base/channel_layout.cc:89: { 0 , 1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 , -1 }, On 2012/10/18 02:24:03, scherkus wrote: > On 2012/10/18 01:56:44, DaleCurtis wrote: > > On 2012/10/18 01:30:39, scherkus wrote: > > > is this a bug fix that didn't happen when DOWNMIX was added? > > > > > > can we document the difference between FL/FR and StL/StR on the Channels > enum? > > > > I'm not sure what those channels actually mean and no one is using them. Do > you > > have some historical context on this? Internally in FFmpeg's rematrixer they > > just set output = STEREO when STEREO_DOWNMIX is encountered. > > > > Barring any other information, I'll probably just delete those last two. > > No clue. You'll have to dig through commit logs + code reviews. Removed. History shows they were just added because FFmpeg has them. I couldn't find any UMA stats using CHANNELS_MAX either, so looks okay to delete. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:40: if (channel_count > 1) { On 2012/10/18 01:30:39, scherkus wrote: > is it worth asserting that channel_count == 1 iff CHANNEL_LAYOUT_MONO? Can't hurt, done. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:74: On 2012/10/17 19:46:50, Chris Rogers wrote: > extra blank line Done. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:212: // Zero initialize |output| so we're accumulating from zero. On 2012/10/18 02:24:03, scherkus wrote: > On 2012/10/18 01:56:44, DaleCurtis wrote: > > On 2012/10/18 01:30:39, scherkus wrote: > > > silly optimization question... (I think chris asked this in a different way) > > > > > > for cases where we don't need to accumulate and the scale is 1 can't we get > by > > > w/ a single memcpy? > > > > > > we can leave it as a follow up TODO > > > > Hmm, we don't need to accumulate when the output layout is a remapping of the > > input layout. Are there any other common cases? > > > > For that case, I can set a bool during construction if there are no > unaccounted > > channels to handle, then simply memcpy. > > Not sure. > > I was thinking of a situation (for whatever silly reason) where a ChannelMixer > was instantiated for stereo -> stereo, in which case it seems like that'd be a > memcpy() instead of FMAC()... but TBH I'm not sure how often this would come up. Added memcpy support for remaps, pretty trivial and should save a bunch in many cases. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.cc... media/base/channel_mixer.cc:214: output->frames() * sizeof(*output->channel(output_ch))); On 2012/10/17 20:06:47, DaleCurtis wrote: > On 2012/10/17 19:46:50, Chris Rogers wrote: > > This cries out for a AudioBus::ZeroChannel() method > > Good idea. Will add. Actually, output->Zero() outside of the loop is better. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.h File media/base/channel_mixer.h (right): http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer.h#... media/base/channel_mixer.h:27: virtual ~ChannelMixer(); On 2012/10/18 01:30:39, scherkus wrote: > drop virtual Done. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer_un... File media/base/channel_mixer_unittest.cc (right): http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer_un... media/base/channel_mixer_unittest.cc:19: ChannelMixer mixer(input_layout, output_layout); On 2012/10/18 01:30:39, scherkus wrote: > should consider using SCOPED_TRACE() to document the combination of input+output > in case of a failure > > either that or parameterized tests, but that seems like a lot of work for little > benefit Done. http://codereview.chromium.org/11150034/diff/9002/media/base/channel_mixer_un... media/base/channel_mixer_unittest.cc:27: mixer.Rematrix(input_bus.get(), output_bus.get()); On 2012/10/18 02:24:03, scherkus wrote: > On 2012/10/18 01:56:44, DaleCurtis wrote: > > On 2012/10/18 01:30:39, scherkus wrote: > > > what exactly are we testing? > > > > Just that construction of all layouts is possible (DCHECKS will explode > > otherwise) at the moment. I haven't thought of a good automated test for this > > yet, other than maybe that some input exists in the output? Ideas welcome. > > How about naming this "AllLayoutCombinations" instead of "Construction"? > > The only idea I have is 3 canned sanity checking tests that cover the three > different mixing cases chris pointed out (mono -> stereo mix using scale=1, > stereo -> mono using scale=1/2, surround mix using scale=1/sqrt(2)). They don't > have to be fancy. Done.
nice tests! http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.c... media/base/channel_mixer.cc:218: // We can only remap if each row contains a single scale of 1. "...or we have more than one input mapped to an output channel" http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.c... media/base/channel_mixer.cc:235: // If we're just remapping we can simply memcpy the correct input to output. memcpy() / copy http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.c... media/base/channel_mixer.cc:241: DCHECK_LT(fabs(scale - 1), std::numeric_limits<float>::epsilon()); what are the cases when scale is not exactly 1? http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.c... media/base/channel_mixer.cc:242: memcpy(output->channel(output_ch), input->channel(input_ch), is |output| zeroed in the first place or should we be zeroing channels that don't have mappings? http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.h File media/base/channel_mixer.h (right): http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.h... media/base/channel_mixer.h:61: bool just_remap_; "just" in variables names is a bit odd how about simply "remapping_"? http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer_u... File media/base/channel_mixer_unittest.cc (right): http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer_u... media/base/channel_mixer_unittest.cc:43: typedef std::tr1::tuple<ChannelLayout, ChannelLayout, float*, int, float> hrmmm... is it possible to use a struct instead of a tuple?
http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.c... media/base/channel_mixer.cc:218: // We can only remap if each row contains a single scale of 1. On 2012/10/18 05:39:23, scherkus wrote: > "...or we have more than one input mapped to an output channel" That's not correct? If more than one input maps to an output channel the memcpy of one will trample the other. http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.c... media/base/channel_mixer.cc:235: // If we're just remapping we can simply memcpy the correct input to output. On 2012/10/18 05:39:23, scherkus wrote: > memcpy() / copy Done. http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.c... media/base/channel_mixer.cc:241: DCHECK_LT(fabs(scale - 1), std::numeric_limits<float>::epsilon()); On 2012/10/18 05:39:23, scherkus wrote: > what are the cases when scale is not exactly 1? None, I'm just wary of float precision. Tests seem to pass fine without, removed. http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.c... media/base/channel_mixer.cc:242: memcpy(output->channel(output_ch), input->channel(input_ch), On 2012/10/18 05:39:23, scherkus wrote: > is |output| zeroed in the first place or should we be zeroing channels that > don't have mappings? Oh, good point, yes output needs to be zeroed. http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.h File media/base/channel_mixer.h (right): http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer.h... media/base/channel_mixer.h:61: bool just_remap_; On 2012/10/18 05:39:23, scherkus wrote: > "just" in variables names is a bit odd > > how about simply "remapping_"? Done. http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer_u... File media/base/channel_mixer_unittest.cc (right): http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer_u... media/base/channel_mixer_unittest.cc:43: typedef std::tr1::tuple<ChannelLayout, ChannelLayout, float*, int, float> On 2012/10/18 05:39:23, scherkus wrote: > hrmmm... is it possible to use a struct instead of a tuple? Yeah, it's about 30 lines of boiler plate though. Done.
http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer_u... File media/base/channel_mixer_unittest.cc (right): http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer_u... media/base/channel_mixer_unittest.cc:43: typedef std::tr1::tuple<ChannelLayout, ChannelLayout, float*, int, float> On 2012/10/18 06:30:38, DaleCurtis wrote: > On 2012/10/18 05:39:23, scherkus wrote: > > hrmmm... is it possible to use a struct instead of a tuple? > > Yeah, it's about 30 lines of boiler plate though. Done. Argh, windows doesn't like this: http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/71716/... Will experiment tomorrow, or just roll back to using a tuple.
On 2012/10/18 06:46:05, DaleCurtis wrote: > http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer_u... > File media/base/channel_mixer_unittest.cc (right): > > http://codereview.chromium.org/11150034/diff/13004/media/base/channel_mixer_u... > media/base/channel_mixer_unittest.cc:43: typedef std::tr1::tuple<ChannelLayout, > ChannelLayout, float*, int, float> > On 2012/10/18 06:30:38, DaleCurtis wrote: > > On 2012/10/18 05:39:23, scherkus wrote: > > > hrmmm... is it possible to use a struct instead of a tuple? > > > > Yeah, it's about 30 lines of boiler plate though. Done. > > Argh, windows doesn't like this: > http://build.chromium.org/p/tryserver.chromium/builders/win_rel/builds/71716/... > > Will experiment tomorrow, or just roll back to using a tuple. OK. If it's too much of a hassle just use tr1::tuple
lgtm w/ nits http://codereview.chromium.org/11150034/diff/15011/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/15011/media/base/channel_mixer.c... media/base/channel_mixer.cc:217: // We can only remap if each row contains a single scale of 1. whoops I meant to negate my comment in other words, explaining the "|| ++input_mappings > 1" part because it's a bit crafty http://codereview.chromium.org/11150034/diff/15011/media/base/channel_mixer_u... File media/base/channel_mixer_unittest.cc (right): http://codereview.chromium.org/11150034/diff/15011/media/base/channel_mixer_u... media/base/channel_mixer_unittest.cc:44: ChannelMixerTestData(ChannelLayout input_layout, ChannelLayout output_layout, perhaps you have to use regular struct initializers instead of a ctor?
http://codereview.chromium.org/11150034/diff/15011/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/15011/media/base/channel_mixer.c... media/base/channel_mixer.cc:217: // We can only remap if each row contains a single scale of 1. On 2012/10/18 16:50:20, scherkus wrote: > whoops I meant to negate my comment > > in other words, explaining the "|| ++input_mappings > 1" part because it's a bit > crafty Done. http://codereview.chromium.org/11150034/diff/15011/media/base/channel_mixer_u... File media/base/channel_mixer_unittest.cc (right): http://codereview.chromium.org/11150034/diff/15011/media/base/channel_mixer_u... media/base/channel_mixer_unittest.cc:44: ChannelMixerTestData(ChannelLayout input_layout, ChannelLayout output_layout, On 2012/10/18 16:50:20, scherkus wrote: > perhaps you have to use regular struct initializers instead of a ctor? Didn't like the const setting on variables below, I guess it's doing some sort of copy internally?
crogers: Any more comments?
http://codereview.chromium.org/11150034/diff/19008/media/base/channel_mixer.h File media/base/channel_mixer.h (right): http://codereview.chromium.org/11150034/diff/19008/media/base/channel_mixer.h... media/base/channel_mixer.h:30: void Rematrix(const AudioBus* input, AudioBus* output); Henrik brought this up on another CL, but is Rematrix() the common term for this? I've heard it used in this way before, but I don't see much support via Google. WDYT crogers?
http://codereview.chromium.org/11150034/diff/19008/media/base/channel_mixer.h File media/base/channel_mixer.h (right): http://codereview.chromium.org/11150034/diff/19008/media/base/channel_mixer.h... media/base/channel_mixer.h:30: void Rematrix(const AudioBus* input, AudioBus* output); On 2012/10/18 21:36:54, DaleCurtis wrote: > Henrik brought this up on another CL, but is Rematrix() the common term for > this? I've heard it used in this way before, but I don't see much support via > Google. WDYT crogers? Well... the dictionary doesn't list "matrix" as a verb so I suppose it's a little odd. Perhaps the right way to think of this is that this class is really a ChannelMatrix (not a mixer?!) and that if we're to use mathematical terms you would have a method called Transform() or Apply().
http://codereview.chromium.org/11150034/diff/19008/media/base/channel_mixer.h File media/base/channel_mixer.h (right): http://codereview.chromium.org/11150034/diff/19008/media/base/channel_mixer.h... media/base/channel_mixer.h:30: void Rematrix(const AudioBus* input, AudioBus* output); On 2012/10/18 21:42:05, scherkus wrote: > On 2012/10/18 21:36:54, DaleCurtis wrote: > > Henrik brought this up on another CL, but is Rematrix() the common term for > > this? I've heard it used in this way before, but I don't see much support via > > Google. WDYT crogers? > > Well... the dictionary doesn't list "matrix" as a verb so I suppose it's a > little odd. > > Perhaps the right way to think of this is that this class is really a > ChannelMatrix (not a mixer?!) and that if we're to use mathematical terms you > would have a method called Transform() or Apply(). I like Transform(). We'll see what Chris thinks.
http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:21: static const float kDefaultScale = static_cast<float>(M_SQRT1_2); A better name might be kEqualPowerScale http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:26: CHECK_NE(layout, CHANNEL_LAYOUT_MAX); Can't we handle the CHANNEL_LAYOUT_UNSUPPORTED case? We could simply copy channels in the order they come in, and either drop extra channels or zero out remaining extra channels. We should at least add a TODO about this case. http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:60: maybe we should at least DCHECK that input!=output. Although technically this class will handle this case, for efficiency don't we want to avoid using this class in this case. I'm not sure, WDYT? http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:174: MixWithoutAccounting(BACK_CENTER, LEFT, kDefaultScale * kDefaultScale); why kDefaultScale*kDefaultScale ? -- I would just use kDefaultScale. You can still leave the TODO comment http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:299: matrix_[output_ch_index][input_ch_index] += scale; I'm confused why we're accumulating |scale| here. Shouldn't we DCHECK that the current location in the matrix is 0 and simply set it here. Maybe I'm missing something. http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.h File media/base/channel_mixer.h (right): http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.h... media/base/channel_mixer.h:22: // channel according to the matrix and summing it into a single output channel. This last sentence is a little confusing. I think what you mean is something like: The transform renders all of the output channels, with each output channel rendered according to a weighted sum of the relevant input channels as defined in the matrix.
http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:21: static const float kDefaultScale = static_cast<float>(M_SQRT1_2); On 2012/10/19 20:48:17, Chris Rogers wrote: > A better name might be kEqualPowerScale Done. http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:26: CHECK_NE(layout, CHANNEL_LAYOUT_MAX); On 2012/10/19 20:48:17, Chris Rogers wrote: > Can't we handle the CHANNEL_LAYOUT_UNSUPPORTED case? We could simply copy > channels in the order they come in, and either drop extra channels or zero out > remaining extra channels. > > We should at least add a TODO about this case. No, this means the AudioParameters are invalid and channel count == 0 since you're required to provide a channel layout to construct that object. Supporting arbitrary layouts is not something I want to get into here. http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:60: On 2012/10/19 20:48:17, Chris Rogers wrote: > maybe we should at least DCHECK that input!=output. Although technically this > class will handle this case, for efficiency don't we want to avoid using this > class in this case. I'm not sure, WDYT? Since these layouts might be chosen dynamically, i'd rather leave it up to the caller. Per Chromium style you're not supposed to DCHECK things you handle anyways. http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:174: MixWithoutAccounting(BACK_CENTER, LEFT, kDefaultScale * kDefaultScale); On 2012/10/19 20:48:17, Chris Rogers wrote: > why kDefaultScale*kDefaultScale ? -- I would just use kDefaultScale. > > You can still leave the TODO comment ffmpeg used that value, my assumption was that you don't want to scale the rear channels equally into the fronts since they are further away. Done. http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:299: matrix_[output_ch_index][input_ch_index] += scale; On 2012/10/19 20:48:17, Chris Rogers wrote: > I'm confused why we're accumulating |scale| here. Shouldn't we DCHECK that the > current location in the matrix is 0 and simply set it here. Maybe I'm missing > something. Ah, yes, you're correct. This is a carryover from earlier algorithms I was experimenting with. http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.h File media/base/channel_mixer.h (right): http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.h... media/base/channel_mixer.h:22: // channel according to the matrix and summing it into a single output channel. On 2012/10/19 20:48:17, Chris Rogers wrote: > This last sentence is a little confusing. I think what you mean is something > like: > > The transform renders all of the output channels, with each output channel > rendered according to a weighted sum of the relevant input channels as defined > in the matrix. Done.
http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:26: CHECK_NE(layout, CHANNEL_LAYOUT_MAX); On 2012/10/19 22:33:20, DaleCurtis wrote: > On 2012/10/19 20:48:17, Chris Rogers wrote: > > Can't we handle the CHANNEL_LAYOUT_UNSUPPORTED case? We could simply copy > > channels in the order they come in, and either drop extra channels or zero out > > remaining extra channels. > > > > We should at least add a TODO about this case. > > No, this means the AudioParameters are invalid and channel count == 0 since > you're required to provide a channel layout to construct that object. > Supporting arbitrary layouts is not something I want to get into here. We might require that *now*, but this needs to be made more general. We won't always have a known channel layout. For the output-side, it's easily possible on OSX to be connected to a Firewire device outputting say 8, 12, or whatever channels which have no speaker layout. They're just raw channels. They could be connected to a multi-channel recording device, or connected to a custom speaker array controlled by WebAudio. For example, at CCRMA they have several studios with large speaker arrays on the inside of a geodesic dome. These are all custom speaker arrangements, so for our purposes would be CHANNEL_LAYOUT_UNSUPPORTED, or maybe a better name would be CHANNEL_LAYOUT_CUSTOM. And believe me, these guys and others are clamoring to have this kind of support in Chrome ASAP. Similarly on the client/input-side, WebAudio might be configured to output 4,5,6,7,13, 17 channels, or whatever which have none of our known layouts, which can then be custom matrix mixed within WebAudio. So, in these types of cases the best we can do is simply copy channels from input to output until we either run out of channels, or there are some remaining which we zero out.
On 2012/10/19 22:52:26, Chris Rogers wrote: > http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.cc > File media/base/channel_mixer.cc (right): > > http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... > media/base/channel_mixer.cc:26: CHECK_NE(layout, CHANNEL_LAYOUT_MAX); > On 2012/10/19 22:33:20, DaleCurtis wrote: > > On 2012/10/19 20:48:17, Chris Rogers wrote: > > > Can't we handle the CHANNEL_LAYOUT_UNSUPPORTED case? We could simply copy > > > channels in the order they come in, and either drop extra channels or zero > out > > > remaining extra channels. > > > > > > We should at least add a TODO about this case. > > > > No, this means the AudioParameters are invalid and channel count == 0 since > > you're required to provide a channel layout to construct that object. > > Supporting arbitrary layouts is not something I want to get into here. > > We might require that *now*, but this needs to be made more general. > > We won't always have a known channel layout. For the output-side, it's easily > possible on OSX to be connected to a Firewire device outputting say 8, 12, or > whatever channels which have no speaker layout. They're just raw channels. > They could be connected to a multi-channel recording device, or connected to a > custom speaker array controlled by WebAudio. For example, at CCRMA they have > several studios with large speaker arrays on the inside of a geodesic dome. > These are all custom speaker arrangements, so for our purposes would be > CHANNEL_LAYOUT_UNSUPPORTED, or maybe a better name would be > CHANNEL_LAYOUT_CUSTOM. And believe me, these guys and others are clamoring to > have this kind of support in Chrome ASAP. > > Similarly on the client/input-side, WebAudio might be configured to output > 4,5,6,7,13, 17 channels, or whatever which have none of our known layouts, which > can then be custom matrix mixed within WebAudio. > > So, in these types of cases the best we can do is simply copy channels from > input to output until we either run out of channels, or there are some remaining > which we zero out. Or using pictures: http://www.zzounds.com/item--PRSSTUDIOLIVE ...which is a firewire device sporting 32 input channels and 18 output channels :)
http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.cc File media/base/channel_mixer.cc (right): http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... media/base/channel_mixer.cc:26: CHECK_NE(layout, CHANNEL_LAYOUT_MAX); On 2012/10/19 22:52:26, Chris Rogers wrote: > On 2012/10/19 22:33:20, DaleCurtis wrote: > > On 2012/10/19 20:48:17, Chris Rogers wrote: > > > Can't we handle the CHANNEL_LAYOUT_UNSUPPORTED case? We could simply copy > > > channels in the order they come in, and either drop extra channels or zero > out > > > remaining extra channels. > > > > > > We should at least add a TODO about this case. > > > > No, this means the AudioParameters are invalid and channel count == 0 since > > you're required to provide a channel layout to construct that object. > > Supporting arbitrary layouts is not something I want to get into here. > > We might require that *now*, but this needs to be made more general. > > We won't always have a known channel layout. For the output-side, it's easily > possible on OSX to be connected to a Firewire device outputting say 8, 12, or > whatever channels which have no speaker layout. They're just raw channels. > They could be connected to a multi-channel recording device, or connected to a > custom speaker array controlled by WebAudio. For example, at CCRMA they have > several studios with large speaker arrays on the inside of a geodesic dome. > These are all custom speaker arrangements, so for our purposes would be > CHANNEL_LAYOUT_UNSUPPORTED, or maybe a better name would be > CHANNEL_LAYOUT_CUSTOM. And believe me, these guys and others are clamoring to > have this kind of support in Chrome ASAP. > > Similarly on the client/input-side, WebAudio might be configured to output > 4,5,6,7,13, 17 channels, or whatever which have none of our known layouts, which > can then be custom matrix mixed within WebAudio. > > So, in these types of cases the best we can do is simply copy channels from > input to output until we either run out of channels, or there are some remaining > which we zero out. Sure we definitely want to handle this some day, but until then we do not and things will not work if someone tries to (even before this CL) so we should just fail if someone attempts to try this. Currently this connection will fail in the AudioOutputController stage because the AudioParameters will be invalid. I'll can a TODO, but that's not going to magically fix the many other places that make this impossible right now :)
On 2012/10/19 23:14:31, DaleCurtis wrote: > http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.cc > File media/base/channel_mixer.cc (right): > > http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... > media/base/channel_mixer.cc:26: CHECK_NE(layout, CHANNEL_LAYOUT_MAX); > On 2012/10/19 22:52:26, Chris Rogers wrote: > > On 2012/10/19 22:33:20, DaleCurtis wrote: > > > On 2012/10/19 20:48:17, Chris Rogers wrote: > > > > Can't we handle the CHANNEL_LAYOUT_UNSUPPORTED case? We could simply copy > > > > channels in the order they come in, and either drop extra channels or zero > > out > > > > remaining extra channels. > > > > > > > > We should at least add a TODO about this case. > > > > > > No, this means the AudioParameters are invalid and channel count == 0 since > > > you're required to provide a channel layout to construct that object. > > > Supporting arbitrary layouts is not something I want to get into here. > > > > We might require that *now*, but this needs to be made more general. > > > > We won't always have a known channel layout. For the output-side, it's easily > > possible on OSX to be connected to a Firewire device outputting say 8, 12, or > > whatever channels which have no speaker layout. They're just raw channels. > > They could be connected to a multi-channel recording device, or connected to a > > custom speaker array controlled by WebAudio. For example, at CCRMA they have > > several studios with large speaker arrays on the inside of a geodesic dome. > > These are all custom speaker arrangements, so for our purposes would be > > CHANNEL_LAYOUT_UNSUPPORTED, or maybe a better name would be > > CHANNEL_LAYOUT_CUSTOM. And believe me, these guys and others are clamoring to > > have this kind of support in Chrome ASAP. > > > > Similarly on the client/input-side, WebAudio might be configured to output > > 4,5,6,7,13, 17 channels, or whatever which have none of our known layouts, > which > > can then be custom matrix mixed within WebAudio. > > > > So, in these types of cases the best we can do is simply copy channels from > > input to output until we either run out of channels, or there are some > remaining > > which we zero out. > > Sure we definitely want to handle this some day, but until then we do not and > things will not work if someone tries to (even before this CL) so we should just > fail if someone attempts to try this. Currently this connection will fail in > the AudioOutputController stage because the AudioParameters will be invalid. > > I'll can a TODO, but that's not going to magically fix the many other places > that make this impossible right now :) Thanks, a TODO is fine. But "someday" is probably sooner than you think. Handling multi-channel, including layouts was a major topic of conversation in last Wednesday's W3C audio telecon, and soon I will need to implement the .maxNumberOfChannels and .numberOfChannels attributes in AudioDestinationNode
On 2012/10/19 23:21:05, Chris Rogers wrote: > On 2012/10/19 23:14:31, DaleCurtis wrote: > > http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.cc > > File media/base/channel_mixer.cc (right): > > > > > http://codereview.chromium.org/11150034/diff/18011/media/base/channel_mixer.c... > > media/base/channel_mixer.cc:26: CHECK_NE(layout, CHANNEL_LAYOUT_MAX); > > On 2012/10/19 22:52:26, Chris Rogers wrote: > > > On 2012/10/19 22:33:20, DaleCurtis wrote: > > > > On 2012/10/19 20:48:17, Chris Rogers wrote: > > > > > Can't we handle the CHANNEL_LAYOUT_UNSUPPORTED case? We could simply > copy > > > > > channels in the order they come in, and either drop extra channels or > zero > > > out > > > > > remaining extra channels. > > > > > > > > > > We should at least add a TODO about this case. > > > > > > > > No, this means the AudioParameters are invalid and channel count == 0 > since > > > > you're required to provide a channel layout to construct that object. > > > > Supporting arbitrary layouts is not something I want to get into here. > > > > > > We might require that *now*, but this needs to be made more general. > > > > > > We won't always have a known channel layout. For the output-side, it's > easily > > > possible on OSX to be connected to a Firewire device outputting say 8, 12, > or > > > whatever channels which have no speaker layout. They're just raw channels. > > > They could be connected to a multi-channel recording device, or connected to > a > > > custom speaker array controlled by WebAudio. For example, at CCRMA they > have > > > several studios with large speaker arrays on the inside of a geodesic dome. > > > These are all custom speaker arrangements, so for our purposes would be > > > CHANNEL_LAYOUT_UNSUPPORTED, or maybe a better name would be > > > CHANNEL_LAYOUT_CUSTOM. And believe me, these guys and others are clamoring > to > > > have this kind of support in Chrome ASAP. > > > > > > Similarly on the client/input-side, WebAudio might be configured to output > > > 4,5,6,7,13, 17 channels, or whatever which have none of our known layouts, > > which > > > can then be custom matrix mixed within WebAudio. > > > > > > So, in these types of cases the best we can do is simply copy channels from > > > input to output until we either run out of channels, or there are some > > remaining > > > which we zero out. > > > > Sure we definitely want to handle this some day, but until then we do not and > > things will not work if someone tries to (even before this CL) so we should > just > > fail if someone attempts to try this. Currently this connection will fail in > > the AudioOutputController stage because the AudioParameters will be invalid. > > > > I'll can a TODO, but that's not going to magically fix the many other places > > that make this impossible right now :) > > Thanks, a TODO is fine. But "someday" is probably sooner than you think. > Handling multi-channel, including layouts was a major topic of conversation in > last Wednesday's W3C audio telecon, and soon I will need to implement the > .maxNumberOfChannels and .numberOfChannels attributes in AudioDestinationNode Done. LGTY?
Thanks Dale, nice work! lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11150034/26008
Change committed as 163158 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
