Closed Bug 1006707 Opened 10 years ago Closed 10 years ago

getUserMedia() fails if it finds facingMode:[] array used in constraints

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35

People

(Reporter: jib, Assigned: jib)

References

Details

(Whiteboard: [p=1])

Attachments

(2 files, 2 obsolete files)

Part 2: add microphone to mediaSource and remove redundant enum in c++
(deleted), patch
jesup
: review+
Details | Diff | Splinter Review
Part 1: change facingMode from enum to DOMString and support it as an array (3) r=smaug, mt
(deleted), patch
jib
: review+
Details | Diff | Splinter Review 
From the spec, users should be able to do this:
> getUserMedia ({ video: { facingMode:['left', 'right'] } }, success, failure);

but they'll get an exception in Firefox right now.

This is a reminder to fix this once Bug 767924 sequences in unions is implemented.

And un-comment http://mxr.mozilla.org/mozilla-central/source/dom/media/tests/mochitest/constraints.js#37

enum VideoFacingModeEnum {
    "user",
    "environment",
    "left",
    "right"
};

dictionary MediaTrackConstraintSet {
    (VideoFacingModeEnum or sequence<VideoFacingModeEnum>) facingMode;
};
Depends on: 767924
(In reply to Jan-Ivar Bruaroey [:jib] from comment #0)
>     (VideoFacingModeEnum or sequence<VideoFacingModeEnum>) facingMode;

To do the union it also depends on Bug 995352.
Depends on: 995352
Whiteboard: [p=1]
Target Milestone: --- → mozilla33
Priority: -- → P2
Target Milestone: mozilla33 → mozilla35
Depends on: 1030192
Depends on: 1116225
No longer depends on: 1116225
Depends on: 1103153
I'll fix facingMode and mediaSource to use DOMString as well here.
Depends on: 1068740
No longer depends on: 995352, 1103153
Also removes old test_getUserMedia_exceptions.html which has morphed over time into a test of the webidl bindings we use, which should be redundant.
Attachment #8551819 - Flags: review?(martin.thomson)
Attachment #8551819 - Flags: review?(bugs)
I cleaned this up since I don't think the redefinitions buy us anything but labor (they're still DOM-dependent).
Attachment #8551821 - Flags: review?(rjesup)
I was talking about attachment 8551819 [details] [diff] [review]
Flags: needinfo?(jib)
Attachment #8551821 - Flags: review?(rjesup) → review+
(In reply to Olli Pettay [:smaug] from comment #5)
> Why the change to MediaSource? That is not in the spec
> http://w3c.github.io/mediacapture-main/getusermedia.html#idl-def-
> MediaTrackConstraintSet

Ah, I should have put that in Part 2 (but this way you get a single review). 

mediaSource itself is not in the spec but in an unofficial extension draft [1]. That draft is in flux, and Martin is one of the drivers. I'll cc him so he can chime in.

The TL;DR is that internally we already support microphone as a choice here (it's mediaSource not videoSource) for something we do with Hello I think, but with awkward duplicate definitions which part 2 rips out. Since the actual API uses DOMString for mediaSource there should be no impact to enumerating internal choices here.

[1] https://fluffy.github.io/w3c-screen-share
Flags: needinfo?(jib) → needinfo?(martin.thomson)
The question is what happens if one passes the dictionary with wrong value in the mediaSource.
By using enums that is handled by the bindings layer and it does what webidl spec says.
How does the implementation deal with that case if enum isn't used?
Comment on attachment 8551819 [details] [diff] [review]
Part 1: change facingMode from enum to DOMString and support it as an array

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

Not sure about this yet.

Can you explain why you removed an entire test file?  It only looks like *some* of the errors are a problem.

::: dom/media/tests/mochitest/constraints.js
@@ -35,5 @@
>                                         { facingMode:'user' },
>                                         { bar:0 }] },
> -                   video: { // TODO: Bug 767924 sequences in unions
> -                            //facingMode:['left', 'right', 'user', 'environment'],
> -                            //require:["facingMode"],

Losing the require here is an odd choice.

@@ +42,1 @@
>                              facingMode:'left',

Why two values for facingMode?  Doesn't that have do undefined things?
The new spec will be different again and it won't even contain mediaSource.  In the meantime, we have a dependency on this feature.  Jan-Ivar, once we have FPWD on the screen sharing stuff, we need to move quickly on implementation, I think.  That way, we can move more quickly to remove this legacy stuff.

See http://lists.w3.org/Archives/Public/public-media-capture/2015Jan/0044.html and http://w3c.github.io/mediacapture-screen-share/
The constraint algorithm is a matching algorithm, so an unknown value is just a non-match. I.e. gUM will fail if the constraint is required, and ignore it if it is not. This lets us add values in the future, which webidl enums wont let us do (pity that [1]).

The mediaSource constraint - being a bit of a hack - is enforced to always be required, so it will fail.

I should add a test for that. It fails with... PermissionDeniedError? [2] Hmm, that should be NotFoundError me thinks. Patch coming up.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=19936
[2] http://jsfiddle.net/jib1/gmsancrq
^ (In reply to Olli Pettay [:smaug] from comment #8)
(In reply to Martin Thomson [:mt] from comment #9)
> Can you explain why you removed an entire test file?  It only looks like
> *some* of the errors are a problem.

This test has been overtaken by events. With the VideoFacingEnum test obsolete with this patch, the remaining ones just check that we cut'n'pasted the spec webidl right. It's an "enter your password twice" test. Are we less likely to be wrong in two places than one? Nah, if we're wrong then I think we'll be consistent about it.

That said, I may co-opt that test to cover comment 11. 

> > -                   video: { // TODO: Bug 767924 sequences in unions
> > -                            //facingMode:['left', 'right', 'user', 'environment'],
> > -                            //require:["facingMode"],
> 
> Losing the require here is an odd choice.

Testing a required facingMode without capability-laden devices on our builders is hard. You'd think ['left', 'right', 'user', 'environment'] would succeed since it contains all the possible values (known today), alas it doesn't since there's no accounting for (most) cameras that don't know where they're facing, and testing failure because four things are wrong instead of one doesn't increase coverage, so I made it optional.

> @@ +42,1 @@
> >                              facingMode:'left',
> 
> Why two values for facingMode?  Doesn't that have do undefined things?

Whoops. You'd think. Then again: http://jsfiddle.net/jib1/gmsancrq/5 - Olli, is this surprising to you?
> Olli, is this surprising to you?

Duh, nevermind. That's just a JS object, not webidl.
- Unknown mediaSource fails w/NotFoundError instead of PermissionDeniedError.
- Added tests for this (in constraints.js in the end).
Attachment #8551819 - Attachment is obsolete: true
Attachment #8551819 - Flags: review?(martin.thomson)
Attachment #8551819 - Flags: review?(bugs)
Flags: needinfo?(martin.thomson)
Attachment #8552021 - Flags: review?(martin.thomson)
Attachment #8552021 - Flags: review?(bugs)
Still don't understand the change to mediaSource.
You linked to http://fluffy.github.io/w3c-screen-share/, and that has an enum.
And if the value actually is used as an enum, it should be enum and let webidl bindings code to deal with the value validation.
Attachment #8552021 - Flags: review?(martin.thomson) → review+
Comment on attachment 8552021 [details] [diff] [review]
Part 1: change facingMode from enum to DOMString and support it as an array (2)

It is weird then to leave MediaSourceEnum there, so perhaps add a comment why it is still there.
Attachment #8552021 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #16)
> You linked to http://fluffy.github.io/w3c-screen-share/, and that has an
> enum.

(Covered on irc, but I'll put an answer here for posterity). That's an omission in that draft which has been overtaken by a new draft, hence is not maintained. More importantly, being a constraint, it should follow the pattern established for constraints by facingMode in the same patch, which is reflected correctly in the spec [1].

> And if the value actually is used as an enum, it should be enum and let
> webidl bindings code to deal with the value validation.

It is most definitely used as an enum in the implementation, and we would love to use enum for constraints, unfortunately there's no option to make it not throw [2]. According to the spec: mediaDevices.getUserMedia({ video { facingMode: "skyward" }) MUST NOT fail.

There are plenty of examples in webidl where unknown things are tolerated rather than throw, most notably: dictionary members, as well as unknown enum-values in attributes (it's a mess).

But on enums in dictionaries and methods, WebIDL has decided to mandate that everyone throw on default: in their switch statements.

[1] http://w3c.github.io/mediacapture-main/getusermedia.html#track-property-registrations

[2] There's a proposal for adding an option to make enums not throw, please go vote for it here:
    https://www.w3.org/Bugs/Public/show_bug.cgi?id=19936
(In reply to Olli Pettay [:smaug] from comment #17)
> It is weird then to leave MediaSourceEnum there, so perhaps add a comment
> why it is still there.

Its binding code is used in the patch (see MediaManager.cpp). It is quite useful.
They're also in the spec, because they're quite useful there too (for documentation & referencing reasons).
Add comment. Carrying forward r=smaug, mt.
Attachment #8552021 - Attachment is obsolete: true
Attachment #8552497 - Flags: review+
Comment on attachment 8552497 [details] [diff] [review]
Part 1: change facingMode from enum to DOMString and support it as an array (3) r=smaug, mt

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

::: dom/media/MediaManager.cpp
@@ +1686,5 @@
>    nsIURI* docURI = aWindow->GetDocumentURI();
>  
>    if (c.mVideo.IsMediaTrackConstraints()) {
>      auto& tc = c.mVideo.GetAsMediaTrackConstraints();
> +    MediaSourceEnum src = StringToEnum(dom::MediaSourceEnumValues::strings,

dom::MediaSourceEnum src = ...
Depends on: 1125588
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: