Closed Bug 1443294 Opened 6 years ago Closed 3 years ago

Wrong camera/mic picked from non-exact deviceId constraint, unless permissions are persisted

Categories

(Core :: WebRTC: Audio/Video, defect, P2)

53 Branch
defect

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox-esr60 --- wontfix
firefox-esr78 --- wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox88 --- wontfix
firefox89 --- wontfix
firefox90 + fixed

People

(Reporter: jib, Assigned: jib)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression)

Attachments

(1 file)

We get a different device chosen by deviceId constraints depending on whether permission is persisted or not.

STRs:
 1. Have two cameras on your system (e.g. one USB and one built-in)
 2. Open https://jsfiddle.net/jib1/bxgszem4/ and do nothing.
 3. If capture auto-starts, click red blinking camera, revoke permission with "×" in dropdown, refresh.
 4. Click "Allow" in both prompts that appear.

Expected result:

  A B A B

Actual result:

  A A A B

Workaround:
  1. Check "☑ Remember this decision" in the camera permission prompt, refresh.

We want the same behavior regardless of permission (A B A B).
Rank: 12

Unless its hand is forced (by exact constraints) Firefox will return a camera that already has permission over triggering another prompt.

This was done for all constraints, not just deviceId — though why it's wrong is perhaps most obvious with that constraint.

Firefox is alone among browsers in supporting per-device permission, and I believe this was done intentionally at some point, out of fear that sites calling getUserMedia a lot (instead of keeping tracks around and using applyConstraints on them) might otherwise trigger a shift in cameras that would prompt only in Firefox.

In hindsight, I think that fear was overblown, and that it's more important to respect application constraints. meaning: we should fix this, but we might want to keep an eye on web compat if we do.

This bug just got a lot worse with bug 1697284, since per-device permissions now have a 60 minute grace period.

This makes trying to switch to a second camera using deviceId: id instead of deviceId: {exact: id} impossible even after the first camera track has been stopped. We'll need to escalate a fix.

[Tracking Requested - why for this release]: Might break camera switching on some sites (that don't use exact constraints in camera switcher under ⚙️ settings) without persistent permission.

Assignee: nobody → jib
Rank: 12 → 10

This being an old bug, I've set recent tracking flags based on the comment 2 regression in behavior from bug 1697284.

Regressed by: 1697284

We are at the end of the beta cycle with one beta left before RC, Jan-Ivar is that something that is important enough to justify a fix before the last beta? Would that be a dot-release driver? If you have a fix, would that be risk free for an uplift this late in the cycle? Thanks

Flags: needinfo?(jib)
Severity: major → --

The checkRequestAllowed() function that determines whether we can bypass the
permission prompt and fulfill immediately with a device, is given a list of devices
sorted by fitness distance from most ideal to least. — It used to grant any
device on that list that was already active, even if less than ideal (not
first in the list).

This caused problems with sites using ideal constraints to switch between
devices because this function has to fail before Firefox prompts for a new
device, and the new grace periods added in bug 1697284 extended "active" to
include any device active within the last 60 minutes, making it impossible
to pick anything else using ideal constraints alone.

The fix is to only look at the first device in the list (per kind). If that
device is not active, we should prompt for that device, not return a
different one, which means failing this function.

(In reply to Pascal Chevrel:pascalc from comment #4)

We are at the end of the beta cycle with one beta left before RC, Jan-Ivar is that something that is important enough to justify a fix before the last beta?

Probably not. It would be nice to get in, considering 89 is when we bumped the grace period from 50 seconds to 60 minutes, which is what made this much worse. That being said:

  • This would only affect users with two or more cameras, or two or more microphones, trying to switch between them.
  • This would only affect sites using ideal (deviceId: id) constraints to do device selection, something they shouldn't be doing (and most sites aren't). We've reached out to at least one major service and they have already corrected to use deviceId: {exact: id} in Firefox as the spec recommends for device switching.

If you have a fix, would that be risk free for an uplift this late in the cycle? Thanks

The fix is trivial, but unfortunately, we don't have a way to test the specific case this fixes, because we lack test infrastructure for testing multiple devices of the same kind. — All we can test is this fix doesn't break permission for obtaining a single device per kind.

Flags: needinfo?(jib)

Workaround

For those arriving here looking for a workaround, the spec recommends:

  1. using ideal deviceId: id when users come to the site, and
  2. using exact deviceId: {exact: id} from your site's camera/mic picker (e.g. typically under a site's ⚙️ settings)

See Example 6 (use ideal on revisit) and Example 7 (use exact in device picker) respectively. This gives the best user experience in all browsers.

Benefits in all browsers:

  • (Option 1) On revisits to your site, it's common to try to use the camera the user used last time, but using ideal constrains (e.g. deviceId: id) here is better, because this allows it to fall back to a different device if the user has removed their device since then, or (in cases where constraints other than deviceId are used) added another more fit device.
  • (Option 2) assumes getUserMedia is called in direct response to a user selecting a specific camera or microphone in your interface. It makes sense to use deviceId: {exact: id} to control that you only get that particular device successfully, or an error. This avoids accidentally showing the wrong combination of preview and label. — If the request fails you may want to give an error to the user (NotAllowedError = no permission, other errors = this camera is not working), and depending on the specifics of your device switching UI, leave the preview blank or go back to an earlier device or not.

Benefits specific to Firefox which grants per-device permission by default:

  • Option 1 gives users who haven't opted out of prompting a chance to use a different camera or microphone this visit, and won't wonder why they're not seeing all their devices listed in the prompt.
  • Option 2 gives sites our simplified permission prompt appearance in Firefox 88+, meant to not compete with in-content device selection.

Paul, any tips on how to test this?

Flags: needinfo?(pbz)

(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #8)

Paul, any tips on how to test this?

I haven't worked with our webrtc tests that much yet so I'm not sure how exactly the test environment handles audio / video devices.
However, if you wanted to test checkRequestAllowed specifically, perhaps you could mock a webrtc:Request call which includes multiple cameras or mics? For example by calling WebRTCParent#receiveMessage directly.

Flags: needinfo?(pbz)

Marking as fix-optional for 89 in case we have a fix we could include in a potential dot release as a ride-along patch.

Johann, could you have a look at this please? I'd ideally like to get this landed before soft code freeze tomorrow, even doing a follow-up test if necessary.

Flags: needinfo?(jhofmann)
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6a31940f60b7
Remove getUserMedia's preference for less than ideal cameras/mics that already have permission (minimal patch). r=pbz
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

Sorry for the delay, seems fine retroactively.

Flags: needinfo?(jhofmann)
Regressions: 1723703
Has Regression Range: --- → yes
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: