Looking up device id by session id for AudioRendererMixerInput

Looking up device id by session id for AudioRendererMixerInput

This is required for being able to correctly mix in WebRtc audio in case
of unified streams, when session id is used to specify output device
matching an open input device.

1) Modifications to AudioRendererHost and AudioOutputDevice to retrieve
the matched device id in OnRequestDeviceAuthorization()/OnDeviceAuthorized()

-- This is only manually tested so far. I tried to come up with a neat unit
test but AudioRendererHost is lacking mocks (uses "for test" settings of
its dependencies instead) so I got lost there. As much as I want to have the
unit test in place, I would prefer to not spend any more time on this now.
The change is pretty straight forward. Please let me know if you think
the unit test is a must-have.

2) AudioRendereMixer key (in AudioRendererMixerManager) is modified
to take into account default device specifics.

BUG=587457
Committed: https://crrev.com/121c63d12d10be3dab0ebb032595f2bea95c42c8
Cr-Commit-Position: refs/heads/master@{#386652}

[o1ka, 2016-03-07 15:25:48]

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

[o1ka, 2016-03-07 15:25:49]

Please take a look.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:21:
AudioRendererMixerManager::AudioRendererMixerManager(){};
nit: I'll add a space before {}

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:116: return (a.device_id <
b.device_id);
nit: I'll remove brackets.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_platform_audio_output.cc (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_platform_audio_output.cc:78: const std::string&
matched_deviec_id) {
nit: matched_device_id

[DaleCurtis, 2016-03-07 23:10:07]

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

[DaleCurtis, 2016-03-07 23:10:07]

guidou@ wrote a lot of this code, so including him for the main review.

[o1ka, 2016-03-08 06:06:28]

On 2016/03/07 23:10:07, DaleCurtis wrote:
> guidou@ wrote a lot of this code, so including him for the main review.

Thank you! I was going to include guidou@ but missed somehow.

[Guido Urdaneta, 2016-03-08 14:53:10]

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_device_factory.h (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_device_factory.h:42: // Creates a sink for
AudioRemdererMixer.
typo: s/Remderer/Renderer

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:161: // AudioOutputDevice
is the only interface we have to communicate with output
Comment no longer matches code. However, I think you can continue to use
AudioOutputDevice (or maybe AudioRendererSink) here, and use the DeviceId method
to OutputDevice.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:172: // TODO(olka):
temporary cash for future reuse. http://crbug.com/586161
typo: s/cash/cache I think, although this doesn't look much like a cache to me.

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_manager_b...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:81: bool
AudioManagerBase::IsDefaultDeviceId(const std::string& device_id) {
Change ARH to use this one instead of the identical function defined there?

https://codereview.chromium.org/1769933002/diff/1/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer_sink.h (right):

https://codereview.chromium.org/1769933002/diff/1/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_sink.h:14: class AudioRendererMixerSink : public
AudioRendererSink {
Why not add GetDeviceId() to OutputDevice instead of introducing a new interface
just to provide the device ID, especially considering that a device ID looks
more related to an OutputDevice than to a Mixer.

https://codereview.chromium.org/1769933002/diff/1/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_sink.h:19: ~AudioRendererMixerSink() override{};
space after override.

https://codereview.chromium.org/1769933002/diff/1/media/base/mock_audio_rende...
File media/base/mock_audio_renderer_sink.h (right):

https://codereview.chromium.org/1769933002/diff/1/media/base/mock_audio_rende...
media/base/mock_audio_renderer_sink.h:21: // use MockAudioRendererMixerSink
Is there a crbug for this?

[Henrik Grunell, 2016-03-08 21:09:57]

Good stuff, adding a few comments to the previous reviewers'.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:38:
scoped_refptr<media::AudioRendererMixerSink> sink_from_session_id =
I know we have been discussing this offline before, but for the record: there
isn't a more elegant way we could add for getting the device ID instead creating
a new sink object?

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:48: // |sesson_id| can
(and should) be safely ignored.
Typo: session_id

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:172: // TODO(olka):
temporary cash for future reuse. http://crbug.com/586161
On 2016/03/08 14:53:09, Guido Urdaneta wrote:
> typo: s/cash/cache I think, although this doesn't look much like a cache to
me.

Yeah it's a typo. The todo is for adding caching, which is not done now.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:55: int session_id,
Is |session_id| optional or compulsory? Seems like we only care about it if
|device_id| is empty. Add info in the comment.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:29: static const
char* const kDefaultDeviceId =
Why not use media::AudioManagerBase::kDefaultDeviceId directly?

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:285:
EXPECT_CALL(*mock_sink_matched_device_.get(), Start()).Times(1);
Nit/Optional: I don't think you need .Times(1).

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:292:
EXPECT_EQ(mixer_count(), 0);
The syntax is actually EXPECT_EQ(expected, actual), i.e. flip the arguments.
Here and elsewhere. I see it's incorrect in the whole file, no need to change
already existing code.

https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:297: "",
kSecurityOrigin));
Nit: Does it take an std::string? Change "" -> std::string(). Elsewhere too.

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_manager_b...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:81: bool
AudioManagerBase::IsDefaultDeviceId(const std::string& device_id) {
Move this so that the order of member functions is the same as in the header
file.

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_output_de...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:323: // even in this case. It means
|session_id_| failed and the default device
What does "|session_id_| failed" mean?

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_output_de...
File media/audio/audio_output_device.h (right):

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.h:124: std::string GetDeviceId() override;
Add comment with description.

https://codereview.chromium.org/1769933002/diff/1/media/base/mock_audio_rende...
File media/base/mock_audio_renderer_sink.cc (right):

https://codereview.chromium.org/1769933002/diff/1/media/base/mock_audio_rende...
media/base/mock_audio_renderer_sink.cc:10: //--- MockAudioRendererSink
Remove comment.

https://codereview.chromium.org/1769933002/diff/1/media/base/mock_audio_rende...
media/base/mock_audio_renderer_sink.cc:28: //--- MockAudioRendererMixerSink
Remove comment.

https://codereview.chromium.org/1769933002/diff/1/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1769933002/diff/1/media/media.gyp#newcode269
media/media.gyp:269: 'base/audio_renderer_mixer_sink.h',
Also add to gn file.

[o1ka, 2016-03-09 15:25:53]

Thank you all for the input, before continuing with the review I'd like to
discuss the approach to introduce GetDeviceId().
I've sent you all a doc with scary pictures to discuss.

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

Description was changed from

==========
Looking up device id by session id for AudioRendererMixerInput

This is required for being able to correctly mix in WebRtc audio in case
of unified streams, when session id is used to specify output device
matching an open input device.

1) Modifications to AudioRendererHost and AudioOutputDevice to retrieve
the matched device id in OnRequestDeviceAuthorization()/OnDeviceAuthorized()

-- This is only manually tested so far. I tried to come up with a neat unit
test but AudioRendererHost is lacking mocks (uses "for test" settings of
its dependencies instead) so I got lost there. As much as I want to have the
unit test in place, I would prefer to not spend any more time on this now.
The change is pretty straight forward. Please let me know if you think
the unit test is a must-have.

2) AudioRendererMixerSink interface added to provide GetDeviceId() method.
This interface also absracts AudioOutputDevice for AudioRendererMixer,
which reduces dependencies for AudioRendererMixer unit tests.

3) sink_for_testing_ is removed from AudioRendererMixerManager, mock
AudioDeviceFactory is used instead to instantiate sinks for its testing.

