Closed Bug 1451798 Opened 2 years ago Closed 2 years ago

Video facingMode regression

Categories

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

60 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + verified
firefox61 + verified

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(1 file)

Calling gUM with a facingMode constraint has regressed in 60. Bug 1299515 is the likely suspect so marking it for now.

STR
1 Use an android device with two cameras
2 Load https://jsfiddle.net/jib1/03ng8wva/
3 Click Start!

Expected:
Permission prompt shows "Front facing camera" first in the list, and selected by default

Actual:
Permission prompt shows "Back facing camera" first in the list, and selected by default
Rank: 9
Priority: -- → P1
Assignee: nobody → apehrson
Status: NEW → ASSIGNED
Comment on attachment 8965667 [details]
Bug 1451798 - Check the variable actually containing the string.

https://reviewboard.mozilla.org/r/234524/#review240384

Wow, what a footgun Move() is here. Is there really no better pattern for this?
Attachment #8965667 - Flags: review?(jib) → review+
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #2)
> Comment on attachment 8965667 [details]
> Bug 1451798 - Check the variable actually containing the string.
> 
> https://reviewboard.mozilla.org/r/234524/#review240384
> 
> Wow, what a footgun Move() is here. Is there really no better pattern for
> this?

The proper way IMO would be to do these checks in the platform backend and signal them up to the MediaEngineSource through IPC. At least on Android this is provided by the system APIs. [1] [2]

A simpler fix to remove the footgun here could be to lift this logic out to a local function that takes a string (the device name) and returns a `Maybe<VideoFacingModeEnum>`.

Feel free to file followups for either of these approaches.


[1] https://developer.android.com/reference/android/hardware/Camera.CameraInfo.html#facing
[2] https://developer.android.com/reference/android/hardware/camera2/CameraCharacteristics.html#LENS_FACING
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cfe13e874f02
Check the variable actually containing the string. r=jib
https://hg.mozilla.org/mozilla-central/rev/cfe13e874f02
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8965667 [details]
Bug 1451798 - Check the variable actually containing the string.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1299515
[User impact if declined]: Websites using camera capture aimed at mobile (and some other platforms potentially) might end up showing the wrong camera.
[Is this code covered by automated tests?]: No, we don't have devices with real cameras in automation.
[Has the fix been verified in Nightly?]: No. But I've verified locally.
[Needs manual test from QE? If yes, steps to reproduce]: Yes. See comment 0.
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Trivial change
[String changes made/needed]: None
Attachment #8965667 - Flags: approval-mozilla-beta?
Comment on attachment 8965667 [details]
Bug 1451798 - Check the variable actually containing the string.

Fix a new regression in Fx60. Approved for 60.0b12.
Attachment #8965667 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Device:
 - Samsung Gala

Verified in the latest Nightly (2017-04-13) and Beta (60.0b12) builds following the steps provided in Comment 0. The issue is no longer reproducible, marking this as verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.