Jason Smith [:jsmith] Reporter
	Description • 12 years ago

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.

	Jason Smith [:jsmith] Reporter
	Comment 1 • 12 years ago

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.

	Jason Smith [:jsmith] Reporter
	Comment 2 • 12 years ago

Do a parity chrome test to see what they do.

	Jason Smith [:jsmith] Reporter
	Comment 3 • 12 years ago

(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.

	Randell Jesup [:jesup] (needinfo me)
	Comment 4 • 11 years ago

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

	Jason Smith [:jsmith] Reporter
	Comment 5 • 11 years ago

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.

	Maire Reavy [:mreavy]
	Comment 6 • 9 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 7 • 9 years ago

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?

	Nils Ohlmeier [:drno]
	Comment 9 • 8 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 10 • 8 years ago

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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 11 • 8 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 12 • 8 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 13 • 8 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 14 • 8 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 15 • 8 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 16 • 8 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 17 • 8 years ago

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

	Florian Quèze [:florian]
	Comment 18 • 8 years ago

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"?

	Florian Quèze [:florian]
	Comment 19 • 8 years ago

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?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 20 • 8 years ago

(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" } }.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 21 • 8 years ago

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

which I believe is correct.

	Randell Jesup [:jesup] (needinfo me)
	Comment 22 • 8 years ago

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

	Randell Jesup [:jesup] (needinfo me)
	Comment 23 • 8 years ago

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

	Randell Jesup [:jesup] (needinfo me)
	Comment 24 • 8 years ago

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

	Randell Jesup [:jesup] (needinfo me)
	Comment 25 • 8 years ago

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

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 26 • 8 years ago

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).

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 27 • 8 years ago

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/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 28 • 8 years ago

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/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 29 • 8 years ago

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/

	Florian Quèze [:florian]
	Comment 30 • 8 years ago

(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.

	Florian Quèze [:florian]
	Comment 31 • 8 years ago

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.

	Florian Quèze [:florian]
	Comment 32 • 8 years ago

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.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 33 • 8 years ago

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/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 34 • 8 years ago

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/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 35 • 8 years ago

(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?

	Florian Quèze [:florian]
	Comment 36 • 8 years ago

(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.

	Sebastian Kaspari (:sebastian; :pocmo)
	Comment 37 • 8 years ago

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.

	Pulsebot
	Comment 38 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 39 • 8 years ago
bugherder

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

	Eric Shepherd [:sheppy]
	Comment 40 • 8 years ago

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.