Use spec-compliant algorithm for video content-capture deviceId selection in getUserMedia.

Use spec-compliant algorithm for video content-capture deviceId selection in getUserMedia.

Other settings are chosen based on the old algorithm.

BUG=657733
Review-Url: https://codereview.chromium.org/2750163002
Cr-Commit-Position: refs/heads/master@{#457844}
Committed: https://chromium.googlesource.com/chromium/src/+/8fca31274c6a41144b4f4ee63496c3cc18f15f54

[Guido Urdaneta, 2017-03-15 20:30:00]

Patchset #1 (id:1) has been deleted

[Guido Urdaneta, 2017-03-15 20:39:16]

Patchset #1 (id:20001) has been deleted

[Guido Urdaneta, 2017-03-15 20:48:26]

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

[commit-bot: I haz the power, 2017-03-15 20:49:03]

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

[commit-bot: I haz the power, 2017-03-15 21:24:05]

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

[commit-bot: I haz the power, 2017-03-15 21:24:05]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Guido Urdaneta, 2017-03-15 22:02:56]

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

[commit-bot: I haz the power, 2017-03-15 22:04:03]

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

[Guido Urdaneta, 2017-03-15 22:49:57]

Description was changed from

==========
Use spec-compliant algorithm for video content-capture deviceId selection in
getUserMedia.

Other settings are chosen based on the old algorithm.

BUG=657733
==========

to

==========
Use spec-compliant algorithm for video content-capture deviceId selection in
getUserMedia.

Other settings are chosen based on the old algorithm.

BUG=657733
==========

[Guido Urdaneta, 2017-03-15 22:49:57]

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

[Guido Urdaneta, 2017-03-15 22:50:07]

Hi, PTAL

[commit-bot: I haz the power, 2017-03-15 23:27:37]

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

[commit-bot: I haz the power, 2017-03-15 23:27:38]

Dry run: This issue passed the CQ dry run.

[Guido Urdaneta, 2017-03-16 00:01:57]

Patchset #1 (id:40001) has been deleted

[Guido Urdaneta, 2017-03-16 00:02:10]

Patchset #1 (id:60001) has been deleted

[Guido Urdaneta, 2017-03-16 00:03:34]

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

[commit-bot: I haz the power, 2017-03-16 00:04:04]

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

[commit-bot: I haz the power, 2017-03-16 01:37:17]

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

[commit-bot: I haz the power, 2017-03-16 01:37:18]

Dry run: This issue passed the CQ dry run.

[hbos_chromium, 2017-03-17 11:53:29]

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

https://codereview.chromium.org/2750163002/diff/80001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:474:
controls->video.device_id.c_str()));
I noticed we do FinalizeRequestUserMedia if !video; are we requesting video
device even if !user_media_request.video()? Likewise with audio?
Does this need to be modified? Should we DCHECK audio() || video()?

How does WebUserMediaRequest.audio()/video() align with
StreamControls.audio/video.requested?

[Guido Urdaneta, 2017-03-17 12:33:42]

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

https://codereview.chromium.org/2750163002/diff/80001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:474:
controls->video.device_id.c_str()));
On 2017/03/17 11:53:28, hbos_chromium wrote:
> I noticed we do FinalizeRequestUserMedia if !video; are we requesting video
> device even if !user_media_request.video()? Likewise with audio?
> Does this need to be modified? Should we DCHECK audio() || video()?
> 
> How does WebUserMediaRequest.audio()/video() align with
> StreamControls.audio/video.requested?

If audio and video are both false, the constraints parser detects that (in
Blink) and getUserMedia aborts with a TypeError. That happens before the request
arrives to UserMediaClientImpl.
In UserMediaClientImpl, we are sure that at least one of them has been provided
and FinalizeRequestUserMedia must be called unless there are errors processing
the constraints.

StreamConstrols.audio/video.requested is set in CopyConstraintsToStreamControl()
for audio, and InitializeStreamControls() for video (starting on this CL).

[hbos_chromium, 2017-03-17 15:27:39]

lgtm assuming the unittest does not need to be updated?

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

