Allow AudioOutputDevice objects to be initialized with a specific hardware output device and store …

Allow AudioOutputDevice objects to be initialized with a specific hardware output device and store its output parameters.

BUG=506507, 506124
Committed: https://crrev.com/85d67f542541472da3a6e0f3808c3ee5116c4dcd
Cr-Commit-Position: refs/heads/master@{#349859}

[Guido Urdaneta, 2015-09-07 11:14:23]

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

[DaleCurtis, 2015-09-09 01:31:58]

Hmm, this is more complex than I expected. Can you think about what we can
simplify if we restrict the API allowed by the renderer?

I also don't see where you're calling Stop() for every implementation now?

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:154: class
AudioRendererHost::AuthorizationData {
Hmm, this is a lot of data and complexity added to the ARH class. I think we can
at least remove all the AudioParameters from this structure by restricting what
the renderer is allowed to call.

If we can't simplify this further, I think this functionality should be moved
into a helper class that AudioRendererHost uses to manage authorizations -- as
otherwise this is too much complexity to add into the main AudioRendererHost
class.

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:477: // A close
request was received while access check was in progress
translate was in progress?

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:515: // If
authorization is ongoing, defer creation.
This ends up adding substantial complexity. Can we drop this from the renderer
side and remove the creation params and such? I.e. never allow an AOD which has
started an authorization request to call create while it's in progress?

It looks like removing that would allow you to get rid of needing to track
AudioParameters in the data.

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:615: void
AudioRendererHost::OnSwitchOutputDevice(int stream_id,
As mentioned in the AOD class, after this CL lands, I think we should remove the
browser side SwitchOutputDevice functionality and require that AOD be recreated
every time. It would allow us to remove this extra state here in favor of a
self-contained helper class on the renderer side which can manage the switch
callbacks and AOD instances.

I.e. you could create something like a SwitchOutputDeviceHelper class which both
WebRTC and AudioRendererMixerInput could construct and use when a
switchOutputDevice() call comes in from JavaScript. That class would handle the
complexities of creating a new AudioOutputDevice for the new device, return
errors via callback if authorization/creation fails, and return the new
AudioOutputDevice if authorization succeeds.

As a separate class it could be well tested and modular w/o having to be weighed
down by internal state details of the AudioOutputDevice.

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:866:
callback.Run(false, {std::string(), std::string(), DummyParams()});
Is this initialization syntax allowed? http://chromium-cpp.appspot.com/

https://codereview.chromium.org/1323403005/diff/140001/content/renderer/media...
File content/renderer/media/audio_device_factory.cc (right):

https://codereview.chromium.org/1323403005/diff/140001/content/renderer/media...
content/renderer/media/audio_device_factory.cc:22: if (factory_) {
This should just use some dummy params and call the deepest method (l.56).

https://codereview.chromium.org/1323403005/diff/140001/content/renderer/media...
content/renderer/media/audio_device_factory.cc:40: if (factory_) {
Ditto.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:66: authorization_failed_(false) {
Should this be another state? That avoid mixing two types of state machines,
bool + fsm.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:73: static_assert(AUTHORIZING <
CREATING_STREAM,
Seems there should be an authorized state?

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:170: if (state_ == IDLE) {
DCHECK_EQ? Nothing should be calling this before the factory right?

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:184: // Request device authorization again if
the stream was stopped
Why when stopped? Shouldn't authorization persist until switch device is called?

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:185: RequestDeviceAuthorizationOnIOThread();
Why is this necessary? If it's a non-default device the authorization must have
completed on the browser side at least before the CreateStream IPC below will
run. For default, the browser side doesn't care.

Are you doing this to ensure the browser side always gets a auth request first?
Seems this should only be called if !AUTHORIZING or !AUTHORIZED.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:186: 
DCHECK(state_ == AUTHORIZING || state_ == AUTHORIZED); ?

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:262: if (state_ >= CREATING_STREAM) {
I think this code should instead store the current state, shutdown the shared
memory and restore the state once the authorization comes through.  Otherwise we
have the shared memory size issue, no?

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:266: switch_output_device_on_start_ = true;
Instead of having this, can we set the state back to AUTHORIZING and let
CreateStreamOnIO handle resending the authorization?

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:305: SetOutputParams(output_params);
Seems small to be its own function, just inline?

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:360: if (switch_output_device_on_start_) {
Hmm, at this point the stream needs to be shutdown and restarted, so perhaps
this should just invoke SwitchOutputDeviceOnIOThread after changing the state to
PAUSED?

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:372: device_id_ = current_switch_device_id_;
Clear switch ids?

[Guido Urdaneta, 2015-09-09 16:22:24]

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:154: class
AudioRendererHost::AuthorizationData {
I removed this class altogether and replaced it with a pair.
A lot of the complexity was to support multiple Start()/Stop().
I moved the complexity to AOD by having an extra AUTHORIZED state and delaying
CreateStream until after that state.

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:477: // A close
request was received while access check was in progress
On 2015/09/09 01:31:57, DaleCurtis wrote:
> translate was in progress?

Done.

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:515: // If
authorization is ongoing, defer creation.
On 2015/09/09 01:31:57, DaleCurtis wrote:
> This ends up adding substantial complexity. Can we drop this from the renderer
> side and remove the creation params and such? I.e. never allow an AOD which
has
> started an authorization request to call create while it's in progress?
> 
> It looks like removing that would allow you to get rid of needing to track
> AudioParameters in the data.

Done. See comment above.

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:615: void
AudioRendererHost::OnSwitchOutputDevice(int stream_id,
On 2015/09/09 01:31:57, DaleCurtis wrote:
> As mentioned in the AOD class, after this CL lands, I think we should remove
the
> browser side SwitchOutputDevice functionality and require that AOD be
recreated
> every time. It would allow us to remove this extra state here in favor of a
> self-contained helper class on the renderer side which can manage the switch
> callbacks and AOD instances.
> 

That is the goal. 

A possible road map after this CL is:
1. Allow ARMI to switch device by using a new AOD (I have a working version of
this already).
2. Allow the WebRTC renderers to switch device by using a new AOD.
3. Remove SwitchOutputDevice() functionality from ARH and AOD.

I haven't studied yet how big the change is for 2. If it's nontrivial, I will
first send a CL to reject SwitchOutputDevice() to devices with different
parameters before working on 2.


> I.e. you could create something like a SwitchOutputDeviceHelper class which
both
> WebRTC and AudioRendererMixerInput could construct and use when a
> switchOutputDevice() call comes in from JavaScript. That class would handle
the
> complexities of creating a new AudioOutputDevice for the new device, return
> errors via callback if authorization/creation fails, and return the new
> AudioOutputDevice if authorization succeeds.
> 
> As a separate class it could be well tested and modular w/o having to be
weighed
> down by internal state details of the AudioOutputDevice.

At this time, I think the solution would be specific for each case. ARMI doesn't
use AOD directly, but through a mixer created by ARMM, while WebRTC uses AOD
directly. I haven't looked at the WebRTC case in detail yet, but let's  talk
more about this in the next CL.

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:866:
callback.Run(false, {std::string(), std::string(), DummyParams()});
On 2015/09/09 01:31:57, DaleCurtis wrote:
> Is this initialization syntax allowed? http://chromium-cpp.appspot.com/

Structs are allowed to be initialized this way, but now I'm not sure if it's
allowed to initialize a function argument.
Using a temp variable now to be sure.

https://codereview.chromium.org/1323403005/diff/140001/content/renderer/media...
File content/renderer/media/audio_device_factory.cc (right):

https://codereview.chromium.org/1323403005/diff/140001/content/renderer/media...
content/renderer/media/audio_device_factory.cc:22: if (factory_) {
On 2015/09/09 01:31:57, DaleCurtis wrote:
> This should just use some dummy params and call the deepest method (l.56).

Done.

https://codereview.chromium.org/1323403005/diff/140001/content/renderer/media...
content/renderer/media/audio_device_factory.cc:40: if (factory_) {
On 2015/09/09 01:31:57, DaleCurtis wrote:
> Ditto.

This one is as deep as the one in l.56.
This one takes a session ID, the other one takes DeviceID/SecurityOrigin.
The real deep one is AOD's constructor.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:66: authorization_failed_(false) {
On 2015/09/09 01:31:57, DaleCurtis wrote:
> Should this be another state? That avoid mixing two types of state machines,
> bool + fsm.

Done. Now we have NOT_AUTHORIZED and AUTHORIZED as states.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:73: static_assert(AUTHORIZING <
CREATING_STREAM,
On 2015/09/09 01:31:57, DaleCurtis wrote:
> Seems there should be an authorized state?

Done.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:170: if (state_ == IDLE) {
On 2015/09/09 01:31:57, DaleCurtis wrote:
> DCHECK_EQ? Nothing should be calling this before the factory right?

Done.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:184: // Request device authorization again if
the stream was stopped
On 2015/09/09 01:31:57, DaleCurtis wrote:
> Why when stopped? Shouldn't authorization persist until switch device is
called?

For practical reasons, authorization also ends at Stop().
The reason is that the browser (ARH) keeps track of authorizations per stream.
Once a stream is closed, ARH forgets everything about the stream, including
authorizations.
An improvement I made is that AOD does not send this IPC if it's the default
device and the parameters are already known.

Alternative approaches would be to keep track of authorizations per AOD or per
renderer frame, but I think that would add a lot of complexity.
Doing it per AOD would require some sort of AOD ID, which currently does not
exist. Doing it per renderer frame does not require a new ID, but in this case
the state in ARH would be more complex, as authorizations would be shared across
all streams/AODs from the same renderer frame.

My take is that Start()/Stop()/Start()/Stop() is not so frequent, and if it
happens I hope it will be mostly with the default device, which does not require
an extra IPC; and if it's not the default device, it will be with a session ID,
which requires an IPC, but not a translation and actual permission check.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:185: RequestDeviceAuthorizationOnIOThread();
On 2015/09/09 01:31:57, DaleCurtis wrote:
> Why is this necessary? If it's a non-default device the authorization must
have
> completed on the browser side at least before the CreateStream IPC below will
> run. 
> For default, the browser side doesn't care.
> 
> Are you doing this to ensure the browser side always gets a auth request
first?
> Seems this should only be called if !AUTHORIZING or !AUTHORIZED.

I guess my comment above explains why it is necessary to request authorization
again for nondefault devices in a Start() after a Stop(), or if parameters are
not known.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:186: 
On 2015/09/09 01:31:57, DaleCurtis wrote:
> DCHECK(state_ == AUTHORIZING || state_ == AUTHORIZED); ?

Can also be NOT_AUTHORIZED, or IDLE after a Stop() (see explanation above).

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:262: if (state_ >= CREATING_STREAM) {
On 2015/09/09 01:31:57, DaleCurtis wrote:
> I think this code should instead store the current state, shutdown the shared
> memory and restore the state once the authorization comes through.  Otherwise
we
> have the shared memory size issue, no?

We still have the shared memory size issue.
The original idea was to reject switch requests in ARH if the parameters for the
new device differ from the current device.
That's easy to do and solves the shared memory size issue. I did not include it
in this CL to make it less complex than it already is.

Also, the plan is to remove SwitchOutputDevice() from AOD by making ARMI and
WebRTC to create new AODs when the device needs to be switched. For ARMI it's
easy. I haven't checked WebRTC yet, but it should be doable.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:266: switch_output_device_on_start_ = true;
On 2015/09/09 01:31:57, DaleCurtis wrote:
> Instead of having this, can we set the state back to AUTHORIZING and let
> CreateStreamOnIO handle resending the authorization?

This isn't intended to address authorization problems, but a  practical problem
that Web developers have reported. Sometimes they call setSinkId() in JavaScript
too quickly and they get an error because the stream has not been created yet.
This fixes that in a way similar to how Play()/Pause() are handled before the
stream is created.
I think this is better than having Web developers implement workarounds due to
an internal implementation detail.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:305: SetOutputParams(output_params);
On 2015/09/09 01:31:57, DaleCurtis wrote:
> Seems small to be its own function, just inline?

Done.

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:360: if (switch_output_device_on_start_) {
On 2015/09/09 01:31:57, DaleCurtis wrote:
> Hmm, at this point the stream needs to be shutdown and restarted, so perhaps
> this should just invoke SwitchOutputDeviceOnIOThread after changing the state
to
> PAUSED?

Why does the stream need to be shutdown and restarted?
The intent here is just to send a SwitchOutputDevice IPC after the stream is
created to avoid sending an error to JavaScript when setSinkId() (javascript's
name for SwitchOutputDevice) is called too quickly (e.g., when the stream is
initially muted).

https://codereview.chromium.org/1323403005/diff/140001/media/audio/audio_outp...
media/audio/audio_output_device.cc:372: device_id_ = current_switch_device_id_;
On 2015/09/09 01:31:57, DaleCurtis wrote:
> Clear switch ids?

We can clear them, but would there be an advantage?
Once |current_switch_callback_| is null, whatever value the switch ids have is
meaningless.

[Guido Urdaneta, 2015-09-09 16:38:39]

On 2015/09/09 01:31:58, DaleCurtis wrote:
> Hmm, this is more complex than I expected. Can you think about what we can
> simplify if we restrict the API allowed by the renderer?
> 

A lot of the complexity comes from the need to support multiple Start()/Stop(),
and Start() immediately after creation and Initialize().
They introduce subtle corner cases. The previous version of the CL put most of
the logic in ARH; now it is in AOD.


> I also don't see where you're calling Stop() for every implementation now?
> 

The most important offender is WebRtcLocalAudioRenderer, which I'm fixing in
this CL.
The only other case I found was an AOD unit test, also fixed.

[DaleCurtis, 2015-09-12 01:17:19]

Almost there, sorry for the review delay been at a manager summit all week.

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1323403005/diff/140001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:615: void
AudioRendererHost::OnSwitchOutputDevice(int stream_id,
On 2015/09/09 16:22:23, Guido Urdaneta wrote:
> On 2015/09/09 01:31:57, DaleCurtis wrote:
> > As mentioned in the AOD class, after this CL lands, I think we should remove
> the
> > browser side SwitchOutputDevice functionality and require that AOD be
> recreated
> > every time. It would allow us to remove this extra state here in favor of a
> > self-contained helper class on the renderer side which can manage the switch
> > callbacks and AOD instances.
> > 
> 
> That is the goal. 
> 
> A possible road map after this CL is:
> 1. Allow ARMI to switch device by using a new AOD (I have a working version of
> this already).
> 2. Allow the WebRTC renderers to switch device by using a new AOD.
> 3. Remove SwitchOutputDevice() functionality from ARH and AOD.
> 
> I haven't studied yet how big the change is for 2. If it's nontrivial, I will
> first send a CL to reject SwitchOutputDevice() to devices with different
> parameters before working on 2.
> 
> 
> > I.e. you could create something like a SwitchOutputDeviceHelper class which
> both
> > WebRTC and AudioRendererMixerInput could construct and use when a
> > switchOutputDevice() call comes in from JavaScript. That class would handle
> the
> > complexities of creating a new AudioOutputDevice for the new device, return
> > errors via callback if authorization/creation fails, and return the new
> > AudioOutputDevice if authorization succeeds.
> > 
> > As a separate class it could be well tested and modular w/o having to be
> weighed
> > down by internal state details of the AudioOutputDevice.
> 
> At this time, I think the solution would be specific for each case. ARMI
doesn't
> use AOD directly, but through a mixer created by ARMM, while WebRTC uses AOD
> directly. I haven't looked at the WebRTC case in detail yet, but let's  talk
> more about this in the next CL.

sgtm!

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:392: void
AudioRendererHost::RequestDeviceAuthorizationAccessChecked(
OnDeviceAuthorized?

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:416: void
AudioRendererHost::RequestDeviceAuthorizationTranslated(
OnDeviceIDTranslated?

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:454: // If
authorization is not finished, ignore this request, as it is invalid.
Can we CHECK this?

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:614: void
AudioRendererHost::OutputDeviceSwitched(
OnDeviceSwitched?

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:177: void
RequestDeviceAuthorizationAccessChecked(int stream_id,
Perhaps something simpler like RequestDeviceAuthorization()

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

https://codereview.chromium.org/1323403005/diff/200001/content/renderer/media...
content/renderer/media/audio_device_factory.h:33: static
scoped_refptr<media::AudioOutputDevice> NewOutputDevice(
Do we have callers for all of these methods? I feel like we should just have one
and callers can specify a zero session id, an empty device_id or security origin
if they don't need it. As this just feels a bit crufty.

https://codereview.chromium.org/1323403005/diff/200001/content/renderer/media...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/1323403005/diff/200001/content/renderer/media...
content/renderer/media/audio_message_filter.cc:205: DLOG(WARNING) << "Got
OnDeviceAuthorized() event for a nonexistent or"
Seems like this could happen when devices are shutdown? Do we want to log a
warning here?

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
media/audio/audio_output_device.cc:206: default:
I think we generally try to avoid using default in state machines. Can you add
the other states with a NOTREACHED() ?

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
media/audio/audio_output_device.cc:325: DCHECK(state_ == IDLE || state_ ==
AUTHORIZING);
Log <<state_ so you can debug this easily.

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
media/audio/audio_output_device.cc:328: did_set_output_params_.Signal();
You always need to signal or a thread might get locked. You should just use an
invalid set of params though; also don't signal until after you set the params
or you'll have a race.

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
File media/audio/audio_output_device.h (right):

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
media/audio/audio_output_device.h:146: NOT_AUTHORIZED,   // Failed device
authorization received.
Should we just reuse IPC_CLOSED for this state?

https://codereview.chromium.org/1323403005/diff/200001/media/base/output_devi...
File media/base/output_device.h (right):

https://codereview.chromium.org/1323403005/diff/200001/media/base/output_devi...
media/base/output_device.h:50: // become available.
Add a clause saying it must never be called on the IO thread?

[Guido Urdaneta, 2015-09-14 11:35:49]

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:392: void
AudioRendererHost::RequestDeviceAuthorizationAccessChecked(
On 2015/09/12 01:17:19, DaleCurtis wrote:
> OnDeviceAuthorized?

Done.

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:416: void
AudioRendererHost::RequestDeviceAuthorizationTranslated(
On 2015/09/12 01:17:19, DaleCurtis wrote:
> OnDeviceIDTranslated?

Done.

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:454: // If
authorization is not finished, ignore this request, as it is invalid.
On 2015/09/12 01:17:19, DaleCurtis wrote:
> Can we CHECK this?

Done.

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:614: void
AudioRendererHost::OutputDeviceSwitched(
On 2015/09/12 01:17:19, DaleCurtis wrote:
> OnDeviceSwitched?

Done.

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.h (right):

https://codereview.chromium.org/1323403005/diff/200001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.h:177: void
RequestDeviceAuthorizationAccessChecked(int stream_id,
On 2015/09/12 01:17:19, DaleCurtis wrote:
> Perhaps something simpler like RequestDeviceAuthorization()

Done. Now OnDeviceAuthorized()

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

https://codereview.chromium.org/1323403005/diff/200001/content/renderer/media...
content/renderer/media/audio_device_factory.h:33: static
scoped_refptr<media::AudioOutputDevice> NewOutputDevice(
On 2015/09/12 01:17:19, DaleCurtis wrote:
> Do we have callers for all of these methods? I feel like we should just have
one
> and callers can specify a zero session id, an empty device_id or security
origin
> if they don't need it. As this just feels a bit crufty.

Done.

https://codereview.chromium.org/1323403005/diff/200001/content/renderer/media...
File content/renderer/media/audio_message_filter.cc (right):

https://codereview.chromium.org/1323403005/diff/200001/content/renderer/media...
content/renderer/media/audio_message_filter.cc:205: DLOG(WARNING) << "Got
OnDeviceAuthorized() event for a nonexistent or"
On 2015/09/12 01:17:19, DaleCurtis wrote:
> Seems like this could happen when devices are shutdown? Do we want to log a
> warning here?

Done. It was there for consistency with the other AMF methods, which have this
warning.

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
media/audio/audio_output_device.cc:206: default:
On 2015/09/12 01:17:19, DaleCurtis wrote:
> I think we generally try to avoid using default in state machines. Can you add
> the other states with a NOTREACHED() ?

Done.

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
media/audio/audio_output_device.cc:325: DCHECK(state_ == IDLE || state_ ==
AUTHORIZING);
On 2015/09/12 01:17:19, DaleCurtis wrote:
> Log <<state_ so you can debug this easily.

Actually, it is impossible to get this while in IDLE since the delegate is
removed when AOD changes to IDLE state. I guess this makes the log unnecessary.

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
media/audio/audio_output_device.cc:328: did_set_output_params_.Signal();
On 2015/09/12 01:17:19, DaleCurtis wrote:
> You always need to signal or a thread might get locked. You should just use an
> invalid set of params though; also don't signal until after you set the params
> or you'll have a race.

Done. Using the default initial invalid params in case of authorization failure.
Also signaling in the destructor just in case.

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
File media/audio/audio_output_device.h (right):

https://codereview.chromium.org/1323403005/diff/200001/media/audio/audio_outp...
media/audio/audio_output_device.h:146: NOT_AUTHORIZED,   // Failed device
authorization received.
On 2015/09/12 01:17:19, DaleCurtis wrote:
> Should we just reuse IPC_CLOSED for this state?

Done.

https://codereview.chromium.org/1323403005/diff/200001/media/base/output_devi...
File media/base/output_device.h (right):

https://codereview.chromium.org/1323403005/diff/200001/media/base/output_devi...
media/base/output_device.h:50: // become available.
On 2015/09/12 01:17:19, DaleCurtis wrote:
> Add a clause saying it must never be called on the IO thread?

Done.

[DaleCurtis, 2015-09-14 20:07:42]

lgtm % couple nits.

https://codereview.chromium.org/1323403005/diff/220001/media/audio/audio_outp...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1323403005/diff/220001/media/audio/audio_outp...
media/audio/audio_output_device.cc:94: if (!did_set_output_params_.IsSignaled())
You can instead DCHECK this since AOD is ref-counted and thus a caller should
never be within AOD at this point.

https://codereview.chromium.org/1323403005/diff/220001/media/audio/audio_outp...
media/audio/audio_output_device.cc:325: if
(!did_set_output_params_.IsSignaled())
Is it possible to get two of these? Should this be a dcheck?

[Guido Urdaneta, 2015-09-15 08:28:28]

guidou@chromium.org changed reviewers:
+ bbudge@chromium.org, palmer@chromium.org

[Guido Urdaneta, 2015-09-15 08:28:29]

palmer@chromium.org: Please review changes in content/common/media

bbudge@chromium.org: Please review changes in content/renderer/pepper

https://codereview.chromium.org/1323403005/diff/220001/media/audio/audio_outp...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1323403005/diff/220001/media/audio/audio_outp...
media/audio/audio_output_device.cc:94: if (!did_set_output_params_.IsSignaled())
On 2015/09/14 20:07:41, DaleCurtis wrote:
> You can instead DCHECK this since AOD is ref-counted and thus a caller should
> never be within AOD at this point.

Done. However, I added a Signal() in OnIPCClosed() instead to cover the case of
early shutdowns.

https://codereview.chromium.org/1323403005/diff/220001/media/audio/audio_outp...
media/audio/audio_output_device.cc:325: if
(!did_set_output_params_.IsSignaled())
On 2015/09/14 20:07:41, DaleCurtis wrote:
> Is it possible to get two of these? Should this be a dcheck?

It is possible to get two of these in a Start()/Stop()/Start() sequence for a
nondefault device.
As discussed before, ARH forgets the authorization after Stop() and a new
RequestAuth IPC may be necessary in the second Start().

[Guido Urdaneta, 2015-09-15 09:45:14]

https://codereview.chromium.org/1323403005/diff/220001/media/audio/audio_outp...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1323403005/diff/220001/media/audio/audio_outp...
media/audio/audio_output_device.cc:94: if (!did_set_output_params_.IsSignaled())
On 2015/09/15 08:28:29, Guido Urdaneta wrote:
> On 2015/09/14 20:07:41, DaleCurtis wrote:
> > You can instead DCHECK this since AOD is ref-counted and thus a caller
should
> > never be within AOD at this point.
> 
> Done. However, I added a Signal() in OnIPCClosed() instead to cover the case
of
> early shutdowns.

Reverted the DCHECK in the destructor. The DCHECK is correct in principle, but
the WebRtcAudioRenderer unit test uses a FakeAOD that derives from AOD and mocks
all methods, so nothing signals the event.
When the destructor runs, the DCHECK fails.

I put the Signal() back until we can find a way to reintroduce the DCHECK.

[bbudge, 2015-09-15 11:39:42]

https://codereview.chromium.org/1323403005/diff/280001/content/renderer/peppe...
File content/renderer/pepper/pepper_platform_audio_output.cc (right):

https://codereview.chromium.org/1323403005/diff/280001/content/renderer/peppe...
content/renderer/pepper/pepper_platform_audio_output.cc:77: const
media::AudioParameters& output_params) {}
I'm assuming 'success' is always 'true' here. If so, DCHECK(success).

[Guido Urdaneta, 2015-09-15 11:55:14]

https://codereview.chromium.org/1323403005/diff/280001/content/renderer/peppe...
File content/renderer/pepper/pepper_platform_audio_output.cc (right):

https://codereview.chromium.org/1323403005/diff/280001/content/renderer/peppe...
content/renderer/pepper/pepper_platform_audio_output.cc:77: const
media::AudioParameters& output_params) {}
On 2015/09/15 11:39:42, bbudge wrote:
> I'm assuming 'success' is always 'true' here. If so, DCHECK(success).

Actually, this is never called because the RequestDeviceAuthorization IPC is
never sent from here. It is not necessary because PPAO always uses the default
device.
Should we use NOTREACHED() instead?

[bbudge, 2015-09-15 12:59:12]

content/renderer/pepper LGTM

https://codereview.chromium.org/1323403005/diff/280001/content/renderer/peppe...
File content/renderer/pepper/pepper_platform_audio_output.cc (right):

https://codereview.chromium.org/1323403005/diff/280001/content/renderer/peppe...
content/renderer/pepper/pepper_platform_audio_output.cc:77: const
media::AudioParameters& output_params) {}
On 2015/09/15 11:55:14, Guido Urdaneta wrote:
> On 2015/09/15 11:39:42, bbudge wrote:
> > I'm assuming 'success' is always 'true' here. If so, DCHECK(success).
> 
> Actually, this is never called because the RequestDeviceAuthorization IPC is
> never sent from here. It is not necessary because PPAO always uses the default
> device.
> Should we use NOTREACHED() instead?

OK that's fine.

[palmer, 2015-09-15 20:29:24]

https://codereview.chromium.org/1323403005/diff/300001/content/common/media/a...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/1323403005/diff/300001/content/common/media/a...
content/common/media/audio_messages.h:101: // Request that is sent to the
browser to authorize use of an audio output
To request the use of?

https://codereview.chromium.org/1323403005/diff/300001/content/common/media/a...
content/common/media/audio_messages.h:108: std::string /* device_id */,
What lexical/syntactic form do these device IDs have?

https://codereview.chromium.org/1323403005/diff/300001/content/common/media/a...
content/common/media/audio_messages.h:109: GURL /* security_origin */)
url::Origin is the canonical class for this purpose, now.

https://codereview.chromium.org/1323403005/diff/300001/content/common/media/a...
content/common/media/audio_messages.h:162: GURL /* security_origin */)
url::Origin here too.

[Guido Urdaneta, 2015-09-16 11:34:49]

https://codereview.chromium.org/1323403005/diff/300001/content/common/media/a...
File content/common/media/audio_messages.h (right):

https://codereview.chromium.org/1323403005/diff/300001/content/common/media/a...
content/common/media/audio_messages.h:101: // Request that is sent to the
browser to authorize use of an audio output
On 2015/09/15 20:29:24, palmer wrote:
> To request the use of?

Done.

https://codereview.chromium.org/1323403005/diff/300001/content/common/media/a...
content/common/media/audio_messages.h:108: std::string /* device_id */,
On 2015/09/15 20:29:24, palmer wrote:
> What lexical/syntactic form do these device IDs have?

It's an opaque ID consisting of a hash of the actual device ID (known only by
the browser) and the security origin. Example value:
"6e18f44f010fdf64599acfe8dba0cc99639a8591c8b8db395360acc0533f3bec"

https://codereview.chromium.org/1323403005/diff/300001/content/common/media/a...
content/common/media/audio_messages.h:109: GURL /* security_origin */)
On 2015/09/15 20:29:24, palmer wrote:
> url::Origin is the canonical class for this purpose, now.

Done.

https://codereview.chromium.org/1323403005/diff/300001/content/common/media/a...
content/common/media/audio_messages.h:162: GURL /* security_origin */)
On 2015/09/15 20:29:24, palmer wrote:
> url::Origin here too.

Done.

[palmer, 2015-09-17 20:31:57]

https://codereview.chromium.org/1323403005/diff/300001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1323403005/diff/300001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:359: if
(LookupById(stream_id) || IsAuthorizationStarted(stream_id))
It would be good to add a check here that validates that the incoming
|device_id| meets the lexical requirements for the opaque ID format.

[Guido Urdaneta, 2015-09-18 09:23:07]

https://codereview.chromium.org/1323403005/diff/300001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1323403005/diff/300001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:359: if
(LookupById(stream_id) || IsAuthorizationStarted(stream_id))
On 2015/09/17 20:31:57, palmer wrote:
> It would be good to add a check here that validates that the incoming
> |device_id| meets the lexical requirements for the opaque ID format.

Done. Note that I made the |device_id| check return a "Not authorized" reply.
The reason I did not put it in the initial |stream_id| check is that that one
checks for a redundant authorization request and ignores it.
For an invalid |device_id| it makes more sense to just deny authorization.
I added a similar check to OnSwitchOutputDevice()

[palmer, 2015-09-18 19:24:24]

https://codereview.chromium.org/1323403005/diff/380001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1323403005/diff/380001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:85: if
(device_id.empty())
Why are empty device IDs valid? What are the semantics of an empty device ID?

[Guido Urdaneta, 2015-09-18 20:17:56]

https://codereview.chromium.org/1323403005/diff/380001/content/browser/render...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/1323403005/diff/380001/content/browser/render...
content/browser/renderer_host/media/audio_renderer_host.cc:85: if
(device_id.empty())
On 2015/09/18 19:24:24, palmer wrote:
> Why are empty device IDs valid? What are the semantics of an empty device ID?

The empty device ID represents the default output device, which is always valid
and authorized.

[palmer, 2015-09-18 20:20:01]

LGTM.

[Guido Urdaneta, 2015-09-18 20:24:09]

The CQ bit was checked by guidou@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-09-18 20:24:28]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323403005/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323403005/380001

[commit-bot: I haz the power, 2015-09-18 21:40:57]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-18 21:40:58]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2015-09-19 07:36:40]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2015-09-19 07:36:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
dalecurtis@chromium.org, bbudge@chromium.org
Link to the patchset: https://codereview.chromium.org/1323403005/#ps380001
(title: "Palmer's comments")

[commit-bot: I haz the power, 2015-09-19 07:36:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1323403005/380001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1323403005/380001

[commit-bot: I haz the power, 2015-09-19 07:40:59]

Committed patchset #20 (id:380001)

[commit-bot: I haz the power, 2015-09-19 07:41:43]

Patchset 20 (id:??) landed as
https://crrev.com/85d67f542541472da3a6e0f3808c3ee5116c4dcd
Cr-Commit-Position: refs/heads/master@{#349859}