Moving SwitchOutputDevice out of OutputDevice interface, eliminating OutputDevice

Getting rid of OutputDevice interface, moving SwitchOutputDevice() to AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and device status, it provides device ID information now (at this stage it's useful for unit tests only, but it will also be used for session ID -> device ID mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from AudioRendererMixerManager, mock AudioDeviceFactory is used instead to instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(), all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit outdated since Patchset 5 which eliminated OutputDevice interface).

BUG=595727, 506507
Committed: https://crrev.com/68b69396916a10dbace194d871c854302bd98347
Cr-Commit-Position: refs/heads/master@{#384550}

[o1ka, 2016-03-17 15:53:43]

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

[commit-bot: I haz the power, 2016-03-17 15:53:59]

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

[o1ka, 2016-03-17 15:54:35]

The CQ bit was unchecked by olka@chromium.org

[o1ka, 2016-03-17 16:09:28]

Description was changed from

==========
Moving SwitchOutputDevice out of OutputDevice interface.
1) SwitchOutputDevice() is removed from OutputDevice interface;
2) OutputDevice inheritance for MediaStreamAudioRenderer implementations
is removed;
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleand up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependant on AudioOutputDevice no more; from
now on AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) GetDeviceId() method is added to OutputDevice (at this stage it's
useful for unit tests only, but it will also be used for
session ID -> device ID mapping in the upcoming CL).


BUG=595727
==========

to

==========
Moving SwitchOutputDevice out of OutputDevice interface.
1) SwitchOutputDevice() is removed from OutputDevice interface;
2) OutputDevice inheritance for MediaStreamAudioRenderer implementations
is removed;
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleand up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependant on AudioOutputDevice no more; from
now on AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) GetDeviceId() method is added to OutputDevice (at this stage it's
useful for unit tests only, but it will also be used for
session ID -> device ID mapping in the upcoming CL).

See bug description for mode details and a link to doc with pictures.

BUG=595727
==========

[o1ka, 2016-03-17 16:41:08]

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

[o1ka, 2016-03-17 16:41:09]

PTAL;

miu@ - added you in CC since you are "no review" now; you might be interested in
track_audio_renderer changes.

[Guido Urdaneta, 2016-03-17 17:38:24]

https://codereview.chromium.org/1809093003/diff/20001/content/public/renderer...
File content/public/renderer/media_stream_audio_renderer.h (right):

https://codereview.chromium.org/1809093003/diff/20001/content/public/renderer...
content/public/renderer/media_stream_audio_renderer.h:47: // the 
media::SwitchOutputDeviceResult enum.
s/media::SwitchOutputDeviceResult/media::OutputDeviceStatus

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

https://codereview.chromium.org/1809093003/diff/20001/content/renderer/media/...
content/renderer/media/audio_device_factory.h:67: // Creates a
RestartableAudioRendererSink bound to an AudioOutputDevice
s/Restartable/Switchable

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

https://codereview.chromium.org/1809093003/diff/20001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.cc:135: media::OutputDevice*
AudioRendererMixerManager::GetOutputDevice(
Why have this method at all. The implementation suggests it is not needed.

https://codereview.chromium.org/1809093003/diff/20001/media/base/audio_render...
File media/base/audio_renderer_mixer_input.cc (right):

https://codereview.chromium.org/1809093003/diff/20001/media/base/audio_render...
media/base/audio_renderer_mixer_input.cc:115: :
get_output_device_cb_.Run(device_id_, security_origin_);
Shouldn't this one return nullptr when there is no mixer?

GetOutputDevice() does not give ownership of the returned OD. Is it possible
that the OD returned by get_output_device_cb_.Run() is a new one that will leak?

https://codereview.chromium.org/1809093003/diff/20001/media/base/audio_render...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/1809093003/diff/20001/media/base/audio_render...
media/base/audio_renderer_sink.h:101: // the  media::SwitchOutputDeviceResult
enum.
s/media::SwitchOutputDeviceResult/media::OutputDeviceStatus

https://codereview.chromium.org/1809093003/diff/20001/media/base/fake_output_...
File media/base/fake_output_device.h (left):

https://codereview.chromium.org/1809093003/diff/20001/media/base/fake_output_...
media/base/fake_output_device.h:21: // OutputDevice implementation.
I think you should keep the comment.

https://codereview.chromium.org/1809093003/diff/20001/media/base/output_device.h
File media/base/output_device.h (right):

https://codereview.chromium.org/1809093003/diff/20001/media/base/output_devic...
media/base/output_device.h:26: typedef base::Callback<void(OutputDeviceStatus)>
SwitchOutputDeviceCB;
You should move this typedef to audio_renderer_sink.h, which is where
SwitchOutputDevice is defined.

https://codereview.chromium.org/1809093003/diff/20001/media/blink/webaudiosou...
File media/blink/webaudiosourceprovider_impl.cc (right):

https://codereview.chromium.org/1809093003/diff/20001/media/blink/webaudiosou...
media/blink/webaudiosourceprovider_impl.cc:212: if (sink_)
Shouldn't this one also have the "!client" check that the other methods have?
Also, can sink_ be null? The other methods are not checking that.

[o1ka, 2016-03-18 10:45:41]

guidou@, thank you for the review!
Comments addressed, renamed SwitchOutputDeviceCB->OutputDeviceStatusCB, removed
GetOutputDeviceCB from AudioRendererMixerInput (this will go into an upcoming
CL)

https://codereview.chromium.org/1809093003/diff/20001/content/public/renderer...
File content/public/renderer/media_stream_audio_renderer.h (right):

https://codereview.chromium.org/1809093003/diff/20001/content/public/renderer...
content/public/renderer/media_stream_audio_renderer.h:47: // the 
media::SwitchOutputDeviceResult enum.
On 2016/03/17 17:38:23, Guido Urdaneta wrote:
> s/media::SwitchOutputDeviceResult/media::OutputDeviceStatus

Done.

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

https://codereview.chromium.org/1809093003/diff/20001/content/renderer/media/...
content/renderer/media/audio_device_factory.h:67: // Creates a
RestartableAudioRendererSink bound to an AudioOutputDevice
On 2016/03/17 17:38:24, Guido Urdaneta wrote:
> s/Restartable/Switchable

Done.

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

https://codereview.chromium.org/1809093003/diff/20001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.cc:135: media::OutputDevice*
AudioRendererMixerManager::GetOutputDevice(
On 2016/03/17 17:38:24, Guido Urdaneta wrote:
> Why have this method at all. The implementation suggests it is not needed.

It's a part of a future CL which leaked in; removing, as per out discussion.

https://codereview.chromium.org/1809093003/diff/20001/media/base/audio_render...
File media/base/audio_renderer_mixer_input.cc (right):

https://codereview.chromium.org/1809093003/diff/20001/media/base/audio_render...
media/base/audio_renderer_mixer_input.cc:115: :
get_output_device_cb_.Run(device_id_, security_origin_);
On 2016/03/17 17:38:24, Guido Urdaneta wrote:
> Shouldn't this one return nullptr when there is no mixer?
> 
> GetOutputDevice() does not give ownership of the returned OD. Is it possible
> that the OD returned by get_output_device_cb_.Run() is a new one that will
leak?

Of cause we need to make sure it does not leak ;) GetOutputDevice does not pass
ownership. Removing this code, since it's a part of a future CL.

https://codereview.chromium.org/1809093003/diff/20001/media/base/audio_render...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/1809093003/diff/20001/media/base/audio_render...
media/base/audio_renderer_sink.h:101: // the  media::SwitchOutputDeviceResult
enum.
On 2016/03/17 17:38:24, Guido Urdaneta wrote:
> s/media::SwitchOutputDeviceResult/media::OutputDeviceStatus

Done.

https://codereview.chromium.org/1809093003/diff/20001/media/base/output_device.h
File media/base/output_device.h (right):

https://codereview.chromium.org/1809093003/diff/20001/media/base/output_devic...
media/base/output_device.h:26: typedef base::Callback<void(OutputDeviceStatus)>
SwitchOutputDeviceCB;
On 2016/03/17 17:38:24, Guido Urdaneta wrote:
> You should move this typedef to audio_renderer_sink.h, which is where
> SwitchOutputDevice is defined.

As we discussed, this type is used in SwitchOutputDevice for both
SwitchableAudioRendererSink and MediaStreamAudioRenderer. Renaming it per your
suggestion.

https://codereview.chromium.org/1809093003/diff/20001/media/blink/webaudiosou...
File media/blink/webaudiosourceprovider_impl.cc (right):

https://codereview.chromium.org/1809093003/diff/20001/media/blink/webaudiosou...
media/blink/webaudiosourceprovider_impl.cc:212: if (sink_)
On 2016/03/17 17:38:24, Guido Urdaneta wrote:
> Shouldn't this one also have the "!client" check that the other methods have?
> Also, can sink_ be null? The other methods are not checking that.

Agree. Done.

[Guido Urdaneta, 2016-03-18 10:59:06]

lgtm

[o1ka, 2016-03-18 11:07:25]

Description was changed from

==========
Moving SwitchOutputDevice out of OutputDevice interface.
1) SwitchOutputDevice() is removed from OutputDevice interface;
2) OutputDevice inheritance for MediaStreamAudioRenderer implementations
is removed;
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleand up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependant on AudioOutputDevice no more; from
now on AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) GetDeviceId() method is added to OutputDevice (at this stage it's
useful for unit tests only, but it will also be used for
session ID -> device ID mapping in the upcoming CL).

