Implicit audio output device selection for getUserMedia.

Implicit audio output device selection for getUserMedia.

When a non-default input device is selected, do a best-effort selection of a matching output device.
This is used to switch output of media stream audio tracks to the output device that matches the currently open capture device (microphone).
An typical example is to support switching to USB headsets when in a audio/video call.
This does not affect the audio output of non-webrtc related audio elements and only happens when there's exactly 1 active audio capture device in the page.

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

[Jói, 2013-09-06 14:49:26]

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_input_device_manager.cc:38: 2048,
false);
Why 2048?

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_renderer_host.cc:307: std::string
input_device_id, output_device_id;
output_device_id will be empty if there is no info. Just checking this is the
appropriate guard condition.

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.cc:606: // Use session_id of 0 to
indicate no association with the capture device
Above you use a guard of -1, which is correct?

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.cc:631: int session_id = -1,
sample_rate = 0, buffer_size = 0;
Same code, DRY.

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.cc:693: DLOG(WARNING)
Not sure this is "bad" enough to demand a warning; maybe INFO?

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.h:163: // Note that if there are more
than one open capture devices
This sentence does not

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.h:164: bool
GetAuthorizedInputDeviceSessionIdForAudioRenderer(int* session_id,
I think style is either all indent to ( or all indent at +4 spaces starting on
next line.

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/webrtc...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_audio_device_unittest.cc:951: new
WebRtcAudioRenderer(kRenderViewId, -1, sample_rate,
These parameters and the boilerplate to get the sample rate and frames per
buffer are repeated a few times in the file; maybe make a factory for
WebRtcAudioRenderer that does that work?

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/webrtc...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_audio_renderer.cc:128: int sample_rate =
sample_rate_;
Maybe comment on why you don't use sample_rate_ below (since sample_rate gets
changed but not sample_rate_).

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_audio_renderer.cc:224: if (session_id_ != -1) {
You used 0 elsewhere, looks like you want -1 everywhere.

[no longer working on chromium, 2013-09-06 15:17:36]

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_input_device_manager.cc:38: 2048,
false);
nit, use 0 as indicating the value is also fake.

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_input_device_manager.cc:173: 0, 0,
2048, false);
ditto

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.cc:676: bool
MediaStreamImpl::GetAuthorizedInputDeviceSessionIdForAudioRenderer(
The name does not completely fit to its functionalities, you are getting the
device related info like session_id, sample_rate, and buffer size. I will
suggest renaming it to GetAurhorizedDeviceInfoForAudioRenderer

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.cc:699: device_id = source.deviceId();
this should be the session_id

[tommi (sloooow) - chröme, 2013-09-06 16:55:51]

Address comments

[tommi (sloooow) - chröme, 2013-09-06 16:56:54]

Addressed comments and had to add a couple of files: webrtc_audio_device_test.*

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/audio_input_device_manager.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_input_device_manager.cc:38: 2048,
false);
On 2013/09/06 14:49:26, Jói wrote:
> Why 2048?

Copied from test code that also used a fake device. Here it doesn't matter
though so changed to 0.

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_input_device_manager.cc:38: 2048,
false);
On 2013/09/06 15:17:37, xians1 wrote:
> nit, use 0 as indicating the value is also fake.

Done.

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_input_device_manager.cc:173: 0, 0,
2048, false);
On 2013/09/06 15:17:37, xians1 wrote:
> ditto

Done.

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
File content/browser/renderer_host/media/audio_renderer_host.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/browser/renderer_host...
content/browser/renderer_host/media/audio_renderer_host.cc:307: std::string
input_device_id, output_device_id;
On 2013/09/06 14:49:26, Jói wrote:
> output_device_id will be empty if there is no info. Just checking this is the
> appropriate guard condition.

Yes, that's intentional. Added comments to explain.

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_impl.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.cc:606: // Use session_id of 0 to
indicate no association with the capture device
On 2013/09/06 14:49:26, Jói wrote:
> Above you use a guard of -1, which is correct?

I changed it to 0 for consistency with what's used in AudioOutputDevice:
https://code.google.com/p/chromium/codesearch#chromium/src/media/audio/audio_...

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.cc:631: int session_id = -1,
sample_rate = 0, buffer_size = 0;
On 2013/09/06 14:49:26, Jói wrote:
> Same code, DRY.

