1162412 - facingMode regression in 38

Status: VERIFIED FIXED

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

Description

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

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.

Comment 1

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

Attachment: Part 1: don't treat plain facingMode constraint as required

Comment 2

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

Attachment: Part 2: order devices by shortest fitness distance

Comment 3

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

Attachment: Part 3: treat plain values as exact in advanced

Comment 4

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

Android and B2G builds to test:

Try - https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2c82cc1801f

Comment 5

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

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

Comment 6

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

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

Comment 7

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

Attachment: Part 1: don't treat plain facingMode constraint as required (2) r=jesup

Fix comment. Carrying forward r=jesup.

Comment 8

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

Attachment: Part 2: order devices by shortest fitness distance (2) r=jesup

Unbitrot.

Comment 9

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

Attachment: Part 3: treat plain values as exact in advanced (2) r=jesup

Update commit msg.

Comment 10

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

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

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d1e073342ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/252c81b4f5d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/98cd3686996b

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8d1e073342ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/252c81b4f5d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/98cd3686996b

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8d1e073342ed
https://hg.mozilla.org/mozilla-central/rev/252c81b4f5d5
https://hg.mozilla.org/mozilla-central/rev/98cd3686996b

Comment 14

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

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

Comment 15

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

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

Comment 16

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

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

Comment 17

[Maire Reavy [:mreavy]]

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.

Comment 18

[Maire Reavy [:mreavy]]

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.

Comment 19

[Maire Reavy [:mreavy]]

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.

Comment 20

[Maire Reavy [:mreavy]]

I've verified on m-c using Win7 that gUM no longer fails.

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 22

[Lawrence Mandel [:lmandel] (use needinfo)]

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.

Comment 23

[Lawrence Mandel [:lmandel] (use needinfo)]

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?

Comment 24

[Maire Reavy [: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!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b1d4b72e99dc
https://hg.mozilla.org/releases/mozilla-beta/rev/ae66eba26a70
https://hg.mozilla.org/releases/mozilla-beta/rev/b9b6b2f1a448

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/c14434ed2197
https://hg.mozilla.org/releases/mozilla-release/rev/9c1d3c0257ec
https://hg.mozilla.org/releases/mozilla-release/rev/e3045256cb27

Comment 28

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 29

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 30

[Bogdan Maris, Desktop Test Engineering]

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.