See bug description for mode details and a link to doc with pictures.

BUG=595727
==========

to

==========
Moving SwitchOutputDevice out of OutputDevice interface.
1) SwitchOutputDevice() is removed from OutputDevice interface;
2) OutputDevice inheritance for MediaStreamAudioRenderer implementations
is removed;
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from
now on AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) GetDeviceId() method is added to OutputDevice (at this stage it's
useful for unit tests only, but it will also be used for
session ID -> device ID mapping in the upcoming CL).

See bug description for mode details and a link to doc with pictures.

BUG=595727
==========

[o1ka, 2016-03-18 15:59:50]

(I'm working on figuring out what's wrong with Andriod tests)

[Guido Urdaneta, 2016-03-21 11:09:20]

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

https://codereview.chromium.org/1809093003/diff/40001/content/renderer/media/...
content/renderer/media/audio_device_factory.cc:24: media::AudioOutputDevice*
NewOutputDevice(int render_frame_id,
This should be scoped_refptr

https://codereview.chromium.org/1809093003/diff/40001/content/renderer/media/...
content/renderer/media/audio_device_factory.cc:29: media::AudioOutputDevice*
device = new media::AudioOutputDevice(
This one too.

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

https://codereview.chromium.org/1809093003/diff/40001/content/renderer/media/...
content/renderer/media/audio_device_factory.h:98: virtual
media::AudioRendererSink* CreateAudioRendererMixerSink(
Shouldn't all these methods return scoped_refptr?

[o1ka, 2016-03-21 11:50:32]

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

https://codereview.chromium.org/1809093003/diff/40001/content/renderer/media/...
content/renderer/media/audio_device_factory.cc:29: media::AudioOutputDevice*
device = new media::AudioOutputDevice(
On 2016/03/21 11:09:20, Guido Urdaneta wrote:
> This one too.

Yes, exactly, I've already found this :)

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

https://codereview.chromium.org/1809093003/diff/40001/content/renderer/media/...
content/renderer/media/audio_device_factory.h:98: virtual
media::AudioRendererSink* CreateAudioRendererMixerSink(
On 2016/03/21 11:09:20, Guido Urdaneta wrote:
> Shouldn't all these methods return scoped_refptr?

They are used in unit tests only now, initial design (CreateAudioOutputDevice)
was returning raw pointer. Do you still think it's better to switch to
scoped_refptr?

[Guido Urdaneta, 2016-03-21 12:01:36]

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

https://codereview.chromium.org/1809093003/diff/40001/content/renderer/media/...
content/renderer/media/audio_device_factory.h:98: virtual
media::AudioRendererSink* CreateAudioRendererMixerSink(
On 2016/03/21 11:50:31, o1ka wrote:
> On 2016/03/21 11:09:20, Guido Urdaneta wrote:
> > Shouldn't all these methods return scoped_refptr?
> 
> They are used in unit tests only now, initial design (CreateAudioOutputDevice)
> was returning raw pointer. Do you still think it's better to switch to
> scoped_refptr?

Yes. If you are returning a pointer that is intended to be owned by the caller,
you should wrap it in a smart pointer that carries ownership. AudioRendererSink
is refcounted, so use scoped_refptr. That it's currently used only for tests
shouldn't matter.

[Guido Urdaneta, 2016-03-21 13:01:45]

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

https://codereview.chromium.org/1809093003/diff/40001/content/renderer/media/...
content/renderer/media/audio_device_factory.cc:29: media::AudioOutputDevice*
device = new media::AudioOutputDevice(
On 2016/03/21 11:50:31, o1ka wrote:
> On 2016/03/21 11:09:20, Guido Urdaneta wrote:
> > This one too.
> 
> Yes, exactly, I've already found this :)

Yes. After out talk, I just wanted to make sure you changed it in all places
where it could lead to a problem.:)

[o1ka, 2016-03-21 13:30:51]

Android issues is fixed,please HOLD ON with the review though:
I do not quite like how raw pointer to AudioOutputDevice is returned now in
GetOutputDevice call, there will be another upload to fix that (we are
discussing the idea with guido@ now).

[o1ka, 2016-03-22 13:11:48]

Description was changed from

==========
Moving SwitchOutputDevice out of OutputDevice interface.
1) SwitchOutputDevice() is removed from OutputDevice interface;
2) OutputDevice inheritance for MediaStreamAudioRenderer implementations
is removed;
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from
now on AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) GetDeviceId() method is added to OutputDevice (at this stage it's
useful for unit tests only, but it will also be used for
session ID -> device ID mapping in the upcoming CL).

See bug description for mode details and a link to doc with pictures.

BUG=595727
==========

to

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 4 which eliminated OutputDevice interface).

BUG=595727
==========

[o1ka, 2016-03-22 13:21:20]

PTAL.
OutputDevice interface eliminated (see updated description).
Rationale:
(1) After removing SwitchOutputDevice, OutputDevice became a read-only interface
providing constant output device information.
(2) Since we were "chaining" GetOutputDevice() methods from upper sinks to
lower, the topmost sink was returning a bare AudiuOutputDevice pointer winch had
no guarantee of lifetime, taking into account the level of indirection.

OutputDeviceInfo is intentionally returned by value, to avoid any object
lifetime complications.

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

https://codereview.chromium.org/1809093003/diff/40001/content/renderer/media/...
content/renderer/media/audio_device_factory.h:98: virtual
media::AudioRendererSink* CreateAudioRendererMixerSink(
On 2016/03/21 12:01:36, Guido Urdaneta wrote:
> On 2016/03/21 11:50:31, o1ka wrote:
> > On 2016/03/21 11:09:20, Guido Urdaneta wrote:
> > > Shouldn't all these methods return scoped_refptr?
> > 
> > They are used in unit tests only now, initial design
(CreateAudioOutputDevice)
> > was returning raw pointer. Do you still think it's better to switch to
> > scoped_refptr?
> 
> Yes. If you are returning a pointer that is intended to be owned by the
caller,
> you should wrap it in a smart pointer that carries ownership.
AudioRendererSink
> is refcounted, so use scoped_refptr. That it's currently used only for tests
> shouldn't matter.

Done.

https://codereview.chromium.org/1809093003/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1809093003/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_device.cc:144: return OutputDeviceInfo(device_id_,
device_status_, output_params_);
OutputDeviceInfo is created on demand instead of being stored as a member of
AudioOutputDevice intentionally: it's a "view" of AOD, so I did not want to bind
AOD implementation to it.

[o1ka, 2016-03-22 13:22:57]

Description was changed from

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 4 which eliminated OutputDevice interface).

BUG=595727
==========

to

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 4 which eliminated OutputDevice interface).

