Closed Bug 1162412 Opened 9 years ago Closed 9 years ago

facingMode regression in 38

Categories

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

38 Branch
defect
Not set
major

Tracking

()

VERIFIED FIXED
mozilla40
Tracking Status
firefox38 --- wontfix
firefox38.0.5 + verified
firefox39 + verified
firefox40 --- verified
relnote-firefox --- 38+

People

(Reporter: jib, Assigned: jib)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

getUserMedia({ video: { facingMode: "user" } }) on dual-cam Androids and B2Gs used to make the front camera the default choice in the camera picker (normally back is default), and would still let users choose.

This regressed in 38 where users now ONLY get the front camera (i.e. no choice) when you do this.

This is observable on http://webcamtoy.com and http://jsfiddle.net/nq0gm4ts - Found in Bug 1161870.

(It's also observable on OSX with a second USB camera attached, since "FaceTime" cameras are considered "user"-facing).

In restoring the previous behavior, I found three regressions in 38. I'll put up separate patches so we can decide on uplift individually. In decreasing order of importance:

1. Plain facingMode mistakenly treated as exact instead of ideal.
2. Default camera is not the best fitting one against constraints.
3. Advanced algorithm wasn't treating plain values as exact (don't ask. odd spec).

#1 and #2 are needed to restore behavior to known sites. #3 is needed for any sites using advanced form (none known).

Note that #2 and #3 are not observable on desktop, because the desktop doorhanger appears to blatantly ignore constraints in its insistence on remembering whatever camera I chose last.

All this pretty much affects facingMode only, since websites choosing a user's camera based on resolution or frame rate seem rare.
Attachment #8602566 - Flags: review?(rjesup)
Attached patch Part 3: treat plain values as exact in advanced (obsolete) — — Splinter Review
Attachment #8602567 - Flags: review?(rjesup)
Comment on attachment 8602565 [details] [diff] [review]
Part 1: don't treat plain facingMode constraint as required

