Introduce AudioHardwareConfig for renderer side audio device info.

Introduce AudioHardwareConfig for renderer side audio device info.

As discussed, this creates a media::AudioHardwareConfig class lazily created,
owned, and filled by RenderThreadImpl via a single synchronous IPC and updated
via AudioMessageFilter.

This change does not plumb part 2 where AudioRendererMixer recreates itself based
on these device change updates.

BUG=157216
TEST=New unittest. Device changes on Windows/Mac continue to work.

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

[DaleCurtis, 2013-01-29 03:01:46]

Still diagnosing a crash on OSX, but this is the gist of it. If all goes well I
intend to land this for M26 + part2 so tab mirroring works properly when firing
device changes.

[miu, 2013-01-29 04:55:43]

Nice work here.

BTW--I'm not sure this will have any impact on tab-level mirroring since it does
not change the audio output format when the "divert mechanism" in
AudioOutputController kicks in.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:193: DelegateMap::const_iterator
it = delegates_.find(stream_id);
Need to use delegates_lock_ here for thread-safety (see other OnXXX() methods).

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.cc:68: sample_rate, 16,
hardware_config_->GetOutputBufferSize());
It's not intuitive to me why we can ignore the hardware's requested sample rate
for Linux/ChromeOS (line 58 above), but not the output buffer size.  Isn't the
calculation of the latter based on the former?

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.h:86:
media::AudioHardwareConfig* hardware_config_;
nit: Tell the compiler the type is "media::AudioHardwareConfig* const" since
this member is never changed.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:79:
RendererAudioHardwareConfig* hardware_config =
Possible root cause of crash on Mac: If GetAudioHardwareConfig() is being called
on the test thread, is this the same thread as the one owning
ChildThread::current()->message_loop()?

Or, maybe there's a wrong-thread issue somewhere else, like in
WebRtcAudioRenderer::Initialize()?

https://codereview.chromium.org/11880009/diff/2002/content/renderer/render_th...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/render_th...
content/renderer/render_thread_impl.cc:907: if (!audio_hardware_config_)
Nice.  I was wondering whether a render process had to block on start-up when
audio is never used.