BUG=595727
==========

[o1ka, 2016-03-22 13:28:42]

Description was changed from

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 4 which eliminated OutputDevice interface).

BUG=595727
==========

to

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 5 which eliminated OutputDevice interface).

BUG=595727
==========

[o1ka, 2016-03-22 14:12:53]

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

[o1ka, 2016-03-22 14:12:54]

adding miu@: PTAL at TrackAudioRenderer

[Guido Urdaneta, 2016-03-22 15:30:19]

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

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/audio_device_factory.h:78: static
media::OutputDeviceStatus GetOutputDeviceStatus(
What if we change this to return the whole OutputDeviceInfo?
I think it can be more useful while having the same cost.

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.h:123: void
SetAudioRendererSinkForTesting(media::AudioRendererSink* sink);
Remove this declaration as this method is now gone.

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:96: // We don't
care about separate sinks for this devices
s/this/these

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:100: // The
think to return device errror
I can't parse this comment. Fix it or remove it.

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
File content/renderer/media/track_audio_renderer.cc (right):

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/track_audio_renderer.cc:286: if (!(sink_ &&
source_params_.IsValid() && playing_))
nit: I think the OR form is easier to read, but up to you :)

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/track_audio_renderer.cc:304: const
media::AudioParameters& preferred_params = device_info.output_params();
preferred_params does not seem to be the best name for this variable since
sink_params are apparently preferred over preferred_params.
perhaps hardware_params?

https://codereview.chromium.org/1809093003/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_device.h (right):

https://codereview.chromium.org/1809093003/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_device.h:107: // Returns output device information.
This documentation is already in audio_renderer_sink.h.
Treat GetOutputDeviceInfo as yet another ARS method.

https://codereview.chromium.org/1809093003/diff/80001/media/base/mock_audio_r...
File media/base/mock_audio_renderer_sink.cc (right):

https://codereview.chromium.org/1809093003/diff/80001/media/base/mock_audio_r...
media/base/mock_audio_renderer_sink.cc:47: // NB: output device won't be changed
if no factory is provided at
Isn't it a lot simpler to just store the device_id in a variable and just change
it when SOD() is invoked?
You can control whether SOD() is enabled with a bool.
The factory approach looks too complicated for this.

https://codereview.chromium.org/1809093003/diff/80001/media/base/mock_audio_r...
File media/base/mock_audio_renderer_sink.h (right):

