Getting rid of AudioHardwareConfig and its synchronous IPC.

Getting rid of AudioHardwareConfig and its synchronous IPC.

Switching to AudioDeviceFactory::GetOutputDeviceInfo() instead.

BUG=611404, 549125, 422522
Committed: https://crrev.com/a148aeb2bc27a99f25b97931dd94e28f2eab6f1d
Cr-Commit-Position: refs/heads/master@{#405737}

[o1ka, 2016-07-05 14:13:47]

Description was changed from

==========
Getting rid of AudioHardwareConfig and its synchronous IPC.

Switching to AudioDeviceFactory::GetOutputDeviceInfo() instead.
AudioRendererSinkCache is modified to cache and refresh default output device
information.

BUG=611404,549125,422522
==========

to

==========
Getting rid of AudioHardwareConfig and its synchronous IPC.

Switching to AudioDeviceFactory::GetOutputDeviceInfo() instead.
AudioRendererSinkCache is modified to cache and refresh default output device
information.

Follow up is to switch blink to provide the actual renderer frame id and
security origin and to take into account/report output device status.

BUG=611404,549125,422522
==========

[o1ka, 2016-07-05 14:13:47]

olka@chromium.org changed reviewers:
+ dalecurtis@chromium.org, guidou@chromium.org

[o1ka, 2016-07-05 14:14:41]

PTAL

[Guido Urdaneta, 2016-07-05 15:10:39]

https://codereview.chromium.org/2120273004/diff/20001/content/renderer/media/...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/2120273004/diff/20001/content/renderer/media/...
content/renderer/media/audio_renderer_sink_cache_impl.cc:59: CacheEntry
cache_entry = {source_render_frame_id,
Should this have a thread check to make sure there are no races?

https://codereview.chromium.org/2120273004/diff/20001/content/renderer/media/...
content/renderer/media/audio_renderer_sink_cache_impl.cc:89: false /*
unused_only */) == cache_.end());
the /* unused_only */ comment does not add any information to me.

[Guido Urdaneta, 2016-07-05 15:16:20]

lgtm