https://codereview.chromium.org/11880009/diff/2002/media/audio/audio_output_c...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/11880009/diff/2002/media/audio/audio_output_c...
media/audio/audio_output_controller.cc:335: handler_->OnDeviceChange(
Should this instead be:

  handler_->OnDeviceChange(
      this, params_.GetBytesPerBuffer(), params_.sample_rate());

I may be misunderstanding something, but here's what I see happening:

1. AudioOutputController::params_ is a constant.
2. When DoCreate() re-creates the stream, it will call
audio_manager_->MakeAudioOutputStreamProxy(params_);
3. AudioManager will use AudioOutputResampler and friends to handle mismatches
in what the hardware actually supports versus the params_ requested.

So, from the render process' point-of-view, it should keep supplying audio data
at the original sampling rate, buffer size, etc., right?

[henrika (OOO until Aug 14), 2013-01-29 10:46:49]

Thanks Dale!

I am not 100% convinced that you really need the lock in the AudioHardwareConfig
implementation.

Also, blocking the ctor duw to sync IPC of RenderAudioHardwareConfig feels a bit
risky.

https://codereview.chromium.org/11880009/diff/2002/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11880009/diff/2002/content/common/view_messag...
content/common/view_messages.h:727: media::ChannelLayout /* channel layout */)
input

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:186: if (MessageLoop::current()
!= ChildThread::current()->message_loop()) {
Please add add a comment on why this part is needed. ChildThread::current()
isn't that intuitive if you ask me.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:197:
RenderThreadImpl::current()->GetAudioHardwareConfig()->UpdateOutputConfig(
I always use DCHECK in these types of situations.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
File content/renderer/media/renderer_audio_hardware_config.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
content/renderer/media/renderer_audio_hardware_config.cc:18:
RenderThreadImpl::current()->Send(new ViewHostMsg_GetAudioHardwareConfig(
Is it OK to call a sync IPC from a ctor? It just feels a bit risky if you ask
me.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
File content/renderer/media/renderer_audio_hardware_config.h (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
content/renderer/media/renderer_audio_hardware_config.h:8: #include
"base/compiler_specific.h"
Why do you need this one here?

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
content/renderer/media/renderer_audio_hardware_config.h:26:
RendererAudioHardwareConfig(int output_buffer_size, int output_sample_rate,
Should it be public?

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
content/renderer/media/renderer_audio_hardware_config.h:41: void
UpdateInputConfig(int sample_rate, media::ChannelLayout channel_layout);
OK, now I get it. Seen as implementation detail and moved from the interface.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
content/renderer/media/renderer_audio_hardware_config.h:46: base::Lock
config_lock_;
How is this class used today? Are all methods only called from the render thread
and if so do we really need the lock?

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/web...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/web...
content/renderer/media/webrtc_audio_renderer.cc:110: // This request is based on
a synchronous IPC message.
Guess this comment is not 100% any longer since we can't say for sure that the
all will render an IPC message.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/render_th...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/render_th...
content/renderer/render_thread_impl.h:251: // renderer side clients.  Creation
requires a synchronous IPC call so it is
Nit, does the sync IPC require lacy init?

https://codereview.chromium.org/11880009/diff/2002/content/test/webrtc_audio_...
File content/test/webrtc_audio_device_test.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/test/webrtc_audio_...
content/test/webrtc_audio_device_test.cc:303:
IPC_MESSAGE_HANDLER(ViewHostMsg_GetAudioHardwareConfig,
Nice to need on only one message here!

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

https://codereview.chromium.org/11880009/diff/2002/media/base/audio_hardware_...
media/base/audio_hardware_config.h:23: 
Two methods are excluded (UpdateOutput/InputConfig) compared with the design
doc. I assume there is a good reason for it.

[DaleCurtis, 2013-01-29 19:36:00]

(just comments)

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:186: if (MessageLoop::current()
!= ChildThread::current()->message_loop()) {
On 2013/01/29 10:46:49, henrika wrote:
> Please add add a comment on why this part is needed. ChildThread::current()
> isn't that intuitive if you ask me.

Yeah, I don't really like the way this is currently, it obviates the
"on-any-thread" property of AudioHardwareConfig in a bad way.  I'd prefer if
AudioMessageFilter already had a pointer to the AudioHardwareConfig by this
point. However we don't want a synchronous IPC issued during construction of
AudioMessageFilter (and thus RenderThreadImpl).

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.cc:68: sample_rate, 16,
hardware_config_->GetOutputBufferSize());
On 2013/01/29 04:55:43, Yuri wrote:
> It's not intuitive to me why we can ignore the hardware's requested sample
rate
> for Linux/ChromeOS (line 58 above), but not the output buffer size.  Isn't the
> calculation of the latter based on the former?

Not on Linux, it's just a fixed 2048.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
File content/renderer/media/renderer_audio_hardware_config.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
content/renderer/media/renderer_audio_hardware_config.cc:18:
RenderThreadImpl::current()->Send(new ViewHostMsg_GetAudioHardwareConfig(
On 2013/01/29 10:46:49, henrika wrote:
> Is it OK to call a sync IPC from a ctor? It just feels a bit risky if you ask
> me.

Why do you think it's riskier? Also what's riskier vs. calling a synchronous IPC
in each method as before?  The idea is that the object is fully constructed with
valid values and can then be safely called from any thread.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
File content/renderer/media/renderer_audio_hardware_config.h (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
content/renderer/media/renderer_audio_hardware_config.h:46: base::Lock
config_lock_;
On 2013/01/29 10:46:49, henrika wrote:
> How is this class used today? Are all methods only called from the render
thread
> and if so do we really need the lock? 

AudioRendererMixerManager::GetMixer() may be called from the media thread, which
is why this class offers thread safety.  Currently, this patch set always calls
from the render thread, but ideally AudioMessageFilter can call the update
method from the IO thread thanks to this lock.

https://codereview.chromium.org/11880009/diff/2002/media/audio/audio_output_c...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/11880009/diff/2002/media/audio/audio_output_c...
media/audio/audio_output_controller.cc:335: handler_->OnDeviceChange(
On 2013/01/29 04:55:43, Yuri wrote:
> Should this instead be:
> 
>   handler_->OnDeviceChange(
>       this, params_.GetBytesPerBuffer(), params_.sample_rate());
> 
> I may be misunderstanding something, but here's what I see happening:
> 
> 1. AudioOutputController::params_ is a constant.
> 2. When DoCreate() re-creates the stream, it will call
> audio_manager_->MakeAudioOutputStreamProxy(params_);
> 3. AudioManager will use AudioOutputResampler and friends to handle mismatches
> in what the hardware actually supports versus the params_ requested.
> 
> So, from the render process' point-of-view, it should keep supplying audio
data
> at the original sampling rate, buffer size, etc., right?

No, the purpose of this call is to update the renderer's cached hardware
configuration values.  In the future AudioRendererMixer will recreate based on
these values.

params_ is already known to the renderer, so issuing the call as you describe it
provides no information to the client.

[miu, 2013-01-29 22:04:34]

Justin Lin filled me in on the design discussion over lunch today, so I think I
understand the ramifications of this change better now.  ;-)

https://codereview.chromium.org/11880009/diff/2002/media/audio/audio_output_c...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/11880009/diff/2002/media/audio/audio_output_c...
media/audio/audio_output_controller.cc:335: handler_->OnDeviceChange(
On 2013/01/29 19:36:00, DaleCurtis wrote:
> No, the purpose of this call is to update the renderer's cached hardware
> configuration values.  In the future AudioRendererMixer will recreate based on
> these values.

Okay, makes sense to me now.  And now the TODO comment makes more sense: I would
have expected AudioManager to push the device change notifications to each
render process.  However, with the media/content component boundaries, this
would be a lot of code.

[DaleCurtis, 2013-01-30 01:31:05]

PTAL. This change collapses the interface and implementation of
AudioHardwareConfig in favor of having RenderThreadImpl handle the one IPC
related item.

I've also added an AudioMessageFilter::SetAudioHardwareConfig() method in favor
of hopping threads to get at the hardware config.  This has the further
advantage of allowing us to skip updates if an audio hardware config hasn't been
created (high latency html5 path today).

https://codereview.chromium.org/11880009/diff/2002/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11880009/diff/2002/content/common/view_messag...
content/common/view_messages.h:727: media::ChannelLayout /* channel layout */)
On 2013/01/29 10:46:49, henrika wrote:
> input

Done.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:186: if (MessageLoop::current()
!= ChildThread::current()->message_loop()) {
On 2013/01/29 19:36:00, DaleCurtis wrote:
> On 2013/01/29 10:46:49, henrika wrote:
> > Please add add a comment on why this part is needed. ChildThread::current()
> > isn't that intuitive if you ask me.
> 
> Yeah, I don't really like the way this is currently, it obviates the
> "on-any-thread" property of AudioHardwareConfig in a bad way.  I'd prefer if
> AudioMessageFilter already had a pointer to the AudioHardwareConfig by this
> point. However we don't want a synchronous IPC issued during construction of
> AudioMessageFilter (and thus RenderThreadImpl).

Removed all this in favor of having RenderThreadImpl provide the
RendererAudioHardwareConfig when created. This avoids any weird ChildThread
stuff.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:193: DelegateMap::const_iterator
it = delegates_.find(stream_id);
On 2013/01/29 04:55:43, Yuri wrote:
> Need to use delegates_lock_ here for thread-safety (see other OnXXX()
methods).

Removed since this function doesn't really need that.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:197:
RenderThreadImpl::current()->GetAudioHardwareConfig()->UpdateOutputConfig(
On 2013/01/29 10:46:49, henrika wrote:
> I always use DCHECK in these types of situations. 

Removed.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.h:86:
media::AudioHardwareConfig* hardware_config_;
On 2013/01/29 04:55:43, Yuri wrote:
> nit: Tell the compiler the type is "media::AudioHardwareConfig* const" since
> this member is never changed.

Done.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
File content/renderer/media/renderer_audio_hardware_config.h (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
content/renderer/media/renderer_audio_hardware_config.h:8: #include
"base/compiler_specific.h"
On 2013/01/29 10:46:49, henrika wrote:
> Why do you need this one here?

Necessary for the DISALLOW_COPY_AND_ASSIGN() macro.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
content/renderer/media/renderer_audio_hardware_config.h:26:
RendererAudioHardwareConfig(int output_buffer_size, int output_sample_rate,
On 2013/01/29 10:46:49, henrika wrote:
> Should it be public?

Easier if it is. Otherwise there's a bunch of ugly friend testing necessary. I
could go either way.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/web...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/web...
content/renderer/media/webrtc_audio_device_unittest.cc:79:
RendererAudioHardwareConfig* hardware_config =
On 2013/01/29 04:55:43, Yuri wrote:
> Possible root cause of crash on Mac: If GetAudioHardwareConfig() is being
called
> on the test thread, is this the same thread as the one owning
> ChildThread::current()->message_loop()?
> 
> Or, maybe there's a wrong-thread issue somewhere else, like in
> WebRtcAudioRenderer::Initialize()?

Crash is in Chrome not tests, may be related to http://crbug.com/158170. Still
digging there. I've removed the ChildThread stuff now.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/web...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/web...
content/renderer/media/webrtc_audio_renderer.cc:110: // This request is based on
a synchronous IPC message.
On 2013/01/29 10:46:49, henrika wrote:
> Guess this comment is not 100% any longer since we can't say for sure that the
> all will render an IPC message.

Done.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/render_th...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/render_th...
content/renderer/render_thread_impl.h:251: // renderer side clients.  Creation
requires a synchronous IPC call so it is
On 2013/01/29 10:46:49, henrika wrote:
> Nit, does the sync IPC require lacy init?

What do you mean? We use lazy initialization because of the synchronous IPC, we
don't want to block render thread startup.

https://codereview.chromium.org/11880009/diff/2002/media/audio/audio_output_c...
File media/audio/audio_output_controller.cc (right):

https://codereview.chromium.org/11880009/diff/2002/media/audio/audio_output_c...
media/audio/audio_output_controller.cc:335: handler_->OnDeviceChange(
On 2013/01/29 22:04:34, Yuri wrote:
> On 2013/01/29 19:36:00, DaleCurtis wrote:
> > No, the purpose of this call is to update the renderer's cached hardware
> > configuration values.  In the future AudioRendererMixer will recreate based
on
> > these values.
> 
> Okay, makes sense to me now.  And now the TODO comment makes more sense: I
would
> have expected AudioManager to push the device change notifications to each
> render process.  However, with the media/content component boundaries, this
> would be a lot of code.

Yeah, I suspect we need to plumb the values a la the TODO for Tab mirroring to
work.  Since it wants values which are not actually the hardware values.

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

https://codereview.chromium.org/11880009/diff/2002/media/base/audio_hardware_...
media/base/audio_hardware_config.h:23: 
On 2013/01/29 10:46:49, henrika wrote:
> Two methods are excluded (UpdateOutput/InputConfig) compared with the design
> doc. I assume there is a good reason for it.

You brought up a good point and with some creativity, I've collapsed
RendererAudioHardwareConfig and AudioHardwareConfig into one.  Now
RenderThreadImpl takes care of the render-thread-ish parts and
AudioHardwareConfig is just a dumb thread safe wrapper class.

[henrika (OOO until Aug 14), 2013-01-30 10:10:06]

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
File content/renderer/media/renderer_audio_hardware_config.cc (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/media/ren...
content/renderer/media/renderer_audio_hardware_config.cc:18:
RenderThreadImpl::current()->Send(new ViewHostMsg_GetAudioHardwareConfig(
I am just used to always making as lightweight tasks as possible during
constructions. That's all. I see your point.

https://codereview.chromium.org/11880009/diff/2002/content/renderer/render_th...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/11880009/diff/2002/content/renderer/render_th...
content/renderer/render_thread_impl.h:251: // renderer side clients.  Creation
requires a synchronous IPC call so it is
It was not clear to me if "sync IPC ===> lazy init" and what the exact
dependence was. I think I understand what you mean now. It was just how the
comment was formulated that made me confused.

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

https://codereview.chromium.org/11880009/diff/2002/media/base/audio_hardware_...
media/base/audio_hardware_config.h:23: 
Great.

[henrika (OOO until Aug 14), 2013-01-30 10:27:58]

Like the new version, especially the RenderThreadImpl::GetAudioHardwareConfig()
parts. Nice, clean and much more easy to follow now.

LGTM

[henrika (OOO until Aug 14), 2013-01-30 10:28:13]

https://codereview.chromium.org/11880009/diff/2007/content/renderer/render_th...
File content/renderer/render_thread_impl.cc (right):

https://codereview.chromium.org/11880009/diff/2007/content/renderer/render_th...
content/renderer/render_thread_impl.cc:920:
audio_message_filter_->SetAudioHardwareConfig(audio_hardware_config_.get());
Nice!

[miu, 2013-01-30 21:22:23]

lgtm

[scherkus (not reviewing), 2013-01-31 02:33:33]

lgtm w/ nits + q

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:187: // If an audio hardware
config hasn't been created, ignore the message.
nit: a more informative comment would suggest when this is possible (if it's
even possible!)

For example ... it looks like renderer processes only get sent this message if
there's an AudioEntry in AudioRendererHost that has this stream. In order for
that to be true, then someone must have created a stream first. Can we guarantee
that if a renderer has requested to create a stream that a_h_c_ has been
created?

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.h:40:
AudioRendererMixerManager(media::AudioHardwareConfig* hardware_config);
raw pointer scares me -- care to document the lifetime + ownership situation?

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

https://codereview.chromium.org/11880009/diff/2007/media/base/audio_hardware_...
media/base/audio_hardware_config.h:16: // renderer side.
nit: renderer isn't important here, but rather that it's a thread safe container
for audio hardware configuation

https://codereview.chromium.org/11880009/diff/2007/media/base/audio_hardware_...
File media/base/audio_hardware_config_unittest.cc (right):

https://codereview.chromium.org/11880009/diff/2007/media/base/audio_hardware_...
media/base/audio_hardware_config_unittest.cc:10: namespace {
nit: us media folks prefer static as opposed to anon namespaces

alternatively you can also use enums for integer constants

[miu, 2013-01-31 05:28:53]

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.h:40:
AudioRendererMixerManager(media::AudioHardwareConfig* hardware_config);
Drive-by comment: Add 'explicit' keyword here.

[DaleCurtis, 2013-01-31 19:50:00]

Yuri: This patch set includes the change to
AudioOutputController::OnDeviceChange we discussed yesterday.

+jam for non-media content/ review
+cevans for *_messages review.

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
content/renderer/media/audio_message_filter.cc:187: // If an audio hardware
config hasn't been created, ignore the message.
On 2013/01/31 02:33:33, scherkus wrote:
> nit: a more informative comment would suggest when this is possible (if it's
> even possible!)
> 
> For example ... it looks like renderer processes only get sent this message if
> there's an AudioEntry in AudioRendererHost that has this stream. In order for
> that to be true, then someone must have created a stream first. Can we
guarantee
> that if a renderer has requested to create a stream that a_h_c_ has been
> created?

Not until we remove the high latency audio path and flags like
--disable-renderer-side-mixing -- which I'd like to do the milestone after Linux
renderer side mixing lands and looks good.  I've updated the comment to include
this information for now.

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.h:40:
AudioRendererMixerManager(media::AudioHardwareConfig* hardware_config);
On 2013/01/31 05:28:53, Yuri wrote:
> Drive-by comment: Add 'explicit' keyword here.

Done.

https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
content/renderer/media/audio_renderer_mixer_manager.h:40:
AudioRendererMixerManager(media::AudioHardwareConfig* hardware_config);
On 2013/01/31 02:33:33, scherkus wrote:
> raw pointer scares me -- care to document the lifetime + ownership situation?

Done.

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

https://codereview.chromium.org/11880009/diff/2007/media/base/audio_hardware_...
media/base/audio_hardware_config.h:16: // renderer side.
On 2013/01/31 02:33:33, scherkus wrote:
> nit: renderer isn't important here, but rather that it's a thread safe
container
> for audio hardware configuation

Done.

https://codereview.chromium.org/11880009/diff/2007/media/base/audio_hardware_...
File media/base/audio_hardware_config_unittest.cc (right):

https://codereview.chromium.org/11880009/diff/2007/media/base/audio_hardware_...
media/base/audio_hardware_config_unittest.cc:10: namespace {
On 2013/01/31 02:33:33, scherkus wrote:
> nit: us media folks prefer static as opposed to anon namespaces
> 
> alternatively you can also use enums for integer constants

Done.

[Chris Evans, 2013-01-31 19:57:33]

On 2013/01/31 19:50:00, DaleCurtis wrote:
> Yuri: This patch set includes the change to
> AudioOutputController::OnDeviceChange we discussed yesterday.
> 
> +jam for non-media content/ review
> +cevans for *_messages review.
> 
>
https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
> File content/renderer/media/audio_message_filter.cc (right):
> 
>
https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
> content/renderer/media/audio_message_filter.cc:187: // If an audio hardware
> config hasn't been created, ignore the message.
> On 2013/01/31 02:33:33, scherkus wrote:
> > nit: a more informative comment would suggest when this is possible (if it's
> > even possible!)
> > 
> > For example ... it looks like renderer processes only get sent this message
if
> > there's an AudioEntry in AudioRendererHost that has this stream. In order
for
> > that to be true, then someone must have created a stream first. Can we
> guarantee
> > that if a renderer has requested to create a stream that a_h_c_ has been
> > created?
> 
> Not until we remove the high latency audio path and flags like
> --disable-renderer-side-mixing -- which I'd like to do the milestone after
Linux
> renderer side mixing lands and looks good.  I've updated the comment to
include
> this information for now.
> 
>
https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
> File content/renderer/media/audio_renderer_mixer_manager.h (right):
> 
>
https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
> content/renderer/media/audio_renderer_mixer_manager.h:40:
> AudioRendererMixerManager(media::AudioHardwareConfig* hardware_config);
> On 2013/01/31 05:28:53, Yuri wrote:
> > Drive-by comment: Add 'explicit' keyword here.
> 
> Done.
> 
>
https://codereview.chromium.org/11880009/diff/2007/content/renderer/media/aud...
> content/renderer/media/audio_renderer_mixer_manager.h:40:
> AudioRendererMixerManager(media::AudioHardwareConfig* hardware_config);
> On 2013/01/31 02:33:33, scherkus wrote:
> > raw pointer scares me -- care to document the lifetime + ownership
situation?
> 
> Done.
> 
>
https://codereview.chromium.org/11880009/diff/2007/media/base/audio_hardware_...
> File media/base/audio_hardware_config.h (right):
> 
>
https://codereview.chromium.org/11880009/diff/2007/media/base/audio_hardware_...
> media/base/audio_hardware_config.h:16: // renderer side.
> On 2013/01/31 02:33:33, scherkus wrote:
> > nit: renderer isn't important here, but rather that it's a thread safe
> container
> > for audio hardware configuation
> 
> Done.
> 
>
https://codereview.chromium.org/11880009/diff/2007/media/base/audio_hardware_...
> File media/base/audio_hardware_config_unittest.cc (right):
> 
>
https://codereview.chromium.org/11880009/diff/2007/media/base/audio_hardware_...
> media/base/audio_hardware_config_unittest.cc:10: namespace {
> On 2013/01/31 02:33:33, scherkus wrote:
> > nit: us media folks prefer static as opposed to anon namespaces
> > 
> > alternatively you can also use enums for integer constants
> 
> Done.

LGTM

[DaleCurtis, 2013-01-31 21:29:46]

Whoops, cevans, apparently you aren't an owner for
content/common/view_messages.h +jschuh for that one.

[jschuh, 2013-01-31 21:34:41]

rubberstamp lgtm. blame it on cevans if it's broke. ;)

[miu, 2013-01-31 22:28:08]

lgtm for audio_output_controller.cc change to not send change events on
diverting start/stop.

[jam, 2013-02-01 00:21:33]

lgtm

https://codereview.chromium.org/11880009/diff/2012/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11880009/diff/2012/content/common/view_messag...
content/common/view_messages.h:727: media::ChannelLayout /* input channel layout
*/)
nit: here and in other ipc files, the convention is to put the parameter name
from the dispatching method inside the comment. so no spaces.

[DaleCurtis, 2013-02-01 00:25:50]

Thanks for review guys.  CQ'ing.

https://codereview.chromium.org/11880009/diff/2012/content/common/view_messag...
File content/common/view_messages.h (right):

https://codereview.chromium.org/11880009/diff/2012/content/common/view_messag...
content/common/view_messages.h:727: media::ChannelLayout /* input channel layout
*/)
On 2013/02/01 00:21:33, jam wrote:
> nit: here and in other ipc files, the convention is to put the parameter name
> from the dispatching method inside the comment. so no spaces.

Done.

[commit-bot: I haz the power, 2013-02-01 00:43:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dalecurtis@chromium.org/11880009/37001

[commit-bot: I haz the power, 2013-02-01 04:42:42]

Change committed as 180068