https://codereview.chromium.org/1809093003/diff/80001/media/base/mock_audio_r...
media/base/mock_audio_renderer_sink.h:23: virtual OutputDeviceInfo*
NewOutputDeviceInfo(
Why should this return pointers?
Why a factory for this?
Is there an implementation of this? I didn't see it.

https://codereview.chromium.org/1809093003/diff/80001/media/base/mock_audio_r...
media/base/mock_audio_renderer_sink.h:66: scoped_ptr<OutputDeviceInfo>
output_device_info_;
I think this shouldn't be a pointer.

[o1ka, 2016-03-22 16:13:56]

Thank you, guidou@!

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

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/audio_device_factory.h:78: static
media::OutputDeviceStatus GetOutputDeviceStatus(
On 2016/03/22 15:30:18, Guido Urdaneta wrote:
> What if we change this to return the whole OutputDeviceInfo?
> I think it can be more useful while having the same cost.

Done.

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.h:123: void
SetAudioRendererSinkForTesting(media::AudioRendererSink* sink);
On 2016/03/22 15:30:18, Guido Urdaneta wrote:
> Remove this declaration as this method is now gone.

Done.

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:96: // We don't
care about separate sinks for this devices
On 2016/03/22 15:30:18, Guido Urdaneta wrote:
> s/this/these

Done.

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:100: // The
think to return device errror
On 2016/03/22 15:30:18, Guido Urdaneta wrote:
> I can't parse this comment. Fix it or remove it.

Done.

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
File content/renderer/media/track_audio_renderer.cc (right):

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/track_audio_renderer.cc:286: if (!(sink_ &&
source_params_.IsValid() && playing_))
On 2016/03/22 15:30:18, Guido Urdaneta wrote:
> nit: I think the OR form is easier to read, but up to you :)

Done.

https://codereview.chromium.org/1809093003/diff/80001/content/renderer/media/...
content/renderer/media/track_audio_renderer.cc:304: const
media::AudioParameters& preferred_params = device_info.output_params();
On 2016/03/22 15:30:18, Guido Urdaneta wrote:
> preferred_params does not seem to be the best name for this variable since
> sink_params are apparently preferred over preferred_params.
> perhaps hardware_params?

Done.

https://codereview.chromium.org/1809093003/diff/80001/media/audio/audio_outpu...
File media/audio/audio_output_device.h (right):

https://codereview.chromium.org/1809093003/diff/80001/media/audio/audio_outpu...
media/audio/audio_output_device.h:107: // Returns output device information.
On 2016/03/22 15:30:18, Guido Urdaneta wrote:
> This documentation is already in audio_renderer_sink.h.
> Treat GetOutputDeviceInfo as yet another ARS method.

Done.

https://codereview.chromium.org/1809093003/diff/80001/media/base/mock_audio_r...
File media/base/mock_audio_renderer_sink.cc (right):

https://codereview.chromium.org/1809093003/diff/80001/media/base/mock_audio_r...
media/base/mock_audio_renderer_sink.cc:47: // NB: output device won't be changed
if no factory is provided at
On 2016/03/22 15:30:18, Guido Urdaneta wrote:
> Isn't it a lot simpler to just store the device_id in a variable and just
change
> it when SOD() is invoked?
> You can control whether SOD() is enabled with a bool.
> The factory approach looks too complicated for this.

It looks cool! :) But I removed it, since it's not used now.

https://codereview.chromium.org/1809093003/diff/80001/media/base/mock_audio_r...
File media/base/mock_audio_renderer_sink.h (right):