https://codereview.chromium.org/2750163002/diff/80001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:474:
controls->video.device_id.c_str()));
On 2017/03/17 12:33:42, Guido Urdaneta wrote:
> On 2017/03/17 11:53:28, hbos_chromium wrote:
> > I noticed we do FinalizeRequestUserMedia if !video; are we requesting video
> > device even if !user_media_request.video()? Likewise with audio?
> > Does this need to be modified? Should we DCHECK audio() || video()?
> > 
> > How does WebUserMediaRequest.audio()/video() align with
> > StreamControls.audio/video.requested?
> 
> If audio and video are both false, the constraints parser detects that (in
> Blink) and getUserMedia aborts with a TypeError. That happens before the
request
> arrives to UserMediaClientImpl.
> In UserMediaClientImpl, we are sure that at least one of them has been
provided
> and FinalizeRequestUserMedia must be called unless there are errors processing
> the constraints.
> 
> StreamConstrols.audio/video.requested is set in
CopyConstraintsToStreamControl()
> for audio, and InitializeStreamControls() for video (starting on this CL).

OK. Can we DCHECK audio or video in the public requestUserMedia?

[Guido Urdaneta, 2017-03-17 18:13:28]

The CQ bit was checked by guidou@chromium.org

[Guido Urdaneta, 2017-03-17 18:13:29]

The patchset sent to the CQ was uploaded after l-g-t-m from hbos@chromium.org
Link to the patchset: https://codereview.chromium.org/2750163002/#ps100001
(title: "address review comments and rebase")

[Guido Urdaneta, 2017-03-17 18:13:46]

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

https://codereview.chromium.org/2750163002/diff/80001/content/renderer/media/...
content/renderer/media/user_media_client_impl.cc:474:
controls->video.device_id.c_str()));
On 2017/03/17 15:27:39, hbos_chromium wrote:
> On 2017/03/17 12:33:42, Guido Urdaneta wrote:
> > On 2017/03/17 11:53:28, hbos_chromium wrote:
> > > I noticed we do FinalizeRequestUserMedia if !video; are we requesting
video
> > > device even if !user_media_request.video()? Likewise with audio?
> > > Does this need to be modified? Should we DCHECK audio() || video()?
> > > 
> > > How does WebUserMediaRequest.audio()/video() align with
> > > StreamControls.audio/video.requested?
> > 
> > If audio and video are both false, the constraints parser detects that (in
> > Blink) and getUserMedia aborts with a TypeError. That happens before the
> request
> > arrives to UserMediaClientImpl.
> > In UserMediaClientImpl, we are sure that at least one of them has been
> provided
> > and FinalizeRequestUserMedia must be called unless there are errors
processing
> > the constraints.
> > 
> > StreamConstrols.audio/video.requested is set in
> CopyConstraintsToStreamControl()
> > for audio, and InitializeStreamControls() for video (starting on this CL).
> 
> OK. Can we DCHECK audio or video in the public requestUserMedia?

Done.

[commit-bot: I haz the power, 2017-03-17 18:14:00]

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

[commit-bot: I haz the power, 2017-03-17 19:16:10]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489774408925750,
"parent_rev": "81a2d796ecde51050ebf057af9426b251cad9028", "commit_rev":
"8fca31274c6a41144b4f4ee63496c3cc18f15f54"}

[commit-bot: I haz the power, 2017-03-17 19:16:54]

Description was changed from

==========
Use spec-compliant algorithm for video content-capture deviceId selection in
getUserMedia.

Other settings are chosen based on the old algorithm.

BUG=657733
==========

to

==========
Use spec-compliant algorithm for video content-capture deviceId selection in
getUserMedia.

Other settings are chosen based on the old algorithm.

BUG=657733

Review-Url: https://codereview.chromium.org/2750163002
Cr-Commit-Position: refs/heads/master@{#457844}
Committed:
https://chromium.googlesource.com/chromium/src/+/8fca31274c6a41144b4f4ee63496...
==========

[commit-bot: I haz the power, 2017-03-17 19:16:55]

Committed patchset #2 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/8fca31274c6a41144b4f4ee63496...