Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Description • 9 years ago

      No description provided.

	Soledad Penades [:sole] [:spenades]
	Comment 1 • 9 years ago

Great, I was just going to file this bug because I want to know the size of the video for a demo I'm building without having to create a <video> element, assigning the stream src, and waiting for metadata to load on the video to get the dimensions. Having the capabilities available from the beginning makes the code *way* more readable.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 2 • 9 years ago

Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 3 • 9 years ago

Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 4 • 9 years ago

Bug 1213517 - make track.applyConstraints() store constraints on success.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 5 • 9 years ago

This fiddle shows getConstraints() working. getSettings() is next.

Note that the bloated dictionary output is a result of bug 1226475.

	Randell Jesup [:jesup] (needinfo me)
	Comment 6 • 9 years ago

Comment on attachment 8689890 [details]
Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.

https://reviewboard.mozilla.org/r/25737/#review23171

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 9 years ago

Comment on attachment 8689889 [details]
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

I need some explanation here why we have only subset of the properties from the spec, and do we really need those mozilla specific extensions?
And also, why do we need both MediaTrackConstraintSet and MediaTrackSettings which are pretty much the same?
(I think MediaTrackConstraintSet is functionally superset of MediaTrackSettings)

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 8 • 9 years ago

(In reply to Olli Pettay [:smaug] from comment #7)
> I need some explanation here why we have only subset of the properties from
> the spec,

Constraints come with a discovery mechanism in the form of getSupportedConstraints() [1] which lets us add constraints as we implement them.

That said, I'm also not a fan of adding dictionary (skeleton) members before they're implemented. I did that by accident with getStats last year and it just seems to confuse people into thinking functionality is present when it isn't, leading to confusion, and comments like Bug 1225723 comment 0.

> and do we really need those mozilla specific extensions?

Yes, ask the Hello team. They're in use by our screen sharing code.

> And also, why do we need both MediaTrackConstraintSet and MediaTrackSettings
> which are pretty much the same?
> (I think MediaTrackConstraintSet is functionally superset of MediaTrackSettings)

I 100% agree with you, but I lost that fight with the mediacapture spec editors back in March, and now we're past last-call. If you can make a better argument than I could [2] then by all means I encourage you to go ahead.

[1] http://w3c.github.io/mediacapture-main/getusermedia.html#widl-MediaDevices-getSupportedConstraints-MediaTrackSupportedConstraints

[2] https://github.com/w3c/mediacapture-main/pull/143#issuecomment-82554486

	Randell Jesup [:jesup] (needinfo me)
	Comment 9 • 9 years ago

Comment on attachment 8689891 [details]
Bug 1213517 - make track.applyConstraints() store constraints on success.

https://reviewboard.mozilla.org/r/25739/#review23223

::: dom/media/MediaStreamTrack.cpp:142
(Diff revision 1)
> +  RefPtr<MediaStreamTrack> that = this;

Perhaps add a comment or two about what's happening and why they need to be on a specific thread, etc

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 10 • 9 years ago

Comment on attachment 8689891 [details]
Bug 1213517 - make track.applyConstraints() store constraints on success.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25739/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 11 • 9 years ago

Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

	Randell Jesup [:jesup] (needinfo me)
	Comment 12 • 9 years ago

Comment on attachment 8694859 [details]
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

https://reviewboard.mozilla.org/r/26905/#review24337

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 13 • 8 years ago

Comment on attachment 8689889 [details]
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25735/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 14 • 8 years ago

Comment on attachment 8689890 [details]
Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25737/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 15 • 8 years ago

Comment on attachment 8689891 [details]
Bug 1213517 - make track.applyConstraints() store constraints on success.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25739/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 16 • 8 years ago

Comment on attachment 8694859 [details]
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26905/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 17 • 8 years ago

Comment on attachment 8689889 [details]
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

Just a rebase. Carrying forward r=smaug.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 18 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/58490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58490/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 19 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/58492/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58492/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 20 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/58494/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58494/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 21 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/58496/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58496/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 22 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/58498/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58498/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 23 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/58500/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58500/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 24 • 8 years ago

Comment on attachment 8689889 [details]
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25735/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 25 • 8 years ago

Comment on attachment 8689890 [details]
Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25737/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 26 • 8 years ago

Comment on attachment 8689891 [details]
Bug 1213517 - make track.applyConstraints() store constraints on success.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25739/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 27 • 8 years ago

Comment on attachment 8694859 [details]
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26905/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 28 • 8 years ago

Carrying forward r=smaug (who set flag in bugzilla instead of mozReview).

First four patches are unchanged (though heavily rebased).

I got concurrent camera access with constraints to a point where I thought I'd start the review process (I can launch URL fiddle in multiple tabs and have camera accommodate multiple requests as best it can). 

More patches are coming, as I still need to get settings to the main thread, and I have a RefPtr bug with BaseAllocationHandle somewhere (bonus if caught in review ;)

I also need to extend this to mics and screensharing (probably move some code).

Tests depend on bug 1088621.

	Randell Jesup [:jesup] (needinfo me)
	Comment 29 • 8 years ago

Comment on attachment 8761183 [details]
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().

https://reviewboard.mozilla.org/r/58490/#review55380

	Randell Jesup [:jesup] (needinfo me)
	Comment 30 • 8 years ago

Comment on attachment 8761184 [details]
Bug 1213517 - Normalize all the constraints internally, not just some.

https://reviewboard.mozilla.org/r/58492/#review55384

	Randell Jesup [:jesup] (needinfo me)
	Comment 31 • 8 years ago

Comment on attachment 8761185 [details]
Bug 1213517 - Add an un-flattened NormalizedConstraints type for downstream use.

https://reviewboard.mozilla.org/r/58494/#review55386

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:451
(Diff revision 1)
> -      mCapability.width = ((c.mWidth.mIdeal.WasPassed() ?
> -        c.mWidth.mIdeal.Value() : 0) & 0xffff) << 16 | (c.mWidth.mMax & 0xffff);
> -      mCapability.height = ((c.mHeight.mIdeal.WasPassed() ?
> -        c.mHeight.mIdeal.Value() : 0) & 0xffff) << 16 | (c.mHeight.mMax & 0xffff);
> +      mCapability.width = (c.mWidth.mIdeal.valueOr(0) & 0xffff) << 16 |
> +                          (c.mWidth.mMax & 0xffff);
> +      mCapability.height = (c.mHeight.mIdeal.valueOr(0) & 0xffff) << 16 |
> +                           (c.mHeight.mMax & 0xffff);

Perhaps a comment about how ideal/max are being encoded into mCapability, since it's non-obvious.  Is there a reason we're not using two 16-bit members? (I realize this was existing usage)

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 32 • 8 years ago

Thanks!

(In reply to Randell Jesup [:jesup] from comment #31)
> > +      mCapability.height = (c.mHeight.mIdeal.valueOr(0) & 0xffff) << 16 |
> > +                           (c.mHeight.mMax & 0xffff);
> 
> Perhaps a comment about how ideal/max are being encoded into mCapability,
> since it's non-obvious.  Is there a reason we're not using two 16-bit
> members? (I realize this was existing usage)

mCapability is used by all devices, whereas this was a hack to a problem unique to screen-sharing, that didn't generalize. See bug 1211656 comment 17.

	Randell Jesup [:jesup] (needinfo me)
	Comment 33 • 8 years ago

> mCapability is used by all devices, whereas this was a hack to a problem
> unique to screen-sharing, that didn't generalize. See bug 1211656 comment 17.

Ok.  Sounds worth a 1-liner comment in the code.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 34 • 8 years ago

Added, just holding off on refresh for more reviews (need to reset smaug's r+ each time).

	Randell Jesup [:jesup] (needinfo me)
	Comment 35 • 8 years ago

Comment on attachment 8761187 [details]
Bug 1213517 - Add a way to merge multiple NormalizedConstraints.

https://reviewboard.mozilla.org/r/58498/#review55790

	Randell Jesup [:jesup] (needinfo me)
	Comment 36 • 8 years ago

Comment on attachment 8761188 [details]
Bug 1213517 - Consider competing constraints in getUserMedia+applyConstraints.

https://reviewboard.mozilla.org/r/58500/#review55794

	Randell Jesup [:jesup] (needinfo me)
	Comment 37 • 8 years ago

Comment on attachment 8761186 [details]
Bug 1213517 - Use NormalizedConstraints in low-level code.

https://reviewboard.mozilla.org/r/58496/#review55796

Very close

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:119
(Diff revision 1)
>    }
>  
>    bool first = true;
>    for (const MediaTrackConstraintSet* cs : aConstraintSets) {
>      for (size_t i = 0; i < candidateSet.Length();  ) {
> +      NormalizedConstraintSet ns(*cs, !first); // XXX: optimize

Bug?  how important is optimizing; perhaps a longer comment if not a bug?

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:278
(Diff revision 1)
>    if (!mInitDone) {
>      LOG(("Init not done"));
>      return NS_ERROR_FAILURE;
>    }
> -  if (!ChooseCapability(aConstraints, aPrefs, aDeviceId)) {
> +  MOZ_ASSERT(aHandle);
> +  RefPtr<AllocationHandle> handle = static_cast<AllocationHandle*>(aHandle);

So this was passed as a raw ptr....  you take a ref to it here (but not using the type passed in), and then release the ref.  If it was a 0-refcount (new BaseAllocationHandle) object, boom (it's freed).  Why store it in a RefPtr here?  Either pass it in as a RefPtr<>&, or as a raw ptr and don't ref it.  Since this is a shared interface, but they have very different uses here, either remove the param (and make it implicit) or use a raw ptr and don't ref it, I think

::: dom/media/webrtc/MediaEngineTabVideoSource.cpp:151
(Diff revision 1)
>    // It has no well-defined behavior in advanced, so ignore it there.
>  
>    mWindowId = aConstraints.mBrowserWindow.WasPassed() ?
>                aConstraints.mBrowserWindow.Value() : -1;
>    aOutHandle = nullptr;
> -  return Restart(aConstraints, aPrefs, aDeviceId);
> +  return Restart(nullptr, aConstraints, aPrefs, aDeviceId);

Restart() has a MOZ_ASSERT that aHandle is not null in RemoteVideoSource, but here we assert it *IS* null.  This is odd.

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:271
(Diff revision 1)
> -MediaEngineWebRTCMicrophoneSource::Restart(const dom::MediaTrackConstraints& aConstraints,
> +MediaEngineWebRTCMicrophoneSource::Restart(BaseAllocationHandle* aHandle,
> +                                           const dom::MediaTrackConstraints& aConstraints,
>                                             const MediaEnginePrefs &aPrefs,
>                                             const nsString& aDeviceId)
>  {
> +  MOZ_ASSERT(!aHandle);

ditto

::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:852
(Diff revision 1)
> +    BaseAllocationHandle* aHandle,
>      const dom::MediaTrackConstraints& aConstraints,
>      const MediaEnginePrefs &aPrefs,
>      const nsString& aDeviceId)
>  {
> +  MOZ_ASSERT(!aHandle);

ditto

::: dom/media/webrtc/MediaTrackConstraints.cpp:222
(Diff revision 1)
>  MediaConstraintsHelper::GetMinimumFitnessDistance(
>      const dom::MediaTrackConstraintSet &aConstraints,
>      bool aAdvanced,
>      const nsString& aDeviceId)
>  {
> -  uint64_t distance =
> +  NormalizedConstraintSet ns(aConstraints, aAdvanced); // XXX

XXX what?

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 38 • 8 years ago

https://reviewboard.mozilla.org/r/58496/#review55796

> Restart() has a MOZ_ASSERT that aHandle is not null in RemoteVideoSource, but here we assert it *IS* null.  This is odd.

Because Allocate returns a null handle for TabVideoSource. I've only implemented contraints-averaging through allocationHandles for cameras so far. I should add them to microphones as well, but it's unclear if this generalizes to e.g tab sharing which I'm not sure is a singular shared resource (though it may be implemented as one).

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 39 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/59652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59652/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 40 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/59654/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59654/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 41 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/59656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59656/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 42 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/59658/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59658/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 43 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/59660/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59660/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 44 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/59662/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59662/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 45 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/59664/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59664/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 46 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/59666/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59666/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 47 • 8 years ago

Comment on attachment 8761183 [details]
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58490/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 48 • 8 years ago

Comment on attachment 8761184 [details]
Bug 1213517 - Normalize all the constraints internally, not just some.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58492/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 49 • 8 years ago

Comment on attachment 8761185 [details]
Bug 1213517 - Add an un-flattened NormalizedConstraints type for downstream use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58494/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 50 • 8 years ago

Comment on attachment 8761186 [details]
Bug 1213517 - Use NormalizedConstraints in low-level code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58496/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 51 • 8 years ago

Comment on attachment 8761187 [details]
Bug 1213517 - Add a way to merge multiple NormalizedConstraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58498/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 52 • 8 years ago

Comment on attachment 8761188 [details]
Bug 1213517 - Consider competing constraints in getUserMedia+applyConstraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58500/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 53 • 8 years ago

Comment on attachment 8761183 [details]
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().

Manually carrying forward r=jesup (see bug 1280858).

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 54 • 8 years ago

Works. Still need to make it work for audio, and then tackle bug 1088621 to test this stuff.

Looking for reviewer victims.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 55 • 8 years ago

Ollie, you already reviewed this (above comment 9), but could you do me a favor and also hit "Ship it!" for that patch [1] from within MozReview? This'll stop spamming you each update, and hopefully enable autoland at the end. Thanks!

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8689889

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 56 • 8 years ago

Comment on attachment 8689889 [details]
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

https://reviewboard.mozilla.org/r/25735/#review56702

I still wonder why we need to add those mozilla specific stuff. Does Hello actually need them for some new functionality? 

Is there a bug to sort out the Hello specific properties? If not, could you file it. We should try hard to get rid of mozilla specific properties here, or get them all to some specification.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 57 • 8 years ago

https://reviewboard.mozilla.org/r/25735/#review56702

getSettings() lets sites see the results of constraints they set. It would be weird to not implement it for all constraints we support (a constraints by definition exists in MediaTrackConstraints, MediaTrackSettings and MediaTrackCapabilities (bug 1179084).

Hello already uses these constraints, and may now use getSettings() to improve and/or debug their code better.

Bot us and Google have our own screensharing APIs atm unfortunately. As to standarizing them, the screensharing spec seems to have stalled, so we or someone needs to breathe life into it again, I think is the first step.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 58 • 8 years ago

Comment on attachment 8694859 [details]
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26905/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 59 • 8 years ago

Comment on attachment 8761183 [details]
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58490/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 60 • 8 years ago

Comment on attachment 8761184 [details]
Bug 1213517 - Normalize all the constraints internally, not just some.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58492/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 61 • 8 years ago

Comment on attachment 8761185 [details]
Bug 1213517 - Add an un-flattened NormalizedConstraints type for downstream use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58494/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 62 • 8 years ago

Comment on attachment 8761186 [details]
Bug 1213517 - Use NormalizedConstraints in low-level code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58496/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 63 • 8 years ago

Comment on attachment 8761187 [details]
Bug 1213517 - Add a way to merge multiple NormalizedConstraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58498/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 64 • 8 years ago

Comment on attachment 8761188 [details]
Bug 1213517 - Consider competing constraints in getUserMedia+applyConstraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58500/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 65 • 8 years ago

Comment on attachment 8763464 [details]
Bug 1213517 - Consider competing required constraints with OverconstrainedError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59652/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 66 • 8 years ago

Comment on attachment 8763465 [details]
Bug 1213517 - Report correct constraint in OverconstrainedError when constraints conflict directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59654/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 67 • 8 years ago

Comment on attachment 8763466 [details]
Bug 1213517 - Normalize even more of the constraints code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59656/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 68 • 8 years ago

Comment on attachment 8763467 [details]
Bug 1213517 - Lift correct constraint out of lower-level code for OverconstrainedError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59658/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 69 • 8 years ago

Comment on attachment 8763468 [details]
Bug 1213517 - Only restart camera if net settings actually change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59660/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 70 • 8 years ago

Comment on attachment 8763469 [details]
Bug 1213517 - Consolidate camera Allocate's and Restart's constraints logic.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59662/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 71 • 8 years ago

Comment on attachment 8763470 [details]
Bug 1213517 - Let cam access in competing tabs get closer to their ideals when a tab closes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59664/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 72 • 8 years ago

Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 73 • 8 years ago

Comment on attachment 8694859 [details]
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

Manually carrying forward r=jesup (see bug 1280858).

	Paul Adenot (:padenot)
	Comment 74 • 8 years ago

Are there tests for this ?

	Paul Adenot (:padenot)
	Comment 75 • 8 years ago

Comment on attachment 8763464 [details]
Bug 1213517 - Consider competing required constraints with OverconstrainedError.

https://reviewboard.mozilla.org/r/59652/#review57210

<p>This is also changing the error that is sent back to content. Looking at github, this is issue #170, PR #188, would be good to split this part off and note the spec changes in the commit message.</p>

::: dom/media/MediaManager.cpp:1188
(Diff revision 2)
>  
>      if (mAudioDevice) {
> -      rv = mAudioDevice->Allocate(GetInvariant(mConstraints.mAudio),
> -                                  mPrefs, mOrigin);
> +      auto& constraints = GetInvariant(mConstraints.mAudio);
> +      rv = mAudioDevice->Allocate(constraints, mPrefs, mOrigin);
>        if (NS_FAILED(rv)) {
> -        LOG(("Failed to allocate audiosource %d",rv));
> +        errorMsg = "Failed to allocate audiosource";

Are those supposed to be localizable ?

	Paul Adenot (:padenot)
	Comment 76 • 8 years ago

Comment on attachment 8763465 [details]
Bug 1213517 - Report correct constraint in OverconstrainedError when constraints conflict directly.

https://reviewboard.mozilla.org/r/59654/#review57198

::: dom/media/webrtc/MediaTrackConstraints.h:184
(Diff revision 2)
>    explicit NormalizedConstraints(const dom::MediaTrackConstraints& aOther);
>    explicit NormalizedConstraints(
>        const nsTArray<const NormalizedConstraints*>& aOthers);
>  
>    nsTArray<NormalizedConstraintSet> mAdvanced;
> -  bool mOverconstrained;
> +  const char* mBadConstraint;

Bare pointers make me nervous, but I don't suppose we have a way of ensuring this is always pointing to a literal string?

::: dom/media/webrtc/MediaTrackConstraints.cpp:292
(Diff revision 2)
> +    if (!mMozNoiseSuppression.Merge(set.mMozNoiseSuppression)) {
> +      mBadConstraint = "mozNoiseSuppression";
> +      return;
> +    }
> +    if (!mMozAutoGainControl.Merge(set.mMozAutoGainControl)) {
> +      mBadConstraint = "mozAutoGainControl";

Don't we need to use `NS_LITERAL_STRING` here ?

::: dom/media/webrtc/MediaTrackConstraints.cpp:293
(Diff revision 2)
> +      mBadConstraint = "mozNoiseSuppression";
> +      return;
> +    }
> +    if (!mMozAutoGainControl.Merge(set.mMozAutoGainControl)) {
> +      mBadConstraint = "mozAutoGainControl";
>        return;

I'd be happier is this would be written by a macro or something.

	Paul Adenot (:padenot)
	Comment 77 • 8 years ago

Comment on attachment 8763467 [details]
Bug 1213517 - Lift correct constraint out of lower-level code for OverconstrainedError.

https://reviewboard.mozilla.org/r/59658/#review57214

Maybe the renaming and the consts could have been put in another patch.

Can you add a comment about going from the Engine level to the Device level to get the bad constraint, and why the logic is at the device level in the first place?

	Paul Adenot (:padenot)
	Comment 78 • 8 years ago

Comment on attachment 8763468 [details]
Bug 1213517 - Only restart camera if net settings actually change.

https://reviewboard.mozilla.org/r/59660/#review57216

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:411
(Diff revision 2)
> +};
> +
> +bool operator != (const webrtc::CaptureCapability& a,
> +                  const webrtc::CaptureCapability& b)
> +{
> +  return !operator == (a, b);

Would simply !(a == b) work ?

::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:126
(Diff revision 2)
>    void Init();
>    size_t NumCapabilities() const override;
>    void GetCapability(size_t aIndex, webrtc::CaptureCapability& aOut) const override;
>  
> +  nsresult
> +  UpdateLive(AllocationHandle* aOldHandle,

Comment here. In particular, mention that aOldHandle can be `nullptr` if we are calling it for the first time.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:136
(Diff revision 2)
> +
>    dom::MediaSourceEnum mMediaSource; // source of media (camera | application | screen)
>    mozilla::camera::CaptureEngine mCapEngine;
>  
>    nsTArray<RefPtr<AllocationHandle>> mRegisteredHandles;
> +  webrtc::CaptureCapability mLastCapability;

Comment here.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:344
(Diff revision 2)
> -    LOG(("StartCapture failed"));
> +      LOG(("StartCapture failed"));
> -    return NS_ERROR_FAILURE;
> +      return NS_ERROR_FAILURE;
> -  }
> +    }
> -  handle->mConstraints = temp->mConstraints;
> +    mLastCapability = mCapability;
> +  }
> +  aOldHandle->mConstraints = aNewHandle->mConstraints;

What if `aOldHandle` is `nullptr` here? If this cannot happend, maybe we could put in an assert with a message.

	Paul Adenot (:padenot)
	Comment 79 • 8 years ago

Comment on attachment 8763469 [details]
Bug 1213517 - Consolidate camera Allocate's and Restart's constraints logic.

https://reviewboard.mozilla.org/r/59662/#review57218

::: dom/media/webrtc/MediaEngineRemoteVideoSource.h:74
(Diff revision 2)
>    // MediaEngineCameraVideoSource
>    MediaEngineRemoteVideoSource(int aIndex, mozilla::camera::CaptureEngine aCapEngine,
>                                 dom::MediaSourceEnum aMediaSource,
>                                 const char* aMonitorName = "RemoteVideo.Monitor");
>  
>    class AllocationHandle : public BaseAllocationHandle

This is not simply an "Allocation Handle" anymore, it's keeping more state and has become more specific.

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:119
(Diff revision 2)
> -  if (netConstraints.mBadConstraint) {
> -    *aOutBadConstraint = netConstraints.mBadConstraint;
> -    return NS_ERROR_NOT_AVAILABLE;
> -  }
>  
> -  if (!ChooseCapability(netConstraints, aPrefs, aDeviceId)) {
> +  nsresult rv = Update(nullptr, handle, aPrefs, aDeviceId, aOutBadConstraint);

I don't like nullptr at call sites too much, it's hard to know what the param would normally be.

I think we should make a function that wraps `Update`, and that curries the first argument, with a meaningful name.

	Paul Adenot (:padenot)
	Comment 80 • 8 years ago

Comment on attachment 8763470 [details]
Bug 1213517 - Let cam access in competing tabs get closer to their ideals when a tab closes.

https://reviewboard.mozilla.org/r/59664/#review57220

::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:180
(Diff revision 2)
> +    MOZ_ASSERT(mRegisteredHandles.Length());
> +    if (!mInShutdown) {
> +      // Whenever constraints are removed, other parties may get closer to ideal.
> +      auto& first = mRegisteredHandles[0];
> +      const char* badConstraint = nullptr;
> +      return Update(nullptr, nullptr,

Same deal, we should have a descriptive name here.

	Paul Adenot (:padenot)
	Comment 81 • 8 years ago

Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

https://reviewboard.mozilla.org/r/59666/#review57206

::: dom/media/MediaStreamTrack.h:140
(Diff revision 2)
>                     const dom::MediaTrackConstraints& aConstraints);
>  
>    /**
> +   * Same for GetSettings (no-op).
> +   */
> +

nit: stray new line.

::: dom/media/webrtc/MediaEngine.h:200
(Diff revision 2)
>  
>    virtual uint32_t GetBestFitnessDistance(
>        const nsTArray<const NormalizedConstraintSet*>& aConstraintSets,
>        const nsString& aDeviceId) const = 0;
>  
> +  // Main-thread only.

Assert it.

	Paul Adenot (:padenot)
	Comment 82 • 8 years ago

Comment on attachment 8763466 [details]
Bug 1213517 - Normalize even more of the constraints code.

https://reviewboard.mozilla.org/r/59656/#review57202

::: dom/media/webrtc/MediaEngineCameraVideoSource.cpp:271
(Diff revision 2)
>      }
>      if (!candidateSet.Length()) {
>        candidateSet.AppendElements(Move(rejects));
>      }
>    }
> -  if (!candidateSet.Length()) {
> +  MOZ_ASSERT(candidateSet.Length());

Document this invariant with a message.

	Paul Adenot (:padenot)
	Comment 83 • 8 years ago

This patch set would benefit from more splitting between functional changes and cleanups, and from more comments and descriptive commit messages, I think.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 84 • 8 years ago

Thanks!

(In reply to Paul Adenot (:padenot) from comment #74)
> Are there tests for this ?

As I mentioned in comment 28 I need to do bug 1088621 next before we can write tree tests. Even then, those tests will be testing the default (fake) device rather than actual devices.

So I've manually tested getSettings() and cross-tab ideal averaging on real devices (Logitech 930e on OSX, and soon on Win10 and Android when I get a chance), using the URL fiddle in multiple tabs.

I've also tested conflicting required constraints using these in different tabs:

 - gum test conflict max 320  https://jsfiddle.net/mxkpaq7z/
 - gum test conflict min 640  https://jsfiddle.net/1w209kg7/

I also still need to extend this to mics and screensharing, which I'll do asap, though I'd love to land the camera side soon, to avoid the patch set growing much larger, as it is starting to become a pain.

I am committed to getting it all in this cycle.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 85 • 8 years ago

https://reviewboard.mozilla.org/r/59652/#review57210

Filed bug 1281866.

> Are those supposed to be localizable ?

No, neither log messages nor DOM errors are localized (Bz says we do not want web-exposed behavior to differ by locale).

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 86 • 8 years ago

https://reviewboard.mozilla.org/r/59654/#review57198

> Bare pointers make me nervous, but I don't suppose we have a way of ensuring this is always pointing to a literal string?

Unfortunately, no. A const char array reference would require a fixed size (worse issues), whereas an enum would need mappings to string somewhere, which would be another thing to update every time someone adds a new constraint, something I want to avoid as much as possible.

If it helps, this is an existing pattern: https://dxr.mozilla.org/mozilla-central/search?q=badConstraint

> Don't we need to use `NS_LITERAL_STRING` here ?

No, mBadConstraint is a const char*, not an nsString.

> I'd be happier is this would be written by a macro or something.

Macros make me sad. Let me see if I can condense it another way.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 87 • 8 years ago

https://reviewboard.mozilla.org/r/59654/#review57198

> Macros make me sad. Let me see if I can condense it another way.

OK so I took a big detour on this. As a goal, I wanted there to be just one place one needs to update to add new constraints, and have the constraints algorithm automatically work for it.

The resulting patch is a bit sick, but uses no macros, so I put it in the win column. Hopefully you do as well. :)

I also spent time tracking down unrelated leaks and a crash, which is reflected by updates to some of the earlier patches.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 88 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/62102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62102/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 89 • 8 years ago

Comment on attachment 8689889 [details]
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25735/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 90 • 8 years ago

Comment on attachment 8689890 [details]
Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25737/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 91 • 8 years ago

Comment on attachment 8689891 [details]
Bug 1213517 - make track.applyConstraints() store constraints on success.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25739/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 92 • 8 years ago

Comment on attachment 8694859 [details]
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26905/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 93 • 8 years ago

Comment on attachment 8761183 [details]
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58490/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 94 • 8 years ago

Comment on attachment 8761184 [details]
Bug 1213517 - Normalize all the constraints internally, not just some.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58492/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 95 • 8 years ago

Comment on attachment 8761185 [details]
Bug 1213517 - Add an un-flattened NormalizedConstraints type for downstream use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58494/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 96 • 8 years ago

Comment on attachment 8761186 [details]
Bug 1213517 - Use NormalizedConstraints in low-level code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58496/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 97 • 8 years ago

Comment on attachment 8761187 [details]
Bug 1213517 - Add a way to merge multiple NormalizedConstraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58498/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 98 • 8 years ago

Comment on attachment 8761188 [details]
Bug 1213517 - Consider competing constraints in getUserMedia+applyConstraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58500/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 99 • 8 years ago

Comment on attachment 8763464 [details]
Bug 1213517 - Consider competing required constraints with OverconstrainedError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59652/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 100 • 8 years ago

Comment on attachment 8763465 [details]
Bug 1213517 - Report correct constraint in OverconstrainedError when constraints conflict directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59654/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 101 • 8 years ago

Comment on attachment 8763466 [details]
Bug 1213517 - Normalize even more of the constraints code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59656/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 102 • 8 years ago

Comment on attachment 8763467 [details]
Bug 1213517 - Lift correct constraint out of lower-level code for OverconstrainedError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59658/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 103 • 8 years ago

Comment on attachment 8763468 [details]
Bug 1213517 - Only restart camera if net settings actually change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59660/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 104 • 8 years ago

Comment on attachment 8763469 [details]
Bug 1213517 - Consolidate camera Allocate's and Restart's constraints logic.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59662/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 105 • 8 years ago

Comment on attachment 8763470 [details]
Bug 1213517 - Let cam access in competing tabs get closer to their ideals when a tab closes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59664/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 106 • 8 years ago

Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/2-3/

	Paul Adenot (:padenot)
	Comment 107 • 8 years ago

Comment on attachment 8763465 [details]
Bug 1213517 - Report correct constraint in OverconstrainedError when constraints conflict directly.

https://reviewboard.mozilla.org/r/59654/#review59036

::: dom/media/webrtc/MediaTrackConstraints.h:134
(Diff revisions 1 - 3)
>          return false;
>        }
>        Intersect(aOther);
>  
>        for (auto& entry : aOther.mIdeal) {
> -        if (!mIdeal.Contains(entry)) {
> +        if (mIdeal.find(entry) == mIdeal.end()) {

This is now a set, you can just insert without checking if the element is present, it's going to deduplicate automatically.

Also, https://www.sgi.com/tech/stl/set_union.html

::: dom/media/webrtc/MediaTrackConstraints.cpp:219
(Diff revisions 1 - 3)
> -  if (!aOther.mExact.Length()) {
> +  if (!aOther.mExact.size()) {
>      return;
>    }
>    for (auto& entry : mExact) {
> -    if (!aOther.mExact.Contains(entry)) {
> -      mExact.RemoveElement(entry);
> +    if (aOther.mExact.find(entry) == aOther.mExact.end()) {
> +      mExact.erase(entry);

Because those are `std::set`, intersection is already implemented for you.

https://www.sgi.com/tech/stl/set_intersection.html

	Paul Adenot (:padenot)
	Comment 108 • 8 years ago

Comment on attachment 8767669 [details]
Bug 1213517 - optimize for maintenance of constraints (member pointer approach).

https://reviewboard.mozilla.org/r/62102/#review59040

	Paul Adenot (:padenot)
	Comment 109 • 8 years ago

Comment on attachment 8763468 [details]
Bug 1213517 - Only restart camera if net settings actually change.

https://reviewboard.mozilla.org/r/59660/#review59042

	Paul Adenot (:padenot)
	Comment 110 • 8 years ago

Comment on attachment 8763469 [details]
Bug 1213517 - Consolidate camera Allocate's and Restart's constraints logic.

https://reviewboard.mozilla.org/r/59662/#review59044

	Paul Adenot (:padenot)
	Comment 111 • 8 years ago

Comment on attachment 8763470 [details]
Bug 1213517 - Let cam access in competing tabs get closer to their ideals when a tab closes.

https://reviewboard.mozilla.org/r/59664/#review59048

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 112 • 8 years ago

Comment on attachment 8763465 [details]
Bug 1213517 - Report correct constraint in OverconstrainedError when constraints conflict directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59654/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 113 • 8 years ago

Comment on attachment 8767669 [details]
Bug 1213517 - optimize for maintenance of constraints (member pointer approach).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62102/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 114 • 8 years ago

Comment on attachment 8763466 [details]
Bug 1213517 - Normalize even more of the constraints code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59656/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 115 • 8 years ago

Comment on attachment 8763467 [details]
Bug 1213517 - Lift correct constraint out of lower-level code for OverconstrainedError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59658/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 116 • 8 years ago

Comment on attachment 8763468 [details]
Bug 1213517 - Only restart camera if net settings actually change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59660/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 117 • 8 years ago

Comment on attachment 8763469 [details]
Bug 1213517 - Consolidate camera Allocate's and Restart's constraints logic.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59662/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 118 • 8 years ago

Comment on attachment 8763470 [details]
Bug 1213517 - Let cam access in competing tabs get closer to their ideals when a tab closes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59664/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 119 • 8 years ago

Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 120 • 8 years ago

Weird,

comment 112 through comment 115 all show the same change, while comment 116 through comment 119 are duds.

	Paul Adenot (:padenot)
	Comment 121 • 8 years ago

Comment on attachment 8763465 [details]
Bug 1213517 - Report correct constraint in OverconstrainedError when constraints conflict directly.

https://reviewboard.mozilla.org/r/59654/#review59352

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 122 • 8 years ago

https://reviewboard.mozilla.org/r/58496/#review55796

> ditto

Going to add microphone in a follow-up bug.

> ditto

Going to add microphone in a follow-up bug.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 123 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/62894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62894/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 124 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/62896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62896/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 125 • 8 years ago

Review commit: https://reviewboard.mozilla.org/r/62898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62898/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 126 • 8 years ago

Comment on attachment 8689889 [details]
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25735/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 127 • 8 years ago

Comment on attachment 8689890 [details]
Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25737/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 128 • 8 years ago

Comment on attachment 8689891 [details]
Bug 1213517 - make track.applyConstraints() store constraints on success.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25739/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 129 • 8 years ago

Comment on attachment 8694859 [details]
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26905/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 130 • 8 years ago

Comment on attachment 8761183 [details]
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58490/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 131 • 8 years ago

Comment on attachment 8761184 [details]
Bug 1213517 - Normalize all the constraints internally, not just some.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58492/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 132 • 8 years ago

Comment on attachment 8761185 [details]
Bug 1213517 - Add an un-flattened NormalizedConstraints type for downstream use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58494/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 133 • 8 years ago

Comment on attachment 8761186 [details]
Bug 1213517 - Use NormalizedConstraints in low-level code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58496/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 134 • 8 years ago

Comment on attachment 8761187 [details]
Bug 1213517 - Add a way to merge multiple NormalizedConstraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58498/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 135 • 8 years ago

Comment on attachment 8761188 [details]
Bug 1213517 - Consider competing constraints in getUserMedia+applyConstraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58500/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 136 • 8 years ago

Comment on attachment 8763464 [details]
Bug 1213517 - Consider competing required constraints with OverconstrainedError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59652/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 137 • 8 years ago

Comment on attachment 8763465 [details]
Bug 1213517 - Report correct constraint in OverconstrainedError when constraints conflict directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59654/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 138 • 8 years ago

Comment on attachment 8767669 [details]
Bug 1213517 - optimize for maintenance of constraints (member pointer approach).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62102/diff/2-3/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 139 • 8 years ago

Comment on attachment 8763466 [details]
Bug 1213517 - Normalize even more of the constraints code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59656/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 140 • 8 years ago

Comment on attachment 8763467 [details]
Bug 1213517 - Lift correct constraint out of lower-level code for OverconstrainedError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59658/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 141 • 8 years ago

Comment on attachment 8763468 [details]
Bug 1213517 - Only restart camera if net settings actually change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59660/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 142 • 8 years ago

Comment on attachment 8763469 [details]
Bug 1213517 - Consolidate camera Allocate's and Restart's constraints logic.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59662/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 143 • 8 years ago

Comment on attachment 8763470 [details]
Bug 1213517 - Let cam access in competing tabs get closer to their ideals when a tab closes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59664/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 144 • 8 years ago

Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 145 • 8 years ago

Comment on attachment 8763469 [details]
Bug 1213517 - Consolidate camera Allocate's and Restart's constraints logic.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59662/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 146 • 8 years ago

Comment on attachment 8763470 [details]
Bug 1213517 - Let cam access in competing tabs get closer to their ideals when a tab closes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59664/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 147 • 8 years ago

Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 148 • 8 years ago

Comment on attachment 8768892 [details]
Bug 1213517 - Make applyConstraints() and getSettings() do nothing after stop and shutdown.

Review commit: https://reviewboard.mozilla.org/r/62894/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62894/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 149 • 8 years ago

Comment on attachment 8768893 [details]
Bug 1213517 - Clamp competing ideal values before considering them to avoid outliers distorting result.

Review commit: https://reviewboard.mozilla.org/r/62896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62896/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 150 • 8 years ago

Comment on attachment 8768894 [details]
Bug 1213517 - Take highest ideal value from competing width, height and frameRate.

Review commit: https://reviewboard.mozilla.org/r/62898/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62898/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 151 • 8 years ago

Gah! All this moz review noise! I just rebased the patches, and forgot one r? so it spammed twice. :(

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 152 • 8 years ago

Comment on attachment 8768894 [details]
Bug 1213517 - Take highest ideal value from competing width, height and frameRate.

OK so the last patch, "Take highest ideal value from competing width, height and frameRate", has the behavior several of us argued they wanted instead of taking the average of competing ideal values.

That said, I don't like it much, as it causes some surprising results on my Logitech C910 in not-entirely edge cases, like this one:

  1. Open https://jsfiddle.net/7udgdbbu/ in two tabs, and allow camera in both.
  2. In tab B, click [640] button to get 640x480.

  Expected result: 640x480x30
  Actual result: 1024x576x10

This isn't a bug, but a result of the opening constraints in this JS fiddle specifying frameRate: 10, and the others not:

  Tab A has { width: 160, height: 120, frameRate: 10 }
  Tab B has { width: 640, height: 480 }

producing:  { width: 640, height: 480, frameRate: 10 }

which, incidentally even today, yields 1024x576x10 on my Logitech C910, due to the spec's fitness distance algorithm:

> D/MediaManager Constraints: width: { min: -2147483647, max: 2147483647, ideal: 640 }
> D/MediaManager              height: { min: -2147483647, max: 2147483647, ideal: 480 }
> D/MediaManager              frameRate: { min: -inf, max: inf, ideal: 10.000000 }
> D/MediaManager Capability:  160 x  120 x 30 maxFps, MJPEG, Unknown codec. Distance = 2166
> D/MediaManager Capability:  176 x  144 x 30 maxFps, MJPEG, Unknown codec. Distance = 2091
> D/MediaManager Capability:  320 x  176 x 30 maxFps, MJPEG, Unknown codec. Distance = 1799
> D/MediaManager Capability:  320 x  240 x 60 maxFps, MJPEG, Unknown codec. Distance = 1833
> D/MediaManager Capability:  352 x  288 x 30 maxFps, MJPEG, Unknown codec. Distance = 1516
> D/MediaManager Capability:  432 x  240 x 30 maxFps, MJPEG, Unknown codec. Distance = 1491
> D/MediaManager Capability:  544 x  288 x 30 maxFps, MJPEG, Unknown codec. Distance = 1216
> D/MediaManager Capability:  640 x  480 x 60 maxFps, MJPEG, Unknown codec. Distance = 833
> D/MediaManager Capability:  752 x  416 x 30 maxFps, MJPEG, Unknown codec. Distance = 947
> D/MediaManager Capability:  800 x  448 x 30 maxFps, MJPEG, Unknown codec. Distance = 932
> D/MediaManager Capability:  864 x  480 x 30 maxFps, MJPEG, Unknown codec. Distance = 925
> D/MediaManager Capability:  800 x  600 x 30 maxFps, MJPEG, Unknown codec. Distance = 1066
> D/MediaManager Capability:  960 x  544 x 30 maxFps, MJPEG, Unknown codec. Distance = 1116
> D/MediaManager Capability: 1024 x  576 x 30 maxFps, MJPEG, Unknown codec. Distance = 1207
> D/MediaManager Capability:  960 x  720 x 30 maxFps, MJPEG, Unknown codec. Distance = 1332
> D/MediaManager Capability: 1184 x  656 x 30 maxFps, MJPEG, Unknown codec. Distance = 1393
> D/MediaManager Capability: 1280 x  720 x 30 maxFps, MJPEG, Unknown codec. Distance = 1499
> D/MediaManager Capability: 1392 x  768 x 15 maxFps, MJPEG, Unknown codec. Distance = 1248
> D/MediaManager Capability: 1280 x  960 x 15 maxFps, MJPEG, Unknown codec. Distance = 1333
> D/MediaManager Capability: 1504 x  832 x 15 maxFps, MJPEG, Unknown codec. Distance = 1330
> D/MediaManager Capability: 1600 x  896 x 15 maxFps, MJPEG, Unknown codec. Distance = 1397
> D/MediaManager Capability: 1712 x  960 x 15 maxFps, MJPEG, Unknown codec. Distance = 1459
> D/MediaManager Capability: 1792 x 1008 x 15 maxFps, MJPEG, Unknown codec. Distance = 1498
> D/MediaManager Capability: 1600 x 1200 x 15 maxFps, MJPEG, Unknown codec. Distance = 1533
> D/MediaManager Capability: 1920 x 1080 x 30 maxFps, MJPEG, Unknown codec. Distance = 1887
> D/MediaManager Capability: 2048 x 1536 x 15 maxFps, MJPEG, Unknown codec. Distance = 1707
> D/MediaManager Capability: 2592 x 1944 x 10 maxFps, MJPEG, Unknown codec. Distance = 1506
> D/MediaManager Capability:  160 x  120 x 30 maxFps, YUY2, Unknown codec. Distance = 2166
> D/MediaManager Capability:  176 x  144 x 30 maxFps, YUY2, Unknown codec. Distance = 2091
> D/MediaManager Capability:  320 x  176 x 30 maxFps, YUY2, Unknown codec. Distance = 1799
> D/MediaManager Capability:  320 x  240 x 30 maxFps, YUY2, Unknown codec. Distance = 1666
> D/MediaManager Capability:  352 x  288 x 30 maxFps, YUY2, Unknown codec. Distance = 1516
> D/MediaManager Capability:  432 x  240 x 30 maxFps, YUY2, Unknown codec. Distance = 1491
> D/MediaManager Capability:  544 x  288 x 30 maxFps, YUY2, Unknown codec. Distance = 1216
> D/MediaManager Capability:  640 x  480 x 30 maxFps, YUY2, Unknown codec. Distance = 666
> D/MediaManager Capability:  752 x  416 x 24 maxFps, YUY2, Unknown codec. Distance = 864
> D/MediaManager Capability:  800 x  448 x 24 maxFps, YUY2, Unknown codec. Distance = 849
> D/MediaManager Capability:  864 x  480 x 20 maxFps, YUY2, Unknown codec. Distance = 759
> D/MediaManager Capability:  800 x  600 x 20 maxFps, YUY2, Unknown codec. Distance = 900
> D/MediaManager Capability:  960 x  544 x 20 maxFps, YUY2, Unknown codec. Distance = 950
> D/MediaManager Capability: 1024 x  576 x 10 maxFps, YUY2, Unknown codec. Distance = 541
> D/MediaManager Capability:  960 x  720 x 15 maxFps, YUY2, Unknown codec. Distance = 999
> D/MediaManager Capability: 1184 x  656 x 10 maxFps, YUY2, Unknown codec. Distance = 727
> D/MediaManager Chosen capability: 1024 x  576 x 10 maxFps, YUY2, Unknown codec. Distance = 541

Quite surprising, given that none of the two tabs would have gone that high up on their own!

I predict this is going to happen a lot, because

  A) lower (max?) frame rates seem to typically exist for higher resolutions, and
  B) the frameRate constraint seems to be used mostly to reduce frame rate (since default is 30)

I argue this is less likely to happen with averaging, since it wont hit the highest resolutions where frame rates interfere, and would also be less surprising, since averaging is an understandable implementation of compromise, whereas the above edge-pushing behavior is not.

I think this is an inherent flaw in the "Take highest ideal value" algorithm, so I vote for averaging.

	Randell Jesup [:jesup] (needinfo me)
	Comment 153 • 8 years ago

Alternative: unspecified framerate could internally be represented as ideal: 30, since that's what everyone's code assumes.  Alternatively (though still surprising to users) would be to consider any framerate which we can decimate down to the requested rate to be an "exact" match, and add a small amount of code to decimate it.  Thus you're get { 640, 480, 10 } -> 640x480@30 decimated to 10.  With a small amount of additional smarts we could decimate only the stream that asked for 10.  

Note that for decimation we'd want to stick to integer decimation values to avoid jerky framerates i.e. 30 can yield 15, 10, 6, 5, 3, 2 and 1, but not 20.  (you could include /4 -> 7.5fps, /7 -> 4.28..., /8 -> 3.75, etc).  Decimation is pretty trivial to implement.

Also: 640x480@30 as a mode from the camera *lies*.  With most cameras, it's *max* 30, but the actual framerate will vary as the lighting changes, even down to 5fps.  So taking framerates in the returned data too seriously is a mistake.  :-(

Probably the best (without per-track scaling) is take the highest framerate (with unspecified = 30), then decimate for any track where the ideal is lower.  Note that the decimation has to be dynamic, not 1 of N, because the input rate isn't actually the requested rate.  There's code in webrtc.org to monitor and decimate framerates we can use and/or copy.

	Randell Jesup [:jesup] (needinfo me)
	Comment 154 • 8 years ago

Comment on attachment 8768892 [details]
Bug 1213517 - Make applyConstraints() and getSettings() do nothing after stop and shutdown.

manually r+ since RB gives 500 Server Error trying to finish the review

	Randell Jesup [:jesup] (needinfo me)
	Comment 155 • 8 years ago

Comment on attachment 8768894 [details]
Bug 1213517 - Take highest ideal value from competing width, height and frameRate.

r+.  However, please note my comments in the bug; I think we should set unspecified framerate to 30, and probably consider a separate patch/bug to decimate framerates when lower is requested.

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 156 • 8 years ago

Comment on attachment 8689889 [details]
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25735/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 157 • 8 years ago

Comment on attachment 8689890 [details]
Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25737/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 158 • 8 years ago

Comment on attachment 8689891 [details]
Bug 1213517 - make track.applyConstraints() store constraints on success.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25739/diff/6-7/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 159 • 8 years ago

Comment on attachment 8694859 [details]
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26905/diff/6-7/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 160 • 8 years ago

Comment on attachment 8761183 [details]
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58490/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 161 • 8 years ago

Comment on attachment 8761184 [details]
Bug 1213517 - Normalize all the constraints internally, not just some.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58492/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 162 • 8 years ago

Comment on attachment 8761185 [details]
Bug 1213517 - Add an un-flattened NormalizedConstraints type for downstream use.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58494/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 163 • 8 years ago

Comment on attachment 8761186 [details]
Bug 1213517 - Use NormalizedConstraints in low-level code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58496/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 164 • 8 years ago

Comment on attachment 8761187 [details]
Bug 1213517 - Add a way to merge multiple NormalizedConstraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58498/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 165 • 8 years ago

Comment on attachment 8761188 [details]
Bug 1213517 - Consider competing constraints in getUserMedia+applyConstraints.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58500/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 166 • 8 years ago

Comment on attachment 8763464 [details]
Bug 1213517 - Consider competing required constraints with OverconstrainedError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59652/diff/4-5/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 167 • 8 years ago

Comment on attachment 8763465 [details]
Bug 1213517 - Report correct constraint in OverconstrainedError when constraints conflict directly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59654/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 168 • 8 years ago

Comment on attachment 8767669 [details]
Bug 1213517 - optimize for maintenance of constraints (member pointer approach).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62102/diff/3-4/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 169 • 8 years ago

Comment on attachment 8763466 [details]
Bug 1213517 - Normalize even more of the constraints code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59656/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 170 • 8 years ago

Comment on attachment 8763467 [details]
Bug 1213517 - Lift correct constraint out of lower-level code for OverconstrainedError.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59658/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 171 • 8 years ago

Comment on attachment 8763468 [details]
Bug 1213517 - Only restart camera if net settings actually change.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59660/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 172 • 8 years ago

Comment on attachment 8763469 [details]
Bug 1213517 - Consolidate camera Allocate's and Restart's constraints logic.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59662/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 173 • 8 years ago

Comment on attachment 8763470 [details]
Bug 1213517 - Let cam access in competing tabs get closer to their ideals when a tab closes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59664/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 174 • 8 years ago

Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/5-6/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 175 • 8 years ago

Comment on attachment 8768892 [details]
Bug 1213517 - Make applyConstraints() and getSettings() do nothing after stop and shutdown.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62894/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 176 • 8 years ago

Comment on attachment 8768893 [details]
Bug 1213517 - Clamp competing ideal values before considering them to avoid outliers distorting result.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62896/diff/1-2/

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 177 • 8 years ago

Comment on attachment 8768894 [details]
Bug 1213517 - Take highest ideal value from competing width, height and frameRate.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/62898/diff/1-2/

	Randell Jesup [:jesup] (needinfo me)
	Comment 179 • 8 years ago

Comment on attachment 8768893 [details]
Bug 1213517 - Clamp competing ideal values before considering them to avoid outliers distorting result.

https://reviewboard.mozilla.org/r/62896/#review61082

::: dom/media/webrtc/MediaTrackConstraints.h:99
(Diff revision 1)
>        }
>        Intersect(aOther);
>  
>        if (aOther.mIdeal.isSome()) {
> +        // Ideal values, as stored, may be outside their min max range, so use
> +        // clamped values in averaging, to avoid extreme outliers skewing results.

	Randell Jesup [:jesup] (needinfo me)
	Comment 180 • 8 years ago

Comment on attachment 8768892 [details]
Bug 1213517 - Make applyConstraints() and getSettings() do nothing after stop and shutdown.

https://reviewboard.mozilla.org/r/62894/#review61080

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 181 • 8 years ago

Another painful rebase (need to get this landed). The only real change here is comment 177.

Randell, if you could try r+ from mozReview again, I would appreciate it, as I otherwise wont be able to autoland.

(In reply to Randell Jesup [:jesup] from comment #155)
> Comment on attachment 8768894 [details]
> Bug 1213517 - Take highest ideal value from competing width, height and
> frameRate.
> 
> r+.  However, please note my comments in the bug; I think we should set
> unspecified framerate to 30, and probably consider a separate patch/bug to
> decimate framerates when lower is requested.

I added code to consider unspecified framerate as 30 fps, as you suggested, and filed Bug 1286945 as a follow-up.

	Randell Jesup [:jesup] (needinfo me)
	Comment 182 • 8 years ago

Comment on attachment 8768894 [details]
Bug 1213517 - Take highest ideal value from competing width, height and frameRate.

https://reviewboard.mozilla.org/r/62898/#review61358

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 183 • 8 years ago

Thanks! Mind re-r+-ing comment 163? Then I can autoland (once inbound re-opens).

	Randell Jesup [:jesup] (needinfo me)
	Comment 184 • 8 years ago

Comment on attachment 8761186 [details]
Bug 1213517 - Use NormalizedConstraints in low-level code.

https://reviewboard.mozilla.org/r/58496/#review61362

hard to really see the changes since reviewboard includes merged changes from the master... but looks good

	Randell Jesup [:jesup] (needinfo me)
	Comment 185 • 8 years ago

Comment on attachment 8761186 [details]
Bug 1213517 - Use NormalizedConstraints in low-level code.

https://reviewboard.mozilla.org/r/58496/#review61364

	Jan-Ivar Bruaroey [:jib] (needinfo? me) Assignee
	Comment 186 • 8 years ago

Comment on attachment 8689889 [details]
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25735/diff/6-7/