Split out from Bug 882145, this covers implementing the following constraints on getUserMedia (example): { mandatory: { width: { min: 640 }, height: { min: 480 } }, optional: [ { width: 650 }, { width: { min: 650 }}, { frameRate: 60 }, { width: { max: 800 }}, ] }

Re-targeting this for Firefox 28 to make room for Bug 904622 (which we want to get into Firefox 27). This has become a nice-to-have for Firefox 28, relative to some of the Stats API work.

1) I tried the constraints Jan-Ivar listed above for Firefox 28.0a2 (Aurora) but they are still ignored. Also, I think specifying width: 640 or width: { max: 640 } throws an exception in Chrome 32. I have the following constraints working in Chrome: { mandatory: { maxWidth: 320, maxHeight: 240, maxFrameRate: 20 }, optional: [ { bandwidth: 200 } ] } How can I get constraints working in Firefox 28.0a2 (Aurora)? 2) Are the gUM constraints still on target for Firefox 28? Thanks.

(In reply to Chris Liu from comment #2) > Are the gUM constraints still on target for Firefox 28? This got delayed, sorry. Working to get width/height in asap, but not 28. > Also, I think specifying width: 640 or width: { max: 640 } throws an exception > in Chrome 32. I have the following constraints working in Chrome: > > { > mandatory: { > maxWidth: 320, > maxHeight: 240, > maxFrameRate: 20 > }, Understood, though please note that that is not spec-compliant. See http://dev.w3.org/2011/webrtc/editor/getusermedia.html#applyconstraints-failure-callback

(In reply to Jan-Ivar Bruaroey [:jib] from comment #3) > > Are the gUM constraints still on target for Firefox 28? > > This got delayed, sorry. Working to get width/height in asap, but not 28. Thanks for your prompt response, Jan-Ivar. Which official Firefox release do you plan on having width/height? How about frame rate? Without these (or bandwidth) constraints I can't stream video consistently in Firefox, limiting me to Chrome for now. My use case is streaming webcam video/audio between two arbitrary users who sometimes may have poor network connections. Beta testing shows that without constraints, sometimes audio or video is lost or lagging significantly. I also tried adding b=AS:xx in the offer SDP following a thread in July (https://groups.google.com/forum/#!topic/mozilla.dev.media/9HxXbLkLWqE) but the setting still appears to be ignored. Is there some way I can constrain bandwidth while the gUM constraints are being added? > > Also, I think specifying width: 640 or width: { max: 640 } throws an exception > > in Chrome 32. I have the following constraints working in Chrome: > > > > { > > mandatory: { > > maxWidth: 320, > > maxHeight: 240, > > maxFrameRate: 20 > > }, > > Understood, though please note that that is not spec-compliant. See > http://dev.w3.org/2011/webrtc/editor/getusermedia.html#applyconstraints- > failure-callback Got it, thanks for the link. I don't mind setting up separate browser-specific variables until the standard is fixed, as long as I know how to set it for both browsers.

(In reply to Chris Liu from comment #4) > Which official Firefox release do you plan on having width/height? How about > frame rate? This is next on my list, so Firefox 30 (which just became Nightly). > Without these (or bandwidth) constraints I can't stream video > consistently in Firefox, limiting me to Chrome for now. My use case is > streaming webcam video/audio between two arbitrary users who sometimes may > have poor network connections. Beta testing shows that without constraints, > sometimes audio or video is lost or lagging significantly. When you say "lag" is audio and video getting out of sync with each other or are things just generally not arriving fast enough to have a conversation? (Just want to rule out other bugs here). > I also tried adding b=AS:xx in the offer SDP following a thread in July > (https://groups.google.com/forum/#!topic/mozilla.dev.media/9HxXbLkLWqE) but > the setting still appears to be ignored. > > Is there some way I can constrain bandwidth while the gUM constraints are > being added? Not that I know of. As Randell says in the thread you link to, "Currently, Firefox does not look at b= values."

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5) > When you say "lag" is audio and video getting out of sync with each other or > are things just generally not arriving fast enough to have a conversation? > (Just want to rule out other bugs here). Generally not arriving fast enough to have a conversation. > > I also tried adding b=AS:xx in the offer SDP following a thread in July > > (https://groups.google.com/forum/#!topic/mozilla.dev.media/9HxXbLkLWqE) but > > the setting still appears to be ignored. > > > > Is there some way I can constrain bandwidth while the gUM constraints are > > being added? > > Not that I know of. As Randell says in the thread you link to, "Currently, > Firefox does not look at b= values." Ok, just wanted to confirm as it was dated in July. On a separate note, I did notice streaming bandwidth requirement dropped significantly in 28.0a2 Aurora. I will test 28.0b1 after it is out to see if it produces the same results.

We'll be using the latest constraints syntax for this. See http://lists.w3.org/Archives/Public/public-media-capture/2014Apr/0120.html

The existing facingMode constraint is only well-implemented for front/back cameras on phones, so we support the old syntax only on mobile, and only for this one constraint. Includes tests to verify the mobile-specific behavior, although this test doesn't seem to run on B2G, so I'm wondering if there are other tests on B2G that I need to hit. Schien, do you know if facingMode is tested on B2G, and is so, where? Also, if you could verify if my UserAgent string test is correct - in case this becomes enabled - that would be great.

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

Comment on attachment 8406926 [details] [diff] [review] Part 2: backwards compatible facingMode constraint on mobile Also, I'm wondering if defined(MOZ_WIDGET_GONK) is the right way to enable code specifically for B2G cameras. This try experiment suggests it may not be, at least not for B2G desktop: https://tbpl.mozilla.org/?tree=Try&rev=55cb6509beea - though then again, are there front/back cameras on B2G desktop?

Comment on attachment 8406926 [details] [diff] [review] Part 2: backwards compatible facingMode constraint on mobile Review of attachment 8406926 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: dom/media/tests/mochitest/test_getUserMedia_constraints.html @@ +68,5 @@ > > runTest(function () { > var i = 0; > + const isMobile = (navigator.userAgent.search(/(android|b2g|gonk)/i) != -1); > + info("isMobile = " + isMobile + ", userAgent = (" + navigator.userAgent + ")"); Why not split the mobile legacy tests into their own file and use skip-if in mochitests.ini? ::: dom/webidl/MediaStreamTrack.webidl @@ +25,5 @@ > }; > > +dictionary MobileLegacyMediaTrackConstraintSet { > + VideoFacingModeEnum facingMode; > +}; Do we want to put a time-limit on this support? If so, it's probably worth entering a bug for it.

Comment on attachment 8406907 [details] [diff] [review] Part 1: update to most recent constraints syntax Review of attachment 8406907 [details] [diff] [review]: ----------------------------------------------------------------- r=me Looks good. I like seeing so much code being deleted. ::: dom/media/MediaManager.cpp @@ +735,5 @@ > + for (int i = 0; i < int(candidateSet.Length()); i++) { > + // Overloading instead of template specialization keeps things local > + if (!SatisfyConstraint(type, required, *candidateSet[i])) { > + candidateSet.RemoveElementAt(i--); > + } That's a strange construct. Have you considered instead: for (size_t i = 0; i < candidateSet.Length(); ) { if (!SatisfyConstraint(type, required, *candidateSet[i])) { candidateSet.RemoveElementAt(i); } else { ++i; } } That avoids the need for a cast. @@ +761,3 @@ > for (int i = 0; i < int(array.Length()); i++) { > SourceSet rejects; > // Note: Iterator must be signed as it can dip below zero See above. @@ +771,5 @@ > } > } > > + // Finally, order any remaining sources by how many nonrequired constraints > + // they satisfy. (TBD when we implement more than one constraint) Bug #? ::: dom/media/tests/mochitest/test_getUserMedia_constraints.html @@ +46,5 @@ > error: "NO_DEVICES_FOUND", > pass: false }, > { message: "Success-path: optional video facingMode + audio ignoring facingMode", > constraints: { fake: true, > + audio: { facingMode:'left', require:["facingMode"] }, Should we be ignoring constraints we know to be inapplicable, or treating them as an impossible-to-meet constraint? I didn't see any discussion of this in the email thread, but I'd have gone with the opposite conclusion to you, so it's probably worth raising. ::: dom/webidl/MediaStreamTrack.webidl @@ +30,5 @@ > }; > > +typedef VideoFacingModeEnum ConstrainVideoFacingMode; > +// TODO: Bug 767924 sequences in unions > +//typedef (VideoFacingModeEnum or sequence<VideoFacingModeEnum>) ConstrainVideoFacingMode; So currently we can't specify facingMode: ["left", "right"]. What are the plans here?

(In reply to Martin Thomson [:mt] from comment #11) > Why not split the mobile legacy tests into their own file and use skip-if in > mochitests.ini? There's also a desktop-only test in there, so that would make it three files. I know that's what I should have done, it just seemed excessive at the outset. But I should probably do that to avoid sniffing the UserAgent. > > +dictionary MobileLegacyMediaTrackConstraintSet { > > + VideoFacingModeEnum facingMode; > > +}; > > Do we want to put a time-limit on this support? If so, it's probably worth > entering a bug for it. Yes, good point. Will do.

(In reply to Martin Thomson [:mt] from comment #12) > > + for (int i = 0; i < int(candidateSet.Length()); i++) { > > + // Overloading instead of template specialization keeps things local > > + if (!SatisfyConstraint(type, required, *candidateSet[i])) { > > + candidateSet.RemoveElementAt(i--); > > + } > > That's a strange construct. Have you considered instead: > > for (size_t i = 0; i < candidateSet.Length(); ) { > if (!SatisfyConstraint(type, required, *candidateSet[i])) { > candidateSet.RemoveElementAt(i); > } else { > ++i; > } > } > > That avoids the need for a cast. Sold. > > + // Finally, order any remaining sources by how many nonrequired constraints > > + // they satisfy. (TBD when we implement more than one constraint) > > Bug #? This one, but yes. > > { message: "Success-path: optional video facingMode + audio ignoring facingMode", > > constraints: { fake: true, > > + audio: { facingMode:'left', require:["facingMode"] }, > > Should we be ignoring constraints we know to be inapplicable, or treating > them as an impossible-to-meet constraint? I didn't see any discussion of > this in the email thread, but I'd have gone with the opposite conclusion to > you, so it's probably worth raising. It's a carryover from my original implementation. Internally, I use the same SatisfyConstraint function for required (mandatory) and optional, which is where I picked up this behavior. http://dev.w3.org/2011/webrtc/editor/archives/20140321/getusermedia.html#methods-3 says: > 5. For each constraint key-value pair in the "optional" sequence of the constraints > that are for the current media type, in order, On re-reading the spec it seems clear by omission that required/mandatory constraints are meant to work differently in this regard, so I'll change it. Thanks for catching it. > > +// TODO: Bug 767924 sequences in unions > > +//typedef (VideoFacingModeEnum or sequence<VideoFacingModeEnum>) ConstrainVideoFacingMode; > > So currently we can't specify facingMode: ["left", "right"]. What are the > plans here? Block on Bug 767924 and/or punt.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #14) > > So currently we can't specify facingMode: ["left", "right"]. What are the > > plans here? > > Block on Bug 767924 and/or punt. I think that it's fine to ship without full support. After all, this improves support over what is there currently infinitely. Let's leave the addition of multiple values to the usual forces of prioritization (i.e., punt).

Addressed feedback. Majorly refactored to follow spec and fail on required constraints of the wrong type, and to meet needs of upcoming width/height patch better. Re-review requested.

Haven't addressed comments yet. Just an unbitrot for try - https://tbpl.mozilla.org/?tree=Try&rev=ca5608da4321

Comment on attachment 8407352 [details] [diff] [review] Part 1: update to most recent constraints syntax (2) Review of attachment 8407352 [details] [diff] [review]: ----------------------------------------------------------------- r=me ::: dom/media/MediaManager.cpp @@ +781,5 @@ > + for (uint32_t i = 0; i < require.Length(); i++) { > + // EnumType arrays have a zero-terminator entry at the end. Skip. > + for (size_t j = 0; j < sizeof(supported)/sizeof(*supported) - 1; j++) { > + if (!require[i].EqualsASCII(supported[j].value)) { > + return result.forget(); This doesn't seem right to me. You probably mean to do this: for (uint32_t i = 0; i < require.Length(); i++) { bool found = false; // EnumType arrays have a zero-terminator entry at the end. Skip. for (size_t j = 0; !found && j < sizeof(supported)/sizeof(*supported) - 1; j++) { found = require[i].EqualsASCII(supported[j].value); } if (!found) { return result.forget(); } } What you have will work for a set size of 1, but not 2. (Unless I'm a little crazy this morning.) I'd put that much in a new method, but I'm funny like that. @@ +795,5 @@ > + > + if (constraints.mFacingMode.WasPassed()) { > + helper.Triage(NS_LITERAL_STRING("facingMode")).mFacingMode.Construct( > + constraints.mFacingMode.Value()); > + } The TriageHelper API isn't particularly intuitive. If the intent is to separate into required+supported and other, this is an odd way to do it. A more capable class could also take the advanced stuff. // excuse the pseudo-code for (c in mainConstraints) { if (isRequired(c)) { for (s in sources) { if (s.satisfies(c)) { sources.remove(s) } } mainConstraints.remove(c) } } // then iterate over what remains in mainConstraints + all from advanced // to find what works best A helper class is a good idea, though I'd have expected a class that covers the entire algorithm. Feed it constraints and sources and get back sources. This method is pretty long and complicated (though it does make sense if you take the time). @@ +844,5 @@ > } > } > > + // Finally, order any remaining sources by how many nonrequired constraints > + // they satisfy. TODO(jib): TBD once we implement >1 constraint (Bug 907352) It would be good if there was a way to indicate that particular sources are "preferred" in the UI so that users know when they are mucking things up. Sorting by score would be nice too.

Addressed feedback. - Split out as three test files and uses skip_if for more traditional platform targeting.

(In reply to Martin Thomson [:mt] from comment #18) > What you have will work for a set size of 1, but not 2. (Unless I'm a > little crazy this morning.) It's me, not you. Once we have more than 1 we'll hopefully not see bugs like this. Thanks! > I'd put that much in a new method, but I'm funny like that. Mozilla seems conservative about templates in global scope, so that's why things aren't broken out more. > The TriageHelper API isn't particularly intuitive. > > If the intent is to separate into required+supported and other, this is an > odd way to do it. Just required+nonrequired. My main goal here is not to complicate the SatisfyConstraintSet() function used in all three algorithms, because it's the one that will grow new constraints. Specifically, I didn't want to teach it about required and non-required, because it works great without this complication. It still has a limited well-defined function: "source, satisfy whole set". All the other stuff can be expressed with the right sets. Thus TriageHelper is there trying to stay out of the algorithm. It exists mostly to split required/nonrequired into discrete sets, a task that in c++ unfortunately needs to be repeated with code for each constraints added. Hopefully its benefit gets clearer with each constraint added. > A more capable class could also take the advanced stuff. The advanced stuff doesn't need any prep as the underlying algorithm was written for it.

Comment on attachment 8407764 [details] [diff] [review] Part 2: backwards compatible facingMode constraint on mobile (3) r=mt, drno Review of attachment 8407764 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Try - https://tbpl.mozilla.org/?tree=Try&rev=1f83bf0190ce

Addressed feedback (fixed bug). Carrying forward r+ from Martin.

Comment on attachment 8407764 [details] [diff] [review] Part 2: backwards compatible facingMode constraint on mobile (3) r=mt, drno Review of attachment 8407764 [details] [diff] [review]: ----------------------------------------------------------------- I did some test on my Peak phone using http://people.mozilla.org/~schien/gum-test-single.html. The list of test items in below: 1. Click Video, show permission prompt with back/front camera listed. 2. Click Front(Legacy), show permission prompt without camera list. Show front camera view after accept. 3. Click Left(Legacy), show NO_DEVICE_ERROR in web page. 4. Click Back, show permission prompt without camera list. Show back camera view after accept. Please note that the item #4 uses the new constraints syntax, i.e. { video: { facingMode:'environment', require:["facingMode"] }.

Great, thanks! (I take it there are no tests in the tree yet for this?)

Whoops, forgot to check try. Please hold off.

Fixed snafu. Carrying forward r+ from mt and drno. Try - https://tbpl.mozilla.org/?tree=Try&rev=5a51a4a13b45

The bash color-coding gets me every time. Actual local absence of failure this time, not just green. Third time's the charm. Try - https://tbpl.mozilla.org/?tree=Try&rev=1aa304fd7320

WebIDL plumbing.

Wires into MediaEngineWebRTC[Video|Audio]Source.

Work in progress. - Shortcuts taken. - No camera selection. - Only works with Mac camera at the moment (e.g. 680x480 vs. 1280x768). - Only required constraints have an effect.

Since Macs don't list capabilities, behavior is poor for wrong values.

Unbitrot.

Mixed merge-mistake.

With bug numbers fixed in the commit messages. https://hg.mozilla.org/integration/mozilla-inbound/rev/5fa0c8f5ccb2 https://hg.mozilla.org/integration/mozilla-inbound/rev/d8d01c95be03

Backed out for Linux64 mochitest-3 crashes. https://hg.mozilla.org/integration/mozilla-inbound/rev/6632c970ef7c https://tbpl.mozilla.org/php/getParsedLog.php?id=38093118&tree=Mozilla-Inbound

Addressed landing crashes. Carrying forward r+ from mt. - Added fake:true to fail-tests.

Addressed landing crashes. Carrying forward r+ from mt. - Added fake:true to fail-tests.

Comment on attachment 8410058 [details] [diff] [review] Part 2: backwards compatible facingMode constraint on mobile (6) r=mt Try - https://tbpl.mozilla.org/?tree=Try&rev=5528fd3b69d4

Try again - https://tbpl.mozilla.org/?tree=Try&rev=b2192ce6bead

Comment on attachment 8408795 [details] [diff] [review] Part 3: webidl for width/height/frameRate gUM constraints Note that TriageHelper is going away in part 4 (you were right, it ugly). I thought about merging Part 3 and 4 to give you less to review, but I think you'll prefer the split: each part builds and runs cleanly, and the changes to TriageHelper are fairly incremental (it handles three more constraints). Re-orged some webidl to ensure new stuff starts out in separate files to work with unions once Bug 995352 is fixed, as they're a pain to move later.

A couple of things going on here: - Added normalized [Audio|Video]TrackConstraintsN classes for downstream code. Cuts down on webidl gunk and replaces TriageHelper. - Won a lot of invariants and reduced codepaths by defining ConstrainLongRange etc. to have defaults. As a result, I had to ditch the requirement that required constraints missing as members in the dictionary fail, but it turns out this isn't needed to detect unknown constraints after all, as the browser knows full well what it supports and what it doesn't. Any remaining benefit would be typos, but there are so many things to type wrong in JS I don't think this typing-twice check is worth it. I'll try to argue this on the list.

Get things down to MediaEngine. - I think splitting Anant's classes up into audio/video helps as we get closer to the metal and have to deal with specific constraints.

Unbitrot. Still barely works and on Mac only. Not ready for review.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #38) > Addressed landing crashes. Carrying forward r+ from mt. > - Added fake:true to fail-tests. I filed bug 999289 for the crashes.

Might slip until tomorrow: a HTTP/2 revision is needed today.

Comment on attachment 8410082 [details] [diff] [review] Part 4: normalized constraints for downstream width/height/frameRate implementation Review of attachment 8410082 [details] [diff] [review]: ----------------------------------------------------------------- r=me A few nits/questions, but it otherwise looks like an improvement. Any reason why more constraints handling wasn't sucked in to this class? More of the constraint-handling logic in MediaManager could be moved fairly easily. ::: content/media/webrtc/MediaTrackConstraints.h @@ +15,5 @@ > +// processing. This implementation-only helper is included as needed by both > +// MediaManager (for gUM camera selection) and MediaEngine (for applyConstraints). > + > +template<typename T> > +class MediaTrackConstraintsN : public dom::MediaTrackConstraints Is inheritance completely necessary here? Wouldn't it be easier if this were a has-a, rather than an is-a? At least that way you could encapsulate more strongly. After the first triage pass in the constructor, you don't need anything that dom::MediaTrackConstraints owns. @@ +48,5 @@ > + if (mRequireN.IndexOf(kind) != mRequireN.NoIndex) { > + return mRequired; > + } else { > + mNonrequired.AppendElement(MediaTrackConstraintSet()); > + return mNonrequired[mNonrequired.Length()-1]; There's a fair bit of copying going on here. Is MediaTrackConstraintSet lightweight enough to get away with that? Is there any reason why you can't use nsTArray<nsAutoPtr<MediaTrackConstraintSet> > instead (other than it being a little nastier template-wise). @@ +52,5 @@ > + return mNonrequired[mNonrequired.Length()-1]; > + } > + } > +private: > + Kind ToEnum(const nsString& aSrc) { nsAString (or else nsSubstring won't be a valid arg) @@ +58,5 @@ > + if (aSrc.EqualsASCII(mStrings[i].value)) { > + return Kind(i); > + } > + } > + return Kind::Other; Is this the only reason you have "other" defined in the two enumerations? Because ToEnum would probably be better as "bool IsSupported(nsAString&)". (I understand the need to have *something* in the enum, but this seems clumsy.)

Comment on attachment 8408795 [details] [diff] [review] Part 3: webidl for width/height/frameRate gUM constraints Review of attachment 8408795 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

Comment on attachment 8410082 [details] [diff] [review] Part 4: normalized constraints for downstream width/height/frameRate implementation Review of attachment 8410082 [details] [diff] [review]: ----------------------------------------------------------------- r=me A few nits/questions, but it otherwise looks like an improvement. Any reason why more constraints handling wasn't sucked in to this class? More of the constraint-handling logic in MediaManager could be moved fairly easily.

Comment on attachment 8410086 [details] [diff] [review] Part 5: wiring for width/height/frameRate gUM constraints (2) Review of attachment 8410086 [details] [diff] [review]: ----------------------------------------------------------------- r=me this is fine. (Ignore the double review of p4. Operator error: too many open tabs.) ::: dom/media/MediaManager.cpp @@ +279,3 @@ > > +VideoDevice::VideoDevice(MediaEngineVideoSource* aSource) > + : MediaDevice(aSource) { Should pass type to MediaDevice here. ::: dom/media/MediaManager.h @@ +436,3 @@ > typedef nsClassHashtable<nsUint64HashKey, StreamListeners> WindowTable; > > class MediaDevice : public nsIMediaDevice template<typename TSource> would avoid casts further down... @@ +441,5 @@ > NS_DECL_THREADSAFE_ISUPPORTS > NS_DECL_NSIMEDIADEVICE > > + static MediaDevice* Create(MediaEngineVideoSource* source); > + static MediaDevice* Create(MediaEngineAudioSource* source); These could be moved to the subclasses.

Fixed an Android build regression. Carrying forward r=mt.

(In reply to Martin Thomson [:mt] from comment #47) > Any reason why more constraints handling wasn't sucked in to this class? > More of the constraint-handling logic in MediaManager could be moved fairly > easily. Class is also used in MediaEngineWebRTCVideo.cpp. More refactoring is needed before the two can work the same. e.g. SatisfyConstraint operates on nsIMediaDevice, a MediaManager-only construct. > Is inheritance completely necessary here? Wouldn't it be easier if this > were a has-a, rather than an is-a? At least that way you could encapsulate > more strongly. > > After the first triage pass in the constructor, you don't need anything that > dom::MediaTrackConstraints owns. I still need mAdvanced, and we need the original constraints for track.getConstraints() eventually. > There's a fair bit of copying going on here. Is MediaTrackConstraintSet > lightweight enough to get away with that? Extremely light-weight. Plain member copy. Happens once per gUM, so I wouldn't worry about it. > > + Kind ToEnum(const nsString& aSrc) { > > nsAString (or else nsSubstring won't be a valid arg) OK fixed. > Is this the only reason you have "other" defined in the two enumerations? > Because ToEnum would probably be better as "bool IsSupported(nsAString&)". > (I understand the need to have *something* in the enum, but this seems > clumsy.) ToEnum also sets up RequireN, which is used in Triage.

Addressed nit and try failures https://tbpl.mozilla.org/?tree=Try&rev=19c8260cf63e - Carrying forward r=mt. - Worked around Windows compiler bug - Worked around B2G ICS compiler bug

(In reply to Martin Thomson [:mt] from comment #50) > > +VideoDevice::VideoDevice(MediaEngineVideoSource* aSource) > > + : MediaDevice(aSource) { > > Should pass type to MediaDevice here. Why? I removed mType, and GetType is virtual. > > class MediaDevice : public nsIMediaDevice > > template<typename TSource> would avoid casts further down... What casts? > > + static MediaDevice* Create(MediaEngineVideoSource* source); > > + static MediaDevice* Create(MediaEngineAudioSource* source); > > These could be moved to the subclasses. Then I could just do new. These are needed in MediaManager.cpp line 747, in template<class SourceType, class ConstraintsType>GetSources - I suppose I could have added another type-arg there but that would have made GetSources() no longer deduce all its type-args from its arguments, which would have affected four call-sites - and another arg to get wrong - so I thought better to deduce one from the other.

Fixed Windows link issue. Carrying forward r=mt.

The best default may vary by camera and resolution (like on mac), so make the about:config setting default to 0 on desktop to not oppress.

Unrot. Avoid unintended aspect squishing on mac.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #56) > Created attachment 8411064 [details] [diff] [review] > Part 6: change media.navigator.video.default_width|height to 0 > > The best default may vary by camera and resolution (like on mac), so make > the about:config setting default to 0 on desktop to not oppress. How is this going to change anything? The default is no longer in prefs, it's in code.

(In reply to Martin Thomson [:mt] from comment #58) > How is this going to change anything? The default is no longer in prefs, it's in code. See Part 7.

Lets you set min and max frameRate. Knobs.

Now works for desktop cameras that DO have capabilities. Camera selection is still missing, but I think this is a good stopping point, as most people just want to control their camera. No change on B2G yet, I suggest a follow-up there. Tested on OSX with built-in camera and Windows 7 with Logitech c920 (56 capabilities). Try - https://tbpl.mozilla.org/?tree=Try&rev=53e7d0d56171

Comment on attachment 8411251 [details] [diff] [review] Part 7: width/height/frameRate gUM constraints (4) Review of attachment 8411251 [details] [diff] [review]: ----------------------------------------------------------------- I'm not sure about this. Did you consider including the aspect ratio constraint in this? I've a couple of concerns with your handling of aspect ratio. ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp @@ +249,5 @@ > + prefWidth = 1280; > + prefHeight = 768; > + } > + // Avoid blind-clamping width and height independently to not skew aspect > + if (!IsWithin(prefWidth, cWidth) == !IsWithin(prefHeight, cHeight)) { Did you mean && here? Otherwise, this is strange logic. It's true if both are within or both are without. @@ +251,5 @@ > + } > + // Avoid blind-clamping width and height independently to not skew aspect > + if (!IsWithin(prefWidth, cWidth) == !IsWithin(prefHeight, cHeight)) { > + mCapability.width = Clamp(prefWidth, cWidth); > + mCapability.height = Clamp(prefHeight, cHeight); Any reason what you aren't attempting to preserve aspect ratio here? @@ +255,5 @@ > + mCapability.height = Clamp(prefHeight, cHeight); > + } else if (!IsWithin(prefWidth, cWidth)) { > + mCapability.width = Clamp(prefWidth, cWidth); > + mCapability.height = Clamp((mCapability.width * prefHeight) / > + prefWidth, cHeight); I think that this needs a comment, explaining why you retain the preferred aspect ratio rather than the native one. I'd have thought that you get better performance if you preserve the native aspect ratio, unless you have an explicit constraint preventing that. @@ +287,5 @@ > + } > + > + SourceSet tailSet; > + > + // Then apply advanced (formerly known as optional) constraints. What happened to the non-advanced constraints? @@ +321,3 @@ > webrtc::CaptureCapability cap; > bool higher = true; > + for (int i = 0; i < candidateSet.Length(); i++) { uint32_t

Comment on attachment 8410057 [details] [diff] [review] Part 1: update to most recent constraints syntax (6) r=mt Review of attachment 8410057 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

Comment on attachment 8410058 [details] [diff] [review] Part 2: backwards compatible facingMode constraint on mobile (6) r=mt Review of attachment 8410058 [details] [diff] [review]: ----------------------------------------------------------------- lgtm

(In reply to Martin Thomson [:mt] from comment #62) > I'm not sure about this. Did you consider including the aspect ratio > constraint in this? Yes, but as a separate patch to make sure things work sanely without it. > > + prefWidth = 1280; > > + prefHeight = 768; > > + } > > + // Avoid blind-clamping width and height independently to not skew aspect > > + if (!IsWithin(prefWidth, cWidth) == !IsWithin(prefHeight, cHeight)) { > > Did you mean && here? Otherwise, this is strange logic. It's true if both > are within or both are without. (This is the "zero capabilities" mac case) It's the logic I intended: If both or neither get clipped: > > + if (!IsWithin(prefWidth, cWidth) == !IsWithin(prefHeight, cHeight)) { > > + mCapability.width = Clamp(prefWidth, cWidth); > > + mCapability.height = Clamp(prefHeight, cHeight); > > Any reason what you aren't attempting to preserve aspect ratio here? If neither get clipped, then we get the default (pref) aspect. If both are clipped, then we get the aspect of the enclosing constraint. Both are presumably reasonable. However... > > + mCapability.height = Clamp(prefHeight, cHeight); > > + } else if (!IsWithin(prefWidth, cWidth)) { > > + mCapability.width = Clamp(prefWidth, cWidth); > > + mCapability.height = Clamp((mCapability.width * prefHeight) / > > + prefWidth, cHeight); > > I think that this needs a comment, explaining why you retain the preferred > aspect ratio rather than the native one. ... if only one is clipped - e.g. when you drag just one slider in the URL test-case - then I think the resulting skew is almost never reasonable: .------------. | constraint | .----+------------+----. | | | | |pref| result | | prefAspect != resultAspect | | | | '----+------------+----' | | '------------' So in that case, preserving the pref aspect seems more likely to be desirable: .------------. | constraint | .------------. |pref | '------------' | | | | '------------' again, this is with no explicit aspect guidance or native info (mac) > I'd have thought that you get > better performance if you preserve the native aspect ratio, unless you have > an explicit constraint preventing that. Not sure what you mean with native. There's only two sources of info in play here: 1. "pref" - either about:config or 640x480 or Facetime HD (1280x768) 2. constraint ranges > > + // Then apply advanced (formerly known as optional) constraints. > > What happened to the non-advanced constraints? I cheat in Part 4 which you already r+'ed: ;-) > + // TODO(jib): Proper non-ordered handling of nonrequired constraints (907352) > + // > + // For now, put nonrequired constraints at tail of Advanced list. > + // This isn't entirely accurate, as order will matter, but few will notice > + // the difference until we get camera selection and a few more constraints.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #65) > (In reply to Martin Thomson [:mt] from comment #62) > > I'm not sure about this. Did you consider including the aspect ratio > > constraint in this? > > Yes, but as a separate patch to make sure things work sanely without it. At some point it gets harder to track the holistic view. I trust that you know what you are doing, but this is going to interact heavily. > > > + } > > > + // Avoid blind-clamping width and height independently to not skew aspect > > > + if (!IsWithin(prefWidth, cWidth) == !IsWithin(prefHeight, cHeight)) { > > > > Did you mean && here? Otherwise, this is strange logic. It's true if both > > are within or both are without. > > (This is the "zero capabilities" mac case) It's the logic I intended: If > both or neither get clipped: That definitely needs a comment then. And you probably want to drop the '!' on each side. > > > + if (!IsWithin(prefWidth, cWidth) == !IsWithin(prefHeight, cHeight)) { > > > + mCapability.width = Clamp(prefWidth, cWidth); > > > + mCapability.height = Clamp(prefHeight, cHeight); > > > > Any reason what you aren't attempting to preserve aspect ratio here? > > If neither get clipped, then we get the default (pref) aspect. > If both are clipped, then we get the aspect of the enclosing constraint. > > Both are presumably reasonable. However... It depends on the direction that the clamping takes. If I specify a range of 4:3 aspect ratios for the Facetime 16:9 camera that are uniformly lower than 1280x768, e.g., 320-640x240-480, then you could easily produce a 16:9 result of (for instance) 640x360, but you won't with this code. Maybe you want to defer that to the point that you do aspect ratio constraints, but commentary wouldn't hurt. > > I'd have thought that you get > > better performance if you preserve the native aspect ratio, unless you have > > an explicit constraint preventing that. > > Not sure what you mean with native. There's only two sources of info in play > here: > 1. "pref" - either about:config or 640x480 or Facetime HD (1280x768) > 2. constraint ranges I'm losing track of what info you have in what context, but in the very same file you have access to capabilities. I'm not sure what those are, but they are probably closer to "native" than any of the prefs. > > What happened to the non-advanced constraints? > > I cheat in Part 4 which you already r+'ed: ;-) > > + // TODO(jib): Proper non-ordered handling of nonrequired constraints (907352) > > + // > > + // For now, put nonrequired constraints at tail of Advanced list. > > + // This isn't entirely accurate, as order will matter, but few will notice > > + // the difference until we get camera selection and a few more constraints. Ahh, I thought so. Any reason why you can't put the non-required first? (I continue to have issues with the choice of the word "advanced". I need to find an alternative.)

(In reply to Martin Thomson [:mt] from comment #66) > That definitely needs a comment then. And you probably want to drop the '!' > on each side. Will do. > If I specify a range > of 4:3 aspect ratios for the Facetime 16:9 camera that are uniformly lower > than 1280x768, e.g., 320-640x240-480, then you could easily produce a 16:9 > result of (for instance) 640x360, but you won't with this code. This is on purpose, as the Facetime HD hardware switches between 16:9 for higher resolutions and 4:3 for lower resolutions, so 640x360 would come out squished. > I'm losing track of what info you have in what context, but in the very same > file you have access to capabilities. I'm not sure what those are, but they > are probably closer to "native" than any of the prefs. Mac supports none of it. In contrast, Logitech c920 gave me a list of 56 capabilities in Windows... I'm commenting the code better to make the two distinct algorithms clearer: 1) "zero capabilities" short algorithm (e.g. mac) and 2) full algorithm for cams with capabilities (e.g. windows logitech). > Ahh, I thought so. Any reason why you can't put the non-required first? We defined it that way in the spec. The thinking was: have non-deterministic follow deterministic, or nothing is.

I don't know if I addressed all your concerns, but I put up r? again.

Fixed try fail. Forgot that MediaEngineDefaultVideoSource also reads it. I added a few more lines for clarity so I should probably r? again.

Unrot.

Comment on attachment 8411545 [details] [diff] [review] Part 7: width/height/frameRate gUM constraints (5) Review of attachment 8411545 [details] [diff] [review]: ----------------------------------------------------------------- Looks ok. But it seems to be obsolete already. ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp @@ +232,2 @@ > > // Mac doesn't support capabilities. This is an extremely long function. Splitting it up might help readability. Here for instance you could create a method called CreateDefaultCapability(). @@ +262,5 @@ > + > + // Test is: if both or neither clip: > + if (IsWithin(prefWidth, cWidth) == IsWithin(prefHeight, cHeight)) { > + mCapability.width = Clamp(prefWidth, cWidth); > + mCapability.height = Clamp(prefHeight, cHeight); The commentary elsewhere is good, even verbose, but this remains uncommented. Is there a problem with the Mac case where you can end up providing stupendously large resolutions simply by providing stupidly large constraints?

Comment on attachment 8411863 [details] [diff] [review] Part 7: width/height/frameRate gUM constraints (6) Review of attachment 8411863 [details] [diff] [review]: ----------------------------------------------------------------- LGTM, see above review for nits.

(In reply to Martin Thomson [:mt] from comment #71) > Looks ok. But it seems to be obsolete already. No worries, just bitrot. > > // Mac doesn't support capabilities. > > This is an extremely long function. Splitting it up might help readability. > Here for instance you could create a method called CreateDefaultCapability(). Good advice as usual. Coming up! > > + // Test is: if both or neither clip: > > + if (IsWithin(prefWidth, cWidth) == IsWithin(prefHeight, cHeight)) { > > + mCapability.width = Clamp(prefWidth, cWidth); > > + mCapability.height = Clamp(prefHeight, cHeight); > > The commentary elsewhere is good, even verbose, but this remains uncommented. This comment was meant to address this: // If neither get clamped, we get the default (pref) aspect. // If both are clamped, we get the aspect of the enclosing constraint. I'll move it closer. > Is there a problem with the Mac case where you can end up providing > stupendously large resolutions simply by providing stupidly large > constraints? No, because constraints are stupidly large by default (see webidl), and pref is a finite starting point.

1280x720, not 1280x768. :-P Carrying forward r=mt.

Split long function into several. Significant enough to require re-review, sorry. Also: - Split made it easy to reuse the capability-agnostic mac formula on B2G, so I did! - Tweaked the Facetime HD default rule some more. It centers around 640x480 and 1280x720 which I believe to be the only real native resolutions. People will have to find the others on their own for now, and the hardware wont always like the in-betweens. Ultimately, we may want to reverse-engineer the actual capabilities for mac at some point for better behavior, but later.

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

Comment on attachment 8412253 [details] [diff] [review] Part 7: width/height/frameRate gUM constraints (7) Review of attachment 8412253 [details] [diff] [review]: ----------------------------------------------------------------- Looks OK. Refactoring is an improvement here, but I think that is needs a little more tweaking for readability when you detect macHD. ::: content/media/webrtc/MediaEngine.h @@ +140,5 @@ > int32_t mMinFPS; > + > + // mWidth and/or mHeight may be zero (=adaptive default), so use functions. > + > + int32_t GetWidth(bool aHD = false) const { uint32_t instead? @@ +156,5 @@ > + static int32_t GetDefWidth(bool aHD = false) { > + return aHD ? MediaEngine::DEFAULT_169_VIDEO_WIDTH : > + MediaEngine::DEFAULT_43_VIDEO_WIDTH; > + } > + Oops, space. ::: content/media/webrtc/MediaEngineWebRTCVideo.cpp @@ +224,3 @@ > > + // The rest is the full algorithm for cameras that can list their capabilities. > + Oops. @@ +349,5 @@ > + aPrefs.GetHeight() < cHeight.mMin) && > + !(aPrefs.GetWidth(true) > cWidth.mMax || > + aPrefs.GetHeight(true) > cHeight.mMax) && > + (aPrefs.GetWidth(true) != aPrefs.GetWidth() || > + aPrefs.GetHeight(true) != aPrefs.GetHeight()); Yowser. That's hard to follow. Let me see if I get this right... If the constraint doesn't allow for lower than 640x480 AND The constraint does allow for more than 1280x720 Then it's a MAC HD camera and you are going to use that option. Otherwise, you get 640x480. You are hiding the part where all of this only applies if you haven't set an explicit preference (the != portion). @@ +358,5 @@ > + > + if (IsWithin(prefWidth, cWidth) == IsWithin(prefHeight, cHeight)) { > + // If both are within, we get the default (pref) aspect. > + // If neither are within, we get the aspect of the enclosing constraint. > + // Either are presumably reasonable (presuming constraints are sane). I'm still not entirely comfortable with this last parenthetical, but I'm not going to object too strongly any more. Is there any reason why you can't do mCapability.height = Clamp(prefHeight * mCapability.width / prefWidth, cHeight); or the same for width (I don't know which...) ? That would at least attempt to maintain aspect ratio within the limits allowed by the constraint. That would allow you to refactor by folding this into the below.

> @@ +348,5 @@ > > + (aPrefs.GetWidth() < cWidth.mMin || > > + aPrefs.GetHeight() < cHeight.mMin) && > > + !(aPrefs.GetWidth(true) > cWidth.mMax || > > + aPrefs.GetHeight(true) > cHeight.mMax) && > > + (aPrefs.GetWidth(true) != aPrefs.GetWidth() || > > + aPrefs.GetHeight(true) != aPrefs.GetHeight()); > > Yowser. That's hard to follow. Let me see if I get this right... This comment above that code didn't help? // We only go HD when low res is too small and high res isn't too big (and // doublecheck that HD made a difference i.e. not testing same values twice). > If the constraint doesn't allow for lower than 640x480 If the constraint doesn't allow for 640x480 or lower > AND > The constraint does allow for more than 1280x720 The constraint does allow for 1280x720 or more > Then it's a MAC HD camera and you are going to use that option. Otherwise, > you get 640x480. Then it's a MAC HD camera AND big enough numbers are in play that we're going to use 1280x720 instead of 640x480 as our starting point (that we apply constraints to). > You are hiding the part where all of this only applies if you haven't set an > explicit preference (the != portion). OK I can test aPrefs.mWidth and aPrefs.mHeight against zero at the head of the expression. Would that help? > > + if (IsWithin(prefWidth, cWidth) == IsWithin(prefHeight, cHeight)) { > > + // If both are within, we get the default (pref) aspect. > > + // If neither are within, we get the aspect of the enclosing constraint. > > + // Either are presumably reasonable (presuming constraints are sane). > > I'm still not entirely comfortable with this last parenthetical, but I'm not > going to object too strongly any more. We're talking about when neither are within, e.g. constraints smaller than the UA hard-coded 640x480 (or what's in about:config) in practice: .-------------------. | pref (640x480) | | .---------------. | | | constraint | | | | | | | | | | | '---------------' | | | '-------------------' Is it better to take the aspect from the UA or script (infer aspect from constraints) in this case? For mac 640x480 sounds right at these small levels. For B2G about:config defaults to 320x240, not 0 (hmm I thought smartphone cams were 16:9!) For other cases, following about:config at least gives users a way to configure this. If someone sets about:config ridiculously large, then aspect handling would still work the same. so I think you're right. > That would allow you to refactor by folding this into the below. Yes.

(In reply to Martin Thomson [:mt] from comment #77) > > + int32_t GetWidth(bool aHD = false) const { > > uint32_t instead? This is for math on signed spec inputs, and mWidth/mHeight in this class are signed.

(In reply to Jan-Ivar Bruaroey [:jib] from comment #78) > > > + // If neither are within, we get the aspect of the enclosing constraint. > > > + // Either are presumably reasonable (presuming constraints are sane). > > > > I'm still not entirely comfortable with this last parenthetical, but I'm not > > going to object too strongly any more. > > We're talking about when neither are within, e.g. constraints smaller than > the UA hard-coded 640x480 (or what's in about:config) in practice: Correction: Since constraints are both min and max, it's not as simple as what I claimed in comment 78, and the code isn't folding nicely. I have a green try in comment 76 and r+ on all the patches, so in the interest of landing I'll do this recommendation in a follow-up (it only affects odd-shaped extreme constraints on mac).

Fixed these nits. Carrying forward r=mt. - Tweaked comment - Moved test for whether mPref width/height defaults are relied on to top of of macHD expression. - whitespace

https://hg.mozilla.org/integration/mozilla-inbound/rev/abd958ec8fec https://hg.mozilla.org/integration/mozilla-inbound/rev/a8aea4c4977e https://hg.mozilla.org/integration/mozilla-inbound/rev/c0b31eca151e https://hg.mozilla.org/integration/mozilla-inbound/rev/f0edcb56a33a https://hg.mozilla.org/integration/mozilla-inbound/rev/66205061c13f https://hg.mozilla.org/integration/mozilla-inbound/rev/093b21bd43f2 https://hg.mozilla.org/integration/mozilla-inbound/rev/6c46001c0f93

https://hg.mozilla.org/mozilla-central/rev/abd958ec8fec https://hg.mozilla.org/mozilla-central/rev/a8aea4c4977e https://hg.mozilla.org/mozilla-central/rev/c0b31eca151e https://hg.mozilla.org/mozilla-central/rev/f0edcb56a33a https://hg.mozilla.org/mozilla-central/rev/66205061c13f https://hg.mozilla.org/mozilla-central/rev/093b21bd43f2 https://hg.mozilla.org/mozilla-central/rev/6c46001c0f93

This test page stopped working on Nightly sometime since Saturday. Here's a fixed version.

Filed followups Bug 1006707 and Bug 1006725 for remaining dependencies.