4) AudioRendereMixer key (in AudioRendererMixerManager) is modified
to take into account default device specifics.

5) AudioRendererSink mocks refactored to be more reusable.


BUG=587457
==========

to

==========
Looking up device id by session id for AudioRendererMixerInput

This is required for being able to correctly mix in WebRtc audio in case
of unified streams, when session id is used to specify output device
matching an open input device.

1) Modifications to AudioRendererHost and AudioOutputDevice to retrieve
the matched device id in OnRequestDeviceAuthorization()/OnDeviceAuthorized()

-- This is only manually tested so far. I tried to come up with a neat unit
test but AudioRendererHost is lacking mocks (uses "for test" settings of
its dependencies instead) so I got lost there. As much as I want to have the
unit test in place, I would prefer to not spend any more time on this now.
The change is pretty straight forward. Please let me know if you think
the unit test is a must-have.

2) AudioRendererMixerSink interface added to provide GetDeviceId() method.
This interface also absracts AudioOutputDevice for AudioRendererMixer,
which reduces dependencies for AudioRendererMixer unit tests.

3) sink_for_testing_ is removed from AudioRendererMixerManager, mock
AudioDeviceFactory is used instead to instantiate sinks for its testing.

4) AudioRendereMixer key (in AudioRendererMixerManager) is modified
to take into account default device specifics.

