802326 - If video and audio is requested in gUM, but one of them fails, we should align with the spec

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Jason Smith [:jsmith]]

Steps:

1. Execute a gUM call for video and audio in which audio fails, video successful

Expected:

We should either do the following (the final decision here is probably dependent on the spec):

1. Not fire any permissions prompt at all and reports NO_DEVICES_FOUND
2. Fire the permissions prompt for video and only give access to the camera in the MediaStream

Actual:

We're a bit inconsistent right now. We fire up a permissions prompt for requesting access to the camera, but then we report NO_DEVICES_FOUND. We should probably do either one of the behaviors stated above, subject to what the spec editors decide on is the right behavior.

Comment 1

[Jason Smith [:jsmith]]

Also - I haven't tested the opposing use case (video fails, audio successful), but I'd imagine that whatever we decide here should fall in alignment with that use case as well.

Comment 2

[Jason Smith [:jsmith]]

Do a parity chrome test to see what they do.

Comment 3

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #2)
> Do a parity chrome test to see what they do.

They do not error out, they instead only give access to one of the two input devices available.

Fun fact also - apparently they don't support the loading of audio resources into an audio tag from gUM.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Let's retest this and see where we are (including parity test against Chrome)

Comment 5

[Jason Smith [:jsmith]]

On Firefox - requesting gUM video/audio where audio fails results in a video gUM perm prompt appearing that a user can grant access to their video only. The video plays from the camera with the visible indicator indicating your camera is active. This is differing behavior than what was seen on filing of this bug.

On Chrome - requesting gUM video/audio where audio fails results in a video/audio gUM prompt appearing that a user can grant access to their video/audio. The video plays from the camera with the visible indicator indicating your camera/mic is active. To me, this feels like a bug in Chrome, as it's indicating a microphone is active that can't possibly be requested to be used.

Firefox's behavior sounds acceptable now in terms of what I think makes sense, but the differing behavior between browsers I think reveals that there needs to be clarification made clearly in the spec on what the right behavior is.

Comment 6

[Maire Reavy [:mreavy]]

Are we aligned with the spec on this now?  If so, we can close it.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

We're not aligned yet.

The spec (URL) says: "The provided media MUST include precisely one track of each media type in requestedMediaTypes from the finalSet."

There are two places we violate this, e.g. for audio:

1. There is no mic on this system (so we ask the user for camera only).
2. We ask the user for camera+mic and the user chooses camera but "No audio".

Vice versa for video. Both 1 and 2 violate the spec, but this bug AFAICT is only 1.

For 1, I think we said we would flip the behavior once we have enumerateDevices(), the rationale being that apps that want to accommodate users with a camera but no microphone, and/or users with a microphone but no camera on their system, now can explicitly check for said people.

We can flip that behavior now.

For 2, I believe the doorhangers need to stop offering "No video" and "No audio" as choices when video + audio is requested (they already do the right thing when only one kind is requested). Do we want/have a separate bug for that?

Comment 9

[Nils Ohlmeier [:drno]]

STR:

On Windows on a machine with a camera which supports only up to 720p do this:
- open https://webrtc.github.io/samples/src/content/peerconnection/constraints/
- move the sliders for min an max width and height all the to the right to request 1080p resolution
- click the "Get Media" button

Actual result:
- Only a prompt for sharing the mic pops up and the page only gets an audio stream

Expected result:
- Because of the constraints we should return an error to make clear that the resolution constraints were not met

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I increased the priority because of comment 9. This use-case is worse than cases I've previously considered, since the site is using required constraints.

Required constraints were designed to allow sites to try to request using their strictest requirements first, and be able to rely on this failing, so the site can fall back to different constraints.

This bug has the unintended consequence of giving the site no camera at all, if audio was also requested, even though the user has a camera, which is quite bad.

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 802326 - make getUserMedia fail if required video constraints aren't met, regardless of audio.

Review commit: https://reviewboard.mozilla.org/r/51003/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51003/

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 802326 - test that getUserMedia fails w/required video constraint, regardless of audio.

Review commit: https://reviewboard.mozilla.org/r/51005/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51005/

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 802326 - make getUserMedia fail audio+video request unless user has both.

Review commit: https://reviewboard.mozilla.org/r/51007/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51007/

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 802326 - make getUserMedia fail audio+video request unless user shares both.

Review commit: https://reviewboard.mozilla.org/r/51009/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51009/

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 802326 - remove "No Video" and "No Audio" choices in cam+mic permission prompt on desktop.

Review commit: https://reviewboard.mozilla.org/r/51011/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51011/

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 802326 - update tests to work wo/"No Video" and "No Audio" in cam+mic permission prompt.

Review commit: https://reviewboard.mozilla.org/r/51247/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51247/