https://codereview.chromium.org/2120273004/diff/20001/content/renderer/media/...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/2120273004/diff/20001/content/renderer/media/...
content/renderer/media/audio_renderer_sink_cache_impl.cc:59: CacheEntry
cache_entry = {source_render_frame_id,
On 2016/07/05 15:10:39, Guido Urdaneta wrote:
> Should this have a thread check to make sure there are no races?

Forget about this. I didn't see the lock.

[o1ka, 2016-07-05 15:16:38]

https://codereview.chromium.org/2120273004/diff/20001/content/renderer/media/...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/2120273004/diff/20001/content/renderer/media/...
content/renderer/media/audio_renderer_sink_cache_impl.cc:59: CacheEntry
cache_entry = {source_render_frame_id,
On 2016/07/05 15:10:39, Guido Urdaneta wrote:
> Should this have a thread check to make sure there are no races?

It's all protected by |cache_lock_| :)

[o1ka, 2016-07-06 12:13:17]

Patchset #3 (id:40001) has been deleted

[o1ka, 2016-07-06 13:18:28]

PTAL: Now renderer frame id fixed.

[Guido Urdaneta, 2016-07-06 14:16:16]

still lgtm % typo

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
File content/renderer/media/audio_device_factory.h (right):

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
content/renderer/media/audio_device_factory.h:51: // TODO(olka, guidou): replace
with actulal frame id everywhere.
fix typo: s/actulal/actual

[DaleCurtis, 2016-07-06 18:53:37]

https://codereview.chromium.org/2120273004/diff/30027/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/2120273004/diff/30027/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:241: return NULL;
Remove this since no need anymore.

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
File content/renderer/media/audio_device_factory.h (right):

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
content/renderer/media/audio_device_factory.h:52: enum {
kUnknownFrameIdForDefaultDevice = 1 };
Why do we need this? Should we instead just have a method which doesn't take a
render frame id?

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
content/renderer/media/audio_renderer_sink_cache_impl.cc:114: // Cache
information for default output device.
Do we want to cache this? We'll carry over bugs around plugging in new default
devices when a renderer has been created and audio started. I.e., old parameters
will be used.

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
content/renderer/media/audio_renderer_sink_cache_impl.h:96: // device is always
allowed, this information can be cashed and shared among
cached.

[miu, 2016-07-06 19:10:57]

miu@chromium.org changed reviewers:
+ miu@chromium.org

[miu, 2016-07-06 19:10:57]

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
File content/renderer/media/audio_device_factory.h (right):

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
content/renderer/media/audio_device_factory.h:52: enum {
kUnknownFrameIdForDefaultDevice = 1 };
On 2016/07/06 18:53:36, DaleCurtis wrote:
> Why do we need this? Should we instead just have a method which doesn't take a
> render frame id?

Not sure if I understand everything going on here, but we should never allow
audio output from a render frame without associating the output stream with the
frame ID. Otherwise, several important browser-level features will break: the
audio indicator, tab OOM-killer management, tab mute, tab capture, etc.

What worries me is that we currently need this "unknown" enum. That infers there
is already broken code creating audio output without specifying its source
render frame.

IIRC, I had a DCHECK in AudioRendererHost to prevent streams with invalid render
frame IDs from being created. Did someone get rid of that?

[chromium-reviews, 2016-07-06 20:19:01]

We do not allow output with a random frame id. We just use some frame id to
create a default output device and get its parameters. This device will be
used for output only if it is requested by somebody holding that frame id.
Otherwise it will be garbage - collected. DCHECK is still in place, I tried
to use ROUTING_NONE for this"no frame id" purpose and hit DCHECK :)
On Jul 6, 2016 21:10, <miu@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
> File content/renderer/media/audio_device_factory.h (right):
>
>
>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
> content/renderer/media/audio_device_factory.h:52: enum {
> kUnknownFrameIdForDefaultDevice = 1 };
> On 2016/07/06 18:53:36, DaleCurtis wrote:
> > Why do we need this? Should we instead just have a method which
> doesn't take a
> > render frame id?
>
> Not sure if I understand everything going on here, but we should never
> allow audio output from a render frame without associating the output
> stream with the frame ID. Otherwise, several important browser-level
> features will break: the audio indicator, tab OOM-killer management, tab
> mute, tab capture, etc.
>
> What worries me is that we currently need this "unknown" enum. That
> infers there is already broken code creating audio output without
> specifying its source render frame.
>
> IIRC, I had a DCHECK in AudioRendererHost to prevent streams with
> invalid render frame IDs from being created. Did someone get rid of
> that?
>
> https://codereview.chromium.org/2120273004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-07-06 20:27:03]

It's a question if we want to cache default parameters. I would cache them
for now, because getting new ones is pretty expensive (and includes device
authorisation). We do not cache them forever, they are updated each time
new default device is requested, so it's already an improvement. I'm adding
cache stats in another CL, after that we can fine-tune garbage collection
and hopefully remove default parameter caching. I think it's a bit smoother
transition than switching to constant default device creation right now.
WDYT?
On Jul 6, 2016 20:53, <dalecurtis@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/2120273004/diff/30027/content/browser/rendere...
> File content/browser/renderer_host/render_message_filter.cc (right):
>
>
>
https://codereview.chromium.org/2120273004/diff/30027/content/browser/rendere...
> content/browser/renderer_host/render_message_filter.cc:241: return NULL;
> Remove this since no need anymore.
>
>
>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
> File content/renderer/media/audio_device_factory.h (right):
>
>
>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
> content/renderer/media/audio_device_factory.h:52: enum {
> kUnknownFrameIdForDefaultDevice = 1 };
> Why do we need this? Should we instead just have a method which doesn't
> take a render frame id?
>
>
>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
> File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):
>
>
>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
> content/renderer/media/audio_renderer_sink_cache_impl.cc:114: // Cache
> information for default output device.
> Do we want to cache this? We'll carry over bugs around plugging in new
> default devices when a renderer has been created and audio started.
> I.e., old parameters will be used.
>
>
>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
> File content/renderer/media/audio_renderer_sink_cache_impl.h (right):
>
>
>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
> content/renderer/media/audio_renderer_sink_cache_impl.h:96: // device is
> always allowed, this information can be cashed and shared among
> cached.
>
> https://codereview.chromium.org/2120273004/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2016-07-06 20:29:32]

