Make AudioParameters a class instead of a struct

Make AudioParameters a class instead of a struct

Also collapses some long parameter lists into AudioParameters and moves some
of the hardcoded values (e.g. 16 bit audio in AudioDevice) to more appropriate
locations.

BUG=115902
TEST=manually testing everything works out

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=128054

[vrk (LEFT CHROMIUM), 2012-03-09 20:59:31]

crogers: WebAudio stuff
tommi: WebRTC stuff
enal: everything
scherkus: FYI!

everyone: The biggest effect of this change is I am forcing all clients to use
ChannelLayout instead of directly setting the number of channels. This will be
necessary if we want to have multichannel audio support.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/audio_device.cc (left):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/audio_device.cc:83: audio_parameters_.bits_per_sample =
16;
Notice that bits_per_sample was hardcoded to 16 for all AudioDevices.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:56: bool
AudioRendererImpl::OnInitialize(int bits_per_channel,
This should also probably be changed to use AudioParameters, but I was going to
change this when I squished AudioRendererImpl and AudioRendererBase into one
(crbug.com/116645).

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:440:
AudioParameters::AUDIO_PCM_LOW_LATENCY, out_channel_layout,
Prior to this CL, these values would have been set in
AudioDevice/AudioInputDevice's constructors, which seemed like the wrong place
for it.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/ren...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/ren...
content/renderer/renderer_webkitplatformsupport_impl.cc:601: // TODO(crogers):
WebKit should give the channel layout instead of the hard
crogers: Note this TODO I assigned to you! What do you think?

[scherkus (not reviewing), 2012-03-09 21:48:59]

precursory ell gee tee em modulo nits

I'll defer to the rest of the gang to give their reviews

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/audio_hardware.h (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/audio_hardware.h:36: // Must be called from
RenderThreadImpl::current().
update docs

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:56: bool
AudioRendererImpl::OnInitialize(int bits_per_channel,
On 2012/03/09 20:59:32, Victoria Kirst wrote:
> This should also probably be changed to use AudioParameters, but I was going
to
> change this when I squished AudioRendererImpl and AudioRendererBase into one
> (crbug.com/116645).

SGTM

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:190: uint32 bytes_per_frame =
isn't there a function on AudioParameters for this?

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/ren...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/ren...
content/renderer/renderer_webkitplatformsupport_impl.cc:628: }
we need a default case here -- perhaps stereo?

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
File media/audio/audio_parameters.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.cc:30: int samples_per_second, int bits_per_sample,
indent

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
File media/audio/audio_parameters.h (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.h:68: };
FYI now that this is a class I want to double check -- do we still want this to
be a copyable object?

(it's OK if it is as it's a POD)

[Chris Rogers, 2012-03-10 00:28:48]

Hi Victoria, I had a quick look through, and the idea to switch to using the
AudioParameters struct is a good thing - seems like a generally good cleanup.

I'm starting to look into use cases where there is no channel layout
(CHANNEL_LAYOUT_NONE I suppose).  An example of this would be a digital DJ setup
with 4 channels (1/2 main house mix, 3/4 headphone cue mix).  Or DAW setup - 8
discrete channels in / 8 discrete channels out (think recording studio mixing
board and multiple microphones on a drum-kit).

I didn't see anything major in this patch that will necessarily make this
difficult, but just wanted you to think about it a little in case I missed
something.

Specifically, I think the Web Audio API will need AudioHardware APIs to query
the number of input and output channels available, especially when there is no
channel layout.  And I see that you're removing the existing
GetInputChannelCount() method...

[tommi (sloooow) - chröme, 2012-03-10 10:11:32]

all sorts of excellent!  I'll wait for the next patch set with my el gutem.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:86: int samples_per_sec =
static_cast<int>(output_sample_rate());
remove cast

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:159: int samples_per_sec =
static_cast<int>(input_sample_rate());
remove cast

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:299:
static_cast<int>(audio_hardware::GetOutputSampleRate());
pretty please request.  to make the APIs consistent, can you make
GetOutputSampleRate (and input) return the same type as is used for the sample
rate and .... remove the cast :)

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:315:
static_cast<int>(audio_hardware::GetInputSampleRate());
ditto

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:440:
AudioParameters::AUDIO_PCM_LOW_LATENCY, out_channel_layout,
On 2012/03/09 20:59:32, Victoria Kirst wrote:
> Prior to this CL, these values would have been set in
> AudioDevice/AudioInputDevice's constructors, which seemed like the wrong place
> for it.

agreed.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/webrtc_audio_device_impl.h (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.h:270: return
static_cast<int>(input_audio_parameters_.samples_per_second());
remove the cast?

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.h:273: return
static_cast<int>(output_audio_parameters_.samples_per_second());
remove the cast?

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.h:309: AudioParameters
output_audio_parameters_;
nice!

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_unittest.cc:35: virtual double
GetAudioHardwareSampleRate() OVERRIDE {
can you please change return value to int to be consistent with AudioParameters?
I believe we're casting the return value to int at all call sites today.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_unittest.cc:38: virtual double
GetAudioInputHardwareSampleRate(
same here

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/ren...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/ren...
content/renderer/renderer_webkitplatformsupport_impl.cc:614: layout =
CHANNEL_LAYOUT_4POINT0;
nit: what about using the underscore in all the value names instead of POINT? 
(i.e. make all follow the CHANNEL_LAYOUT_2_1 format)

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
File media/audio/audio_parameters.h (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.h:52: Format format() const { return format_; }
Would it be worth it to have a DCHECK(IsValid()) in some/all of these?

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.h:54: int samples_per_second() const { return
samples_per_second_; }
absolute nit: sample_rate()?
I realize you'd have to change this in multiple places so unless someone else
agrees, it's up to you.

[Chris Rogers, 2012-03-12 20:07:13]

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
File media/audio/audio_parameters.h (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.h:36: int samples_per_second, int bits_per_sample,
It's incorrect to change the name "sample_rate" to "samples_per_second" because
what you really mean is "sample_frames_per_second".  For interleaved audio
streams (such as we use):

samples_per_second = channels * sample_frames_per_second;

I would stick with the original name "sample_rate", or use
"sample_frames_per_second" (but in my opinion "sample_frames_per_second" is a
lot clunkier sounding)

[enal1, 2012-03-13 21:16:09]

LGTM, assuming you reach consensus on terminology.

I'll also ned to change AudioParameters soon, it's necessary to add new flag
'use mixer'. Hopefully it still would be possible...

On 2012/03/12 20:07:13, Chris Rogers wrote:
>
https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
> File media/audio/audio_parameters.h (right):
> 
>
https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
> media/audio/audio_parameters.h:36: int samples_per_second, int
bits_per_sample,
> It's incorrect to change the name "sample_rate" to "samples_per_second"
because
> what you really mean is "sample_frames_per_second".  For interleaved audio
> streams (such as we use):
> 
> samples_per_second = channels * sample_frames_per_second;
> 
> I would stick with the original name "sample_rate", or use
> "sample_frames_per_second" (but in my opinion "sample_frames_per_second" is a
> lot clunkier sounding)

[vrk (LEFT CHROMIUM), 2012-03-16 18:30:41]

Thanks everyone!

I followed the suggestions and rebased to ToT. I also:
 - Removed "immutable" from the CL name the class isn't really immutable! (Would
like to make it such, but IPC makes it tricky)
 - Fixed some DEPS issues
 - Fixed some mac/win compile issues

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/audio_hardware.h (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/audio_hardware.h:36: // Must be called from
RenderThreadImpl::current().
On 2012/03/09 21:48:59, scherkus wrote:
> update docs

Done.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/audio_renderer_impl.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/audio_renderer_impl.cc:190: uint32 bytes_per_frame =
On 2012/03/09 21:48:59, scherkus wrote:
> isn't there a function on AudioParameters for this?

There isn't, but while I'm here I can add one! Mostly done; I can use this
function in a few more other places, but after the
AudioRendererImpl/AudioRendererBase merge.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/webrtc_audio_device_impl.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:86: int samples_per_sec =
static_cast<int>(output_sample_rate());
On 2012/03/10 10:11:32, tommi wrote:
> remove cast

Done.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:159: int samples_per_sec =
static_cast<int>(input_sample_rate());
On 2012/03/10 10:11:32, tommi wrote:
> remove cast

Done.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:299:
static_cast<int>(audio_hardware::GetOutputSampleRate());
On 2012/03/10 10:11:32, tommi wrote:
> pretty please request.  to make the APIs consistent, can you make
> GetOutputSampleRate (and input) return the same type as is used for the sample
> rate and .... remove the cast :)

Really good idea! I changed everything to int.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.cc:315:
static_cast<int>(audio_hardware::GetInputSampleRate());
On 2012/03/10 10:11:32, tommi wrote:
> ditto

Done.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/webrtc_audio_device_impl.h (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.h:270: return
static_cast<int>(input_audio_parameters_.samples_per_second());
On 2012/03/10 10:11:32, tommi wrote:
> remove the cast?

Good catch! Done.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.h:273: return
static_cast<int>(output_audio_parameters_.samples_per_second());
On 2012/03/10 10:11:32, tommi wrote:
> remove the cast?

Done.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_impl.h:309: AudioParameters
output_audio_parameters_;
On 2012/03/10 10:11:32, tommi wrote:
> nice!

:)

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_unittest.cc:35: virtual double
GetAudioHardwareSampleRate() OVERRIDE {
On 2012/03/10 10:11:32, tommi wrote:
> can you please change return value to int to be consistent with
AudioParameters?
> I believe we're casting the return value to int at all call sites today.

Done.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/med...
content/renderer/media/webrtc_audio_device_unittest.cc:38: virtual double
GetAudioInputHardwareSampleRate(
On 2012/03/10 10:11:32, tommi wrote:
> same here

Done.

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/ren...
File content/renderer/renderer_webkitplatformsupport_impl.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/ren...
content/renderer/renderer_webkitplatformsupport_impl.cc:614: layout =
CHANNEL_LAYOUT_4POINT0;
On 2012/03/10 10:11:32, tommi wrote:
> nit: what about using the underscore in all the value names instead of POINT? 
> (i.e. make all follow the CHANNEL_LAYOUT_2_1 format)

SGTM. Looks like we were doing that to be consistent with FFmpeg's
POINT/underscore inconsistency (see revised ffmpeg_common.cc). Fixed to use
underscores!

https://chromiumcodereview.appspot.com/9655018/diff/2001/content/renderer/ren...
content/renderer/renderer_webkitplatformsupport_impl.cc:628: }
On 2012/03/09 21:48:59, scherkus wrote:
> we need a default case here -- perhaps stereo?

Done.

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
File media/audio/audio_parameters.cc (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.cc:30: int samples_per_second, int bits_per_sample,
On 2012/03/09 21:48:59, scherkus wrote:
> indent

Done.

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
File media/audio/audio_parameters.h (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.h:36: int samples_per_second, int bits_per_sample,
On 2012/03/12 20:07:13, Chris Rogers wrote:
> It's incorrect to change the name "sample_rate" to "samples_per_second"
because
> what you really mean is "sample_frames_per_second".  For interleaved audio
> streams (such as we use):
> 
> samples_per_second = channels * sample_frames_per_second;
> 
> I would stick with the original name "sample_rate", or use
> "sample_frames_per_second" (but in my opinion "sample_frames_per_second" is a
> lot clunkier sounding)

sample_rate it is, as per offline discussion!

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.h:52: Format format() const { return format_; }
On 2012/03/10 10:11:32, tommi wrote:
> Would it be worth it to have a DCHECK(IsValid()) in some/all of these?

It's a good idea that we should be doing checks within the object to make sure
it's valid. However, instead of checking whether each member is valid on each
access, it would probably make more sense to DCHECK IsValid upon construction,
similar to how ValidateConfigs() works in AudioRendererAlgorithmBase upon
initialization. I'd like to do this in a follow-up CL, though, since I have a
feeling this may be a non-trivial change!

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.h:54: int samples_per_second() const { return
samples_per_second_; }
On 2012/03/10 10:11:32, tommi wrote:
> absolute nit: sample_rate()?
> I realize you'd have to change this in multiple places so unless someone else
> agrees, it's up to you.

sample_rate it is, as per offline discussion!

Aside: samples_per_second is used in a *lot* of the audio-tag specific code.
I'll fix that in a separate CL...

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.h:68: };
On 2012/03/09 21:48:59, scherkus wrote:
> FYI now that this is a class I want to double check -- do we still want this
to
> be a copyable object?
> 
> (it's OK if it is as it's a POD)

Yup! Thanks for checking!

[tommi (sloooow) - chröme, 2012-03-16 20:57:39]

lgtm. great job!

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
File media/audio/audio_parameters.h (right):

https://chromiumcodereview.appspot.com/9655018/diff/2001/media/audio/audio_pa...
media/audio/audio_parameters.h:52: Format format() const { return format_; }
On 2012/03/16 18:30:41, Victoria Kirst wrote:
> On 2012/03/10 10:11:32, tommi wrote:
> > Would it be worth it to have a DCHECK(IsValid()) in some/all of these?
> 
> It's a good idea that we should be doing checks within the object to make sure
> it's valid. However, instead of checking whether each member is valid on each
> access, it would probably make more sense to DCHECK IsValid upon construction,
> similar to how ValidateConfigs() works in AudioRendererAlgorithmBase upon
> initialization. I'd like to do this in a follow-up CL, though, since I have a
> feeling this may be a non-trivial change!

sounds good.

[Chris Rogers, 2012-03-16 21:37:10]

Victoria, wow that's a lot of files - thanks for the massive cleanup!

LGTM

[commit-bot: I haz the power, 2012-03-19 21:40:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/9655018/15098

[commit-bot: I haz the power, 2012-03-19 21:41:32]

Presubmit check for 9655018-15098 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content
    content/browser
    content/renderer

Presubmit checks took 5.3s to calculate.

[vrk (LEFT CHROMIUM), 2012-03-19 23:42:38]

jam: OWNERS review for content

(one thing in particular: not sure if I put the AudioParamTraits in the right
place, or if I did the right thing in content/common/DEPS)

[jam, 2012-03-20 15:29:45]

why move from using the macros to do IPC serialization to doing it manually? the
macro approach is the preferred way. aside from writing less code, it's better
for security as it avoids the chance of subtle errors

[jam, 2012-03-20 15:30:19]

http://codereview.chromium.org/9655018/diff/15098/content/common/DEPS
File content/common/DEPS (right):

http://codereview.chromium.org/9655018/diff/15098/content/common/DEPS#newcode2
content/common/DEPS:2: "+media/base/channel_layout.h",
nit: just do "+media/base"

[jam, 2012-03-20 15:50:52]

On 2012/03/20 15:29:45, John Abd-El-Malek wrote:
> why move from using the macros to do IPC serialization to doing it manually?
the
> macro approach is the preferred way. aside from writing less code, it's better
> for security as it avoids the chance of subtle errors

ok nm, I should have looked at the rest of the change in more details. I agree
class is better (what was there before was against the google style guide btw,
since structs can't have methods).

All the audio code though in media/audio needs to be in the "media" namespace
according to chrome style. how come none of it is in a namespace? since you're
touching this struct and updating all the callers, can you start by putting this
one in a namespace?

[jam, 2012-03-20 15:51:16]

On 2012/03/20 15:50:52, John Abd-El-Malek wrote:
> On 2012/03/20 15:29:45, John Abd-El-Malek wrote:
> > why move from using the macros to do IPC serialization to doing it manually?
> the
> > macro approach is the preferred way. aside from writing less code, it's
better
> > for security as it avoids the chance of subtle errors
> 
> ok nm, I should have looked at the rest of the change in more details. I agree
> class is better (what was there before was against the google style guide btw,
> since structs can't have methods).
> 
> All the audio code though in media/audio needs to be in the "media" namespace
> according to chrome style. how come none of it is in a namespace? since you're
> touching this struct and updating all the callers, can you start by putting
this
> one in a namespace?

btw some of my confusion was that I thought this was a struct in content because
it wasn't in the media namespace :)

[jam, 2012-03-20 15:58:47]

http://codereview.chromium.org/9655018/diff/15098/content/common/media/audio_...
File content/common/media/audio_param_traits.h (right):

http://codereview.chromium.org/9655018/diff/15098/content/common/media/audio_...
content/common/media/audio_param_traits.h:16: // These datatypes are used by
utility_messages.h and render_messages.h.
nit: did you mean to copy this comment?

[vrk (LEFT CHROMIUM), 2012-03-21 00:35:24]

Thanks jam!

On 2012/03/20 15:50:52, John Abd-El-Malek wrote:
> All the audio code though in media/audio needs to be in the "media" namespace
> according to chrome style. how come none of it is in a namespace?

Ha, scherkus says you have pointed this out before, and the result was this bug:
crbug.com/115187

> since you're
> touching this struct and updating all the callers, can you start by putting
this
> one in a namespace?

Eh, if I have to update all the callers again anyway (which includes a lot of
files in media/audio), might as well do the right thing instead -- in a
different CL! 

CL to move all media/audio files into a namespace is here:
http://codereview.chromium.org/9655018/

http://codereview.chromium.org/9655018/diff/15098/content/common/DEPS
File content/common/DEPS (right):

http://codereview.chromium.org/9655018/diff/15098/content/common/DEPS#newcode2
content/common/DEPS:2: "+media/base/channel_layout.h",
On 2012/03/20 15:30:19, John Abd-El-Malek wrote:
> nit: just do "+media/base"

Done.

http://codereview.chromium.org/9655018/diff/15098/content/common/media/audio_...
File content/common/media/audio_param_traits.h (right):

http://codereview.chromium.org/9655018/diff/15098/content/common/media/audio_...
content/common/media/audio_param_traits.h:16: // These datatypes are used by
utility_messages.h and render_messages.h.
On 2012/03/20 15:58:47, John Abd-El-Malek wrote:
> nit: did you mean to copy this comment?

gaah d'oh. I did not! Thanks for the catch, deleted!

[vrk (LEFT CHROMIUM), 2012-03-21 00:36:32]

Whoops, sent the wrong CL link:
https://chromiumcodereview.appspot.com/9805001/

[jam, 2012-03-21 04:31:01]

lgtm, thanks

[commit-bot: I haz the power, 2012-03-21 17:30:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/9655018/46001

[commit-bot: I haz the power, 2012-03-21 17:56:00]

Try job failure for 9655018-46001 (retry) on android for steps "Compile, build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-03-21 20:02:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vrk@chromium.org/9655018/56001

[commit-bot: I haz the power, 2012-03-21 20:03:49]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>