Implement facingMode as first gUM constraint

RESOLVED FIXED in mozilla26

Status

()

Core
WebRTC: Audio/Video
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: u459114, Assigned: jib)

Tracking

(Blocks: 3 bugs, {dev-doc-needed})

Trunk
mozilla26
x86_64
Linux
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [getUserMedia][blocking-gum-])

Attachments

(3 attachments, 23 obsolete attachments)

46.60 KB, patch
jib
: review+
Details | Diff | Splinter Review
19.36 KB, patch
jib
: review+
Details | Diff | Splinter Review
1.39 KB, patch
jesup
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
1. Implement MediaStreamTrack constrains function
   MediaStreamTrack::constraints 
   MediaStreamTrack::applyConstraints
2. Implement MediaTrackConstraint IDL and native class
Note - we've discontinued using that webrtc tracking bug and use the whiteboards instead.
No longer blocks: 665909
Whiteboard: [WebRTC][blocking-webrtc-]

Updated

5 years ago
Whiteboard: [WebRTC][blocking-webrtc-] → [getUserMedia][blocking-gum-]
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
(Reporter)

Comment 3

5 years ago
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.
Flags: needinfo?(rjesup)
Flags: needinfo?(mreavy)
Flags: needinfo?(ekr)

Comment 4

5 years ago
Yes, I think this is clearly the right direction.
Flags: needinfo?(ekr)
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.
Assignee: nobody → jib
Flags: needinfo?(rjesup)
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.
Flags: needinfo?(mreavy)
Flags: needinfo?(gpascutto)
Flags: needinfo?(cku)
(Reporter)

Comment 7

5 years ago
(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?
Flags: needinfo?(cku) → needinfo?(rjesup)
>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.
Flags: needinfo?(gpascutto)
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.
(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.
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?

Updated

5 years ago
Blocks: 905696

Updated

5 years ago
Blocks: 905699

Updated

5 years ago
Blocks: 905701
Blocks: 907352
I've created Bug 907352 to cover width/height/framerate constraints on getUserMedia.

That way we can focus here on facingMode: "user" | "environment".
Summary: Implement MediaTrackConstraint → Implement facingMode as first gUM constraint
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.
Depends on: 903277
Depends on: 767926
Target Milestone: --- → mozilla26
Blocks: 910249
Created attachment 797577 [details] [diff] [review]
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)
Created attachment 797660 [details] [diff] [review]
Part 1: Existing gUM constraints converted to webidl (2)

Forgot to hg add two files.
Attachment #797577 - Attachment is obsolete: true
Created attachment 797721 [details] [diff] [review]
Part 1: Existing gUM constraints converted to webidl (3)

webidl nit (now uses sequence<> instead of [])
Attachment #797660 - Attachment is obsolete: true
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.
s/app/page/
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).
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.
> 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.
Depends on: 767924
Created attachment 802366 [details] [diff] [review]
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).
Attachment #797721 - Attachment is obsolete: true
Created attachment 802369 [details] [diff] [review]
Part 2: GetUserMediaDevices() takes constraints. Adds nsIMediaDevice::facingMode
Created attachment 802374 [details] [diff] [review]
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.
Created attachment 802507 [details] [diff] [review]
Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints

Refactored into two patches. Part 1 of 2.
Attachment #802366 - Attachment is obsolete: true
Attachment #802369 - Attachment is obsolete: true
Attachment #802374 - Attachment is obsolete: true
Created attachment 802508 [details] [diff] [review]
Part 2: Implements first gUM constraint facingMode
Created attachment 802874 [details] [diff] [review]
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).
Attachment #802507 - Attachment is obsolete: true
Created attachment 803187 [details] [diff] [review]
Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (3)

Modified a mochitest to pass (was testing on specific error msg that changed)
Attachment #802874 - Attachment is obsolete: true
Created attachment 803194 [details] [diff] [review]
Part 2: Implements first gUM constraint facingMode (2)

Implements optional constraint algorithm.
- Testing on phones with two cameras welcome.
Attachment #802508 - Attachment is obsolete: true
Depends on: 915368
Created attachment 803853 [details] [diff] [review]
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.
Attachment #803187 - Attachment is obsolete: true
Created attachment 804302 [details] [diff] [review]
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.
Attachment #803194 - Attachment is obsolete: true
Attachment #803853 - Attachment is obsolete: true
Attachment #804302 - Flags: review?(rjesup)
Attachment #804302 - Flags: review?(bzbarsky)
Created attachment 804313 [details] [diff] [review]
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
Attachment #804313 - Flags: review?(rjesup)
Note that this removes the privileged {device:''} gUM constraint. AFAICT it was unused. If you know otherwise, please let me know asap.
Created attachment 804500 [details] [diff] [review]
Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (6)