Or we can do all the way around: skip caching defaults and look at the
stats first...
On Jul 6, 2016 22:27, "Olga Sharonova" <olka@google.com> wrote:

> It's a question if we want to cache default parameters. I would cache them
> for now, because getting new ones is pretty expensive (and includes device
> authorisation). We do not cache them forever, they are updated each time
> new default device is requested, so it's already an improvement. I'm adding
> cache stats in another CL, after that we can fine-tune garbage collection
> and hopefully remove default parameter caching. I think it's a bit smoother
> transition than switching to constant default device creation right now.
> WDYT?
> On Jul 6, 2016 20:53, <dalecurtis@chromium.org> wrote:
>
>>
>>
>>
https://codereview.chromium.org/2120273004/diff/30027/content/browser/rendere...
>> File content/browser/renderer_host/render_message_filter.cc (right):
>>
>>
>>
https://codereview.chromium.org/2120273004/diff/30027/content/browser/rendere...
>> content/browser/renderer_host/render_message_filter.cc:241: return NULL;
>> Remove this since no need anymore.
>>
>>
>>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
>> File content/renderer/media/audio_device_factory.h (right):
>>
>>
>>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
>> content/renderer/media/audio_device_factory.h:52: enum {
>> kUnknownFrameIdForDefaultDevice = 1 };
>> Why do we need this? Should we instead just have a method which doesn't
>> take a render frame id?
>>
>>
>>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
>> File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):
>>
>>
>>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
>> content/renderer/media/audio_renderer_sink_cache_impl.cc:114: // Cache
>> information for default output device.
>> Do we want to cache this? We'll carry over bugs around plugging in new
>> default devices when a renderer has been created and audio started.
>> I.e., old parameters will be used.
>>
>>
>>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
>> File content/renderer/media/audio_renderer_sink_cache_impl.h (right):
>>
>>
>>
https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
>> content/renderer/media/audio_renderer_sink_cache_impl.h:96: // device is
>> always allowed, this information can be cashed and shared among
>> cached.
>>
>> https://codereview.chromium.org/2120273004/
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[miu, 2016-07-06 21:53:08]

On 2016/07/06 20:19:01, chromium-reviews wrote:
> We do not allow output with a random frame id. We just use some frame id to
> create a default output device and get its parameters. This device will be
> used for output only if it is requested by somebody holding that frame id.
> Otherwise it will be garbage - collected. DCHECK is still in place, I tried
> to use ROUTING_NONE for this"no frame id" purpose and hit DCHECK :)

OOC, why do we need to create an output device to get the default output
parameters? If ARH is starting audio (AudioRendererHost::DoCreateStream()),
there is the potential for users to hear a "pop" when their audio system turns
on/off. (For example, on my cheap personal laptop at home, I can hear the laptop
turn on/off the amplifier for the speakers.)

Perhaps a better solution would be to add an IPC for querying the default audio
parameters?

[o1ka, 2016-07-07 06:30:28]

On 2016/07/06 21:53:08, miu wrote:
> On 2016/07/06 20:19:01, chromium-reviews wrote:
> > We do not allow output with a random frame id. We just use some frame id to
> > create a default output device and get its parameters. This device will be
> > used for output only if it is requested by somebody holding that frame id.
> > Otherwise it will be garbage - collected. DCHECK is still in place, I tried
> > to use ROUTING_NONE for this"no frame id" purpose and hit DCHECK :)
> 
> OOC, why do we need to create an output device to get the default output
> parameters? If ARH is starting audio (AudioRendererHost::DoCreateStream()),
> there is the potential for users to hear a "pop" when their audio system turns
> on/off. (For example, on my cheap personal laptop at home, I can hear the
laptop
> turn on/off the amplifier for the speakers.)
> 
> Perhaps a better solution would be to add an IPC for querying the default
audio
> parameters?

