Desktop Capture API: Pass Audio Selection Information To Javascript

Desktop Capture API: Pass Audio Selection Information To Javascript

The API mediaDevices.getUserMedia will throw an exception if the caller
requests audio but we cannot provide.

Here is the situation for desktop capture, the user could uncheck the
audio check box, and thus revoke the audio share. But the current
setting of chooseDesktopMedia does not pass this piece of information
to Javascript, which will put the caller into a dilemma whether to
request audio or not in getUserMedia call.

BUG=670407
Committed: https://crrev.com/9ec1ad9cf69edcb08db2237ddb1b4b4f293491ab
Cr-Commit-Position: refs/heads/master@{#438351}

[qiangchen, 2016-12-01 19:46:08]

Description was changed from

==========
Desktop Capture API: Pass Audio Selection Information To Javascript

The API mediaDevices.getUserMedia will throw an exception if the caller
requests audio but we cannot provide.

Here is the situation for desktop capture, the user could uncheck the
audio check box, and thus revoke the audio share. But the current
setting of chooseDesktopMedia does not pass this piece of information
to Javascript, which will put the caller into a dilemma whether to
request audio or not in getUserMedia call.

BUG=
==========

to

==========
Desktop Capture API: Pass Audio Selection Information To Javascript

The API mediaDevices.getUserMedia will throw an exception if the caller
requests audio but we cannot provide.

Here is the situation for desktop capture, the user could uncheck the
audio check box, and thus revoke the audio share. But the current
setting of chooseDesktopMedia does not pass this piece of information
to Javascript, which will put the caller into a dilemma whether to
request audio or not in getUserMedia call.

BUG=670407
==========

[qiangchen, 2016-12-01 19:46:08]

qiangchen@chromium.org changed reviewers:
+ rockot@chromium.org

[qiangchen, 2016-12-01 19:49:43]

Hi, Ken

   Can you take a look at this CL? Or any better idea to resolve the dilemma?

Qiang

[Ken Rockot(use gerrit already), 2016-12-05 20:37:36]

rockot@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[Ken Rockot(use gerrit already), 2016-12-05 20:37:36]

Maybe this needs some more careful API review consideration. Over to Devlin.

[qiangchen, 2016-12-06 18:25:45]

On 2016/12/05 20:37:36, Ken Rockot wrote:
> Maybe this needs some more careful API review consideration. Over to Devlin.

Thanks.

Follow up. As we just add a second parameter for the callback function, it does
not break the compatibility for javascript. The old code (without audio) will
still work.

[qiangchen, 2016-12-08 18:32:35]

ping Devlin. Can you have some input?

[Devlin, 2016-12-08 19:09:44]

I don't love the solution, but I can't really think of anything better either if
we're assuming that the getUserMedia function always fails if it's requested for
something the user didn't approve.

But this should have a test.

https://codereview.chromium.org/2547633002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/desktop_capture.json (right):

https://codereview.chromium.org/2547633002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/desktop_capture.json:45: "name": "audioApproved",
Do you think we'd ever want to include more than just this here?  If so, it
might make sense to have it be a details object or something.

[qiangchen, 2016-12-08 20:01:28]

Thanks. Fixed by your suggestion.

The failure of getUserMedia is aligned with spec. So we should not change the
behavior of getUserMedia.

Currently the audio share functionality is not widely used yet, so the change
will not affect many. We found this issue because Hangouts would like to support
audio share during desktop capture, and the issue shows up in experiment.

https://codereview.chromium.org/2547633002/diff/1/chrome/common/extensions/ap...
File chrome/common/extensions/api/desktop_capture.json (right):

https://codereview.chromium.org/2547633002/diff/1/chrome/common/extensions/ap...
chrome/common/extensions/api/desktop_capture.json:45: "name": "audioApproved",
On 2016/12/08 19:09:44, Devlin (in MON - PST plus 3) wrote:
> Do you think we'd ever want to include more than just this here?  If so, it
> might make sense to have it be a details object or something.

Good Suggestion.

[qiangchen, 2016-12-12 21:04:03]

Any way I can progress? Thanks.

[Devlin, 2016-12-13 16:05:45]

Sorry for the delay; was traveling and this got lost in the shuffle.  Thanks for
the ping. :)

Looks like this still needs a test?

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/desktop_capture.json (right):

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/desktop_capture.json:49: "can_request_audio_track":
{
Variables here should be JS-style, so canRequestAudioTrack. Though this might
just make more sense as "audioAllowed" or something.

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/desktop_capture.json:51: "description": "True if
audio track is well generated. Otherwise false, and in this case, one should not
ask for audio stream through getUserMedia call."
What does "well generated" mean? From the bug/description, I thought this was if
the user unchecked a box, rather than chrome being unable to generate it?

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
File chrome/common/extensions/docs/examples/api/desktopCapture/app.js (right):

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
chrome/common/extensions/docs/examples/api/desktopCapture/app.js:38: var
audio_constraint = {
js-style variable, so audioConstraint

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
chrome/common/extensions/docs/examples/api/desktopCapture/app.js:41:
chromeMediaSourceId: id} };
The wrapping here was already weird, but let's clean it up. :)
var audioConstraint = {
  mandatory: {
    chromeMediaSource: 'desktop',
    chromeMediaSourceId: id,
  }
};

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
chrome/common/extensions/docs/examples/api/desktopCapture/app.js:43: if
(!options.can_request_audio_track) audio_constraint = false;
if bodies should go on separate lines, so this would be
if (...)
  audioConstraint = false;

