927358 - getUserMedia() in Firefox Beta effectively fails all mandatory constraints, which is overly harsh and no longer the direction we want

Status: VERIFIED FIXED

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

Description

[dominik.steiner]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_5) AppleWebKit/536.30.1 (KHTML, like Gecko) Version/6.0.5 Safari/536.30.1

Steps to reproduce:

Call navigator.mozGetUserMedia() in FF Nightly with the following constraints (which is working in FF 24)
 
{ audio: true, video: { mandatory: { maxWidth: 320, maxHeight: 240 }}}


Actual results:

This will produce an error 

navigator.getUserMedia error: NOT_SUPPORTED_ERR: maxWidth


Expected results:

Investigating into the src code the reason seems to be in the changes to MediaStreamTrack.webidl

https://github.com/mozilla/mozilla-central/blob/FIREFOX_24_0_RELEASE/dom/webidl/MediaStreamTrack.webidl

https://github.com/mozilla/mozilla-central/blob/master/dom/webidl/MediaStreamTrack.webidl

Is there any plans to add validation of minWidth/maxWidth/minHeight/maxHeight to the media constraints? Or is there any reason for having removed the possibility to set those in mozGetUserMedia?

Comment 1

[Jan Gerber]

ran into the same issue, the spec now lists a new syntax:

{ audio: true, video: { mandatory: { width: {max: 320}, height: {max: 240} }}}

but thats also throwing an error

Comment 2

[Jen Fong-Adwent [:ednapiranha]]

I'm using this currently and it works in Chrome, does not cause an error in Firefox Nightly but doesn't do anything except 4:3:

{ 
  audio: false, 
  video: {
    optional: [
      { minHeight: 180 },
      { maxHeight: 180 },
      { minWidth: 320 },
      { maxWidth: 320 }
    ]
  }
}

Comment 3

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

We don't support the resolution constraints yet in Firefox.  Jib?  We probably should dup or invalid this bug.

Comment 4

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

Correct, we don't support resolution constraints yet, that is Bug 907352.

Before I close as a dup, should we be concerned that as of Firefox 26 this (correctly) produces an error callback, whereas in earlier versions these constraints were ignored, which means at least it played?

This might be foreshadowing problems with the 'mandatory' constraint. When would someone ever use mandatory? A question for the list perhaps.

Comment 5

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

We decided on Tuesday to pull back this new behavior before it goes to Beta, so I'm taking it to work on.

Having mandatory constraints fail when most constraints are not implemented is too harsh and will cause complaints.

Rather than revert the change, I plan to change mandatory to only work on implemented constraints (facingMode), as outlined in part A in this proposal http://lists.w3.org/Archives/Public/public-media-capture/2013Nov/0161.html pending resolution to this problem in the spec.

Comment 6

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

I've renamed the subject to clarify the immediate problem.

I say "Firefox Beta" because this started with Bug 882145 which landed right at the end of mozilla26.

I say "effectively all mandatory constraints" because facingMode, the only implemented one, only really works on phones and OSX cameras for now.

Comment 7

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

Attachment: Too soon to begin failing on unsupported mandatory constraints.

No longer fires error-callback or bails if unknown keys are detected in aRawConstraints. Silently ignore them instead, like we used to.

Try - https://tbpl.mozilla.org/?tree=Try&rev=9ae6242b570a

Comment 8

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

Actually, "Too soon" is misleading, I regret phrasing it that way. Instead it's more likely we don't want to go in this direction at all, which is why I'm hoping to get this super-safe change all the way to Beta, so things don't stop working and programmers aren't forced to remove all their mandatory constraints.

Comment 9

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

Attachment: Was mistake to begin failing on unsupported mandatory constraints. r=jesup

No change. New commit message. Carrying forward r+ from jesup.

Comment 10

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

Comment on attachment 8337024 [details] [diff] [review]
Was mistake to begin failing on unsupported mandatory constraints. r=jesup

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 882145
User impact if declined: 

getUserMedia() stops working in Firefox for any web-site that specifies mandatory constraints such as width and height, things that Chrome supports and we used to ignore before (which at least made things work, albeit in the wrong size).