5) AudioRendererSink mocks refactored to be more reusable.


BUG=587457
==========

[o1ka, 2016-04-05 15:02:55]

Description was changed from

==========
Looking up device id by session id for AudioRendererMixerInput

This is required for being able to correctly mix in WebRtc audio in case
of unified streams, when session id is used to specify output device
matching an open input device.

1) Modifications to AudioRendererHost and AudioOutputDevice to retrieve
the matched device id in OnRequestDeviceAuthorization()/OnDeviceAuthorized()

-- This is only manually tested so far. I tried to come up with a neat unit
test but AudioRendererHost is lacking mocks (uses "for test" settings of
its dependencies instead) so I got lost there. As much as I want to have the
unit test in place, I would prefer to not spend any more time on this now.
The change is pretty straight forward. Please let me know if you think
the unit test is a must-have.

2) AudioRendererMixerSink interface added to provide GetDeviceId() method.
This interface also absracts AudioOutputDevice for AudioRendererMixer,
which reduces dependencies for AudioRendererMixer unit tests.

3) sink_for_testing_ is removed from AudioRendererMixerManager, mock
AudioDeviceFactory is used instead to instantiate sinks for its testing.

4) AudioRendereMixer key (in AudioRendererMixerManager) is modified
to take into account default device specifics.

5) AudioRendererSink mocks refactored to be more reusable.


BUG=587457
==========

to

==========
Looking up device id by session id for AudioRendererMixerInput

This is required for being able to correctly mix in WebRtc audio in case
of unified streams, when session id is used to specify output device
matching an open input device.

1) Modifications to AudioRendererHost and AudioOutputDevice to retrieve
the matched device id in OnRequestDeviceAuthorization()/OnDeviceAuthorized()

-- This is only manually tested so far. I tried to come up with a neat unit
test but AudioRendererHost is lacking mocks (uses "for test" settings of
its dependencies instead) so I got lost there. As much as I want to have the
unit test in place, I would prefer to not spend any more time on this now.
The change is pretty straight forward. Please let me know if you think
the unit test is a must-have.

2) AudioRendereMixer key (in AudioRendererMixerManager) is modified
to take into account default device specifics.



BUG=587457
==========

[o1ka, 2016-04-05 15:13:39]

