882145 - Implement facingMode as first gUM constraint

Status: RESOLVED FIXED

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

Description

[u459114]

1. Implement MediaStreamTrack constrains function
   MediaStreamTrack::constraints 
   MediaStreamTrack::applyConstraints
2. Implement MediaTrackConstraint IDL and native class

Comment 1

[Jason Smith [:jsmith]]

Note - we've discontinued using that webrtc tracking bug and use the whiteboards instead.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

MediaStreamConstraints contains MediaTrackConstrants that allows user to specify the back/front camera with |facingMode|. [1]
We should prioritize this bug because choosing front/back camera is a basic feature of video call and camera application.

[1] http://www.w3.org/TR/mediacapture-streams/#constraint-registrations

Comment 3

[u459114]

Hi Ekr/ Randell/ Maire,
Any suggestion for this?

With this API change, we allow application owners to define which camera they choose as the source of media stream. Currently, the only way to choose camera HW is by using video permission dialog, which is still under discussion. 

My plan is
1. Support MediaStreamConstraints in gUM to allow app developers define which camera they want to use at programming time.
2. (Fallback)If user do not specify facingMode in gUM, we still allow device user to choose camera on the fly.

Comment 4

[Eric Rescorla (:ekr)]

Yes, I think this is clearly the right direction.

Comment 5

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

mreavy and I talked, and jib was going to be working on this anyways soon - at least the DOM and desktop side of it.  We'll need UI changes to respect implement changing the default shown to the user, which should be simple, so a small change to the B2G, Android and Desktop UI's will be needed.  We should file dependent bugs for the UI parts.

It's most useful currently on Android & B2G, until we implement persistent device IDs, but we'll want it soon for Desktop too.

Comment 6

[Maire Reavy [:mreavy]]

Sounds like we all agree that we should do this.  The only question is when.  Gecko 26 is the current trunk and will be the basis for v1.2.  Gecko 28 is the basis for v1.3.   

Do we want this for Gecko 26 (which uplifts Sept 16)?  I believe the answer is "yes", and I should add it to the Gecko 26 platform workload.  

As Randell says in comment 5, there is a little bit of UI work needed.  The UI work would be to respect an indicator from the gUM request as to which camera is preferred.  We can file dependent bugs for B2G and Android to describe exactly what's needed.

CJ -- Can you have the UI piece for B2G completed in time for Gecko 26?

Gian-Carlo -- Can you do the UI piece for Android in time for Gecko 26?

Thanks.

Comment 7

[u459114]

