Pass more detailed audio hardware configuration information to the renderer

Pass more detailed audio hardware configuration information to the renderer

AudioHardwareConfig currently contains an ad-hoc mix of pieces of information about
the audio input and output hardware.  This CL adds more complete and symmetric information
about the audio hardware as tracked in AudioHardwareConfig.

The ChannelMixer is also upgraded to allow for "discrete" up and down mixing.

BUG=none
TEST=manual - several tests updated
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187936

[Chris Rogers, 2013-02-28 01:56:32]

Not for formal review yet.

The actual code here will need to change somewhat based on Shijing's CL landing:
https://codereview.chromium.org/12316131/

So far, this is just to show some early attempts at getting more detailed audio
channel information to the renderer.

Shijing, I like your approach of using two AudioParameters, one for input and
one for output, so will likely re-base off this concept once your CL lands.

WDYT?

[DaleCurtis, 2013-02-28 02:18:08]

- I don't see the CHANNEL_LAYOUT_UNSUPPORTED stuff we discussed?

- Changing to AudioParameters everywhere sounds fine to me, but I suspect it
might be a bit unwieldy in places which only want a single value.

https://codereview.chromium.org/12387006/diff/6002/media/base/audio_hardware_...
File media/base/audio_hardware_config.h (right):

https://codereview.chromium.org/12387006/diff/6002/media/base/audio_hardware_...
media/base/audio_hardware_config.h:18: AudioHardwareConfig();
Is this necessary?