Comment 17

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 802326 - remove "No Video" and "No Audio" choices in cam+mic permission prompt on android.

Review commit: https://reviewboard.mozilla.org/r/51013/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51013/

Comment 18

[Florian Quèze [:florian]]

Comment on attachment 8749921 [details]
MozReview Request: Bug 802326 - remove "No Video" and "No Audio" choices in cam+mic permission prompt on desktop.

https://reviewboard.mozilla.org/r/51011/#review48407

::: browser/modules/webrtcUI.jsm
(Diff revision 1)
> -      if (requestTypes.length == 2) {
> -        let stringBundle = chromeDoc.defaultView.gNavigatorBundle;
> -        if (!sharingScreen)
> -          addDeviceToList(camMenupopup, stringBundle.getString("getUserMedia.noVideo.label"), "-1");
> -        if (!sharingAudio)
> -          addDeviceToList(micMenupopup, stringBundle.getString("getUserMedia.noAudio.label"), "-1");

If you are removing these strings, you also want to remove them from the browser.properties file.

What about the case where the website requests both audio and screen/app/tab/window sharing, and the user selects "No Window"?

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 8749922 [details]
MozReview Request: Bug 802326 - update tests to work wo/"No Video" and "No Audio" in cam+mic permission prompt.

https://reviewboard.mozilla.org/r/51247/#review48409

::: browser/base/content/test/general/browser_devices_get_user_media.js:16
(Diff revision 1)
>                   this);
>  
>  function enableDevice(aType, aEnabled) {
>    let menulist = document.getElementById("webRTC-select" + aType + "-menulist");
>    let menupopup = document.getElementById("webRTC-select" + aType + "-menupopup");
>    menulist.value = aEnabled ? menupopup.firstChild.getAttribute("value") : "-1";

I don't see how setting this to -1 can work anymore if we remove the "No Audio" / "No Video" item from the menulist.

::: browser/base/content/test/general/browser_devices_get_user_media.js:292
(Diff revision 1)
>      yield checkPerm(true, false, true, undefined, true, undefined);
>      info("video only, user grants, check video perm set to allow, audio perm not set");
>      yield checkPerm(false, true, undefined, true, undefined, true);
>  
>      // 3 cases where the user rejects the device request.
>      // First test these cases by setting the device to 'No Audio'/'No Video'

Did you mean to rephrase this comment? What's the meaning of "No Audio"/"No Video" now?

Comment 20

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Florian Quèze [:florian] [:flo] from comment #18)
> If you are removing these strings, you also want to remove them from the
> browser.properties file.

Thanks, I'll address those.

> What about the case where the website requests both audio and
> screen/app/tab/window sharing, and the user selects "No Window"?

Is there a test I missed?

I've verified manually that previously you would have gotten audio only, whereas with these patches you get SecurityError.

On a side-note: I see there's no UX for { video: true, audio: { mediaSource: "audioCapture" } }.

Comment 21

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #20)
> whereas with these patches you get SecurityError.

which I believe is correct.

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8749917 [details]
MozReview Request: Bug 802326 - make getUserMedia fail if required video constraints aren't met, regardless of audio.

https://reviewboard.mozilla.org/r/51003/#review48499

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8749918 [details]
MozReview Request: Bug 802326 - test that getUserMedia fails w/required video constraint, regardless of audio.

https://reviewboard.mozilla.org/r/51005/#review48501

Comment 24

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8749919 [details]
MozReview Request: Bug 802326 - make getUserMedia fail audio+video request unless user has both.

https://reviewboard.mozilla.org/r/51007/#review48503

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8749920 [details]
MozReview Request: Bug 802326 - make getUserMedia fail audio+video request unless user shares both.

https://reviewboard.mozilla.org/r/51009/#review48505

Comment 26

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://reviewboard.mozilla.org/r/51011/#review48407

> If you are removing these strings, you also want to remove them from the browser.properties file.
> 
> What about the case where the website requests both audio and screen/app/tab/window sharing, and the user selects "No Window"?

Replied in comment 20 (forgot to comment through mozReview).

Comment 27

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8749921 [details]
MozReview Request: Bug 802326 - remove "No Video" and "No Audio" choices in cam+mic permission prompt on desktop.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51011/diff/1-2/

Comment 28

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8749922 [details]
MozReview Request: Bug 802326 - update tests to work wo/"No Video" and "No Audio" in cam+mic permission prompt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51247/diff/1-2/

Comment 29

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8749923 [details]
MozReview Request: Bug 802326 - remove "No Video" and "No Audio" choices in cam+mic permission prompt on android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51013/diff/1-2/

Comment 30

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #20)
> (In reply to Florian Quèze [:florian] [:flo] from comment #18)
> > If you are removing these strings, you also want to remove them from the
> > browser.properties file.
> 
> Thanks, I'll address those.
> 
> > What about the case where the website requests both audio and
> > screen/app/tab/window sharing, and the user selects "No Window"?
> 
> Is there a test I missed?
> 
> I've verified manually that previously you would have gotten audio only,
> whereas with these patches you get SecurityError.

I meant this code:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.jsm?rev=8c55d8beba75#462
462         // "No <type>" is the default because we can't pick a
463         // 'default' window to share.
464         addDeviceToList(menupopup,
465                         stringBundle.getString("getUserMedia.no" + typeName + ".label"),
466                         "-1");

It seems inconsistent to keep "No Window" in the UI when we don't have "No Video" anymore. But as the comment there says, we can't pick a default window to share.

I guess a less inconsistent UI would be to replace the "No Window" item with a disabled "(select window)" item, and to disable the "Share Selected Items" button until something is selected. This requires significantly more code changes though.

Comment 31

[Florian Quèze [:florian]]

Comment on attachment 8749922 [details]
MozReview Request: Bug 802326 - update tests to work wo/"No Video" and "No Audio" in cam+mic permission prompt.

https://reviewboard.mozilla.org/r/51247/#review48723

The test changes look good, in that they match what's implemented in the webrtcUI.jsm patch.

::: browser/base/content/test/general/browser_devices_get_user_media.js:274
(Diff revision 2)
>      info("audio only, user grants, check audio perm set to allow, video perm not set");
> -    yield checkPerm(true, false, true, undefined, true, undefined);
> +    yield checkPerm(true, false, true, undefined);
>      info("video only, user grants, check video perm set to allow, audio perm not set");
> -    yield checkPerm(false, true, undefined, true, undefined, true);
> +    yield checkPerm(false, true, undefined, true);
>  
>      // 3 cases where the user rejects the device request.

I would like this comment to keep the "by using the 'Never Share' action." mention, to explain the additional true parameter in these calls.

Comment 32

[Florian Quèze [:florian]]

Comment on attachment 8749921 [details]
MozReview Request: Bug 802326 - remove "No Video" and "No Audio" choices in cam+mic permission prompt on desktop.

https://reviewboard.mozilla.org/r/51011/#review48841

Thinking about what I wrote in comment 30 again, I still think the situation with the "No Window" item where the user can click "Share Selected Items" and we end up returning a security error to the website is a bug, but it already exists when only the screen/window/app share is requested without audio, so that's probably not something to worry about here, we can open a follow-up.

Comment 33

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8749922 [details]
MozReview Request: Bug 802326 - update tests to work wo/"No Video" and "No Audio" in cam+mic permission prompt.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51247/diff/2-3/

Comment 34

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8749923 [details]
MozReview Request: Bug 802326 - remove "No Video" and "No Audio" choices in cam+mic permission prompt on android.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/51013/diff/2-3/

Comment 35

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Florian Quèze [:florian] [:flo] from comment #30)
> I guess a less inconsistent UI would be to replace the "No Window" item with
> a disabled "(select window)" item, and to disable the "Share Selected Items"ree.
> button until something is selected.

Wfm. Do you want to file the follow-up?

Comment 36

[Florian Quèze [:florian]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #35)
> (In reply to Florian Quèze [:florian] [:flo] from comment #30)
> > I guess a less inconsistent UI would be to replace the "No Window" item with
> > a disabled "(select window)" item, and to disable the "Share Selected Items"ree.
> > button until something is selected.
> 
> Wfm. Do you want to file the follow-up?

Filed bug 1272304.

Comment 37

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8749923 [details]
MozReview Request: Bug 802326 - remove "No Video" and "No Audio" choices in cam+mic permission prompt on android.

https://reviewboard.mozilla.org/r/51013/#review49385

LGTM.

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e132a7cbb60b
https://hg.mozilla.org/integration/mozilla-inbound/rev/1978cb1da8dc
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ad6216b2447
https://hg.mozilla.org/integration/mozilla-inbound/rev/2189a25ea1c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/079cacde7af0
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef717a46cf0
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff5d91a2420f

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e132a7cbb60b
https://hg.mozilla.org/mozilla-central/rev/1978cb1da8dc
https://hg.mozilla.org/mozilla-central/rev/4ad6216b2447
https://hg.mozilla.org/mozilla-central/rev/2189a25ea1c6
https://hg.mozilla.org/mozilla-central/rev/079cacde7af0
https://hg.mozilla.org/mozilla-central/rev/cef717a46cf0
https://hg.mozilla.org/mozilla-central/rev/ff5d91a2420f

Comment 40

[Eric Shepherd [:sheppy]]

I've added this information to Firefox 49 for developers. I didn't see any other documentation that needed updating for this; this is basically a UX change -- the behavior is just updated to match the spec.