(In reply to Maire Reavy [:mreavy] from comment #6)
> CJ -- Can you have the UI piece for B2G completed in time for Gecko 26?
Randell, could you file an issue on UI change that we need?

Comment 8

[Gian-Carlo Pascutto [:gcp]]

>Can you do the UI piece for Android in time for Gecko 26?

This isn't my call to make (mfinkle for implementation, ibarlow for Ui design). Note that UI design changes like this will very likely have string freeze implications, which means uplifting will be severely restricted.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

The Android part could be done during this cycle Fx26, but we do need UI mocks from Ian, which means we need to better understand what the feature (MediaTrackConstraint) means. Anyone have a layman's version?

Also, I assume the UI work is blocked by any DOM work that needs to happen first.

Comment 10

[Maire Reavy [:mreavy]]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> The Android part could be done during this cycle Fx26, but we do need UI
> mocks from Ian, which means we need to better understand what the feature
> (MediaTrackConstraint) means. Anyone have a layman's version?
> 
> Also, I assume the UI work is blocked by any DOM work that needs to happen
> first.

We'll spin off a bug later today describing what UI work is needed.  I believe it's all "under the hood".  No string changes, no mock ups needed.  The UI work would be to respect an indicator from the gUM request as to which camera is preferred. So instead of always popping up the rear camera (which is what we do now), the app would be able to indicate a camera preference.

Comment 11

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

This task seems broadly defined.

> mreavy and I talked, and jib was going to be working on this anyways soon - at
> least the DOM and desktop side of it.

My immediate pragmatic interest was to work on gUM constraints for width and height as this dovetailed nicely into the "Constraints and Stats" showcase in Bug 902003 Comment 4, which requires no UI work.

Everyone else here seems excited about facingMode, which is great but has a dependency on UI work.

Are we sure we wish to discuss both in the same bug?

There seem to be many constraints we *could* support, so could we narrow this bug with a list of constraints required to close it, or break this up?

The example in the spec calls out these, which may be a good starting list:

{
  mandatory: {
    width: { min: 640 },
    height: { min: 480 }
  },
  optional: [
    { width: 650 },
    { width: { min: 650 }},
    { frameRate: 60 },
    { width: { max: 800 }},
    { facingMode: "user" }
  ]
}                           

The testcase I linked to plays with:
   "minWidth": "300",
   "maxWidth": "640",
   "minHeight": "200",
   "maxHeight": "480",
   "minFrameRate": "30"

> We'll need UI changes to respect implement changing the default shown to the user,
> which should be simple, so a small change to the B2G, Android and Desktop UI's will
> be needed.  We should file dependent bugs for the UI parts.

(As an example of a topic that has nothing to do with width/height) This may be the wrong place to discuss this, but did we finalize that we're tying camera selection to site access?

Comment 12

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

I've created Bug 907352 to cover width/height/framerate constraints on getUserMedia.

That way we can focus here on facingMode: "user" | "environment".

Comment 13

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

Implementation comment: gUM call is in webidl, while args to that call are xpidl. Rather than write more xpidl, I think it makes sense to convert said bits to webidl as I go. Will see how that goes and reevaluate.

Comment 14

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

Attachment: Part 1: Existing gUM constraints converted to webidl

Part 1. Converts existing gUM constraints to webidl.

- Replaces nsIMediaStreamOptions with MediaStreamConstraints
- New GetUserMediaRequest wrapper-interface shuttles constraints to observers.
- Updates webspeech/recognition/SpeechRecognition.cpp to use MediaStreamConstraints
- Removed non-standard audioDevice and videoDevice constraints, as I found no
  uses of them. If I am mistaken, let me know asap so I can put them back.
- Tweaked gUM success/failure callbacks to match spec.

Dependencies/Known problems:

- Needs Bug 767926 (union support) and Bug 903277 fixed before non-boolean audio &
  video constraints will be tolerated:

    dictionary MediaStreamConstraints {
      (boolean or MediaTrackConstraints) video = false;
      (boolean or MediaTrackConstraints) audio = false;
    };

Next:

Part 2 will add facingMode (I'm hoping dependencies can be addressed in FF26 timeframe, but if it ends up not looking good I can look at a non-webidl short-term fallback for part 2)

Comment 15

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

Attachment: Part 1: Existing gUM constraints converted to webidl (2)

Forgot to hg add two files.

Comment 16

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

Attachment: Part 1: Existing gUM constraints converted to webidl (3)

webidl nit (now uses sequence<> instead of [])

Comment 17

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

After a discussion on irc today, I'd like to clear up a misconception and manage expectations for this bug:

gUM's optional constraints *filter*, they do not merely convey preference.

The spec http://dev.w3.org/2011/webrtc/editor/getusermedia.html#methods-5 is pretty clear about optional constraints following a "remove from list"-filtering algorithm.

This means that on a phone with [{ name:"Back",  facingMode:"environment" },
                                 { name:"Front", facingMode:"user"}],

a web page setting a constraint of { optional:[{ facingMode:"user" }] }

reduces Firefox's permission list to 1 choice [{ name:"Front", facingMode:"user"}].

Importantly, the user will NOT be able to pick the back camera in this case!
The picker is effectively reduced to an information field.

"Fine", you may say, "apps wont do that". Wrong:

This problem is limited to Firefox because it offers camera selection in the permissions popup.

Consider writing an appRtc+ page aimed at Google Chrome for Android.

1. The whole screen is dedicated to video
2. A "Flip Front/Back" symbol (button) hovers in the corner
3. Constraint is { optional:[{ facingMode: (flipFront? "user" : "environment") }] }
4. gUM is called on startup and whenever symbol is pressed.

In Google Chrome for Android, the user hits "Accept" on their first ever visit only. The user flips the camera by pressing the "flip" symbol. The page stores the last camera used for subsequent visits. Works.

On this same Chrome-tailored page in Firefox for Android, the user must hit "OK" on the lone "Front" choice in the permission popup) on every visit, and again on pressing the "flip" symbol ("OK" on the lone "Back" choice in the permission popup) once per visit. The page stores the last camera used for subsequent visits. Still works, but importantly, switching the camera from within the permission popup never works, even if users bring up the popup themselves in the URL bar.

Conversely, an app tailored for Firefox for Android would not need to add any flip symbol, which would leave Google Chrome users without a way to switch. Any decent app would need to browser-sniff.

Comment 18

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

s/app/page/

Comment 19

[Eric Rescorla (:ekr)]

I think you're conflating a number of issues:

1. Device permissions versus device selection.
2. How constraints filters.
3. Persistent versus one-time permissions.


In Firefox when the site asks for access to the camera
and the user selects one of the cameras and clicks "OK", he
is granting access to that one device and no other
devices. If the site wants to access another camera,
we can't assume that the user wanted to grant access
and so we need to re-prompt the user. This isn't a selection
issue, it's a permissions issue. Given that, there is no
reason why the user shouldn't be re-prompted with only
one choice (the app's new request), since we're asking
their permission.

If the user wants to change their camera from the chrome
camera icon (something which Firefox does not appear to support
now, though Chrome does) I would expect them to presented
with the same list as they would have been presented at the
initial prompt, namely those which matched the provided
constraints. This seems totally normal. I haven't checked
that Chrome filters the list in this case, but I would expect
it to based on the specification.

In other words, this all seems normal and fine, given that
Firefox does not support persistent permissions; the
"Chrome-tailored" version you suggests seems to be just fine
for Firefox as well.


If Firefox had persistent permissions, I would expect the user
to be able to grant a given site access to some or all of his
cameras and the site to then be able to select any of those
without a new permissions prompt, just as they can in Chrome
(except that Chrome assumes that it's just all of them).

Comment 20

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

I didn't know you could change the camera in Google Chrome when I wrote the above. Neat! ;-)

Regardless, ekr agreed on irc that it is reasonable for "dialing-type" phone apps to start using facingMode to request the front camera. He and I are in agreement on how this will work by following the spec, so my message was instead aimed at anyone who would be surprised that they would not be able to change to the "Back" camera using the permissions popup for said dialing app.

Misconceptions in this bug:

- Comment 2 seems to think facingMode is the solution to webrtc on android coming
  up with the back camera each time. Only true if you are ok with webapp limiting
  your choices to front only.

- Comment 5 says "We'll need UI changes to respect implement changing the default
  shown to the user" - facingMode can't be used to change the default show, WITHOUT
  limiting the list.

Comment 21

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

> facingMode can't be used to change the default shown, WITHOUT limiting the list.

I've posted a proposal to the public-webrtc list to address this. We'll see.

Comment 22

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

Attachment: Part 1: Existing gUM constraints converted to webidl (4)

Updated with feedback from #content.
- Now handles unsupported mandatory constraints
- Proper rooting for JS object (hopefully).
- Figured out webidl-part of optional constraints.

WORK IN PROGRESS:

Webidl unions don't work sufficiently with dictionaries yet (Bug 767924), so {video:x) can't be boolean or constraints right now. I'm able to test with mock inputs like this: { video:true, videom:{mandatory: { facingMode:'environment'} } } where videom is a fake cousin.

My workaround is not ready, so I'm uploading what I have so others can get started (3 parts).

Comment 23

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

Attachment: Part 2: GetUserMediaDevices() takes constraints. Adds nsIMediaDevice::facingMode

Comment 24

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

Attachment: Part 3: Implements constraints algorithm

Initial working version.

- Please see notes in part 1 before testing.

- Only mandatory constraints work at the moment (and there's just one:
  facingMode).

- Contains kludge on OSX to treat Facetime cameras (and all other cameras
  containing the words "Face") as user-facing. All other cameras have
  userFacing unspecified, which means that if you specify facingMode
  as a mandatory constraint they'll be filtered out!

Patches contain incremental work and overlap a bit. I hope to refactor some before finishing and starting reviews.

Comment 25

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

Attachment: Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints

Refactored into two patches. Part 1 of 2.

Comment 26

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

Attachment: Part 2: Implements first gUM constraint facingMode

Comment 27

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

Attachment: Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (2)

Bugfix: Now detects unsupported mandatory constraints properly (in this patch, as a webidl-like feature).

Comment 28

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

Attachment: Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (3)

Modified a mochitest to pass (was testing on specific error msg that changed)

Comment 29

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

Attachment: Part 2: Implements first gUM constraint facingMode (2)

Implements optional constraint algorithm.
- Testing on phones with two cameras welcome.

Comment 30

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

Attachment: Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (4)

Changed GetUserMediaRequest interface, which is ChromeOnly, to return MediaStreamConstraintsInternal rather than MediaStreamConstraints, to avoid page-visibility.

Uploading so schien can test. Depends on (patch in) Bug 915368.

This is also the last patch before the bug 767924 workaround, which is uglier and coming next. So this patch is good to have here for reference.

Comment 31

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

Attachment: Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (5)

Now works with proper constraint syntax. Contains workaround for bug 767924.

I've filed followup bug 916012 to remove workaround later. Hopefully good.

Comment 32

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

Attachment: Part 2: Implements gUM constraints. First constraint: facingMode (3)

Refactored to work with workaround patch.

- Constraints (facingMode) now works correctly with
  media.navigator.permission.disabled=true as well.

Part 1+2 need to be on top of Bug 915368 and Bug 915419 to work.

Try - https://tbpl.mozilla.org/?tree=Try&rev=42078c3763f0

Comment 33

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

Note that this removes the privileged {device:''} gUM constraint. AFAICT it was unused. If you know otherwise, please let me know asap.

Comment 34

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

Attachment: Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (6)

Unbitrotted.

Comment 35

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

Attachment: Part 2: Implements gUM constraints. First constraint: facingMode (4)

Unbitrotted.

Comment 36

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

Comment on attachment 804500 [details] [diff] [review]
Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (6)

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

::: dom/media/GetUserMediaRequest.h
@@ +35,5 @@
> +  void GetCallID(nsString& retval);
> +  void GetConstraints(MediaStreamConstraintsInternal &result);
> +
> +private:
> +  nsPIDOMWindow * const mInnerWindow;

extra space.  Choose one style for "*" in a file.  I don't care which (foo* bar or foo *bar), but please choose one.

::: dom/media/MediaManager.cpp
@@ +1042,5 @@
>    nsCOMPtr<nsIDOMGetUserMediaErrorCallback> onError(aOnError);
>  
> +  Maybe<JSAutoCompartment> ac;
> +  if (aRawConstraints.mAudio.IsObject() || aRawConstraints.mVideo.IsObject()) {
> +    ac.construct(aCx, (aRawConstraints.mVideo.IsObject()?

space before ?

Comment 37

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 804500 [details] [diff] [review]
Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (6)

>+++ b/dom/base/Navigator.h
>+                       const mozilla::dom::MediaStreamConstraints& aConstraints,

Drop the "mozilla::dom::" bit: we're already in that namespace.

>+++ b/dom/bindings/Bindings.conf
>+    'headerFile': 'GetUserMediaRequest.h',

This shouldn't be needed if you export the header to mozilla/dom.  Which you should do.

>+++ b/dom/media/GetUserMediaRequest.cpp
>+GetUserMediaRequest::GetUserMediaRequest(nsPIDOMWindow* aInnerWindow,
>+    const nsAString& aCallID,
>+    const MediaStreamConstraintsInternal& aConstraints)

Please line up the args?  Either put the first one one the next line or just let the third one go over 80 chars.

>+nsISupports* GetUserMediaRequest::GetParentObject() { return mInnerWindow; }
>+void GetUserMediaRequest::GetCallID(nsString& retval) { retval = mCallID; }

These both need some newlines: before the '{', after the '{', and before the '}'.

>+uint64_t GetUserMediaRequest::WindowID()
>+  return mInnerWindow->GetOuterWindow()->WindowID();

Hrm.  What if GetOuterWindow() returns null because someone has held on to this thing for too long?

The property should probably make it clearer that the _outer_ window id is returned...  that said, why is it the outer's ID?

>+++ b/dom/media/GetUserMediaRequest.h
>+  nsPIDOMWindow * const mInnerWindow;

What makes sure this doesn't die?  I assume this should be a strong (and cycle-colected) reference.

>+++ b/dom/media/MediaManager.cpp
>+using dom::MediaStreamConstraints;         // Outside API (requires tracing)

You should just assume all on-stack dictionaries require tracing, fwiw.

>+static nsresult CompareDictionaries(JSContext* aCx, JSObject *aA,
>+  JSAutoCompartment ac(aCx, aA);
>+  JS::Rooted<JSObject*> a(aCx, aA);

The order of those two lines needs to be reversed.  Otherwise you have a GC hazard.

>+      // Unknown property found. Bail with name

Maybe "found in A"?

>+  aDifference->Truncate(0);

No need for the 0.

>+template<class MediaTrackConstraintsInternalType>
>+static nsresult ValidateTrackConstraints(JSContext *aCx, JSObject *aRaw,

Why does this need to be a template?  It's always passed a const MediaTrackConstraintsInternal for aNormalized, as far as I can tell.

>+  JS::Rooted<JS::Value> rawval(aCx, OBJECT_TO_JSVAL(aRaw));

  JS::ObjectValue(*aRaw);

>+  track.Init(aCx, rawval);

That can fail.  You need to handle it failing.

>@@ -1074,16 +1156,32 @@ MediaManager::GetUserMedia(bool aPrivile
>+  if (unknownConstraintFound.Length()) {

!unknownConstraintFound.IsEmpty()

>+++ b/dom/media/MediaManager.h
> namespace mozilla {
>+  namespace dom {
>+    class MediaStreamConstraints;
>+    class NavigatorUserMediaSuccessCallback;
>+    class NavigatorUserMediaErrorCallback;
>+  }

Shouldn't be indented.

> EXPORTS.mozilla += [
>+    'GetUserMediaRequest.h',

Export to mozilla/dom, please.

>+++ b/dom/webidl/GetUserMediaRequest.webidl
>+[ChromeOnly]
>+interface GetUserMediaRequest {

Even better, NoInterfaceObject?  The only reason we need an interface here at all, instead of a dictionary, is the silly nsIObserver thing, right?  Otherwise we could just use a dictionary for this thing.

>+++ b/dom/webidl/MediaStreamTrack.webidl
>+// Four possible internal representations to work around Bug 767924 for now

As discussed on irc, we don't need the *_[mb]{2} dictionaries here.

Also, add documentation about not adding anything that might need tracing to MediaStreamConstraintsInternal, ever.

>+++ b/dom/webidl/Navigator.webidl
>-                       MozDOMGetUserMediaSuccessCallback? onsuccess,
>-                       MozDOMGetUserMediaErrorCallback? onerror);
>+                       NavigatorUserMediaSuccessCallback successCallback,
>+                       NavigatorUserMediaErrorCallback errorCallback);

This used to allow passing null.  Now it doesn't.  Is that purposeful?

>-  void mozGetUserMediaDevices(MozGetUserMediaDevicesSuccessCallback? onsuccess,
>-                              MozDOMGetUserMediaErrorCallback? onerror);
>+                              MozGetUserMediaDevicesSuccessCallback onsuccess,
>+                              NavigatorUserMediaErrorCallback onerror);

Likewise.

I'd like to see an interdiff addressing these changes.

Comment 38

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

(In reply to Boris Zbarsky [:bz] from comment #37)
> >+uint64_t GetUserMediaRequest::WindowID()
> >+  return mInnerWindow->GetOuterWindow()->WindowID();
> 
> Hrm.  What if GetOuterWindow() returns null because someone has held on to
> this thing for too long?

Thanks, my code is wrong here. I should have the sender get the WindowID and stuff it in like the old code did. Will fix.

> The property should probably make it clearer that the _outer_ window id is
> returned...  that said, why is it the outer's ID?

This is unchanged code effectively. Before: http://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#1171

This outer id gets used here for UI: http://mxr.mozilla.org/mozilla-central/source/browser/modules/webrtcUI.jsm#122

> >+++ b/dom/media/GetUserMediaRequest.h
> >+  nsPIDOMWindow * const mInnerWindow;
> 
> What makes sure this doesn't die?  I assume this should be a strong (and
> cycle-colected) reference.

Basically I'm just looking for something to pass up as parent here:

  nsISupports* GetUserMediaRequest::GetParentObject()
  {
    return mInnerWindow;
  }

Basically I don't understand this part very well. Help appreciated.

> >+template<class MediaTrackConstraintsInternalType>
> >+static nsresult ValidateTrackConstraints(JSContext *aCx, JSObject *aRaw,
> 
> Why does this need to be a template?  It's always passed a const
> MediaTrackConstraintsInternal for aNormalized, as far as I can tell.

Removed template. A leftover from an experiment. Thanks.

> >+++ b/dom/media/MediaManager.h
> > namespace mozilla {
> >+  namespace dom {
> >+    class MediaStreamConstraints;
> >+    class NavigatorUserMediaSuccessCallback;
> >+    class NavigatorUserMediaErrorCallback;
> >+  }
> 
> Shouldn't be indented.
> 
> >+++ b/dom/webidl/GetUserMediaRequest.webidl
> >+[ChromeOnly]
> >+interface GetUserMediaRequest {
> 
> Even better, NoInterfaceObject?  The only reason we need an interface here
> at all, instead of a dictionary, is the silly nsIObserver thing, right?

Right.

> >+++ b/dom/webidl/Navigator.webidl
> >-                       MozDOMGetUserMediaSuccessCallback? onsuccess,
> >-                       MozDOMGetUserMediaErrorCallback? onerror);
> >+                       NavigatorUserMediaSuccessCallback successCallback,
> >+                       NavigatorUserMediaErrorCallback errorCallback);
> 
> This used to allow passing null.  Now it doesn't.  Is that purposeful?

Updated to match the spec. Jesup says he is fine with it, and it will ride the trains. Thanks for pointing it out though.

> >-  void mozGetUserMediaDevices(MozGetUserMediaDevicesSuccessCallback? onsuccess,
> >-                              MozDOMGetUserMediaErrorCallback? onerror);
> >+                              MozGetUserMediaDevicesSuccessCallback onsuccess,
> >+                              NavigatorUserMediaErrorCallback onerror);
> 
> Likewise.

This one is ChromeOnly.

> I'd like to see an interdiff addressing these changes.

Coming up.

I agree with all the other comments. Thanks!

Comment 39

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

Attachment: Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (7). r=bz r=jesup

Fixed all bugs and nits, except:

>>+++ b/dom/media/GetUserMediaRequest.h
>>+  nsPIDOMWindow * const mInnerWindow;
>
> What makes sure this doesn't die?  I assume this should be a strong
> (and cycle-colected) reference.

This is the only part I didn't address yet. Not sure how to fix. Can I pass up something simpler perhaps in GetParent()? How do people usually do this? I'm new at XPCOM interfaces so any advice is good advice. :-)

Comment 40

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

Attachment: interdiff to Part 1 patch

Comment 41

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

Attachment: Part 2: Implements gUM constraints. First constraint: facingMode (5) r=jesup

Unbitrotted

Comment 42

[Boris Zbarsky [:bzbarsky]]

> Basically I'm just looking for something to pass up as parent here:

Right, I understand that.  Hold a strong ref to the window, and cycle-collect it.

Comment 43

[Boris Zbarsky [:bzbarsky]]

The interdiff looks great, thank you.

So just need to fix the mInnerWindow ownership bit, and I'm happy.  So make that an nsCOMPtr<nsPIDOMWindow>, and change this bit:

+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_0(GetUserMediaRequest)

to:

+NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE_1(GetUserMediaRequest, mInnerWindow)

Comment 44

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

Attachment: Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (7). r=bz r=jesup

Done. Thanks!

Comment 45

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

Attachment: 2nd interdiff - Part 1 patch

Comment 46

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

Attachment: Part 2: Implements gUM constraints. First constraint: facingMode (6) r=jesup

Rebased

Comment 47

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

Comment on attachment 804834 [details] [diff] [review]
Part 2: Implements gUM constraints. First constraint: facingMode (6) r=jesup

Forgot to forward r+ from jesup on rebase.

Comment 48

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

Attachment: Part 3: Mochitests for testing gum constraints

Adding coverage. Not required to land.

Try - https://tbpl.mozilla.org/?tree=Try&rev=31806314302a

Comment 49

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

Try looks good. Ready to land.

Comment 50

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf373e408a6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc4ee7cbd7bd

Comment 51

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

Thanks! Wondering, should we land mochitest as well?

Comment 52

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

We can uplift tests easily, so we can wait for review from jsmith.  Do a try with the test (I suspect you already have, I didn't check the trys).

Comment 53

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cf373e408a6b
https://hg.mozilla.org/mozilla-central/rev/bc4ee7cbd7bd

Comment 54

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

(In reply to Randell Jesup [:jesup] from comment #52)
> We can uplift tests easily, so we can wait for review from jsmith.  Do a try
> with the test (I suspect you already have, I didn't check the trys).

Yes that's the full try in Comment 48. All green.

Comment 55

[Jason Smith [:jsmith]]

This is confirmed to have caused bug 916609, which breaks getUserMedia entirely on FxAndroid. As such, this needs to be backed out for causing a severe regression.

Ryan - Can you back this out?

Comment 56

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c833bcf12ad8

Comment 57

[Boris Zbarsky [:bzbarsky]]

I think backing this out was the wrong call, because there is no sane way to tell when/whether it can reland.  See bug 916609 comment 11.

Comment 58

[Jason Smith [:jsmith]]

(In reply to Boris Zbarsky [:bz] from comment #57)
> I think backing this out was the wrong call, because there is no sane way to
> tell when/whether it can reland.  See bug 916609 comment 11.

Issues that cause severe regressions need to be backed out, fixed up, and relanded. I don't see a process problem here - we should not leave bustage in the build when it's known immediately within a day of landing. That includes landing of large patches.

Comment 59

[Jason Smith [:jsmith]]

Process-wise this also followed the right approach - we sticked to the same bug where the landing and back out took place, which is why NEW --> RESOLVED FIXED --> REOPENED was followed. I think tree management will be aware when this can reland with the checkin-needed keyword. So I think there is a sane way of tracking here that can be followed effectively.

Comment 60

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

Once we have reviews to land bug 916609 I'll reland everything backed out, and will land the 916609 patch on m-c directly (and maybe inbound as well for good measure).

Comment 61

[Boris Zbarsky [:bzbarsky]]

> Issues that cause severe regressions need to be backed out, fixed up, and relanded.

That's not exactly workable when there is no way to tell when a patch is "fixed up", imo.

> we should not leave bustage in the build when it's known immediately within a day of
> landing

I agree; the right thing here was to land the obvious fix (which would only affect Firefox for Android), instead of backing out everything and causing tons of extra work for people and builders.  In my opinion.

If the fix here were not obvious, I'd agree that a backout is the right thing and the Android lack of tests fail and consequent lack of ability to tell whether the patch is "fixed up" would just need to be dealt with.  But the fix is obvious, even if untestable without relanding everything and waiting for the people who reported the problem to say whether it's still there.

Comment 62

[Jason Smith [:jsmith]]

(In reply to Boris Zbarsky [:bz] from comment #61)
> > Issues that cause severe regressions need to be backed out, fixed up, and relanded.
> 
> That's not exactly workable when there is no way to tell when a patch is
> "fixed up", imo.
> 
> > we should not leave bustage in the build when it's known immediately within a day of
> > landing
> 
> I agree; the right thing here was to land the obvious fix (which would only
> affect Firefox for Android), instead of backing out everything and causing
> tons of extra work for people and builders.  In my opinion.
> 
> If the fix here were not obvious, I'd agree that a backout is the right
> thing and the Android lack of tests fail and consequent lack of ability to
> tell whether the patch is "fixed up" would just need to be dealt with.  But
> the fix is obvious, even if untestable without relanding everything and
> waiting for the people who reported the problem to say whether it's still
> there.

To avoid a Bugzilla argument here, feel free to take this offline with me & others as appropriate (e.g. tree managers). I don't entirely understand the builder & people resource concerns. The workflow concern I think could be worked out with some more discussion on tracking processes in Bugzilla.

Comment 63

[Boris Zbarsky [:bzbarsky]]

There is no workflow concern; assuming the patch was going to get backed out, that part was done right.

Comment 64

[Ryan VanderMeulen [:RyanVM]]

Merge of backout.
https://hg.mozilla.org/mozilla-central/rev/c833bcf12ad8

Comment 65

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

Attachment: This should do it (Fixes webrtcUI bustage from this bug on android)

Moved here from closed bug 916609 - Thanks bz!

Comment 66

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

Comment on attachment 805150 [details] [diff] [review]
This should do it (Fixes webrtcUI bustage from this bug on android)

This was written by bz; gcp also looked at it but got distracted from marking it due to another bug that breaks android video capture (with or without this bug landed).  Landing bz's breakage patch with r=jesup,jib, and implied at least f+ from gcp via IRC.

Comment 67

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

Relanded with the Android UI fix

https://hg.mozilla.org/mozilla-central/rev/843ab8455028
https://hg.mozilla.org/mozilla-central/rev/c4bcef90cef9

Comment 68

[Jason Smith [:jsmith]]

Comment on attachment 804853 [details] [diff] [review]
Part 3: Mochitests for testing gum constraints

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

The test looks like it's on the right track, although I've got a bunch of nits below to address.

Also - commit message needs to include r=jesup.

::: dom/media/tests/mochitest/test_getUserMedia_constraints.html
@@ +10,5 @@
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <script type="application/javascript" src="head.js"></script>
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=795367">Test mozGetUserMedia Constraints</a>

Nit - bug # link isn't right here and in the above HTML comment

@@ +18,5 @@
> +</div>
> +<pre id="test">
> +<script type="application/javascript">
> +/**
> +  These tests verify that appropriate errors are returned when various

Nit - this looks to be doing argument API testing of successes and errors, not just error conditions.

@@ +21,5 @@
> +/**
> +  These tests verify that appropriate errors are returned when various
> +  constraints are provided to a mozGetUserMedia call.
> +*/
> +var tests = [

There's other tests you could include here such as:

- video & audio together with facingMode
- Other success cases with facingMode with optional included

What's the facingMode of fake video? If there is one, we should aim to test for that as well.

@@ +31,5 @@
> +  { message: "unknown mandatory constraint on audio fails",
> +    constraints: { audio: { mandatory: { somethingUnknown:0 } } },
> +    error: "NOT_SUPPORTED_ERR: somethingUnknown",
> +    pass: false },
> +  { message: "video overconstrained by facingMode fails",

I'm not sure this codepath will be executed here in the mochitest since we'll fail with NO_DEVICES_FOUND first before even getting to the overconstrained check.

@@ +37,5 @@
> +    error: "NO_DEVICES_FOUND",
> +    pass: false },
> +  { message: "fake video overconstrained by facingMode fails",
> +    constraints: { fake: true, video: { mandatory: { facingMode:'left' } } },
> +    error: "NO_DEVICES_FOUND",

This seems like the wrong error name being fired here. Per the spec, I'd expect some sort of error code involving the constraint not being satisfied to be fired.

@@ +40,5 @@
> +    constraints: { fake: true, video: { mandatory: { facingMode:'left' } } },
> +    error: "NO_DEVICES_FOUND",
> +    pass: false },
> +  { message: "fake audio ignoring video constraints succeeds",
> +    constraints: { fake: true, audio: { mandatory: { facingMode:'left' } } },

Why is this considered valid?

@@ +48,5 @@
> +    constraints: { fake: true, video: { optional: [{ facingMode:'left' },
> +                                                   { facingMode:'right' },
> +                                                   { facingMode:'environment' },
> +                                                   { facingMode:'user' },
> +                                                   { foo:0 }] } },

Why would including an unknown optional constraint not generate an error?

@@ +60,5 @@
> + * test by verifying that the right callback and error message is fired.
> + */
> +
> +runTest(function () {
> +  var i=0;

Nit - spacing (var i = 0)

@@ +63,5 @@
> +runTest(function () {
> +  var i=0;
> +  next();
> +
> +  function unexpectedSuccess() {

This is a bit confusing naming wise as that having the success callback isn't always unexpected per the test "fake video with optional facingMode succeeds." I'd fix the naming here to reflect that this is a general success callback that's only expected if an error isn't provided.

@@ +65,5 @@
> +  next();
> +
> +  function unexpectedSuccess() {
> +    info("successcallback");
> +    tests[i].pass = tests[i].error? false : true;

Nit - you don't need the ? condition here. Just use:

tests[i].pass = !tests[i].error;

Comment 69

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

(In reply to Jason Smith [:jsmith] from comment #68)
> There's other tests you could include here such as:
> 
> - video & audio together with facingMode

It doesn't increase coverage much for teh failure cases because of how the code is structured (does video, then audio, then appends results together). For success I think it makes sense to include...

> - Other success cases with facingMode with optional included

I was worried about repeat calls here, but when I think about it, I can bundle all the success calls anyway, so I'll do that. Thanks.

> What's the facingMode of fake video? If there is one, we should aim to test
> for that as well.

Good question. I don't think fake video faces anything. :-)

The fake tests are redundant now that I think about it. I included them to test the (unfortunate) separate codepath in MediaManager for them, but I just realized that the test suite probably sets "media.navigator.permission.disabled=true" anyway (right?) - So I can remove them.

> @@ +31,5 @@
> > +  { message: "unknown mandatory constraint on audio fails",
> > +    constraints: { audio: { mandatory: { somethingUnknown:0 } } },
> > +    error: "NOT_SUPPORTED_ERR: somethingUnknown",
> > +    pass: false },
> > +  { message: "video overconstrained by facingMode fails",
> 
> I'm not sure this codepath will be executed here in the mochitest since
> we'll fail with NO_DEVICES_FOUND first before even getting to the
> overconstrained check.

Each test is run individually, so I'm not sure what you mean here. A wrong error message would trigger as unexpected. Look at the try logs in Comment 28 and search for "constraints" to see all the tests succeeding.

> @@ +37,5 @@
> > +    error: "NO_DEVICES_FOUND",
> > +    pass: false },
> > +  { message: "fake video overconstrained by facingMode fails",
> > +    constraints: { fake: true, video: { mandatory: { facingMode:'left' } } },
> > +    error: "NO_DEVICES_FOUND",
> 
> This seems like the wrong error name being fired here. Per the spec, I'd
> expect some sort of error code involving the constraint not being satisfied
> to be fired.

The test matches the error we fire here, as landed, so this seems out of scope for the review of this test.

That said, this patch increments on our existing error behavior (adds constraint name as a hint), rather than address our larger spec compliance issue on errors. I think we have a separate bug on that (I just can't find). Please file if you can't find it either.

> @@ +40,5 @@
> > +    constraints: { fake: true, video: { mandatory: { facingMode:'left' } } },
> > +    error: "NO_DEVICES_FOUND",
> > +    pass: false },
> > +  { message: "fake audio ignoring video constraints succeeds",
> > +    constraints: { fake: true, audio: { mandatory: { facingMode:'left' } } },
> 
> Why is this considered valid?

Because the current implementation ignores VideoFacingMode for audio. The spec isn't clear so I went with what was minimally requested (VideoFacingMode for video only, ignored for audio).

> @@ +48,5 @@
> > +    constraints: { fake: true, video: { optional: [{ facingMode:'left' },
> > +                                                   { facingMode:'right' },
> > +                                                   { facingMode:'environment' },
> > +                                                   { facingMode:'user' },
> > +                                                   { foo:0 }] } },
> 
> Why would including an unknown optional constraint not generate an error?

It shouldn't. See Sub-step 8->5->1 under http://dev.w3.org/2011/webrtc/editor/getusermedia.html#methods-5 :
> 1. If the constraint is not supported by the browser, skip it and continue with the next constraint.

> @@ +63,5 @@
> > +runTest(function () {
> > +  var i=0;
> > +  next();
> > +
> > +  function unexpectedSuccess() {
> 
> This is a bit confusing naming wise as that having the success callback
> isn't always unexpected per the test "fake video with optional facingMode
> succeeds." I'd fix the naming here to reflect that this is a general success
> callback that's only expected if an error isn't provided.

You can probably tell I started out with just errors here. :-) thanks.

> @@ +65,5 @@
> > +  next();
> > +
> > +  function unexpectedSuccess() {
> > +    info("successcallback");
> > +    tests[i].pass = tests[i].error? false : true;
> 
> Nit - you don't need the ? condition here. Just use:
> 
> tests[i].pass = !tests[i].error;

Some symmetry lost with the Failure() callback here, but sure. willdo.

I agree with all the other comments.

Comment 70

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

Attachment: Part 3: Mochitests for testing gum constraints, r=jesup

Carrying forward r+ from jesup.

Jason, I meant to make you a reviewer. Can I consider that an r+ from you?

Comment 71

[Jason Smith [:jsmith]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #70)
> Created attachment 805592 [details] [diff] [review]
> Part 3: Mochitests for testing gum constraints, r=jesup
> 
> Carrying forward r+ from jesup.
> 
> Jason, I meant to make you a reviewer. Can I consider that an r+ from you?

Yeah. It's a r+ with nits addressed.

Comment 72

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

Attachment: Part 3: Mochitests for testing gum constraints (3), r=jesup, jsmith

Updated with nits. Carrying forward r+ from jesup and jsmith.

Try - https://tbpl.mozilla.org/?tree=Try&rev=62aca31d5c98

Comment 73

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

Comment on attachment 805676 [details] [diff] [review]
Part 3: Mochitests for testing gum constraints (3), r=jesup, jsmith

Moved mochitest patch to Bug 917298 to avoid reopening this bug.