907352 - Implement width/height/framerate gUM constraints

Status: RESOLVED FIXED

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

Description

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

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 }},
  ]
}

Comment 1

[Maire Reavy [:mreavy]]

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.

Comment 2

[Chris Liu]

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.

Comment 3

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

(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

Comment 4

[Chris Liu]

(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.

Comment 5

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

(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."

Comment 6

[Chris Liu]

(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.

Comment 7

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

Attachment: Part 1: update to most recent constraints syntax

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

Comment 8

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

Attachment: Part 2: backwards compatible facingMode constraint on mobile

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.

Comment 9

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

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

Comment 10

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

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 11

[Martin Thomson [:mt:]]

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 12

[Martin Thomson [:mt:]]

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?

Comment 13

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

(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.

Comment 14

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

(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.

Comment 15

[Martin Thomson [:mt:]]

(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).

Comment 16

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

Attachment: Part 1: update to most recent constraints syntax (2)

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.

Comment 17

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

Attachment: Part 2: backwards compatible facingMode constraint on mobile (2)

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

Comment 18

[Martin Thomson [:mt:]]

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.

Comment 19

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

Attachment: Part 2: backwards compatible facingMode constraint on mobile (3) r=mt, drno

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

Comment 20

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

(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 21

[Nils Ohlmeier [:drno]]

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

Comment 22

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

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

Comment 23

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

Attachment: Part 1: update to most recent constraints syntax (3) r=mt

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

Comment 24

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

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"] }.

Comment 25

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

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

Comment 26

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

Whoops, forgot to check try. Please hold off.

Comment 27

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

Attachment: Part 2: backwards compatible facingMode constraint on mobile (4) r=mt, drno

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

Comment 28

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

Attachment: Part 2: backwards compatible facingMode constraint on mobile (5) r=mt, drno

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

Comment 29

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

Attachment: Part 3: webidl for width/height/frameRate gUM constraints

WebIDL plumbing.

Comment 30

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

Attachment: Part 5: wiring for width/height/frameRate gUM constraints

Wires into MediaEngineWebRTC[Video|Audio]Source.

Comment 31

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

Attachment: Part 6: width/height/frameRate gUM constraints WIP

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.

Comment 32

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

Attachment: specstats.html - test page for width/height/framerate constraints

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

Comment 33

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

Attachment: Part 1: update to most recent constraints syntax (4) r=mt

Unbitrot.

Comment 34

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

Attachment: Part 1: update to most recent constraints syntax (5) r=mt

Mixed merge-mistake.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Ryan VanderMeulen [:RyanVM]]

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

Comment 37

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

Attachment: Part 1: update to most recent constraints syntax (6) r=mt

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

Comment 38

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

Attachment: Part 2: backwards compatible facingMode constraint on mobile (6) r=mt

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

Comment 39

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

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

Comment 40

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

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

Comment 41

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

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.

Comment 42

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

Attachment: Part 4: normalized constraints for downstream width/height/frameRate implementation

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.

Comment 43

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

Attachment: Part 5: wiring for width/height/frameRate gUM constraints (2)

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.

Comment 44

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

Attachment: Part 7: width/height/frameRate gUM constraints WIP (2)

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

Comment 45

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

(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.

Comment 46

[Martin Thomson [:mt:]]

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

Comment 47

[Martin Thomson [:mt:]]

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 48

[Martin Thomson [:mt:]]

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 49

[Martin Thomson [:mt:]]

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 50

[Martin Thomson [:mt:]]

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.

Comment 51

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

Attachment: Part 3: webidl for width/height/frameRate gUM constraints (2) r=mt

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

Comment 52

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

(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.

Comment 53

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

Attachment: Part 4: normalized constraints for downstream width/height/frameRate implementation (2) r=mt

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

Comment 54

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

(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.

Comment 55

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

Attachment: Part 5: wiring for width/height/frameRate gUM constraints (3) r=mt

Fixed Windows link issue. Carrying forward r=mt.

Comment 56

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

Attachment: 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.

Comment 57

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

Attachment: Part 7: width/height/frameRate gUM constraints WIP (3)

Unrot. Avoid unintended aspect squishing on mac.

Comment 58

[Martin Thomson [:mt:]]

(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.

Comment 59

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

(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.

Comment 60

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

Attachment: specstats.html - test page for width/height/framerate constraints (2)

Lets you set min and max frameRate.
Knobs.

Comment 61

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

Attachment: Part 7: width/height/frameRate gUM constraints (4)

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 62

[Martin Thomson [:mt:]]

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 63

[Nils Ohlmeier [:drno]]

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 64

[Nils Ohlmeier [:drno]]

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

Comment 65

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

(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.

Comment 66

[Martin Thomson [:mt:]]

(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.)

Comment 67

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

(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.

Comment 68

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

Attachment: Part 7: width/height/frameRate gUM constraints (5)

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

Comment 69

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

Attachment: Part 6: change media.navigator.video.default_width|height to 0 (2)

Fixed try fail. Forgot that MediaEngineDefaultVideoSource also reads it.

I added a few more lines for clarity so I should probably r? again.

Comment 70

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

Attachment: Part 7: width/height/frameRate gUM constraints (6)

Unrot.

Comment 71

[Martin Thomson [:mt:]]

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 72

[Martin Thomson [:mt:]]

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.

Comment 73

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

(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.

Comment 74

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

Attachment: Part 6: change media.navigator.video.default_width|height to 0 (3) r=mt

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

Comment 75

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

Attachment: Part 7: width/height/frameRate gUM constraints (7)

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.

Comment 76

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

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

Comment 77

[Martin Thomson [:mt:]]

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.

Comment 78

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

> @@ +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.

Comment 79

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

(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.

Comment 80

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

(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).

Comment 81

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

Attachment: Part 7: width/height/frameRate gUM constraints (7) r=mt

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

Comment 82

[Ryan VanderMeulen [:RyanVM]]

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

Comment 83

[Ryan VanderMeulen [:RyanVM]]

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

Comment 84

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

Attachment: specstats.html - test page for width/height/framerate constraints (3)

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

Comment 85

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

Filed followups Bug 1006707 and Bug 1006725 for remaining dependencies.