"an IPC for querying the default audio parameters"  - we've got one(for default
device only), it's what AudioHardwareConfig has been using up to this CL.
What sink cache is about: if there is no sink for a device cached yet, a new one
is created and cached, and usually is reused later for output.
The idea is to save up on IPC, because the course of action is usually: default
parameters of a device are requested, output parameters are calculated based on
that, output sink for the device is requested/initialized, playout starts. (This
is probably not ideal, but this is how all the code works now.)

Also, re: random render frame id: As TODO is the code says, it will be fixed to
plumb the actual frame id (guidou@ will implement blink part, and I'll take care
of pepper).

[o1ka, 2016-07-07 14:50:45]

Summing up:

1) After a discussion with giudou@ we decided to first land a CL which replaces
AudioHardwareConfig usage with AudioDeviceFactory::GetOutputDeviceInfo() in
blink and WebRtcLocalSourceProvider. It will also plumb the correct render frame
id (WIP now by guidou@).

2) Then this CL is rebased to 1), and I'm getting rid of both default device
parameters caching and kUnknownFrameIdForDefaultDevice - not needed any more.

3) There is also a CL coming (https://codereview.chromium.org/2124503002/) which
introduces UMA stats for sink cache utilization. It will help to fine-tune
garbage collection timeout and make sure GetOutputDeviceInfo() does not create
devices (especially default ones) all the time.

I'll let you know when a new patch is ready - thanks for the feedback!

[miu, 2016-07-08 01:43:30]

On 2016/07/07 06:30:28, o1ka wrote:
> On 2016/07/06 21:53:08, miu wrote:
> > Perhaps a better solution would be to add an IPC for querying the default
> audio
> > parameters?
> 
> "an IPC for querying the default audio parameters"  - we've got one(for
default
> device only), it's what AudioHardwareConfig has been using up to this CL.
> ...
> 
> Also, re: random render frame id: As TODO is the code says, it will be fixed
to
> plumb the actual frame id (guidou@ will implement blink part, and I'll take
care
> of pepper).

Ok, SGTM.

[o1ka, 2016-07-13 09:52:41]

PTAL:
Rebased to guidou@ changes, dropped unknown frame id and default device param
caching.

https://codereview.chromium.org/2120273004/diff/30027/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (right):

https://codereview.chromium.org/2120273004/diff/30027/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:241: return NULL;
On 2016/07/06 18:53:36, DaleCurtis wrote:
> Remove this since no need anymore.

Done.

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
File content/renderer/media/audio_device_factory.h (right):

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
content/renderer/media/audio_device_factory.h:52: enum {
kUnknownFrameIdForDefaultDevice = 1 };
On 2016/07/06 18:53:36, DaleCurtis wrote:
> Why do we need this? Should we instead just have a method which doesn't take a
> render frame id?

removed

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
File content/renderer/media/audio_renderer_sink_cache_impl.cc (right):

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
content/renderer/media/audio_renderer_sink_cache_impl.cc:114: // Cache
information for default output device.
On 2016/07/06 18:53:37, DaleCurtis wrote:
> Do we want to cache this? We'll carry over bugs around plugging in new default
> devices when a renderer has been created and audio started. I.e., old
parameters
> will be used.

removed

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
File content/renderer/media/audio_renderer_sink_cache_impl.h (right):

https://codereview.chromium.org/2120273004/diff/30027/content/renderer/media/...
content/renderer/media/audio_renderer_sink_cache_impl.h:96: // device is always
allowed, this information can be cashed and shared among
On 2016/07/06 18:53:37, DaleCurtis wrote:
> cached.

n/a any more

[o1ka, 2016-07-13 10:00:28]

olka@chromium.org changed reviewers:
+ avi@chromium.org

[o1ka, 2016-07-13 10:00:29]