Review of attachment 8602565 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/MediaManager.cpp
@@ +529,5 @@
>  uint32_t
>  VideoDevice::GetBestFitnessDistance(
>      const nsTArray<const MediaTrackConstraintSet*>& aConstraintSets)
>  {
> +  // TODO: Minimal kludge to fix plain and ideal facingMode regression. Improve.

File/add bug #
We should talk about ways to reduce the amount of hand-rolled code involved in the constraint algorithm processing, to ease impl and to reduce chances of missing something when adding constraints
Attachment #8602565 - Flags: review?(rjesup) → review+
Attachment #8602566 - Flags: review?(rjesup) → review+
Attachment #8602567 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #5)
> > +  // TODO: Minimal kludge to fix plain and ideal facingMode regression. Improve.
> 
> File/add bug #
> We should talk about ways to reduce the amount of hand-rolled code involved
> in the constraint algorithm processing, to ease impl and to reduce chances
> of missing something when adding constraints

It's aligned and centralized better in Bug 1037389 comment 6, but I made the smaller one here for uplift.
Fix comment. Carrying forward r=jesup.
Attachment #8602565 - Attachment is obsolete: true
Attachment #8602842 - Flags: review+
Unbitrot.
Attachment #8602566 - Attachment is obsolete: true
Attachment #8602843 - Flags: review+
Update commit msg.
Attachment #8602567 - Attachment is obsolete: true
Attachment #8602844 - Flags: review+
Try in comment 4 looks green and I've verified the fix on Android and OSX.

(Note though that http://webcamtoy.com crashes, but that is Bug 1162720).
Keywords: checkin-needed
Comment on attachment 8602842 [details] [diff] [review]
Part 1: don't treat plain facingMode constraint as required (2) r=jesup

Approval Request Comment
[Feature/regressing bug #]: Regressed in Bug 1119335

[User impact if declined]:

(Sorry for the late notice, just discovered this bug's effect on Windows and Linux)

Users on Windows or Linux who visit sites like http://webcamtoy.com (*) or http://jsfiddle.net/nq0gm4ts that use constraints to prefer (rather than require) the front camera, no longer get asked for their camera.

*) Update: Looks like http://webcamtoy.com uses (or has reverted to using?) flash for Firefox on Windows, maybe because of this bug? (They still use WebRTC on Mac, where this bug isn't happening.)

Users on Fennec and B2G (and OSX w/USB cam) who visit sites like http://webcamtoy.com or http://jsfiddle.net/nq0gm4ts that use constraints to prefer (rather than require) the front camera, no longer get the option to choose between the front and back camera on mobile (or between the built-in mac camera and a connected non-built-in/USB camera on OSX), and instead get asked for the front camera only.

[Describe test coverage new/current, TreeHerder]:

On m-c since Saturday. Verified locally on Windows and fennec, and on B2G in Bug 1161870 comment 9.

[Risks and why]: 

Low. Patch is specifically written to be as surgical as possible, and only changes how facingMode constraints are interpreted in the constraints algorithm.

However, if part 2 is not also approved, then this fix is a mixed blessing on mobile since while it brings back the missing camera chooser, it would default to the wrong (back) camera.

[String/UUID change made/needed]: none
Attachment #8602842 - Flags: approval-mozilla-release?
Attachment #8602842 - Flags: approval-mozilla-beta?
Comment on attachment 8602843 [details] [diff] [review]
Part 2: order devices by shortest fitness distance (2) r=jesup

Approval Request Comment
[Feature/regressing bug #]: Regressed in Bug 1119335

[User impact if declined]: (Assumes approval of part 1)

Users on Fennec and B2G who visit sites like http://webcamtoy.com or http://jsfiddle.net/nq0gm4ts that use constraints to prefer the front camera, get the back camera as the default choice.

[Describe test coverage new/current, TreeHerder]:

On m-c since Saturday. Verified locally on fennec, and on B2G in Bug 1161870 comment 9.

[Risks and why]: 

Low. The addition of sorting would only affect users with multiple cameras or multiple mics, and only to the extent that one is shown as default over the other.

[String/UUID change made/needed]: none
Attachment #8602843 - Flags: approval-mozilla-beta?
Comment on attachment 8602844 [details] [diff] [review]
Part 3: treat plain values as exact in advanced (2) r=jesup

Approval Request Comment
[Feature/regressing bug #]: First featured in Bug 1119335

[User impact if declined]:

(Not really a regression, since plain values in constraints, like { width: 1280 } weren't supported before 38 (would throw error), but since 38, such plain values when used in "advanced" constraints are ignored, which isn't much better.)

Users who visit web sites like http://jsfiddle.net/56q7oy5u that use "advanced" constraints  to set preferred values for camera properties like dimensions, frame rate, and (on mobile) facing mode, will find these preferences ignored.

I doubt there are many sites like that, though who knows, so this is more of a problem for web developers, who are looking for clarity around how constraints work and how to use them compatibly right now across versions. While "advanced" is the "old" way of specifying constraints, they may still have some merit when attempting to write constraints that work across browsers, since webkit still uses an outdated mandatory/optional syntax (where "optional" works the same as "advanced").

Basically, it would be great to not have to add more caveats to:
https://developer.mozilla.org/en-US/docs/Web/API/MediaDevices/getUserMedia#Browser_compatibility

[Describe test coverage new/current, TreeHerder]: On m-c since Saturday. Verified locally on OSX.

[Risks and why]: 

Low and contained to constraints. This patch to the constraints algorithm basically adds a boolean 'advanced' flag to the algorithm sub-functions that effectively treat plain values as 'exact' when in the advanced algorithm and 'ideal' otherwise, per spec.

[String/UUID change made/needed]: none
Attachment #8602844 - Flags: approval-mozilla-beta?
Comment on attachment 8602842 [details] [diff] [review]
Part 1: don't treat plain facingMode constraint as required (2) r=jesup

Review of attachment 8602842 [details] [diff] [review]:
-----------------------------------------------------------------

I talked with jib and jesup, and we agree that we need part 1 in Fx 38.0.5 , but parts 2 and 3 can wait until Fx39.  I believe approval-mozilla-beta is the correct flag to set for 38.0.5 and approval-mozilla-aurora is the right one for Fx39.  If I'm mistaken, I'd appreciate it if relman could update our request.  I will update the requests for part 2 and part 3 to be approval-mozilla-aurora (Fx39) only.
Attachment #8602842 - Flags: approval-mozilla-release? → approval-mozilla-aurora?
Comment on attachment 8602843 [details] [diff] [review]
Part 2: order devices by shortest fitness distance (2) r=jesup

Review of attachment 8602843 [details] [diff] [review]:
-----------------------------------------------------------------

Moving the request to ask for uplift to Fx39.
Attachment #8602843 - Flags: approval-mozilla-beta? → approval-mozilla-aurora?
Comment on attachment 8602844 [details] [diff] [review]
Part 3: treat plain values as exact in advanced (2) r=jesup

Review of attachment 8602844 [details] [diff] [review]:
-----------------------------------------------------------------

Moving the request to ask for uplift to Fx39.
Attachment #8602844 - Flags: approval-mozilla-beta? → approval-mozilla-aurora?
I've verified on m-c using Win7 that gUM no longer fails.
Comment on attachment 8602842 [details] [diff] [review]
Part 1: don't treat plain facingMode constraint as required (2) r=jesup

release = 38.0.5
beta = 39
Adjusting the approval requests appropriately.
Attachment #8602842 - Flags: approval-mozilla-aurora? → approval-mozilla-release?
Attachment #8602843 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Attachment #8602844 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8602842 [details] [diff] [review]
Part 1: don't treat plain facingMode constraint as required (2) r=jesup

I spoke with maire and jib on irc. This bug affects sites that use the new constraints mechanism's facingMode constraint. It's unclear how many sites this impacts but for those impacted (the ones that use facingMode) gUM fails on Windows and Linux. Although webcamtoy.com was able to fallback to Flash, most sites are not built to support this option. We're going to take this verified fix in 38.0.5.

Note that release approval is for 38.0.5 and NOT 38.0 relbranch.
Attachment #8602842 - Flags: approval-mozilla-release?
Attachment #8602842 - Flags: approval-mozilla-release+
Attachment #8602842 - Flags: approval-mozilla-beta?
Attachment #8602842 - Flags: approval-mozilla-beta+
This is a relnote for 38.0.5.

Release Note Request (optional, but appreciated)
[Why is this notable]: Use of the facingMode constraint caused gUM failure on Windows and Linux. (See comment 22.)
[Suggested wording]: Fixed: facingMode constraint resulted in gUM failure on Windows and Linux
[Links (documentation, blog post, etc)]:

mreavy - Can you review the relnote?
Flags: needinfo?(mreavy)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #23)
> This is a relnote for 38.0.5.
> 
> Release Note Request (optional, but appreciated)
> [Why is this notable]: Use of the facingMode constraint caused gUM failure
> on Windows and Linux. (See comment 22.)
> [Suggested wording]: Fixed: facingMode constraint resulted in gUM failure on
> Windows and Linux
> [Links (documentation, blog post, etc)]:
> 
> mreavy - Can you review the relnote?

Looks good.  Thanks!
Flags: needinfo?(mreavy)
Flags: qe-verify+
Comment on attachment 8602843 [details] [diff] [review]
Part 2: order devices by shortest fitness distance (2) r=jesup

This already landed on beta for 39.
Attachment #8602843 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8602844 [details] [diff] [review]
Part 3: treat plain values as exact in advanced (2) r=jesup

This patch also already landed on beta for 39.
Attachment #8602844 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Reproduced the initial issues using Firefox 38.0 RC on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit with pages http://webcamtoy.com and http://jsfiddle.net/nq0gm4ts.
Verified as fixed using Firefox 38.0.5, Firefox 39.0 RC and latest Aurora.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.