That said, you could also just do:
audioConstraint = options.canRequestAdioTrack && audioConstraint;

[qiangchen, 2016-12-13 17:42:10]

Patchset #3 (id:40001) has been deleted

[qiangchen, 2016-12-13 19:04:33]

Fix the style issue and added api test cases.

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
File chrome/common/extensions/api/desktop_capture.json (right):

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/desktop_capture.json:49: "can_request_audio_track":
{
On 2016/12/13 16:05:44, Devlin wrote:
> Variables here should be JS-style, so canRequestAudioTrack. Though this might
> just make more sense as "audioAllowed" or something.

Done.

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
chrome/common/extensions/api/desktop_capture.json:51: "description": "True if
audio track is well generated. Otherwise false, and in this case, one should not
ask for audio stream through getUserMedia call."
On 2016/12/13 16:05:44, Devlin wrote:
> What does "well generated" mean? From the bug/description, I thought this was
if
> the user unchecked a box, rather than chrome being unable to generate it?

Done. Changed to describing what happened in the foreground that JS developer
must be familiar with, rather than describing what happened behind the scenes.

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
File chrome/common/extensions/docs/examples/api/desktopCapture/app.js (right):

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
chrome/common/extensions/docs/examples/api/desktopCapture/app.js:38: var
audio_constraint = {
On 2016/12/13 16:05:44, Devlin wrote:
> js-style variable, so audioConstraint

Done.

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
chrome/common/extensions/docs/examples/api/desktopCapture/app.js:41:
chromeMediaSourceId: id} };
On 2016/12/13 16:05:44, Devlin wrote:
> The wrapping here was already weird, but let's clean it up. :)
> var audioConstraint = {
>   mandatory: {
>     chromeMediaSource: 'desktop',
>     chromeMediaSourceId: id,
>   }
> };

Done.

https://codereview.chromium.org/2547633002/diff/20001/chrome/common/extension...
chrome/common/extensions/docs/examples/api/desktopCapture/app.js:43: if
(!options.can_request_audio_track) audio_constraint = false;
On 2016/12/13 16:05:44, Devlin wrote:
> if bodies should go on separate lines, so this would be
> if (...)
>   audioConstraint = false;
> 
> That said, you could also just do:
> audioConstraint = options.canRequestAdioTrack && audioConstraint;

Done.

[Devlin, 2016-12-13 21:41:58]

lgtm

[qiangchen, 2016-12-13 22:33:43]

The CQ bit was checked by qiangchen@chromium.org

[commit-bot: I haz the power, 2016-12-13 22:34:17]

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, 2016-12-14 00:11:53]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481668423614020,
"parent_rev": "312ac40b387bb4af442bb54ba29bc945ae60438c", "commit_rev":
"2867382173d91a8b54a6a8f2a8acd848ac4a5501"}

[commit-bot: I haz the power, 2016-12-14 00:12:23]

Description was changed from

==========
Desktop Capture API: Pass Audio Selection Information To Javascript

The API mediaDevices.getUserMedia will throw an exception if the caller
requests audio but we cannot provide.

Here is the situation for desktop capture, the user could uncheck the
audio check box, and thus revoke the audio share. But the current
setting of chooseDesktopMedia does not pass this piece of information
to Javascript, which will put the caller into a dilemma whether to
request audio or not in getUserMedia call.

BUG=670407
==========

to

==========
Desktop Capture API: Pass Audio Selection Information To Javascript

The API mediaDevices.getUserMedia will throw an exception if the caller
requests audio but we cannot provide.

Here is the situation for desktop capture, the user could uncheck the
audio check box, and thus revoke the audio share. But the current
setting of chooseDesktopMedia does not pass this piece of information
to Javascript, which will put the caller into a dilemma whether to
request audio or not in getUserMedia call.

BUG=670407

Review-Url: https://codereview.chromium.org/2547633002
==========

[commit-bot: I haz the power, 2016-12-14 00:12:24]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-12-14 00:15:48]

Description was changed from

==========
Desktop Capture API: Pass Audio Selection Information To Javascript

The API mediaDevices.getUserMedia will throw an exception if the caller
requests audio but we cannot provide.

Here is the situation for desktop capture, the user could uncheck the
audio check box, and thus revoke the audio share. But the current
setting of chooseDesktopMedia does not pass this piece of information
to Javascript, which will put the caller into a dilemma whether to
request audio or not in getUserMedia call.

BUG=670407

Review-Url: https://codereview.chromium.org/2547633002
==========

to

==========
Desktop Capture API: Pass Audio Selection Information To Javascript

The API mediaDevices.getUserMedia will throw an exception if the caller
requests audio but we cannot provide.

Here is the situation for desktop capture, the user could uncheck the
audio check box, and thus revoke the audio share. But the current
setting of chooseDesktopMedia does not pass this piece of information
to Javascript, which will put the caller into a dilemma whether to
request audio or not in getUserMedia call.

BUG=670407

Committed: https://crrev.com/9ec1ad9cf69edcb08db2237ddb1b4b4f293491ab
Cr-Commit-Position: refs/heads/master@{#438351}
==========

[commit-bot: I haz the power, 2016-12-14 00:15:49]

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