avi@chromium.org: Please review
content/browser/renderer_host/render_message_filter.*
content/renderer/pepper/* content/renderer/renderer_thread_impl.*

kenrb@chromium.org: Please review  content/common/view_messages.h

guidou@ - still lgtm?

[o1ka, 2016-07-13 11:20:36]

Description was changed from

==========
Getting rid of AudioHardwareConfig and its synchronous IPC.

Switching to AudioDeviceFactory::GetOutputDeviceInfo() instead.
AudioRendererSinkCache is modified to cache and refresh default output device
information.

Follow up is to switch blink to provide the actual renderer frame id and
security origin and to take into account/report output device status.

BUG=611404,549125,422522
==========

to

==========
Getting rid of AudioHardwareConfig and its synchronous IPC.

Switching to AudioDeviceFactory::GetOutputDeviceInfo() instead.


BUG=611404,549125,422522
==========

[Guido Urdaneta, 2016-07-13 11:52:36]

still lgtm

[Avi (use Gerrit), 2016-07-13 15:57:02]

LGTM

[DaleCurtis, 2016-07-13 16:49:27]

lgtm

[kenrb, 2016-07-14 13:52:36]

kenrb@chromium.org changed reviewers:
+ kenrb@chromium.org

[kenrb, 2016-07-14 13:52:37]

ipc lgtm

[miu, 2016-07-14 21:17:04]

lgtm

https://codereview.chromium.org/2120273004/diff/70001/content/browser/rendere...
File content/browser/renderer_host/render_message_filter.cc (left):

https://codereview.chromium.org/2120273004/diff/70001/content/browser/rendere...
content/browser/renderer_host/render_message_filter.cc:244: if (message.type()
== ViewHostMsg_GetAudioHardwareConfig::ID)
Yuk. We used to use VideoHostMsg for getting *audio* config?!? I'm super-glad
you're removing this now! :-)

[o1ka, 2016-07-15 08:53:11]

On 2016/07/14 21:17:04, miu wrote:
> lgtm
> 
>
https://codereview.chromium.org/2120273004/diff/70001/content/browser/rendere...
> File content/browser/renderer_host/render_message_filter.cc (left):
> 
>
https://codereview.chromium.org/2120273004/diff/70001/content/browser/rendere...
> content/browser/renderer_host/render_message_filter.cc:244: if (message.type()
> == ViewHostMsg_GetAudioHardwareConfig::ID)
> Yuk. We used to use VideoHostMsg for getting *audio* config?!? I'm super-glad
> you're removing this now! :-)

It was ViewHost, not VideoHost :)

[o1ka, 2016-07-15 08:53:45]

Thanks everybody for the review

[o1ka, 2016-07-15 08:53:54]

The CQ bit was checked by olka@chromium.org

[commit-bot: I haz the power, 2016-07-15 08:54:05]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-15 10:24:25]

Description was changed from

==========
Getting rid of AudioHardwareConfig and its synchronous IPC.

Switching to AudioDeviceFactory::GetOutputDeviceInfo() instead.


BUG=611404,549125,422522
==========

to

==========
Getting rid of AudioHardwareConfig and its synchronous IPC.

Switching to AudioDeviceFactory::GetOutputDeviceInfo() instead.


BUG=611404,549125,422522
==========

[commit-bot: I haz the power, 2016-07-15 10:24:27]

Committed patchset #4 (id:70001)

[commit-bot: I haz the power, 2016-07-15 10:24:33]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 10:26:28]

Description was changed from

==========
Getting rid of AudioHardwareConfig and its synchronous IPC.

Switching to AudioDeviceFactory::GetOutputDeviceInfo() instead.


BUG=611404,549125,422522
==========

to

==========
Getting rid of AudioHardwareConfig and its synchronous IPC.

Switching to AudioDeviceFactory::GetOutputDeviceInfo() instead.

BUG=611404,549125,422522

Committed: https://crrev.com/a148aeb2bc27a99f25b97931dd94e28f2eab6f1d
Cr-Commit-Position: refs/heads/master@{#405737}
==========

[commit-bot: I haz the power, 2016-07-15 10:26:31]

Patchset 4 (id:??) landed as
https://crrev.com/a148aeb2bc27a99f25b97931dd94e28f2eab6f1d
Cr-Commit-Position: refs/heads/master@{#405737}