Unbitrotted.
Attachment #804302 - Attachment is obsolete: true
Attachment #804302 - Flags: review?(rjesup)
Attachment #804302 - Flags: review?(bzbarsky)
Attachment #804500 - Flags: review?(rjesup)
Attachment #804500 - Flags: review?(bzbarsky)
Created attachment 804501 [details] [diff] [review]
Part 2: Implements gUM constraints. First constraint: facingMode (4)

Unbitrotted.
Attachment #804313 - Attachment is obsolete: true
Attachment #804313 - Flags: review?(rjesup)
Attachment #804501 - Flags: review?(rjesup)
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 ?
Attachment #804500 - Flags: review?(rjesup) → review+

Updated

4 years ago
Attachment #804501 - Flags: review?(rjesup) → review+
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.
Attachment #804500 - Flags: review?(bzbarsky) → review+
(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!
Created attachment 804748 [details] [diff] [review]
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. :-)
Attachment #804500 - Attachment is obsolete: true
Attachment #804748 - Flags: review+
Created attachment 804749 [details] [diff] [review]
interdiff to Part 1 patch
Created attachment 804756 [details] [diff] [review]
Part 2: Implements gUM constraints. First constraint: facingMode (5) r=jesup

Unbitrotted
Attachment #804501 - Attachment is obsolete: true
Attachment #804756 - Flags: review+
> 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.
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)
Created attachment 804831 [details] [diff] [review]
Part 1: gUM constraints converted to webidl + gUMDevices() takes constraints (7). r=bz r=jesup

Done. Thanks!
Attachment #804748 - Attachment is obsolete: true
Attachment #804831 - Flags: review+
Created attachment 804832 [details] [diff] [review]
2nd interdiff - Part 1 patch
Created attachment 804834 [details] [diff] [review]
Part 2: Implements gUM constraints. First constraint: facingMode (6) r=jesup

Rebased
Attachment #804756 - Attachment is obsolete: true
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.
Attachment #804834 - Flags: review+
Created attachment 804853 [details] [diff] [review]
Part 3: Mochitests for testing gum constraints

Adding coverage. Not required to land.

Try - https://tbpl.mozilla.org/?tree=Try&rev=31806314302a
Attachment #804749 - Attachment is obsolete: true
Attachment #804832 - Attachment is obsolete: true
Attachment #804853 - Flags: review?(rjesup)
Attachment #804853 - Flags: review?
Attachment #804853 - Flags: feedback?(jsmith)

Updated

4 years ago
Attachment #804853 - Flags: review?(rjesup)
Attachment #804853 - Flags: review?
Attachment #804853 - Flags: review+
Try looks good. Ready to land.
Flags: needinfo?(rjesup)
Keywords: checkin-needed
Thanks! Wondering, should we land mochitest as well?
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).
https://hg.mozilla.org/mozilla-central/rev/cf373e408a6b
https://hg.mozilla.org/mozilla-central/rev/bc4ee7cbd7bd
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
(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.

Updated

4 years ago
Depends on: 916609
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?
Flags: needinfo?(ryanvm)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c833bcf12ad8
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(ryanvm)
Target Milestone: mozilla26 → ---
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.
(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.
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.
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).
> 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.
(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.
There is no workflow concern; assuming the patch was going to get backed out, that part was done right.
Created attachment 805150 [details] [diff] [review]
This should do it (Fixes webrtcUI bustage from this bug on android)

Moved here from closed bug 916609 - Thanks bz!
Attachment #805150 - Flags: review?(gpascutto)
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.
Attachment #805150 - Flags: review?(gpascutto) → review+
Relanded with the Android UI fix

https://hg.mozilla.org/mozilla-central/rev/843ab8455028
https://hg.mozilla.org/mozilla-central/rev/c4bcef90cef9
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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;
Attachment #804853 - Flags: feedback?(jsmith) → feedback+
(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.
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?
Attachment #804853 - Attachment is obsolete: true
Attachment #805592 - Flags: review+
Flags: needinfo?(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.
Flags: needinfo?(jsmith)
Created attachment 805676 [details] [diff] [review]
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
Attachment #805592 - Attachment is obsolete: true
Attachment #805676 - Flags: review+
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.
Attachment #805676 - Attachment is obsolete: true
Blocks: 916012
No longer depends on: 767924
No longer depends on: 928221
No longer depends on: 933447
Depends on: 995352
No longer depends on: 995352

Updated

3 years ago
Blocks: 1070216
Duplicate of this bug: 780811
Duplicate of this bug: 905699
Keywords: dev-doc-needed
Blocks: 1050930
You need to log in before you can comment on or make changes to this bug.