Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10823100:
Adds support for multi-channel output audio for the low-latency path in Windows. (Closed)
|
Created:
8 years, 4 months ago by henrika (OOO until Aug 14) Modified:
8 years, 4 months ago CC:
chromium-reviews, feature-media-reviews_chromium.org, DaleCurtis Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdds support for multi-channel output audio for the low-latency path in Windows.
What's new?
========
Output stream is always opened up using the native number of channels and channel configuration.
The user can set a lower number of channels during construction and for this case up-mixing (typically 2 -> 7.1) takes place since the audio source does not deliver sufficient number of channels for this case.
It is still possible to avoid up-mixing simply by creating up the stream using the native channel count (which can be queried before construction).
TODO
====
- Clean up ChannelUpMix().
- Add support for 8-bit and 24-bit audio?
- Add support for 2 -> 1 down-mixing?
- More mixing combinations?
- Add accessors for ChannelCount/Layout.
- Improve usage of channel_factor to detect mixing mode.
BUG=138762
TEST=media_unittests --gtest_filter=WASAPIAudioOutput*
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=150307
Patch Set 1 #
Total comments: 52
Patch Set 2 : First review round #
Total comments: 14
Patch Set 3 : More changes proposed by Andrew #Patch Set 4 : Added more mixing cases and improved the tests #
Total comments: 21
Patch Set 5 : Added test files and 2 -> 5.1 up-mixing #Patch Set 6 : Fixed nit #
Total comments: 2
Patch Set 7 : Fixed bug in GetMixFormat #
Total comments: 2
Patch Set 8 : Added TODO(henrika) #Patch Set 9 : Rebased #
Messages
Total messages: 24 (0 generated)
Initial (rough) version intended as base for discussions on how to proceed. Will add client usage next and improve the details.
Looks like a great start!
For the record. The new design does *not* cover the following cases (yet): 1) Client provides mono and the native channel count is stereo. I assume we want this and I can add it. 2) Client provides stereo and the native channel count is mono (would require a down-mix 2->1). I don't think I have ever found an output device which only supports mono hence it does not feel like a realistic case. 3) Client provides multi-channel (e.g. 5.1 or 7.1) but native channel count is stereo. Would require down-mix (folding) and I would like to skip this case for now as well. On Tue, Jul 31, 2012 at 7:22 PM, <crogers@google.com> wrote: > Looks like a great start! > > https://chromiumcodereview.**appspot.com/10823100/<https://chromiumcodereview... >
On 2012/07/31 21:03:17, henrika wrote: > For the record. The new design does *not* cover the following cases (yet): > > 1) Client provides mono and the native channel count is stereo. I assume we > want this and I can add it. Yes > > 2) Client provides stereo and the native channel count is mono (would > require a down-mix 2->1). I don't think I have ever found an output device > which only supports mono hence it does not feel like a realistic case. Sounds good > > 3) Client provides multi-channel (e.g. 5.1 or 7.1) but native channel count > is stereo. Would require down-mix (folding) and I would like to skip this > case for now as well. Even if we skip this case in this CL. I think we'll very soon need to add it in to support 5.1 <audio> for Dale's renderer-side mixing. I'm not sure if we currently support 7.1 so maybe that's not necessary, but Dale can chime in here. I'm 100% in favor of not over-engineering the channel mixing, but I think we need to handle the common mono, stereo, and 5.1 cases for current <audio> support. Also, in the general case where the audio hardware outputs an arbitrary number of channels (say 13) with no particular channel layout (just raw channels), then the "up-mixing" strategy would simply be to fill in the number of client channels available in the order they arrive, then zero out the remaining unused channels. Having thought about this a little bit, Dale has mentioned that he would like to change the data sent -> browser in shared memory to float32. One advantage of this, is that we can handle all of this channel up/down mixing in cross-platform browser-side code instead of re-writing these blitters for each OS. For example, in your current patch you have a function called "ChannelUpMix()". Ideally, we would share this as a utility function. I'm not saying we should hold up landing this CL since we need to make incremental progress. But, I think we should do this sooner rather than later because we also urgently need this for the low-latency OS X path. > > > On Tue, Jul 31, 2012 at 7:22 PM, <mailto:crogers@google.com> wrote: > > > Looks like a great start! > > > > > https://chromiumcodereview.**appspot.com/10823100/%3Chttps://chromiumcoderevi...> > >
Excellent start indeed. Some suggestions below. https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... File media/audio/win/audio_low_latency_output_win.cc (right): https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:24: bool ChannelUpMix(void* input, I guess this will accept in/out layout parameters as well? https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:34: int16* in16 = static_cast<int16*>(input); reinterpret_cast https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:37: if (in_channels == 2) { and here check for in_layout == STEREO? https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:41: for (int i = 0; i < number_of_input_stereo_samples; i++) { ++i https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:41: for (int i = 0; i < number_of_input_stereo_samples; i++) { as discussed offline, when it comes time to do this in place (to reduce number of memory allocations), we can process the samples in reverse order when sizeof(out) > sizeof(in) (upmixing). https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:42: // Copy Front Left and Front Right channels as is. I know this is just a start, so if I may make a suggestion, it would be nice and make the code simpler and easy to read if we have structs for each layout. Something like: (structs like this might possibly need to be inside a #pragma pack(1)) struct LayoutStereo_16bit { int16 left; int16 right; }; struct Layout7_1_16bit { int16 front_left; int16 front_right; ...(remaining channels here)... }; bool ChannelUpMix(...) { ... if (in_layout == STEREO && out_layout == 7_1) { LayoutStereo_16bit* in = reinterpret_cast<LayoutStereo_16bit*>(input); Layout7_1_16bit* out = reinterpret_cast<Layout7_1_16bit*>(output); // zero out all frames first. memset(out, 0, number_of_input_stereo_samples * sizeof(out[0])); for (int i = 0; i < number_of_input_stereo_samples; ++i) { out->front_left = in->left; out->front_right = in->right; ++in; ++out; } } ... } https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:128: format->cbSize = 22; 22? use sizeof? could also move this to the top of setting the struct since this is the struct's header. https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:370: eConsole, reinterpret_cast<WAVEFORMATEX**>(&format_ex)); the reinterpret_cast shouldn't be needed. operator& returns T**. https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:393: eConsole, reinterpret_cast<WAVEFORMATEX**>(&format_ex)); same here https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:407: std::string speaker_config("Undefined"); move this into an #ifndef NDEBUG? also, you don't need std::string. const char* is enough. https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:408: if (format_ex->dwChannelMask == KSAUDIO_SPEAKER_MONO) switch()? https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:448: DVLOG(1) << "Unsupported channel layout: " << config; add break; https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:697: (packet_size_bytes_ - num_filled_bytes)); indent? (looks off by 1) https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:807: reinterpret_cast<WAVEFORMATEX**>(&audio_engine_mix_format_)); no cast should be necessary https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:826: share_mode(), reinterpret_cast<WAVEFORMATEX*>(&format_), don't cast format_. instead use operatorT*() and remove & share_mode(), format_, https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:827: reinterpret_cast<WAVEFORMATEX**>(&closest_match)); cast not needed https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:957: reinterpret_cast<WAVEFORMATEX*>(&format_), use operator T*() https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:982: reinterpret_cast<WAVEFORMATEX*>(&format_), operator https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... File media/audio/win/audio_low_latency_output_win.h (right): https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.h:280: static uint32 ChannelConfig(); Maybe we could define our own type for channel config to avoid mixing it up with other ints. e.g. typedef uint32 ChannelConfig; Down the line, we could then upgrade that typedef to a class that takes care of all the bit manipulation code as it starts to spread. https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.h:309: AudioParameters client_audio_parameters_; are there any other audio parameters that this class needs to worry about? (just wondering about the 'client_' qualifier)
few quick nits -- I'll take a deeper look tomorrow https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... File media/audio/win/audio_low_latency_output_win.cc (right): https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:75: client_audio_parameters_(params), note: this isn't used anywhere https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:84: endpoint_channel_count_(HardwareChannelCount()), // <=> default device can't these be derived from format_? even though I commented about the accessors ... what if we had: int endpoint_channel_count() { return format_.Format->nChannels; } https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:85: endpoint_channel_config_(ChannelConfig()), // <=> default device this is only used to set format_.dwChannelMask, which makes me wonder whether we need a new member variable https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.cc:111: channel_factor_ = endpoint_channel_count() / params.channels(); hmmm if you end up keeping client_audio_parameters_ around this can become a derived member: int channel_factor() { return endpoint_channel_count() / client_audio_parameters_.channels(); } if client_audio_parameters_ goes away, then you can keep channel_factor_ :) https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... File media/audio/win/audio_low_latency_output_win.h (right): https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.h:252: static HRESULT GetMixFormat(ERole device_role, WAVEFORMATEX** device_format); any reason static private methods need to be here? if they don't need to access private/protected members of this class then they should all be moved into the .cc sames goes w/ ChannelConfig() and ChannelConfigToChromeChannelLayout() https://chromiumcodereview.appspot.com/10823100/diff/1/media/audio/win/audio_... media/audio/win/audio_low_latency_output_win.h:293: int endpoint_channel_count() const { return endpoint_channel_count_; } huh? these are all private methods -- why the accessors instead of accessing the variables directly? also I think we can compute these values as opposed to storing additional data -- see my notes in the .cc
On 2012/07/31 21:21:04, Chris Rogers wrote: > Having thought about this a little bit, Dale has mentioned that he would like to > change the data sent -> browser in shared memory to float32. One advantage of > this, is that we can handle all of this channel up/down mixing in cross-platform > browser-side code instead of re-writing these blitters for each OS. For > example, in your current patch you have a function called "ChannelUpMix()". > Ideally, we would share this as a utility function. I'm not saying we should > hold up landing this CL since we need to make incremental progress. But, I > think we should do this sooner rather than later because we also urgently need > this for the low-latency OS X path. > If Dale pings me when he gets to this topic, I am sure we can come up with a solution together. Thanks for pointing out that we might need this part.
Maybe instead of keeping the buffer in a vector of float*, we could have a special buffer management class that allows the same functionality but over single memory allocation of an already interleaved buffer? For the ideal case, this could be mapped on top of the shared memory section assuming we'll be using that going forward and we could reduce the number of memcpy's and allocs. On Wed, Aug 1, 2012 at 2:18 PM, <henrika@chromium.org> wrote: > On 2012/07/31 21:21:04, Chris Rogers wrote: > > Having thought about this a little bit, Dale has mentioned that he would >> like >> > to > >> change the data sent -> browser in shared memory to float32. One >> advantage of >> this, is that we can handle all of this channel up/down mixing in >> > cross-platform > >> browser-side code instead of re-writing these blitters for each OS. For >> example, in your current patch you have a function called >> "ChannelUpMix()". >> Ideally, we would share this as a utility function. I'm not saying we >> should >> hold up landing this CL since we need to make incremental progress. But, >> I >> think we should do this sooner rather than later because we also urgently >> need >> this for the low-latency OS X path. >> > > > If Dale pings me when he gets to this topic, I am sure we can come up with > a > solution together. Thanks for pointing out that we might need this part. > > http://codereview.chromium.**org/10823100/<http://codereview.chromium.org/108... >
Thanks Andrew and Tommi. tommi: I think that you and I need to discuss reinterpret_cast<WAVEFORMATEX*>(&format_). http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:34: int16* in16 = static_cast<int16*>(input); On 2012/07/31 21:39:30, tommi wrote: > reinterpret_cast Done. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:37: if (in_channels == 2) { see comment below http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:41: for (int i = 0; i < number_of_input_stereo_samples; i++) { On 2012/07/31 21:39:30, tommi wrote: > ++i Done. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:41: for (int i = 0; i < number_of_input_stereo_samples; i++) { Will add a TODO() on that one for now. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:42: // Copy Front Left and Front Right channels as is. I actually did something like this initially but the thing is that we don't know the exact channel config for 7.1 and since we will zero out all the surround channels (and not use most parts of Layout7_1_16bit) I decided to skip it. Will think again. Thanks for the code snippets. OK. I used most parts of your proposal but was actually not able to come up with a good solution to the channel-layout input since we are mixing "Chrome-style from the renderer" for the input with "Microsoft internal" for the output. Will add TODO. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:75: client_audio_parameters_(params), It is used in call to ChannelUpMix() to feed in the input channel count. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:84: endpoint_channel_count_(HardwareChannelCount()), // <=> default device I was able to remove this member by doing almost as you propose. Check new version. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:85: endpoint_channel_config_(ChannelConfig()), // <=> default device Removed. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:111: channel_factor_ = endpoint_channel_count() / params.channels(); Added client_channel_count_ instead and turned channel_factor() into simple getter. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:128: format->cbSize = 22; It is actually "MSDN-standard" to hard code 22 here. We can discuss off-line. But yes, it is possible to derive as well. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:370: eConsole, reinterpret_cast<WAVEFORMATEX**>(&format_ex)); Tell that to the compiler ;-) error C2664: 'media::GetMixFormat' : cannot convert parameter 2 from 'WAVEFORMATPCMEX **' to 'WAVEFORMATEX **' Note that WAVEFORMATEX != WAVEFORMATPCMEX http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:393: eConsole, reinterpret_cast<WAVEFORMATEX**>(&format_ex)); see above http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:407: std::string speaker_config("Undefined"); On 2012/07/31 21:39:30, tommi wrote: > move this into an #ifndef NDEBUG? > also, you don't need std::string. const char* is enough. Done. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:408: if (format_ex->dwChannelMask == KSAUDIO_SPEAKER_MONO) On 2012/07/31 21:39:30, tommi wrote: > switch()? Done. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:448: DVLOG(1) << "Unsupported channel layout: " << config; On 2012/07/31 21:39:30, tommi wrote: > add break; Done. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:697: (packet_size_bytes_ - num_filled_bytes)); On 2012/07/31 21:39:30, tommi wrote: > indent? (looks off by 1) Done. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:807: reinterpret_cast<WAVEFORMATEX**>(&audio_engine_mix_format_)); see previous comment. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:826: share_mode(), reinterpret_cast<WAVEFORMATEX*>(&format_), Same comment as before. IsFormatSupported takes WAVEFORMATEX*. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:827: reinterpret_cast<WAVEFORMATEX**>(&closest_match)); ditto http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:957: reinterpret_cast<WAVEFORMATEX*>(&format_), ditto http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.cc:982: reinterpret_cast<WAVEFORMATEX*>(&format_), ditto http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.h:252: static HRESULT GetMixFormat(ERole device_role, WAVEFORMATEX** device_format); Great input, thanks. I actually did use private members here before but no longer. Will fix. Question: comments; do they go above the function definition or "inside" (first line below def.)? http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.h:280: static uint32 ChannelConfig(); Added typedef ChannelConfigMask in media. Got conflict if I used ChannelConfig (same as ChannelConfig()). http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.h:293: int endpoint_channel_count() const { return endpoint_channel_count_; } Got it. Will clean up. http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.h:309: AudioParameters client_audio_parameters_; I store this one to keep track of the parameters set by the user at construction. It helps me detect if there is any difference between the #input channels and the #native channels. Might be overkill to store the complete class.
few more nits http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10823100/diff/1/media/audio/win/audio_low_late... media/audio/win/audio_low_latency_output_win.h:252: static HRESULT GetMixFormat(ERole device_role, WAVEFORMATEX** device_format); On 2012/08/01 16:11:09, henrika wrote: > Great input, thanks. I actually did use private members here before but no > longer. Will fix. > > Question: comments; do they go above the function definition or "inside" (first > line below def.)? Above the function. http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:51: HRESULT GetMixFormat(ERole device_role, WAVEFORMATEX** device_format) { static http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:117: ChannelConfigMask ChannelConfig() { static http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:117: ChannelConfigMask ChannelConfig() { ChannelConfig() isn't a terribly good message name -- what about GetChannelConfig() ? http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:167: ChannelLayout ChannelConfigToChromeChannelLayout(ChannelConfigMask config) { nit: drop the Chrome part from function name -- having ChannelLayout (which is a type) is good enough ditto for comment http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:168: switch (config) { fix indent http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:202: DCHECK(input); the DCHECKs for input/output aren't needed http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:224: LOG(ERROR) << "Up-mixing is not supported."; document # of channels?
Fixed all comments from Andrew. http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:51: HRESULT GetMixFormat(ERole device_role, WAVEFORMATEX** device_format) { Done. However, is it really needed when I am in the media namespace? http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:117: ChannelConfigMask ChannelConfig() { On 2012/08/02 00:41:57, scherkus wrote: > ChannelConfig() isn't a terribly good message name -- what about > GetChannelConfig() ? Done. http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:117: ChannelConfigMask ChannelConfig() { On 2012/08/02 00:41:57, scherkus wrote: > static Done. http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:167: ChannelLayout ChannelConfigToChromeChannelLayout(ChannelConfigMask config) { Got it. Took it from FFmpeg ;-) Removed Mask from ChannelConfigMask typedef as well. Give cleaner code I think. BTW - why isn't ChannelLayout in media? http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:168: switch (config) { On 2012/08/02 00:41:57, scherkus wrote: > fix indent Done. http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:202: DCHECK(input); On 2012/08/02 00:41:57, scherkus wrote: > the DCHECKs for input/output aren't needed Done. http://codereview.chromium.org/10823100/diff/8002/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:224: LOG(ERROR) << "Up-mixing is not supported."; On 2012/08/02 00:41:57, scherkus wrote: > document # of channels? Done.
Added support for 1->2 and 1->7.1 channel up-mixing. Added new test for playing out mono files (uses new channel mixer). Fixed most TODO:s and cleaned up the code. Question: My plan was to land this CL this week before I go on vacation. I have not yet added access methods (and IPC) in audio_util.h/cc to allow the renderer to read the hardware channel count (and config). Does is seem like a reasonable compromise to wait with this support until after the current CL is landed? I assume that all clients will only uses this info as FYI anyhow.
http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:77: HRESULT hr = CoCreateInstance(__uuidof(MMDeviceEnumerator), remove extra space between = and CCI() http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:127: eConsole, reinterpret_cast<WAVEFORMATEX**>(&format_ex)); indent 2 more spaces http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:142: const char* speaker_config("Undefined"); () notation is a bit odd ... use = instead http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:153: case KSAUDIO_SPEAKER_5POINT1: remove extra spaces http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:156: case KSAUDIO_SPEAKER_7POINT1_SURROUND: remove extra spaces http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:210: static int ChannelUpMix(void* input, man this function is going to get massive over time -- any ideas on how to parameterize things? also won't this break if we do 8-bit or 24-bit audio? http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:913: share_mode_, reinterpret_cast<WAVEFORMATEX*>(&format_), indent 2 more spaces http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:1126: DWORD new_state) { nit: this should be aligned at the ( if it doesn't fit then drop device_id to this line as well http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.h:80: // then 2 -> 7.1 up-mixing will take place for each OnMoreData() callback. what about opening up 8 channels when HardwareChannelCount() is 2? I don't think we need to fix it now but I would mention what clients should expect (error? downmixing?) http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.h:166: class WASAPIAudioOutputStreamTest; AFAIK you don't need to fwd decl this for FRIEND_TEST
Hey Henrik, please go ahead with the cl once other reviewers are happy. I won't have time to look at it closely before the weekend. Great job! Sent from my phone. On Aug 2, 2012 7:14 PM, <scherkus@chromium.org> wrote: > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.cc<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.cc> > File media/audio/win/audio_low_**latency_output_win.cc (right): > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.cc#newcode77<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.cc#newcode77> > media/audio/win/audio_low_**latency_output_win.cc:77: HRESULT hr = > CoCreateInstance(__uuidof(**MMDeviceEnumerator), > remove extra space between = and CCI() > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.cc#newcode127<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.cc#newcode127> > media/audio/win/audio_low_**latency_output_win.cc:127: eConsole, > reinterpret_cast<WAVEFORMATEX****>(&format_ex)); > indent 2 more spaces > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.cc#newcode142<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.cc#newcode142> > media/audio/win/audio_low_**latency_output_win.cc:142: const char* > speaker_config("Undefined"); > () notation is a bit odd ... use = instead > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.cc#newcode153<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.cc#newcode153> > media/audio/win/audio_low_**latency_output_win.cc:153: case > KSAUDIO_SPEAKER_5POINT1: > remove extra spaces > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.cc#newcode156<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.cc#newcode156> > media/audio/win/audio_low_**latency_output_win.cc:156: case > KSAUDIO_SPEAKER_7POINT1_**SURROUND: > remove extra spaces > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.cc#newcode210<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.cc#newcode210> > media/audio/win/audio_low_**latency_output_win.cc:210: static int > ChannelUpMix(void* input, > man this function is going to get massive over time -- any ideas on how > to parameterize things? > > also won't this break if we do 8-bit or 24-bit audio? > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.cc#newcode913<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.cc#newcode913> > media/audio/win/audio_low_**latency_output_win.cc:913: share_mode_, > reinterpret_cast<WAVEFORMATEX***>(&format_), > indent 2 more spaces > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.cc#newcode1126<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.cc#newcode1126> > media/audio/win/audio_low_**latency_output_win.cc:1126: DWORD new_state) { > nit: this should be aligned at the ( > > if it doesn't fit then drop device_id to this line as well > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.h<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.h> > File media/audio/win/audio_low_**latency_output_win.h (right): > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.h#newcode80<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.h#newcode80> > media/audio/win/audio_low_**latency_output_win.h:80: // then 2 -> 7.1 > up-mixing will take place for each OnMoreData() callback. > what about opening up 8 channels when HardwareChannelCount() is 2? > > I don't think we need to fix it now but I would mention what clients > should expect (error? downmixing?) > > http://codereview.chromium.**org/10823100/diff/1009/media/** > audio/win/audio_low_latency_**output_win.h#newcode166<http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_latency_output_win.h#newcode166> > media/audio/win/audio_low_**latency_output_win.h:166: class > WASAPIAudioOutputStreamTest; > AFAIK you don't need to fwd decl this for FRIEND_TEST > > http://codereview.chromium.**org/10823100/<http://codereview.chromium.org/108... >
Turns out this CL was pretty much needed for m21. We have a bit of a bloodbath since now flapper might the wasapi codepath. I would love for this CL to land very soon (like this week) but also it would be very nice to have 2 --> 5.1 mapping. Besides that we think that the next common case is actually 2 -> 1 but I don't think we are hurting there as much. I might not be able to merge this to m21, but I think I can maybe fast-track it for dev channel.
rubberstamp lgtm.
Did my best to fix the most critical things before I go on vacation. I hope we are ready to land now. Summary of changes in last version: - Added 2 -> 5.1 channel up-mixing. - Open() now fails for invalid mixing modes (e.g. 8 -> 2). - Open() now fails for 8-bit audio in combination with up-mixing. - Added additional test for playing out a mono file. What remains? - Clean up ChannelUpMix(). - Add support for 8-bit and 24-bit audio? - Add support for 2 -> 1 down-mixing? - Add accessors for ChannelCount/Layout. Perhaps someone can press the commit button for me if things look OK. If not, I might be able to make some (minor) last changes on Saturday. I will only have limited Internet access next week. http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:77: HRESULT hr = CoCreateInstance(__uuidof(MMDeviceEnumerator), On 2012/08/02 17:14:14, scherkus wrote: > remove extra space between = and CCI() Done. http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:127: eConsole, reinterpret_cast<WAVEFORMATEX**>(&format_ex)); On 2012/08/02 17:14:14, scherkus wrote: > indent 2 more spaces Done. http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:142: const char* speaker_config("Undefined"); On 2012/08/02 17:14:14, scherkus wrote: > () notation is a bit odd ... use = instead Done. http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:153: case KSAUDIO_SPEAKER_5POINT1: On 2012/08/02 17:14:14, scherkus wrote: > remove extra spaces Done. http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:156: case KSAUDIO_SPEAKER_7POINT1_SURROUND: On 2012/08/02 17:14:14, scherkus wrote: > remove extra spaces Done. http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:210: static int ChannelUpMix(void* input, I have ideas on how to make it smaller, yes. Have no time to finalize today before I go on vacation. Any input from your side is of course appreciated. Regarding 8-bit and 24-bit audio: The old implementation has not been tested at all for 8-bits but there is nothing in there that prevents it from supporting 8 bits. To my knowledge, no low-latency client requires 8-bit support today and I have not written any unit tests for it. 24-bits will not work since it lacks fundamental parts to support it. One example: AdjustVolume() does not support 24-bit. As you point out, the new implementation will not work for 8-bits if up-mixing is required. Same for 24-bit. Best I can do for now is to add a error logging for it. http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:913: share_mode_, reinterpret_cast<WAVEFORMATEX*>(&format_), On 2012/08/02 17:14:14, scherkus wrote: > indent 2 more spaces Done. http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:1126: DWORD new_state) { On 2012/08/02 17:14:14, scherkus wrote: > nit: this should be aligned at the ( > > if it doesn't fit then drop device_id to this line as well Done. http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.h:80: // then 2 -> 7.1 up-mixing will take place for each OnMoreData() callback. Added comment about that we don't support it. Also, ensured that we can create an object for this case, log that we are in a "bad state" and then any call to Open() will return false. Also added a test case to verify that we are unable to call Open() for 2->8. http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.h:166: class WASAPIAudioOutputStreamTest; Correct. Thanks ;-)
http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10823100/diff/1009/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:210: static int ChannelUpMix(void* input, More on the 8-bit case. I am actually not sure that we should ever open in 8-bit mode since I don't think that any device supports is as default mode (i.e., user can't set it in his device properties). Hence, the more correct way should be to open up at 16 and (same as we could report back to the client). Then if the client feeds 8-bit we should do 8->16 bit conversion, or better, the client does this and feeds converted 16-bit samples. 24-bit is another story since the user can sometimes select that and it will then be the preferred bit depth.
I think I know what can be causing the failing tests on the bots. See comments below. http://codereview.chromium.org/10823100/diff/8004/media/audio/win/audio_low_l... File media/audio/win/audio_low_latency_output_win.cc (right): http://codereview.chromium.org/10823100/diff/8004/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:93: return 0.0; BUG: We should return hr here and not 0.0. http://codereview.chromium.org/10823100/diff/8004/media/audio/win/audio_low_l... media/audio/win/audio_low_latency_output_win.cc:105: return 0.0; ditto
LGTM w/ one Q. let's get this in and iterate further http://codereview.chromium.org/10823100/diff/14008/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10823100/diff/14008/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:293: return (format_.Format.nChannels / client_channel_count_); OOC and something we can TODO for now, but do we always want integer truncation? if nChannels (which is HardwareChannelCount()) is 3 (2.1) and client_channel_count_ is 2, we end up with 1 and don't hit the up-mixing path will we do the right thing in that scenario? (only output to L+R channels?)
also don't forget to update your CL description w/ the TODOs :)
Thanks Andrew. Added reply to your question, updated the description and added a TOOD(henrika) in the code. Andrew: could you land this one for me? I only have limited possibilites to work and to fix any issues that might pop up. Let me know if you want me to land it manually and I'll try to do it. http://codereview.chromium.org/10823100/diff/14008/media/audio/win/audio_low_... File media/audio/win/audio_low_latency_output_win.h (right): http://codereview.chromium.org/10823100/diff/14008/media/audio/win/audio_low_... media/audio/win/audio_low_latency_output_win.h:293: return (format_.Format.nChannels / client_channel_count_); We only support the mixing modes listed in the "Overview of operation and performance:" section. Your example will fail since it will not be detected as a mixing case. If I changed to float, we would detect it but no support exists for it in the mixing phase. Both these cases are bad and just removing int will not be the optimal solution. Given that I don't have access to hardware right now, I don't dare to change these parts. Will add a TODO and improve when I get back. I have better ideas on how to detect mixing mode than a plain integer. This CL has grown from "please make my G35 headset work with Chrome" to something larger ;-)
I can't download + apply the .wav files -- you'll either: 1) email them to me 2) land them 3) tell me to DISABLE the tests for now + land the code
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/henrika@chromium.org/10823100/14010 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