https://codereview.chromium.org/12387006/diff/6002/media/base/audio_hardware_...
media/base/audio_hardware_config.h:29: void Reset(int buffer_size,
output_buffer_size here and elsewhere. Why not just call it Update() and put it
with the other Update methods below? Which should also be updated to include
your new parameters.

[DaleCurtis, 2013-02-28 02:18:17]

Otherwise the idea looks fine to me!

[Chris Rogers, 2013-03-06 02:47:57]

PTAL - I've rebased after Shijing's nice AudioUtilities re-factoring.

p.s. I'm not crazy about using CHANNEL_LAYOUT_UNSUPPORTED to represent discrete
channels.  I'd rather use CHANNEL_LAYOUT_NONE or create a new one
CHANNEL_LAYOUT_DISCRETE

[DaleCurtis, 2013-03-06 19:00:40]

Whoops, when I suggested that name I thought it wasn't in use already. You're
right that we shouldn't be overloading these. CHANNEL_LAYOUT_DISCRETE is a
better name.

[DaleCurtis, 2013-03-06 19:19:30]

https://codereview.chromium.org/12387006/diff/24001/media/audio/mac/audio_man...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/12387006/diff/24001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:343: channel_layout =
input_params.channel_layout();
Instead of doing this I think you should use FFmpegCommon::GuessChannelLayout()
(please move this function into the channel_layout.{cc,h} file when
CHANNEL_LAYOUT is NOT channel_layout_discrete. This will fix
http://crbug.com/169396

https://codereview.chromium.org/12387006/diff/24001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:368: &channels, &channels_per_frame);
4 space indent.

https://codereview.chromium.org/12387006/diff/24001/media/audio/mac/audio_uni...
File media/audio/mac/audio_unified_mac.cc (right):

https://codereview.chromium.org/12387006/diff/24001/media/audio/mac/audio_uni...
media/audio/mac/audio_unified_mac.cc:398: bool
AudioHardwareUnifiedStream::GetDefaultOutputDevice(
I think this code should live in AudioManagerMac. Additionally this
GetDefaultOutputDevice code has at least 3 or 4 copies around the OSX code base.
It should be deduplicated.

https://codereview.chromium.org/12387006/diff/24001/media/base/audio_hardware...
File media/base/audio_hardware_config.h (right):

https://codereview.chromium.org/12387006/diff/24001/media/base/audio_hardware...
media/base/audio_hardware_config.h:38: // TODO(crogers): These methods should be
updated to take AudioParameters!
Seems like that should be done in this CL? You're already tinkering with the
complete path here.

1. UpdateInputConfig has no current users, so that's easy :)
2. UpdateOutputConfig could be easily changed to take the full AudioParameters
object from browser side. Just change the IPC type to AudioParameters.

[Chris Rogers, 2013-03-07 00:34:17]

PTAL

https://codereview.chromium.org/12387006/diff/24001/media/audio/mac/audio_man...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/12387006/diff/24001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:343: channel_layout =
input_params.channel_layout();
On 2013/03/06 19:19:30, DaleCurtis wrote:
> Instead of doing this I think you should use
FFmpegCommon::GuessChannelLayout()
> (please move this function into the channel_layout.{cc,h} file when
> CHANNEL_LAYOUT is NOT channel_layout_discrete. This will fix
> http://crbug.com/169396 

I think I've changed it to be more in line with your thinking here.

https://codereview.chromium.org/12387006/diff/24001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:368: &channels, &channels_per_frame);
On 2013/03/06 19:19:30, DaleCurtis wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/12387006/diff/24001/media/audio/mac/audio_uni...
File media/audio/mac/audio_unified_mac.cc (right):

https://codereview.chromium.org/12387006/diff/24001/media/audio/mac/audio_uni...
media/audio/mac/audio_unified_mac.cc:398: bool
AudioHardwareUnifiedStream::GetDefaultOutputDevice(
On 2013/03/06 19:19:30, DaleCurtis wrote:
> I think this code should live in AudioManagerMac. Additionally this
> GetDefaultOutputDevice code has at least 3 or 4 copies around the OSX code
base.
> It should be deduplicated.

I've moved the functions to AudioManagerMac.  I agree about the code
duplication, but I'm cautious to change all the potential benefactors of these
new functions in this CL, since I'd like to be careful about that move and it
will involve other specific testing.  I'm just trying to keep the number of
changes sane in the CL and work in smaller work units.

Adding TODO in latest patchset.

[DaleCurtis, 2013-03-07 01:52:46]

Haven't rereviewed yet, but xians CL just got reverted so you may want to wait
until that gets relanded.

[DaleCurtis, 2013-03-07 02:41:39]

https://codereview.chromium.org/12387006/diff/43001/content/common/media/medi...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/common/media/medi...
content/common/media/media_param_traits.cc:47: if (channel_layout ==
media::CHANNEL_LAYOUT_DISCRETE)
Does anything use Reset() that doesn't provide all values? If not, I'd just add
it to that function. It's a bit weird to do this check here.

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
content/renderer/media/audio_message_filter.cc:175: // TODO(crogers): fix
OnOutputDeviceChanged() to pass AudioParameters.
I assumed you'd change this now as otherwise this will break your code since
channels_ will get overwritten?

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
content/renderer/media/audio_message_filter.cc:177:
media::AudioParameters::AUDIO_PCM_LOW_LATENCY,
4 space indent.

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:30: //
AudioParameters(
?

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:41: :
fake_config_(AudioParameters(), AudioParameters()) {
I think this test needs real parameters, but I don't remember now.

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/we...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/we...
content/renderer/media/webrtc_audio_device_unittest.cc:222:
AudioParameters::AUDIO_PCM_LOW_LATENCY,
4 space.

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/we...
content/renderer/media/webrtc_audio_device_unittest.cc:229:
AudioParameters::AUDIO_PCM_LOW_LATENCY,
4 space.

https://codereview.chromium.org/12387006/diff/43001/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:635: unsigned
RendererWebKitPlatformSupportImpl::audioHardwareOutputChannels() {
Should this just be size_t?

https://codereview.chromium.org/12387006/diff/43001/content/test/webrtc_audio...
File content/test/webrtc_audio_device_test.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/test/webrtc_audio...
content/test/webrtc_audio_device_test.cc:19: #include
"content/common/media/media_param_traits.h"
??

https://codereview.chromium.org/12387006/diff/43001/content/test/webrtc_audio...
content/test/webrtc_audio_device_test.cc:274: ChannelLayout
output_channels_layout =
Seems AudioHardwareConfig should offer to return AudioParameter objects ?

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:26: #include "media/ffmpeg/ffmpeg_common.h"
You can't include this here which is why I said the method should be moved to
channel_layout. ffmpeg_common pulls in a bunch of FFmpeg dependencies.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:271: if (!device)
CHECK().

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:275: AudioObjectPropertyAddress pa;
static const this like:

 static const AudioObjectPropertyAddress kAddress = {
    kAudioHardwarePropertyDefaultOutputDevice,
    kAudioObjectPropertyScopeGlobal,
    kAudioObjectPropertyElementMaster
  };

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:291: LOG(ERROR) << "Error getting default
output AudioDevice.";
DLOG?

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:314: int* channels_per_frame) {
Nothing seems to be using this? Drop for now?

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:315: if (!channels || !channels_per_frame)
these should be CHECK().

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:319: AudioObjectPropertyAddress pa;
Same static const comment.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:326: OSSTATUS_DCHECK(result == noErr,
result);
Don't DCHECK, this should just return false.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:328: if (result == noErr && size > 0) {
early return instead of indenting the entire next section.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:341: OSSTATUS_DCHECK(result == noErr,
result);
Ditto.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:437: ChannelLayout channel_layout =
GuessChannelLayout(hardware_channels);
Hmm, this isn't what I thought you wanted. This will guess layout for any
channel count. Don't you only want to guess the layout when input_params !=
DISCRETE?

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:461: if (channel_layout ==
CHANNEL_LAYOUT_UNSUPPORTED)
DISCRETE?

https://codereview.chromium.org/12387006/diff/43001/media/base/channel_mixer.cc
File media/base/channel_mixer.cc (right):

https://codereview.chromium.org/12387006/diff/43001/media/base/channel_mixer....
media/base/channel_mixer.cc:24: static int ValidateLayout(ChannelLayout layout)
{
No reason to return an int now.

https://codereview.chromium.org/12387006/diff/43001/media/base/channel_mixer....
media/base/channel_mixer.cc:59: ChannelMixer::ChannelMixer(ChannelLayout
input_layout,
Just remove this one, it's really sketchy to initialize it like this.

https://codereview.chromium.org/12387006/diff/43001/media/base/channel_mixer.h
File media/base/channel_mixer.h (right):

https://codereview.chromium.org/12387006/diff/43001/media/base/channel_mixer....
media/base/channel_mixer.h:27: ChannelMixer(ChannelLayout input_layout,
ChannelLayout output_layout);
Can this constructor just be removed?

https://codereview.chromium.org/12387006/diff/43001/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

https://codereview.chromium.org/12387006/diff/43001/media/ffmpeg/ffmpeg_commo...
media/ffmpeg/ffmpeg_common.h:96: ChannelLayout GuessChannelLayout(int channels);
Needs to be moved to channel_layout

[Chris Rogers, 2013-03-09 01:37:50]

Dale, thanks for your detailed review so far.  I think I've addressed the
comments.

PTAL

https://codereview.chromium.org/12387006/diff/43001/content/common/media/medi...
File content/common/media/media_param_traits.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/common/media/medi...
content/common/media/media_param_traits.cc:47: if (channel_layout ==
media::CHANNEL_LAYOUT_DISCRETE)
On 2013/03/07 02:41:39, DaleCurtis wrote:
> Does anything use Reset() that doesn't provide all values? If not, I'd just
add
> it to that function. It's a bit weird to do this check here.

Done.

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
content/renderer/media/audio_message_filter.cc:175: // TODO(crogers): fix
OnOutputDeviceChanged() to pass AudioParameters.
On 2013/03/07 02:41:39, DaleCurtis wrote:
> I assumed you'd change this now as otherwise this will break your code since
> channels_ will get overwritten?

Good catch.  I've fixed the code to take channels into account.  I'm still
deferring changing OnOutputDeviceChanged() since it's more "rabbit hole"

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
content/renderer/media/audio_message_filter.cc:177:
media::AudioParameters::AUDIO_PCM_LOW_LATENCY,
On 2013/03/07 02:41:39, DaleCurtis wrote:
> 4 space indent.

Done.

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:30: //
AudioParameters(
On 2013/03/07 02:41:39, DaleCurtis wrote:
> ?

Done.

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/au...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:41: :
fake_config_(AudioParameters(), AudioParameters()) {
On 2013/03/07 02:41:39, DaleCurtis wrote:
> I think this test needs real parameters, but I don't remember now.

I've improved this a bit to make sure the output config is set to the same
values as you had.  This input config is still empty, but your values were too,
so that should be fine...

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/we...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/we...
content/renderer/media/webrtc_audio_device_unittest.cc:222:
AudioParameters::AUDIO_PCM_LOW_LATENCY,
On 2013/03/07 02:41:39, DaleCurtis wrote:
> 4 space.

Done.

https://codereview.chromium.org/12387006/diff/43001/content/renderer/media/we...
content/renderer/media/webrtc_audio_device_unittest.cc:229:
AudioParameters::AUDIO_PCM_LOW_LATENCY,
On 2013/03/07 02:41:39, DaleCurtis wrote:
> 4 space.

Done.

https://codereview.chromium.org/12387006/diff/43001/content/renderer/renderer...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/renderer/renderer...
content/renderer/renderer_webkitplatformsupport_impl.cc:635: unsigned
RendererWebKitPlatformSupportImpl::audioHardwareOutputChannels() {
On 2013/03/07 02:41:39, DaleCurtis wrote:
> Should this just be size_t?

It's part of the WebKit API, so it has to match that.  Also, it's consistent
with how we represent channels in WebKit

https://codereview.chromium.org/12387006/diff/43001/content/test/webrtc_audio...
File content/test/webrtc_audio_device_test.cc (right):

https://codereview.chromium.org/12387006/diff/43001/content/test/webrtc_audio...
content/test/webrtc_audio_device_test.cc:19: #include
"content/common/media/media_param_traits.h"
On 2013/03/07 02:41:39, DaleCurtis wrote:
> ??

I found that it was necessary to include this for the serializing of
AudioParameters, since we're now using them for the AudioHardwareConfig. 
There's a compile error otherwise.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:26: #include "media/ffmpeg/ffmpeg_common.h"
On 2013/03/07 02:41:39, DaleCurtis wrote:
> You can't include this here which is why I said the method should be moved to
> channel_layout. ffmpeg_common pulls in a bunch of FFmpeg dependencies.

Ah I didn't understand what you had meant before - Fixed :)

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:271: if (!device)
On 2013/03/07 02:41:39, DaleCurtis wrote:
> CHECK().

Done.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:275: AudioObjectPropertyAddress pa;
On 2013/03/07 02:41:39, DaleCurtis wrote:
> static const this like:
> 
>  static const AudioObjectPropertyAddress kAddress = {
>     kAudioHardwarePropertyDefaultOutputDevice,
>     kAudioObjectPropertyScopeGlobal,
>     kAudioObjectPropertyElementMaster
>   };

Done.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:291: LOG(ERROR) << "Error getting default
output AudioDevice.";
On 2013/03/07 02:41:39, DaleCurtis wrote:
> DLOG?

Done.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:314: int* channels_per_frame) {
On 2013/03/07 02:41:39, DaleCurtis wrote:
> Nothing seems to be using this? Drop for now?

It will soon be using it (AudioHardwareUnifiedStream) so leaving as is.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:315: if (!channels || !channels_per_frame)
On 2013/03/07 02:41:39, DaleCurtis wrote:
> these should be CHECK().

Done.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:319: AudioObjectPropertyAddress pa;
On 2013/03/07 02:41:39, DaleCurtis wrote:
> Same static const comment.

It can't be static const because the scope is set from a method arg, leaving as
is.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:326: OSSTATUS_DCHECK(result == noErr,
result);
On 2013/03/07 02:41:39, DaleCurtis wrote:
> Don't DCHECK, this should just return false.

Done.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:328: if (result == noErr && size > 0) {
On 2013/03/07 02:41:39, DaleCurtis wrote:
> early return instead of indenting the entire next section.

Done.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:341: OSSTATUS_DCHECK(result == noErr,
result);
On 2013/03/07 02:41:39, DaleCurtis wrote:
> Ditto.

Done.

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:437: ChannelLayout channel_layout =
GuessChannelLayout(hardware_channels);
On 2013/03/07 02:41:39, DaleCurtis wrote:
> Hmm, this isn't what I thought you wanted. This will guess layout for any
> channel count. Don't you only want to guess the layout when input_params !=
> DISCRETE?

Actually, I'm just being consistent with the AudioManagerWin WASAPI case where
it doesn't use the passed in channel_layout, but instead is militant and insists
to use the hardware layout.

I'd rather become more like the WASAPI case.  It turns out that the
CHANNEL_LAYOUT_DISCRETE layout will be passed in from AudioOutputDevice and the
ChannelMixer will use discrete mixing even though the manager says it's a real
channel layout.

The code in the renderer side to pass in CHANNEL_LAYOUT_DISCRETE from WebAudio
is not yet there yet, so that may not be obvious, but that's the plan...

https://codereview.chromium.org/12387006/diff/43001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:461: if (channel_layout ==
CHANNEL_LAYOUT_UNSUPPORTED)
On 2013/03/07 02:41:39, DaleCurtis wrote:
> DISCRETE?

Actually, I think that's right because GuessChannelLayout() can return
CHANNEL_LAYOUT_UNSUPPORTED, so I'm just trying to translate that to a sane
setting which falls back to discrete.  It should never happen for any channel
layouts <= 8 channels, but for large numbers of channels it will.

https://codereview.chromium.org/12387006/diff/43001/media/base/channel_mixer.cc
File media/base/channel_mixer.cc (right):

https://codereview.chromium.org/12387006/diff/43001/media/base/channel_mixer....
media/base/channel_mixer.cc:24: static int ValidateLayout(ChannelLayout layout)
{
On 2013/03/07 02:41:39, DaleCurtis wrote:
> No reason to return an int now.

Done.

https://codereview.chromium.org/12387006/diff/43001/media/base/channel_mixer....
media/base/channel_mixer.cc:59: ChannelMixer::ChannelMixer(ChannelLayout
input_layout,
On 2013/03/07 02:41:39, DaleCurtis wrote:
> Just remove this one, it's really sketchy to initialize it like this.

I don't think it's that unusual to have multiple constructors which share common
initialization code.

The reason I left your original constructor is because there are several places
(in the low-level OS-specific AudioOutputStreams) that use a ChannelMixer and it
would be much more cumbersome to have to create AudioParameters for each client
of ChannelMixer.  This way, we consolidate the boilerplate code all in one place
right here.

https://codereview.chromium.org/12387006/diff/43001/media/ffmpeg/ffmpeg_common.h
File media/ffmpeg/ffmpeg_common.h (right):

https://codereview.chromium.org/12387006/diff/43001/media/ffmpeg/ffmpeg_commo...
media/ffmpeg/ffmpeg_common.h:96: ChannelLayout GuessChannelLayout(int channels);
On 2013/03/07 02:41:39, DaleCurtis wrote:
> Needs to be moved to channel_layout

Done.

[DaleCurtis, 2013-03-11 19:21:50]

lgtm % comments.

https://chromiumcodereview.appspot.com/12387006/diff/43001/media/base/channel...
File media/base/channel_mixer.cc (right):

https://chromiumcodereview.appspot.com/12387006/diff/43001/media/base/channel...
media/base/channel_mixer.cc:59: ChannelMixer::ChannelMixer(ChannelLayout
input_layout,
On 2013/03/09 01:37:50, Chris Rogers wrote:
> On 2013/03/07 02:41:39, DaleCurtis wrote:
> > Just remove this one, it's really sketchy to initialize it like this.
> 
> I don't think it's that unusual to have multiple constructors which share
common
> initialization code.
> 
> The reason I left your original constructor is because there are several
places
> (in the low-level OS-specific AudioOutputStreams) that use a ChannelMixer and
it
> would be much more cumbersome to have to create AudioParameters for each
client
> of ChannelMixer.  This way, we consolidate the boilerplate code all in one
place
> right here.

What I meant is it's sketchy to have generated parameters here which are
useless.  Instead of having an Initialize() method which takes AudioParameters,
how about one which takes a pair of channel layout and channel count?

https://chromiumcodereview.appspot.com/12387006/diff/84001/media/audio/audio_...
File media/audio/audio_parameters.cc (left):

https://chromiumcodereview.appspot.com/12387006/diff/84001/media/audio/audio_...
media/audio/audio_parameters.cc:56: channels_ =
ChannelLayoutToChannelCount(channel_layout);
Keep this as:

if (channel_layout_ != CHANNEL_LAYOUT_DISCRETE)
  DCHECK_EQ(channels_, ChannelLayoutToChannelCount(channel_layout));

https://chromiumcodereview.appspot.com/12387006/diff/84001/media/audio/mac/au...
File media/audio/mac/audio_manager_mac.cc (right):

https://chromiumcodereview.appspot.com/12387006/diff/84001/media/audio/mac/au...
media/audio/mac/audio_manager_mac.cc:429:
GetDefaultOutputChannels(&hardware_channels, &hardware_channels_per_frame);
Check return value? hardware_channels_per_frame should be initialized to 0 since
its possible for this function to leave it uninitialized.

https://chromiumcodereview.appspot.com/12387006/diff/84001/media/base/channel...
File media/base/channel_mixer.cc (right):

https://chromiumcodereview.appspot.com/12387006/diff/84001/media/base/channel...
media/base/channel_mixer.cc:29: // Channels must be received externally with
discrete layout.
Add this to the CHECK_NE above and then move this if() in front of the
ValidateLayout() calls, it makes more sense that way I think.  I.e, why validate
the layout for DISCRETE?

[Chris Rogers, 2013-03-12 22:32:31]

+jamesr: content OWNERS review

https://codereview.chromium.org/12387006/diff/84001/media/audio/audio_paramet...
File media/audio/audio_parameters.cc (left):

https://codereview.chromium.org/12387006/diff/84001/media/audio/audio_paramet...
media/audio/audio_parameters.cc:56: channels_ =
ChannelLayoutToChannelCount(channel_layout);
On 2013/03/11 19:21:50, DaleCurtis wrote:
> Keep this as:
> 
> if (channel_layout_ != CHANNEL_LAYOUT_DISCRETE)
>   DCHECK_EQ(channels_, ChannelLayoutToChannelCount(channel_layout));

Done.

https://codereview.chromium.org/12387006/diff/84001/media/audio/mac/audio_man...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/12387006/diff/84001/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:429:
GetDefaultOutputChannels(&hardware_channels, &hardware_channels_per_frame);
On 2013/03/11 19:21:50, DaleCurtis wrote:
> Check return value? hardware_channels_per_frame should be initialized to 0
since
> its possible for this function to leave it uninitialized.

Fixed - although hardware_channels_per_frame is unused here - it's best to have
a default value of 1 which means non-interleaved.  0 is not valid.

https://codereview.chromium.org/12387006/diff/84001/media/base/channel_mixer.cc
File media/base/channel_mixer.cc (right):

https://codereview.chromium.org/12387006/diff/84001/media/base/channel_mixer....
media/base/channel_mixer.cc:29: // Channels must be received externally with
discrete layout.
On 2013/03/11 19:21:50, DaleCurtis wrote:
> Add this to the CHECK_NE above and then move this if() in front of the
> ValidateLayout() calls, it makes more sense that way I think.  I.e, why
validate
> the layout for DISCRETE?

Done.

[Chris Rogers, 2013-03-12 23:01:21]

+piman: content OWNERS review

[piman, 2013-03-12 23:21:02]

On 2013/03/12 23:01:21, Chris Rogers wrote:
> +piman: content OWNERS review

LGTM for content/

You'll need a security person to review the IPC changes.

[Chris Rogers, 2013-03-12 23:30:52]

+palmer: security review for IPC changes

[palmer, 2013-03-13 19:22:31]

IPC security LGTM.

https://codereview.chromium.org/12387006/diff/98007/media/audio/mac/audio_man...
File media/audio/mac/audio_manager_mac.cc (right):

https://codereview.chromium.org/12387006/diff/98007/media/audio/mac/audio_man...
media/audio/mac/audio_manager_mac.cc:438: int user_buffer_size =
GetUserBufferSize();
Sizes should generally be size_t, or at least unsigned. Is that fixable in this
CL?

[Chris Rogers, 2013-03-13 20:36:54]

Committed patchset #14 manually as r187936 (presubmit successful).