This patch prevents things from stopping by restoring the old behavior of ignoring unsupported mandatory constraints, hopefully in time (aiming for Beta). We now think it was wrong-headed in first place to be this strict, and we have decided to go a new direction that only constrains by properties the browser knows, and light of this the original behavior which this patch brings back is correct, and desirable.

Testing completed (on m-c, etc.): Successful try with updated mochitest. Works locally.

Risk to taking this patch (and alternatives if risky):

Extremely low risk. The code used to bail for no technical reason if an unknown key was detected in a raw object. All subsequent code uses the normalized dictionary copy that the object got copied into, which is clean of any unknown keys since dictionaries don't support unknown keys. 

String or IDL/UUID changes made by this patch: None

Comment 11

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

https://hg.mozilla.org/integration/b2g-inbound/rev/f0a7ca5a20a8

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f0a7ca5a20a8

Comment 13

[bhavana bajaj [:bajaj]]

Comment on attachment 8337024 [details] [diff] [review]
Was mistake to begin failing on unsupported mandatory constraints. r=jesup

Approving on aurora given we have some time and this can get baked/verified.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8f1a8bfca0df

Comment 15

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

Comment on attachment 8337024 [details] [diff] [review]
Was mistake to begin failing on unsupported mandatory constraints. r=jesup

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:


[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 882145
User impact if declined:

getUserMedia() stops working in Firefox for any web-site that specifies mandatory constraints such as width and height, things that Chrome supports and we used to ignore before (which at least made things work, albeit in the wrong size).

This patch prevents things from stopping by restoring the old behavior of ignoring unsupported mandatory constraints, hopefully in time (aiming for Beta). We now think it was wrong-headed in first place to be this strict, and we have decided to go a new direction that only constrains by properties the browser knows, and light of this the original behavior which this patch brings back is correct, and desirable.

Testing completed (on m-c, etc.): Successful try with updated mochitest. Works locally. Landed on nightly and Aurora.

Risk to taking this patch (and alternatives if risky):

Extremely low risk. The code used to bail for no technical reason if an unknown key was detected in a raw object. All subsequent code uses the normalized dictionary copy that the object got copied into, which is clean of any unknown keys since dictionaries don't support unknown keys. This patch simply removes the code that bails, effectively ignoring that there ever was an unknown key in the origin raw object. This effectively neutralizes the validation code that was added, in the safest possible way.

String or IDL/UUID changes made by this patch: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0c2f6fb212aa

Bug 917298 landed on 27 and wasn't uplifted, so the changes to test_getUserMedia_constraints.html were ignored.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/0c2f6fb212aa

Comment 18

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:27.0) Gecko/20100101 Firefox/27.0, User Agent 20131016030202

I've tried to reproduce the initial issue using the Nightly build from 16th October, where I set a pref as in Comment 0 (or Comment 1) and then went to several websites such as https://apprtc.webrtc.org, https://apprtc.appspot.com/, http://shinydemos.com/cracked/ but I didn't saw the error message.

Sometimes on apprtc websites I get the error "Failed to get access to local media. Error code was undefined.", but it's intermittently and it's happening also on latest beta version (8).
Is there something else I should do? Thank you.

Comment 19

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

Attachment: SimplePeerConnectionTest.html with { mandatory: { foo:true } }

(In reply to petruta.rasa from comment #18)
> I've tried to reproduce the initial issue using the Nightly build from 16th
> October, where I set a pref as in Comment 0 (or Comment 1) and then went to
> several websites such as https://apprtc.webrtc.org,
> https://apprtc.appspot.com/, http://shinydemos.com/cracked/ but I didn't saw
> the error message.

What pref are you using? I'm not aware of a pref for this. Constraints need to be specified in the mozGetUserMedia() call in the JS.

See attached test case.

Comment 20

[Petruta Horea [:phorea], Desktop Test Engineering (back on Sep 5th)]

Thanks for explaining. 
I manage to reproduce the initial issue and I verified that it is fixed on Win 7 64-bit, Ubuntu 32-bit and Mac OS X 10.8.5 using Firefox 26 beta 8 (20131125215016), latest Aurora (20131127004001) and latest Nightly (20131127030201).