https://codereview.chromium.org/1809093003/diff/80001/media/base/mock_audio_r...
media/base/mock_audio_renderer_sink.h:23: virtual OutputDeviceInfo*
NewOutputDeviceInfo(
On 2016/03/22 15:30:18, Guido Urdaneta wrote:
> Why should this return pointers?
> Why a factory for this?
> Is there an implementation of this? I didn't see it.

Ok, removing this for now.

https://codereview.chromium.org/1809093003/diff/80001/media/base/mock_audio_r...
media/base/mock_audio_renderer_sink.h:66: scoped_ptr<OutputDeviceInfo>
output_device_info_;
On 2016/03/22 15:30:18, Guido Urdaneta wrote:
> I think this shouldn't be a pointer.

Done.

[Guido Urdaneta, 2016-03-22 16:34:37]

lgtm

[miu, 2016-03-24 21:32:02]

Looks great!  Comments:

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
File content/public/renderer/media_stream_audio_renderer.h (right):

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
content/public/renderer/media_stream_audio_renderer.h:41: // Must never be
called on the IO thread.
nit: You can probably omit the "never be called on the IO thread" comment.  It's
generally assumed a MediaStreamAudioRenderer's methods are all called on the
main thread.

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
content/public/renderer/media_stream_audio_renderer.h:49: // be invoked, so
users are advised to use media::BindToCurrentLoop() to
Because this is a public interface, it's usually best not to discuss internal
implementation details.  So, I'd suggest NOT mentioning "advised to use
media::BindToCurrentLoop()".  It's enough to just say that |callback| could be
run an any thread.

Alternatively, consider guaranteeing that, since this method is called on the
main thread, then the callback will also be run on the same thread. 
Thread-hopping is something the implementation has to do internally in order to
"get the job done," so philosophically this is probably not something we should
be burdening client code to deal with.

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
content/public/renderer/media_stream_audio_renderer.h:51: // Note also that copy
constructors and destructors for arguments bound to
You can remove lines 51-54.  This is generally understood throughout the
Chromium project (i.e., if it needs to be documented, it should be in
base/bind.h).

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

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
content/renderer/media/audio_device_factory.h:29: class CONTENT_EXPORT
AudioDeviceFactory {
Maybe now this class should be named AudioRendererFactory?

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.cc:64:
AudioDeviceFactory::NewAudioRendererMixerSink(source_render_frame_id, 0,
MUCH cleaner!  Thanks for fixing this.  :)

https://codereview.chromium.org/1809093003/diff/100001/media/base/audio_rende...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:49: OutputDeviceInfo GetOutputDeviceInfo();
Consider adding bug 506507 to the BUG= line in the change description, since
you've addressed the TODO.

https://codereview.chromium.org/1809093003/diff/100001/media/base/audio_rende...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/audio_rende...
media/base/audio_renderer_sink.h:68: // If the sink does not support output
device, |device_status| of
nit: s/does not support output device/is not an output device implementation/

https://codereview.chromium.org/1809093003/diff/100001/media/base/audio_rende...
media/base/audio_renderer_sink.h:94: // There is no guarantee about the thread
where |callback| will
ditto here (see comments in
content/public/renderer/media_stream_audio_renderer.h).

https://codereview.chromium.org/1809093003/diff/100001/media/base/fake_audio_...
File media/base/fake_audio_renderer_sink.cc (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/fake_audio_...
media/base/fake_audio_renderer_sink.cc:22: 16,
Side Note: At some point, we should evaluate whether the bits_per_sample
property in AudioParameters can be eliminated.  IMO, we should try to get rid of
it since audio data is passed around in planar float format everywhere.

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
File media/base/output_device_info.cc (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.cc:15:
AudioParameters(AudioParameters::AUDIO_FAKE,
OOC, why specify the AUDIO_FAKE device here?  Any particular reason why you
don't just use the zero-arg AudioParameters constructor call here?

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
File media/base/output_device_info.h (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.h:39: OutputDeviceInfo(const OutputDeviceInfo&
other);
nit: Copy constructor and assignment operator should be together (here, and in
.cc file).

[Henrik Grunell, 2016-03-25 10:44:40]

Really nice job this CL!

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

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
content/renderer/media/audio_device_factory.h:29: class CONTENT_EXPORT
AudioDeviceFactory {
On 2016/03/24 21:32:02, miu wrote:
> Maybe now this class should be named AudioRendererFactory?

A rename would be nice. It actually creates renderer sinks though, not
renderers, and InputDevice is still there. So some other name maybe?
AudioSinkFactory? (And rename file too if so.)

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager.h (left):

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager.h:137:
media::AudioRendererSink* sink_for_testing_;
This is great to get rid of. :)

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:131:
EXPECT_EQ(0, mixer_count());
Oh, you changed all these. Kudos! :)

https://codereview.chromium.org/1809093003/diff/100001/media/base/mock_audio_...
File media/base/mock_audio_renderer_sink.cc (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/mock_audio_...
media/base/mock_audio_renderer_sink.cc:14:
MockAudioRendererSink::MockAudioRendererSink(const std::string& device_id,
There seems to be one missing ctor?

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
File media/base/output_device_info.cc (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.cc:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
Ditto.

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
File media/base/output_device_info.h (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.h:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
I think "(c)" should be omitted. Check the style guide.

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.h:27: class MEDIA_EXPORT OutputDeviceInfo {
Add a comment with brief high-level description of the class.

[o1ka, 2016-03-29 11:01:58]

Description was changed from

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 5 which eliminated OutputDevice interface).

BUG=595727
==========

to

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 5 which eliminated OutputDevice interface).

BUG=595727, 506507
==========

[o1ka, 2016-03-29 13:39:54]

miu@ and grunell@: Thanks a lot for the comments! - Addressed now.
I also changed AudioDeviceFactory to produce AudioCapturerSource instead of
AudioInputDevice; need your advice on its naming, see
https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media....

PTAL

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
File content/public/renderer/media_stream_audio_renderer.h (right):

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
content/public/renderer/media_stream_audio_renderer.h:41: // Must never be
called on the IO thread.
On 2016/03/24 21:32:02, miu wrote:
> nit: You can probably omit the "never be called on the IO thread" comment. 
It's
> generally assumed a MediaStreamAudioRenderer's methods are all called on the
> main thread.

I would probably leave it as it is now: Since the method looks like a simple
getter, somebody can probably attempt to call it in the render pipeline to
calculate some parameters and will end up debugging all this stuff. It's no harm
to have an extra warning. WDYT?

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
content/public/renderer/media_stream_audio_renderer.h:49: // be invoked, so
users are advised to use media::BindToCurrentLoop() to
On 2016/03/24 21:32:02, miu wrote:
> Because this is a public interface, it's usually best not to discuss internal
> implementation details.  So, I'd suggest NOT mentioning "advised to use
> media::BindToCurrentLoop()".  It's enough to just say that |callback| could be
> run an any thread.
> 
> Alternatively, consider guaranteeing that, since this method is called on the
> main thread, then the callback will also be run on the same thread. 
> Thread-hopping is something the implementation has to do internally in order
to
> "get the job done," so philosophically this is probably not something we
should
> be burdening client code to deal with.

I just copied the old SwitchOutputDevice() comment here, and I agree it's TMI -
fixing it.
Yes, callbacks seem to always run on the same thread now, but I don't quite want
to weaken callback requirements now since the client code deals with that
anyways and we may probably need to utilize that again at some point.
WDYT?

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
content/public/renderer/media_stream_audio_renderer.h:51: // Note also that copy
constructors and destructors for arguments bound to
On 2016/03/24 21:32:02, miu wrote:
> You can remove lines 51-54.  This is generally understood throughout the
> Chromium project (i.e., if it needs to be documented, it should be in
> base/bind.h).

Done.

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

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
content/renderer/media/audio_device_factory.h:29: class CONTENT_EXPORT
AudioDeviceFactory {
On 2016/03/25 10:44:40, Henrik Grunell wrote:
> On 2016/03/24 21:32:02, miu wrote:
> > Maybe now this class should be named AudioRendererFactory?
> 
> A rename would be nice. It actually creates renderer sinks though, not
> renderers, and InputDevice is still there. So some other name maybe?
> AudioSinkFactory? (And rename file too if so.)

Now with my last change it creates AudioRendererSink and AudioCapturerSource.
I'm thinking of splitting it into two corresponding factories:
AudioRendererSinkFactory and AudioCapturerSourceFactory (and putting them into
two separate files).

Any objections and/or alternative proposals?

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:131:
EXPECT_EQ(0, mixer_count());
On 2016/03/25 10:44:40, Henrik Grunell wrote:
> Oh, you changed all these. Kudos! :)

That was easy :)

https://codereview.chromium.org/1809093003/diff/100001/media/base/audio_rende...
File media/base/audio_renderer_mixer.h (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/audio_rende...
media/base/audio_renderer_mixer.h:49: OutputDeviceInfo GetOutputDeviceInfo();
On 2016/03/24 21:32:02, miu wrote:
> Consider adding bug 506507 to the BUG= line in the change description, since
> you've addressed the TODO.

Done.

https://codereview.chromium.org/1809093003/diff/100001/media/base/audio_rende...
File media/base/audio_renderer_sink.h (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/audio_rende...
media/base/audio_renderer_sink.h:68: // If the sink does not support output
device, |device_status| of
On 2016/03/24 21:32:02, miu wrote:
> nit: s/does not support output device/is not an output device implementation/

(The sink needs to be associated with OD, but not necessarily implements it:
since we don't have OD interface, "implement" is not quite applicable any more.
Also, a sink can be build on top of another sink and thus it's associated with
OD through that lower sink.)

Fixed.

https://codereview.chromium.org/1809093003/diff/100001/media/base/audio_rende...
media/base/audio_renderer_sink.h:94: // There is no guarantee about the thread
where |callback| will
On 2016/03/24 21:32:02, miu wrote:
> ditto here (see comments in
> content/public/renderer/media_stream_audio_renderer.h).

Done.

https://codereview.chromium.org/1809093003/diff/100001/media/base/fake_audio_...
File media/base/fake_audio_renderer_sink.cc (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/fake_audio_...
media/base/fake_audio_renderer_sink.cc:22: 16,
On 2016/03/24 21:32:02, miu wrote:
> Side Note: At some point, we should evaluate whether the bits_per_sample
> property in AudioParameters can be eliminated.  IMO, we should try to get rid
of
> it since audio data is passed around in planar float format everywhere.

Yes, we should consider this.

https://codereview.chromium.org/1809093003/diff/100001/media/base/mock_audio_...
File media/base/mock_audio_renderer_sink.cc (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/mock_audio_...
media/base/mock_audio_renderer_sink.cc:14:
MockAudioRendererSink::MockAudioRendererSink(const std::string& device_id,
On 2016/03/25 10:44:40, Henrik Grunell wrote:
> There seems to be one missing ctor?

Removed it from the header.

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
File media/base/output_device_info.cc (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.cc:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
On 2016/03/25 10:44:40, Henrik Grunell wrote:
> Ditto.

Done.

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.cc:15:
AudioParameters(AudioParameters::AUDIO_FAKE,
On 2016/03/24 21:32:02, miu wrote:
> OOC, why specify the AUDIO_FAKE device here?  Any particular reason why you
> don't just use the zero-arg AudioParameters constructor call here?

This is a very good question. Default constructor creates some intentionally
invalid non-serializable paramters. On the other hand, if output device is not
available/not authorized, AudioRendererHost provides us some fake parameters. We
want to be consistent with AudioRendererHost here. I fixed it a little bit.

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
File media/base/output_device_info.h (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.h:1: // Copyright (c) 2016 The Chromium Authors.
All rights reserved.
On 2016/03/25 10:44:40, Henrik Grunell wrote:
> I think "(c)" should be omitted. Check the style guide.

Done.

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.h:27: class MEDIA_EXPORT OutputDeviceInfo {
On 2016/03/25 10:44:40, Henrik Grunell wrote:
> Add a comment with brief high-level description of the class.

Done.

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.h:39: OutputDeviceInfo(const OutputDeviceInfo&
other);
On 2016/03/24 21:32:02, miu wrote:
> nit: Copy constructor and assignment operator should be together (here, and in
> .cc file).

Done.

[miu, 2016-03-29 19:46:27]

lgtm

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
File content/public/renderer/media_stream_audio_renderer.h (right):

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
content/public/renderer/media_stream_audio_renderer.h:49: // be invoked, so
users are advised to use media::BindToCurrentLoop() to
On 2016/03/29 13:39:53, o1ka wrote:
> I just copied the old SwitchOutputDevice() comment here, and I agree it's TMI
-
> fixing it.
> Yes, callbacks seem to always run on the same thread now, but I don't quite
want
> to weaken callback requirements now since the client code deals with that
> anyways and we may probably need to utilize that again at some point.
> WDYT?

I suppose it's fine to leave it alone for this change.  However, for code
readability and maintainability, interfaces should avoid the "any old thread
will do" pattern.  They force people to write ugly code like this:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

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

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
content/renderer/media/audio_device_factory.h:29: class CONTENT_EXPORT
AudioDeviceFactory {
On 2016/03/29 13:39:54, o1ka wrote:
> On 2016/03/25 10:44:40, Henrik Grunell wrote:
> > On 2016/03/24 21:32:02, miu wrote:
> > > Maybe now this class should be named AudioRendererFactory?
> > 
> > A rename would be nice. It actually creates renderer sinks though, not
> > renderers, and InputDevice is still there. So some other name maybe?
> > AudioSinkFactory? (And rename file too if so.)
> 
> Now with my last change it creates AudioRendererSink and AudioCapturerSource.
> I'm thinking of splitting it into two corresponding factories:
> AudioRendererSinkFactory and AudioCapturerSourceFactory (and putting them into
> two separate files).
> 
> Any objections and/or alternative proposals?

SGTM, but given how big this change is, you might want to put a TODO comment
here and then follow-up with another change.  Up to you.

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
File media/base/output_device_info.cc (right):

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
media/base/output_device_info.cc:18:
AudioParameters(AudioParameters::UnavailableDeviceParams())) {}
I think you meant just this for the 3rd argument:

  AudioParameters::UnavailableDeviceParams()

(What you have here is also redundantly invoking the AudioParameters copy ctor.)

[DaleCurtis, 2016-03-29 19:57:43]

skimmed, looks pretty good, just a few questions.

https://codereview.chromium.org/1809093003/diff/120001/content/renderer/media...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1809093003/diff/120001/content/renderer/media...
content/renderer/media/webrtc_audio_renderer.cc:626:
DCHECK(device_info.device_status() == media::OUTPUT_DEVICE_STATUS_OK);
DCHECK_EQ?

https://codereview.chromium.org/1809093003/diff/120001/media/audio/audio_para...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/1809093003/diff/120001/media/audio/audio_para...
media/audio/audio_parameters.h:180: // Creates reasonable dummy parameters in
case no device is available.
Hmm, this meaning is a bit unclear. It seems to imply this specifically means
that when these parameters are seen they mean they are for an unavailable
device. Which doesn't seem strictly true. They only appear used in one place
too, maybe they should live there instead?

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
File media/base/output_device_info.h (right):

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
media/base/output_device_info.h:25: typedef
base::Callback<void(OutputDeviceStatus)> OutputDeviceStatusCB;
use 'using' instead.

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
media/base/output_device_info.h:41: OutputDeviceInfo& operator=(const
OutputDeviceInfo&);
There are a fair number of fields in this class. Chromium style typically avoids
assignment/pass by value except for primitive types. It seems we might want to
disallow copy and assign here and instead require that it's passed by
scoped_ptr? Is there a reason you went this route?

[o1ka, 2016-03-30 14:02:40]

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

[o1ka, 2016-03-30 14:02:40]

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

[o1ka, 2016-03-30 14:02:42]

Comments addressed/questions answered.


avi@chromium.org: need your RS on
content/public/renderer/media_stream_audio_renderer.h
content/renderer/render_frame_impl.cc
- Thanks!

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
File content/public/renderer/media_stream_audio_renderer.h (right):

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
content/public/renderer/media_stream_audio_renderer.h:49: // be invoked, so
users are advised to use media::BindToCurrentLoop() to
On 2016/03/29 19:46:27, miu wrote:
> On 2016/03/29 13:39:53, o1ka wrote:
> > I just copied the old SwitchOutputDevice() comment here, and I agree it's
TMI
> -
> > fixing it.
> > Yes, callbacks seem to always run on the same thread now, but I don't quite
> want
> > to weaken callback requirements now since the client code deals with that
> > anyways and we may probably need to utilize that again at some point.
> > WDYT?
> 
> I suppose it's fine to leave it alone for this change.  However, for code
> readability and maintainability, interfaces should avoid the "any old thread
> will do" pattern.  They force people to write ugly code like this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

Agree. We need to come back to it after a couple of other refactoring efforts.

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

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
content/renderer/media/audio_device_factory.h:29: class CONTENT_EXPORT
AudioDeviceFactory {
On 2016/03/29 19:46:27, miu wrote:
> On 2016/03/29 13:39:54, o1ka wrote:
> > On 2016/03/25 10:44:40, Henrik Grunell wrote:
> > > On 2016/03/24 21:32:02, miu wrote:
> > > > Maybe now this class should be named AudioRendererFactory?
> > > 
> > > A rename would be nice. It actually creates renderer sinks though, not
> > > renderers, and InputDevice is still there. So some other name maybe?
> > > AudioSinkFactory? (And rename file too if so.)
> > 
> > Now with my last change it creates AudioRendererSink and
AudioCapturerSource.
> > I'm thinking of splitting it into two corresponding factories:
> > AudioRendererSinkFactory and AudioCapturerSourceFactory (and putting them
into
> > two separate files).
> > 
> > Any objections and/or alternative proposals?
> 
> SGTM, but given how big this change is, you might want to put a TODO comment
> here and then follow-up with another change.  Up to you.

Yes, good idea, thanks!

https://codereview.chromium.org/1809093003/diff/120001/content/renderer/media...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1809093003/diff/120001/content/renderer/media...
content/renderer/media/webrtc_audio_renderer.cc:626:
DCHECK(device_info.device_status() == media::OUTPUT_DEVICE_STATUS_OK);
On 2016/03/29 19:57:43, DaleCurtis wrote:
> DCHECK_EQ?

Done.

https://codereview.chromium.org/1809093003/diff/120001/media/audio/audio_para...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/1809093003/diff/120001/media/audio/audio_para...
media/audio/audio_parameters.h:180: // Creates reasonable dummy parameters in
case no device is available.
On 2016/03/29 19:57:43, DaleCurtis wrote:
> Hmm, this meaning is a bit unclear. It seems to imply this specifically means
> that when these parameters are seen they mean they are for an unavailable
> device. Which doesn't seem strictly true. They only appear used in one place
> too, maybe they should live there instead?

Not quite in one place, it's actuallly freshly borrowed for here
https://codereview.chromium.org/1809093003/diff/120001/content/browser/render...
and used initially by AudioRendererHost only. We want to be consistent with it
and return the same values when the sink is no associated with a output
device(yet or at all) or output device is not available/not authorized.

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
File media/base/output_device_info.cc (right):

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
media/base/output_device_info.cc:18:
AudioParameters(AudioParameters::UnavailableDeviceParams())) {}
On 2016/03/29 19:46:27, miu wrote:
> I think you meant just this for the 3rd argument:
> 
>   AudioParameters::UnavailableDeviceParams()
> 
> (What you have here is also redundantly invoking the AudioParameters copy
ctor.)

Oops :) Sure, thanks!

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
File media/base/output_device_info.h (right):

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
media/base/output_device_info.h:25: typedef
base::Callback<void(OutputDeviceStatus)> OutputDeviceStatusCB;
On 2016/03/29 19:57:43, DaleCurtis wrote:
> use 'using' instead.

Done.

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
media/base/output_device_info.h:41: OutputDeviceInfo& operator=(const
OutputDeviceInfo&);
On 2016/03/29 19:57:43, DaleCurtis wrote:
> There are a fair number of fields in this class. Chromium style typically
avoids
> assignment/pass by value except for primitive types. It seems we might want to
> disallow copy and assign here and instead require that it's passed by
> scoped_ptr? Is there a reason you went this route?

OutputDeviceInfo is AudioParameters + int (device status) + std::string (device
ID). AudioParameters are returned by value everywhere, so it seems to be not a
big overhead to add the rest, and copying should be optimized(=omitted) by
compiler anyways. It seems not worth it to mess up with heap memory.

[o1ka, 2016-03-30 14:02:42]

Comments addressed/questions answered.


avi@chromium.org: need your RS on
content/public/renderer/media_stream_audio_renderer.h
content/renderer/render_frame_impl.cc
- Thanks!

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
File content/public/renderer/media_stream_audio_renderer.h (right):

https://codereview.chromium.org/1809093003/diff/100001/content/public/rendere...
content/public/renderer/media_stream_audio_renderer.h:49: // be invoked, so
users are advised to use media::BindToCurrentLoop() to
On 2016/03/29 19:46:27, miu wrote:
> On 2016/03/29 13:39:53, o1ka wrote:
> > I just copied the old SwitchOutputDevice() comment here, and I agree it's
TMI
> -
> > fixing it.
> > Yes, callbacks seem to always run on the same thread now, but I don't quite
> want
> > to weaken callback requirements now since the client code deals with that
> > anyways and we may probably need to utilize that again at some point.
> > WDYT?
> 
> I suppose it's fine to leave it alone for this change.  However, for code
> readability and maintainability, interfaces should avoid the "any old thread
> will do" pattern.  They force people to write ugly code like this:
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

Agree. We need to come back to it after a couple of other refactoring efforts.

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

https://codereview.chromium.org/1809093003/diff/100001/content/renderer/media...
content/renderer/media/audio_device_factory.h:29: class CONTENT_EXPORT
AudioDeviceFactory {
On 2016/03/29 19:46:27, miu wrote:
> On 2016/03/29 13:39:54, o1ka wrote:
> > On 2016/03/25 10:44:40, Henrik Grunell wrote:
> > > On 2016/03/24 21:32:02, miu wrote:
> > > > Maybe now this class should be named AudioRendererFactory?
> > > 
> > > A rename would be nice. It actually creates renderer sinks though, not
> > > renderers, and InputDevice is still there. So some other name maybe?
> > > AudioSinkFactory? (And rename file too if so.)
> > 
> > Now with my last change it creates AudioRendererSink and
AudioCapturerSource.
> > I'm thinking of splitting it into two corresponding factories:
> > AudioRendererSinkFactory and AudioCapturerSourceFactory (and putting them
into
> > two separate files).
> > 
> > Any objections and/or alternative proposals?
> 
> SGTM, but given how big this change is, you might want to put a TODO comment
> here and then follow-up with another change.  Up to you.

Yes, good idea, thanks!

https://codereview.chromium.org/1809093003/diff/120001/content/renderer/media...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/1809093003/diff/120001/content/renderer/media...
content/renderer/media/webrtc_audio_renderer.cc:626:
DCHECK(device_info.device_status() == media::OUTPUT_DEVICE_STATUS_OK);
On 2016/03/29 19:57:43, DaleCurtis wrote:
> DCHECK_EQ?

Done.

https://codereview.chromium.org/1809093003/diff/120001/media/audio/audio_para...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/1809093003/diff/120001/media/audio/audio_para...
media/audio/audio_parameters.h:180: // Creates reasonable dummy parameters in
case no device is available.
On 2016/03/29 19:57:43, DaleCurtis wrote:
> Hmm, this meaning is a bit unclear. It seems to imply this specifically means
> that when these parameters are seen they mean they are for an unavailable
> device. Which doesn't seem strictly true. They only appear used in one place
> too, maybe they should live there instead?

Not quite in one place, it's actuallly freshly borrowed for here
https://codereview.chromium.org/1809093003/diff/120001/content/browser/render...
and used initially by AudioRendererHost only. We want to be consistent with it
and return the same values when the sink is no associated with a output
device(yet or at all) or output device is not available/not authorized.

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
File media/base/output_device_info.cc (right):

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
media/base/output_device_info.cc:18:
AudioParameters(AudioParameters::UnavailableDeviceParams())) {}
On 2016/03/29 19:46:27, miu wrote:
> I think you meant just this for the 3rd argument:
> 
>   AudioParameters::UnavailableDeviceParams()
> 
> (What you have here is also redundantly invoking the AudioParameters copy
ctor.)

Oops :) Sure, thanks!

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
File media/base/output_device_info.h (right):

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
media/base/output_device_info.h:25: typedef
base::Callback<void(OutputDeviceStatus)> OutputDeviceStatusCB;
On 2016/03/29 19:57:43, DaleCurtis wrote:
> use 'using' instead.

Done.

https://codereview.chromium.org/1809093003/diff/120001/media/base/output_devi...
media/base/output_device_info.h:41: OutputDeviceInfo& operator=(const
OutputDeviceInfo&);
On 2016/03/29 19:57:43, DaleCurtis wrote:
> There are a fair number of fields in this class. Chromium style typically
avoids
> assignment/pass by value except for primitive types. It seems we might want to
> disallow copy and assign here and instead require that it's passed by
> scoped_ptr? Is there a reason you went this route?

OutputDeviceInfo is AudioParameters + int (device status) + std::string (device
ID). AudioParameters are returned by value everywhere, so it seems to be not a
big overhead to add the rest, and copying should be optimized(=omitted) by
compiler anyways. It seems not worth it to mess up with heap memory.

[Avi (use Gerrit), 2016-03-30 16:13:14]

lgtm

stampity stamp

[DaleCurtis, 2016-03-31 15:55:45]

defer to miu@, grunell@ and guidou@ who I'm sure have this covered :) -- rs lgtm

[miu, 2016-03-31 19:26:26]

still lgtm

[o1ka, 2016-04-01 07:43:50]

The CQ bit was checked by olka@chromium.org

[o1ka, 2016-04-01 07:43:51]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org
Link to the patchset: https://codereview.chromium.org/1809093003/#ps140001
(title: "Comments addressed")

[commit-bot: I haz the power, 2016-04-01 07:44:13]

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

[commit-bot: I haz the power, 2016-04-01 07:47:00]

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

[commit-bot: I haz the power, 2016-04-01 07:47:03]

Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[o1ka, 2016-04-01 09:40:33]

The CQ bit was checked by olka@chromium.org

[o1ka, 2016-04-01 09:40:33]

The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org,
guidou@chromium.org, miu@chromium.org, dalecurtis@chromium.org
Link to the patchset: https://codereview.chromium.org/1809093003/#ps160001
(title: "rebase")

[commit-bot: I haz the power, 2016-04-01 09:40:52]

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

[commit-bot: I haz the power, 2016-04-01 11:42:21]

Description was changed from

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 5 which eliminated OutputDevice interface).

BUG=595727, 506507
==========

to

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 5 which eliminated OutputDevice interface).

BUG=595727, 506507
==========

[commit-bot: I haz the power, 2016-04-01 11:42:23]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-04-01 11:43:47]

Description was changed from

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 5 which eliminated OutputDevice interface).

BUG=595727, 506507
==========

to

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 5 which eliminated OutputDevice interface).

BUG=595727, 506507

Committed: https://crrev.com/68b69396916a10dbace194d871c854302bd98347
Cr-Commit-Position: refs/heads/master@{#384550}
==========

[commit-bot: I haz the power, 2016-04-01 11:43:49]

Patchset 9 (id:??) landed as
https://crrev.com/68b69396916a10dbace194d871c854302bd98347
Cr-Commit-Position: refs/heads/master@{#384550}

[Henrik Grunell, 2016-04-04 11:15:29]

Sorry for the post-commit final review. :)

lgtm, but I wonder if you missed one of my comments?

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
File media/base/output_device_info.h (right):

https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
media/base/output_device_info.h:27: class MEDIA_EXPORT OutputDeviceInfo {
On 2016/03/29 13:39:54, o1ka wrote:
> On 2016/03/25 10:44:40, Henrik Grunell wrote:
> > Add a comment with brief high-level description of the class.
> 
> Done.

I don't see the comment/description.

[o1ka, 2016-04-04 11:18:04]

On 2016/04/04 11:15:29, Henrik Grunell wrote:
> Sorry for the post-commit final review. :)
> 
> lgtm, but I wonder if you missed one of my comments?
> 
>
https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
> File media/base/output_device_info.h (right):
> 
>
https://codereview.chromium.org/1809093003/diff/100001/media/base/output_devi...
> media/base/output_device_info.h:27: class MEDIA_EXPORT OutputDeviceInfo {
> On 2016/03/29 13:39:54, o1ka wrote:
> > On 2016/03/25 10:44:40, Henrik Grunell wrote:
> > > Add a comment with brief high-level description of the class.
> > 
> > Done.
> 
> I don't see the comment/description.

Oops, it got lost somewhere along the way. Will add in the next CL.

[o1ka, 2016-04-05 13:43:00]

Description was changed from

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 5 which eliminated OutputDevice interface).

BUG=595727, 506507

Committed: https://crrev.com/68b69396916a10dbace194d871c854302bd98347
Cr-Commit-Position: refs/heads/master@{#384550}
==========

to

==========
Getting rid of OutputDevice interface, moving SwitchOutputDevice() to
AudioRendererSink and MediaStreamAudioRenderer interfaces.

1) OutputDevice interface eliminated;
2) OutputDeviceInfo data structure introduced; besides output parameters and
device status, it provides device ID information now (at this stage it's useful
for unit tests only, but it will also be used for session ID -> device ID
mapping in the upcoming CL).
3) AudioRendererSink::GetOutputDevice() is replaced with
AudioRendererSink::GetOutputDeviceInfo(), which returns a snapshot of output
device information.
3) SwitchOutputDevice() method added to  MediaStreamAudioRenderer;
4) SwitchOutputDevice() method added to AudioRendererSink;
5) Mocks and unit tests cleaned up; sink_for_testing_ is removed from
AudioRendererMixerManager, mock AudioDeviceFactory is used instead to
instantiate sinks for its testing;
6) AudioRendererMixer is dependent on AudioOutputDevice no more; from now on
AudioOutputDevice is visible to upper layers as AudioRendererSink only;
7) AudioOutputDevice is the only sink which implements GetOutputDeviceInfo(),
all the other sinks redirect the call to their bottom layer.

See bug description for mode details and a link to doc with pictures (a bit
outdated since Patchset 5 which eliminated OutputDevice interface).

BUG=595727, 506507

Committed: https://crrev.com/68b69396916a10dbace194d871c854302bd98347
Cr-Commit-Position: refs/heads/master@{#384550}
==========