PTAL
CL updated taking into account OutputDevice->OutputDeviceInfo replacement,
comments addressed; also looks like there was race in AOD which I hopefully
fixed.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_device_factory.h (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_device_factory.h:42: // Creates a sink for
AudioRemdererMixer.
On 2016/03/08 14:53:09, Guido Urdaneta wrote:
> typo: s/Remderer/Renderer

Done.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager.cc (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:21:
AudioRendererMixerManager::AudioRendererMixerManager(){};
On 2016/03/07 15:25:49, o1ka wrote:
> nit: I'll add a space before {}

Done.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:38:
scoped_refptr<media::AudioRendererMixerSink> sink_from_session_id =
On 2016/03/08 21:09:56, Henrik Grunell wrote:
> I know we have been discussing this offline before, but for the record: there
> isn't a more elegant way we could add for getting the device ID instead
creating
> a new sink object?

No, not for now.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:48: // |sesson_id| can
(and should) be safely ignored.
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> Typo: session_id

Done.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.cc:161: // AudioOutputDevice
is the only interface we have to communicate with output
On 2016/03/08 14:53:09, Guido Urdaneta wrote:
> Comment no longer matches code. However, I think you can continue to use
> AudioOutputDevice (or maybe AudioRendererSink) here, and use the DeviceId
method
> to OutputDevice.

The method does not exist any more

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager.h (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:55: int session_id,
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> Is |session_id| optional or compulsory? Seems like we only care about it if
> |device_id| is empty. Add info in the comment.

Done.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager.h:116: return (a.device_id <
b.device_id);
On 2016/03/07 15:25:49, o1ka wrote:
> nit: I'll remove brackets.

Done.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
File content/renderer/media/audio_renderer_mixer_manager_unittest.cc (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:29: static const
char* const kDefaultDeviceId =
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> Why not use media::AudioManagerBase::kDefaultDeviceId directly?

Because this is shorter?

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:285:
EXPECT_CALL(*mock_sink_matched_device_.get(), Start()).Times(1);
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> Nit/Optional: I don't think you need .Times(1).

Acknowledged.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:292:
EXPECT_EQ(mixer_count(), 0);
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> The syntax is actually EXPECT_EQ(expected, actual), i.e. flip the arguments.
> Here and elsewhere. I see it's incorrect in the whole file, no need to change
> already existing code.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

Done.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/media/audi...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:297: "",
kSecurityOrigin));
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> Nit: Does it take an std::string? Change "" -> std::string(). Elsewhere too.

Done.

https://codereview.chromium.org/1769933002/diff/1/content/renderer/pepper/pep...
File content/renderer/pepper/pepper_platform_audio_output.cc (right):

https://codereview.chromium.org/1769933002/diff/1/content/renderer/pepper/pep...
content/renderer/pepper/pepper_platform_audio_output.cc:78: const std::string&
matched_deviec_id) {
On 2016/03/07 15:25:49, o1ka wrote:
> nit: matched_device_id

Done.

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_manager_b...
File media/audio/audio_manager_base.cc (right):

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:81: bool
AudioManagerBase::IsDefaultDeviceId(const std::string& device_id) {
On 2016/03/08 14:53:09, Guido Urdaneta wrote:
> Change ARH to use this one instead of the identical function defined there?

Done.

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_manager_b...
media/audio/audio_manager_base.cc:81: bool
AudioManagerBase::IsDefaultDeviceId(const std::string& device_id) {
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> Move this so that the order of member functions is the same as in the header
> file.

Which order is broken?

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_output_de...
File media/audio/audio_output_device.cc (right):

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:323: // even in this case. It means
|session_id_| failed and the default device
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> What does "|session_id_| failed" mean?

Done.

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.cc:327: matched_device_id_ = matched_device_id;
This races with a getter: |matched_device_id_| should be set before
did_receive_auth_.Signal().

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_output_de...
File media/audio/audio_output_device.h (right):

https://codereview.chromium.org/1769933002/diff/1/media/audio/audio_output_de...
media/audio/audio_output_device.h:124: std::string GetDeviceId() override;
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> Add comment with description.

Acknowledged.

https://codereview.chromium.org/1769933002/diff/1/media/base/audio_renderer_m...
File media/base/audio_renderer_mixer_sink.h (right):

https://codereview.chromium.org/1769933002/diff/1/media/base/audio_renderer_m...
media/base/audio_renderer_mixer_sink.h:14: class AudioRendererMixerSink : public
AudioRendererSink {
On 2016/03/08 14:53:09, Guido Urdaneta wrote:
> Why not add GetDeviceId() to OutputDevice instead of introducing a new
interface
> just to provide the device ID, especially considering that a device ID looks
> more related to an OutputDevice than to a Mixer.

Addressed in another CL

https://codereview.chromium.org/1769933002/diff/1/media/base/mock_audio_rende...
File media/base/mock_audio_renderer_sink.cc (right):

https://codereview.chromium.org/1769933002/diff/1/media/base/mock_audio_rende...
media/base/mock_audio_renderer_sink.cc:10: //--- MockAudioRendererSink
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> Remove comment.

Acknowledged.

https://codereview.chromium.org/1769933002/diff/1/media/base/mock_audio_rende...
media/base/mock_audio_renderer_sink.cc:28: //--- MockAudioRendererMixerSink
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> Remove comment.

Acknowledged.

https://codereview.chromium.org/1769933002/diff/1/media/base/mock_audio_rende...
File media/base/mock_audio_renderer_sink.h (right):

https://codereview.chromium.org/1769933002/diff/1/media/base/mock_audio_rende...
media/base/mock_audio_renderer_sink.h:21: // use MockAudioRendererMixerSink
On 2016/03/08 14:53:09, Guido Urdaneta wrote:
> Is there a crbug for this?

No more MockAudioRendererMixerSink.

https://codereview.chromium.org/1769933002/diff/1/media/media.gyp
File media/media.gyp (right):

https://codereview.chromium.org/1769933002/diff/1/media/media.gyp#newcode269
media/media.gyp:269: 'base/audio_renderer_mixer_sink.h',
On 2016/03/08 21:09:57, Henrik Grunell wrote:
> Also add to gn file.

Not applicable any more: mixer sink has gone.

[Guido Urdaneta, 2016-04-07 11:14:28]

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

https://codereview.chromium.org/1769933002/diff/20001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.cc:33: std::string
new_device_id(device_id);
We can avoid this string copy.
The code will be slightly less clean, but I think it would be worth it.

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

https://codereview.chromium.org/1769933002/diff/20001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:391:
TEST_F(AudioRendererMixerManagerTest, OneMixerDiffferenrOriginsDefaultDevice) {
typo: s/Diffferenr/Different

[o1ka, 2016-04-07 12:40:49]

guidou@ - Thanks for comments! Addressed.

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

https://codereview.chromium.org/1769933002/diff/20001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.cc:33: std::string
new_device_id(device_id);
On 2016/04/07 11:14:28, Guido Urdaneta wrote:
> We can avoid this string copy.
> The code will be slightly less clean, but I think it would be worth it.

Done.

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

https://codereview.chromium.org/1769933002/diff/20001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager_unittest.cc:391:
TEST_F(AudioRendererMixerManagerTest, OneMixerDiffferenrOriginsDefaultDevice) {
On 2016/04/07 11:14:28, Guido Urdaneta wrote:
> typo: s/Diffferenr/Different
Wow, what a typo!
Done.

[Guido Urdaneta, 2016-04-07 15:27:03]

lgtm, but consider removing the string copy that remains.

[Guido Urdaneta, 2016-04-07 15:27:15]

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

https://codereview.chromium.org/1769933002/diff/40001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.cc:42:
device_id_from_session =
You can also avoid this copy.
Save the callbacks produced by base::Bind in local variables before the if, and
call the ARMI constructor inside the if (and else), passing the device_id
directly.

[o1ka, 2016-04-07 15:56:01]

On 2016/04/07 15:27:15, Guido Urdaneta wrote:
>
https://codereview.chromium.org/1769933002/diff/40001/content/renderer/media/...
> File content/renderer/media/audio_renderer_mixer_manager.cc (right):
> 
>
https://codereview.chromium.org/1769933002/diff/40001/content/renderer/media/...
> content/renderer/media/audio_renderer_mixer_manager.cc:42:
> device_id_from_session =
> You can also avoid this copy.
> Save the callbacks produced by base::Bind in local variables before the if,
and
> call the ARMI constructor inside the if (and else), passing the device_id
> directly.

Thanks! Done.

[o1ka, 2016-04-08 12:50:58]

PleeeeaseTAL

Patch Set 5 : Switch to use AudioDeviceFactory::GetOutputDevice() in
AudioRendererMixerManager; AudioDeviceFactory cleanup.

[tommi (sloooow) - chröme, 2016-04-08 13:47:48]

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

https://codereview.chromium.org/1769933002/diff/80001/content/renderer/media/...
content/renderer/media/audio_device_factory.h:82: static const
media::OutputDeviceInfo GetOutputDeviceInfo(
why this change?  the caller can make the variable const if required, right?
...or did you mean to return by const ref?

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

https://codereview.chromium.org/1769933002/diff/80001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.cc:33: const
media::AudioRendererMixerInput::GetMixerCB& get_mixer_cb =
nit: the "&" is a bit misleading since this will become a reference to an
anonymous temporary variable.  You might as well make this the variable.

https://codereview.chromium.org/1769933002/diff/80001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.cc:38:
base::Unretained(this), source_render_frame_id);
sure you want to use base::Unretained?  how are we protected against use after
free?

[o1ka, 2016-04-08 19:07:25]

tommi@: comments addressed - thanks!

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

https://codereview.chromium.org/1769933002/diff/80001/content/renderer/media/...
content/renderer/media/audio_device_factory.h:82: static const
media::OutputDeviceInfo GetOutputDeviceInfo(
On 2016/04/08 13:47:47, tommi-sloooow wrote:
> why this change?  the caller can make the variable const if required, right?
> ...or did you mean to return by const ref?

No-no, no ref. Removing const to avoid confusion.

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

https://codereview.chromium.org/1769933002/diff/80001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.cc:33: const
media::AudioRendererMixerInput::GetMixerCB& get_mixer_cb =
On 2016/04/08 13:47:47, tommi-sloooow wrote:
> nit: the "&" is a bit misleading since this will become a reference to an
> anonymous temporary variable.  You might as well make this the variable.

I just removed locals :)

https://codereview.chromium.org/1769933002/diff/80001/content/renderer/media/...
content/renderer/media/audio_renderer_mixer_manager.cc:38:
base::Unretained(this), source_render_frame_id);
On 2016/04/08 13:47:47, tommi-sloooow wrote:
> sure you want to use base::Unretained?  how are we protected against use after
> free?

Added a comment

[o1ka, 2016-04-11 09:58:02]

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

[o1ka, 2016-04-11 09:58:03]

nasko@chromium.org: Need your RS on
content/common/media/audio_messages.h
content/renderer/pepper/pepper_platform_audio_output.h
content/renderer/pepper/pepper_platform_audio_output.cc

DaleCurtis@: WDYT of AudioRendererMixerManager?

tommi@ - please see your comments addressed.

Thanks to all.

[tommi (sloooow) - chröme, 2016-04-11 10:50:42]

lgtm

[Henrik Grunell, 2016-04-11 16:56:57]

lgtm

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

https://codereview.chromium.org/1769933002/diff/100001/media/base/output_devi...
media/base/output_device_info.h:28: // AudioRendererSink::GetOutputDeviceInfo()
Thanks!

[DaleCurtis, 2016-04-11 17:45:22]

media/audio is traditionally browser side only; so seeing it used renderer side
is a bit confusing. I wonder if instead we should have something like
media/audio/renderer or just refactor those functions into media/base? I'm okay
with a TODO for now, but I don't think we should blur the layering like this for
very long.

lgtm % todo+bug and cleanup or doing that now.

[nasko, 2016-04-11 18:54:27]

IPC LGTM

[o1ka, 2016-04-12 10:24:16]

On 2016/04/11 17:45:22, DaleCurtis wrote:
> media/audio is traditionally browser side only; so seeing it used renderer
side
> is a bit confusing. I wonder if instead we should have something like
> media/audio/renderer or just refactor those functions into media/base? I'm
okay
> with a TODO for now, but I don't think we should blur the layering like this
for
> very long.
> 
> lgtm % todo+bug and cleanup or doing that now.

Agree that AudioManagerBase dependency is not nice. Updated with TODO/bug
(https://codereview.chromium.org/1769933002/#ps120001)
Will do in a followup CL to keep this one clean, since moving those device IDs
out
of AudiomanagerBase will cause a lot of renaming.

Thanks everybody for the review.

[o1ka, 2016-04-12 10:26:31]

The CQ bit was checked by olka@chromium.org

[o1ka, 2016-04-12 10:26:31]

The patchset sent to the CQ was uploaded after l-g-t-m from guidou@chromium.org,
grunell@chromium.org, tommi@chromium.org, dalecurtis@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1769933002/#ps120001
(title: "comment and bug ref to remove AudioManagerBase dependency")

[commit-bot: I haz the power, 2016-04-12 10:26:38]

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

[commit-bot: I haz the power, 2016-04-12 11:39:30]

Description was changed from

==========
Looking up device id by session id for AudioRendererMixerInput

This is required for being able to correctly mix in WebRtc audio in case
of unified streams, when session id is used to specify output device
matching an open input device.

1) Modifications to AudioRendererHost and AudioOutputDevice to retrieve
the matched device id in OnRequestDeviceAuthorization()/OnDeviceAuthorized()

-- This is only manually tested so far. I tried to come up with a neat unit
test but AudioRendererHost is lacking mocks (uses "for test" settings of
its dependencies instead) so I got lost there. As much as I want to have the
unit test in place, I would prefer to not spend any more time on this now.
The change is pretty straight forward. Please let me know if you think
the unit test is a must-have.

2) AudioRendereMixer key (in AudioRendererMixerManager) is modified
to take into account default device specifics.



BUG=587457
==========

to

==========
Looking up device id by session id for AudioRendererMixerInput

This is required for being able to correctly mix in WebRtc audio in case
of unified streams, when session id is used to specify output device
matching an open input device.

1) Modifications to AudioRendererHost and AudioOutputDevice to retrieve
the matched device id in OnRequestDeviceAuthorization()/OnDeviceAuthorized()

-- This is only manually tested so far. I tried to come up with a neat unit
test but AudioRendererHost is lacking mocks (uses "for test" settings of
its dependencies instead) so I got lost there. As much as I want to have the
unit test in place, I would prefer to not spend any more time on this now.
The change is pretty straight forward. Please let me know if you think
the unit test is a must-have.

2) AudioRendereMixer key (in AudioRendererMixerManager) is modified
to take into account default device specifics.



BUG=587457
==========

[commit-bot: I haz the power, 2016-04-12 11:39:32]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-04-12 11:40:44]

Description was changed from

==========
Looking up device id by session id for AudioRendererMixerInput

This is required for being able to correctly mix in WebRtc audio in case
of unified streams, when session id is used to specify output device
matching an open input device.

1) Modifications to AudioRendererHost and AudioOutputDevice to retrieve
the matched device id in OnRequestDeviceAuthorization()/OnDeviceAuthorized()

-- This is only manually tested so far. I tried to come up with a neat unit
test but AudioRendererHost is lacking mocks (uses "for test" settings of
its dependencies instead) so I got lost there. As much as I want to have the
unit test in place, I would prefer to not spend any more time on this now.
The change is pretty straight forward. Please let me know if you think
the unit test is a must-have.

2) AudioRendereMixer key (in AudioRendererMixerManager) is modified
to take into account default device specifics.



BUG=587457
==========

to

==========
Looking up device id by session id for AudioRendererMixerInput

This is required for being able to correctly mix in WebRtc audio in case
of unified streams, when session id is used to specify output device
matching an open input device.

1) Modifications to AudioRendererHost and AudioOutputDevice to retrieve
the matched device id in OnRequestDeviceAuthorization()/OnDeviceAuthorized()

-- This is only manually tested so far. I tried to come up with a neat unit
test but AudioRendererHost is lacking mocks (uses "for test" settings of
its dependencies instead) so I got lost there. As much as I want to have the
unit test in place, I would prefer to not spend any more time on this now.
The change is pretty straight forward. Please let me know if you think
the unit test is a must-have.

2) AudioRendereMixer key (in AudioRendererMixerManager) is modified
to take into account default device specifics.

BUG=587457

Committed: https://crrev.com/121c63d12d10be3dab0ebb032595f2bea95c42c8
Cr-Commit-Position: refs/heads/master@{#386652}
==========

[commit-bot: I haz the power, 2016-04-12 11:40:45]

Patchset 7 (id:??) landed as
https://crrev.com/121c63d12d10be3dab0ebb032595f2bea95c42c8
Cr-Commit-Position: refs/heads/master@{#386652}