Done.

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.cc:676: bool
MediaStreamImpl::GetAuthorizedInputDeviceSessionIdForAudioRenderer(
On 2013/09/06 15:17:37, xians1 wrote:
> The name does not completely fit to its functionalities, you are getting the
> device related info like session_id, sample_rate, and buffer size. I will
> suggest renaming it to GetAurhorizedDeviceInfoForAudioRenderer

Done (fixed typo).

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.cc:693: DLOG(WARNING)
On 2013/09/06 14:49:26, Jói wrote:
> Not sure this is "bad" enough to demand a warning; maybe INFO?

Yeah, that's why I picked DLOG so that while debugging we would notice it
(likely only webrtc engs would run into this), but changed it to DVLOG(1).

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.cc:699: device_id = source.deviceId();
On 2013/09/06 15:17:37, xians1 wrote:
> this should be the session_id

Renamed the variable.  Also added a todo to store the session_id in the
MediaStreamSourceExtraData struct.

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
File content/renderer/media/media_stream_impl.h (right):

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.h:163: // Note that if there are more
than one open capture devices
On 2013/09/06 14:49:26, Jói wrote:
> This sentence does not

Doh! fixed

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/media_...
content/renderer/media/media_stream_impl.h:164: bool
GetAuthorizedInputDeviceSessionIdForAudioRenderer(int* session_id,
On 2013/09/06 14:49:26, Jói wrote:
> I think style is either all indent to ( or all indent at +4 spaces starting on
> next line.

Done.

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/webrtc...
File content/renderer/media/webrtc_audio_device_unittest.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_audio_device_unittest.cc:951: new
WebRtcAudioRenderer(kRenderViewId, -1, sample_rate,
On 2013/09/06 14:49:26, Jói wrote:
> These parameters and the boilerplate to get the sample rate and frames per
> buffer are repeated a few times in the file; maybe make a factory for
> WebRtcAudioRenderer that does that work?

Done.  Added a factory method to WebRTCAudioDeviceTest.

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/webrtc...
File content/renderer/media/webrtc_audio_renderer.cc (right):

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_audio_renderer.cc:128: int sample_rate =
sample_rate_;
On 2013/09/06 14:49:26, Jói wrote:
> Maybe comment on why you don't use sample_rate_ below (since sample_rate gets
> changed but not sample_rate_).

I added a todo for me and Henrik to look into changing this.

https://codereview.chromium.org/23731007/diff/1/content/renderer/media/webrtc...
content/renderer/media/webrtc_audio_renderer.cc:224: if (session_id_ != -1) {
On 2013/09/06 14:49:26, Jói wrote:
> You used 0 elsewhere, looks like you want -1 everywhere.

Removed the if and added a DCHECK_GE(session_id_, 0);
I'm following the convention in AudioOutputDevice that 0 means an invalid
session.

[Jói, 2013-09-06 18:03:45]

LGTM

[no longer working on chromium, 2013-09-06 18:48:11]

On 2013/09/06 18:03:45, Jói wrote:
> LGTM

lgtm

[tommi (sloooow) - chröme, 2013-09-06 19:04:01]

Thanks for the super quick turnaround!


On Fri, Sep 6, 2013 at 8:48 PM, <xians@chromium.org> wrote:

> On 2013/09/06 18:03:45, Jói wrote:
>
>> LGTM
>>
>
> lgtm
>
>
>
https://codereview.chromium.**org/23731007/<https://codereview.chromium.org/2...
>
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
chromium-reviews+unsubscribe@**chromium.org<chromium-reviews%2Bunsubscribe@ch...
> .
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2013-09-06 19:04:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/23731007/23001

[commit-bot: I haz the power, 2013-09-06 19:17:39]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[tommi (sloooow) - chröme, 2013-09-06 19:25:21]

Justin - can you please take a look at:
content/common/media/media_stream_messages.h

[tommi (sloooow) - chröme, 2013-09-06 19:27:10]

On 2013/09/06 19:25:21, tommi wrote:
> Justin - can you please take a look at:
> content/common/media/media_stream_messages.h

The struct being updated is in media_stream_request.h

[tommi (sloooow) - chröme, 2013-09-09 17:23:50]

ping for media_**stream_messages.h
(sorry for pushing but if possible, it would be good to land this today -
assuming no issues of course).


On Fri, Sep 6, 2013 at 9:27 PM, <tommi@chromium.org> wrote:

> On 2013/09/06 19:25:21, tommi wrote:
>
>> Justin - can you please take a look at:
>> content/common/media/media_**stream_messages.h
>>
>
> The struct being updated is in media_stream_request.h
>
>
https://codereview.chromium.**org/23731007/<https://codereview.chromium.org/2...
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jschuh, 2013-09-09 17:28:53]

it looks like the new fields are used on the browser side only for matching
purposes. so, ipc security lgtm.

[commit-bot: I haz the power, 2013-09-09 17:34:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/23731007/23001

[commit-bot: I haz the power, 2013-09-09 17:35:07]

Failed to apply patch for
content/renderer/media/webrtc_audio_device_unittest.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file content/renderer/media/webrtc_audio_device_unittest.cc
  Hunk #1 FAILED at 520.
  Hunk #2 FAILED at 701.
  Hunk #3 FAILED at 778.
  Hunk #4 succeeded at 943 (offset 20 lines).
  3 out of 4 hunks FAILED -- saving rejects to file
content/renderer/media/webrtc_audio_device_unittest.cc.rej

Patch:       content/renderer/media/webrtc_audio_device_unittest.cc
Index: content/renderer/media/webrtc_audio_device_unittest.cc
diff --git a/content/renderer/media/webrtc_audio_device_unittest.cc
b/content/renderer/media/webrtc_audio_device_unittest.cc
index
7074c95bb09c3e28966169cb62e91300918c89ec..e127ad09ab9429718371a069bedac85f7b2448cc
100644
--- a/content/renderer/media/webrtc_audio_device_unittest.cc
+++ b/content/renderer/media/webrtc_audio_device_unittest.cc
@@ -520,8 +520,9 @@ TEST_F(MAYBE_WebRTCAudioDeviceTest, DISABLED_StartPlayout) {
   EXPECT_CALL(media_observer(),
       OnDeleteAudioStream(_, 1)).Times(AnyNumber());
 
-  scoped_refptr<WebRtcAudioRenderer> renderer =
-      new WebRtcAudioRenderer(kRenderViewId);
+  scoped_refptr<WebRtcAudioRenderer> renderer(
+      CreateDefaultWebRtcAudioRenderer(kRenderViewId));
+
   scoped_refptr<WebRtcAudioDeviceImpl> webrtc_audio_device(
       new WebRtcAudioDeviceImpl());
   EXPECT_TRUE(webrtc_audio_device->SetAudioRenderer(renderer.get()));
@@ -701,8 +702,9 @@ TEST_F(MAYBE_WebRTCAudioDeviceTest, DISABLED_PlayLocalFile)
{
   EXPECT_CALL(media_observer(),
       OnDeleteAudioStream(_, 1)).Times(AnyNumber());
 
-  scoped_refptr<WebRtcAudioRenderer> renderer =
-      new WebRtcAudioRenderer(kRenderViewId);
+  scoped_refptr<WebRtcAudioRenderer> renderer(
+      CreateDefaultWebRtcAudioRenderer(kRenderViewId));
+
   scoped_refptr<WebRtcAudioDeviceImpl> webrtc_audio_device(
       new WebRtcAudioDeviceImpl());
   EXPECT_TRUE(webrtc_audio_device->SetAudioRenderer(renderer.get()));
@@ -778,8 +780,9 @@ TEST_F(MAYBE_WebRTCAudioDeviceTest,
MAYBE_FullDuplexAudioWithAGC) {
   EXPECT_CALL(media_observer(),
       OnDeleteAudioStream(_, 1)).Times(AnyNumber());
 
-  scoped_refptr<WebRtcAudioRenderer> renderer =
-      new WebRtcAudioRenderer(kRenderViewId);
+  scoped_refptr<WebRtcAudioRenderer> renderer(
+      CreateDefaultWebRtcAudioRenderer(kRenderViewId));
+
   scoped_refptr<WebRtcAudioDeviceImpl> webrtc_audio_device(
       new WebRtcAudioDeviceImpl());
   EXPECT_TRUE(webrtc_audio_device->SetAudioRenderer(renderer.get()));
@@ -923,8 +926,9 @@ TEST_F(MAYBE_WebRTCAudioDeviceTest, WebRtcPlayoutSetupTime)
{
   base::WaitableEvent event(false, false);
   scoped_ptr<MockWebRtcAudioRendererSource> renderer_source(
       new MockWebRtcAudioRendererSource(&event));
-  scoped_refptr<WebRtcAudioRenderer> renderer =
-      new WebRtcAudioRenderer(kRenderViewId);
+
+  scoped_refptr<WebRtcAudioRenderer> renderer(
+      CreateDefaultWebRtcAudioRenderer(kRenderViewId));
   renderer->Initialize(renderer_source.get());
 
   // Start the timer and playout.

[tommi (sloooow) - chröme, 2013-09-09 18:25:44]

Rebase

[commit-bot: I haz the power, 2013-09-09 18:28:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/23731007/38001

[commit-bot: I haz the power, 2013-09-09 20:33:30]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-09-09 20:50:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommi@chromium.org/23731007/38001

[commit-bot: I haz the power, 2013-09-10 02:18:29]

Change committed as 222187