Implement MediaStreamTrack.getConstraints() + getSettings()

RESOLVED FIXED in Firefox 50

Status

()

defect
P1
normal
Rank:
18
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: jib, Assigned: jib)

Tracking

(Depends on 1 bug, Blocks 3 bugs, {dev-doc-complete, DevAdvocacy})

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(platform-rel ?, firefox44 affected, firefox50 fixed)

Details

(Whiteboard: [DevRel:P2], )

Attachments

(24 attachments)

40 bytes, text/x-review-board-request
smaug
: review+
Details
40 bytes, text/x-review-board-request
jesup
: review+
Details
40 bytes, text/x-review-board-request
jesup
: review+
Details
40 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
Details
58 bytes, text/x-review-board-request
padenot
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
58 bytes, text/x-review-board-request
jesup
: review+
Details
No description provided.
backlog: --- → webrtc/webaudio+
Rank: 21
Priority: -- → P2
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.
Keywords: DevAdvocacy
Assignee: nobody → jib
Depends on: 1226448
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().
Attachment #8689889 - Flags: review?(bugs)
Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.
Attachment #8689890 - Flags: review?(rjesup)
Bug 1213517 - make track.applyConstraints() store constraints on success.
Attachment #8689891 - Flags: review?(rjesup)
This fiddle shows getConstraints() working. getSettings() is next.

Note that the bloated dictionary output is a result of bug 1226475.
Comment on attachment 8689890 [details]
Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.

https://reviewboard.mozilla.org/r/25737/#review23171
Attachment #8689890 - Flags: review?(rjesup) → review+
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)
(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
Attachment #8689889 - Flags: review?(bugs) → review+
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
Attachment #8689891 - Flags: review?(rjesup) → review+
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/
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.
Attachment #8694859 - Flags: review?(rjesup)
Comment on attachment 8694859 [details]
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

https://reviewboard.mozilla.org/r/26905/#review24337
Attachment #8694859 - Flags: review?(rjesup) → review+
Rank: 21 → 18
Priority: P2 → P1
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/
Attachment #8689889 - Flags: review+ → review?(bugs)
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/
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/
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/
Comment on attachment 8689889 [details]
Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().

Just a rebase. Carrying forward r=smaug.
Attachment #8689889 - Flags: review?(bugs) → review+
Whiteboard: [DevRel:P2]
Review commit: https://reviewboard.mozilla.org/r/58490/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/58490/
Attachment #8689889 - Attachment description: MozReview Request: Bug 1213517 - add webidl for track.getConstraints() and track.getSettings(). → Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().
Attachment #8689890 - Attachment description: MozReview Request: Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies. → Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.
Attachment #8689891 - Attachment description: MozReview Request: Bug 1213517 - make track.applyConstraints() store constraints on success. → Bug 1213517 - make track.applyConstraints() store constraints on success.
Attachment #8694859 - Attachment description: MozReview Request: Bug 1213517 - make getUserMedia store initial constraints on resulting tracks. → Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.
Attachment #8761183 - Flags: review?(rjesup)
Attachment #8761184 - Flags: review?(rjesup)
Attachment #8761185 - Flags: review?(rjesup)
Attachment #8761186 - Flags: review?(rjesup)
Attachment #8761187 - Flags: review?(rjesup)
Attachment #8761188 - Flags: review?(rjesup)
Attachment #8689889 - Flags: review+ → review?(bugs)
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/
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/
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/
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/
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.
Depends on: 1088621
Comment on attachment 8761183 [details]
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().

https://reviewboard.mozilla.org/r/58490/#review55380
Attachment #8761183 - Flags: review?(rjesup) → review+
Attachment #8761184 - Flags: review?(rjesup) → review+
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)
Attachment #8761185 - Flags: review?(rjesup) → review+
Attachment #8689889 - Flags: review?(bugs) → review+
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.
> 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.
Added, just holding off on refresh for more reviews (need to reset smaug's r+ each time).
Attachment #8761187 - Flags: review?(rjesup) → review+
Comment on attachment 8761188 [details]
Bug 1213517 - Consider competing constraints in getUserMedia+applyConstraints.

https://reviewboard.mozilla.org/r/58500/#review55794
Attachment #8761188 - Flags: review?(rjesup) → review+
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?
Attachment #8761186 - Flags: review?(rjesup)
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).
Flags: platform-rel?
platform-rel: --- → ?
Review commit: https://reviewboard.mozilla.org/r/59652/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59652/
Attachment #8761188 - Attachment description: Consider competing constraints in getUserMedia+applyConstraints. → Bug 1213517 - Consider competing constraints in getUserMedia+applyConstraints.
Attachment #8761183 - Flags: review+
Attachment #8761184 - Flags: review+
Attachment #8761185 - Flags: review+
Attachment #8761187 - Flags: review+
Attachment #8761188 - Flags: review+
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/
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/
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/
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/
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/
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/
Comment on attachment 8761183 [details]
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().

Manually carrying forward r=jesup (see bug 1280858).
Attachment #8761183 - Flags: review+
Attachment #8761184 - Flags: review+
Attachment #8761185 - Flags: review+
Attachment #8761186 - Flags: review+
Attachment #8761187 - Flags: review+
Attachment #8761188 - Flags: review+
Works. Still need to make it work for audio, and then tackle bug 1088621 to test this stuff.

Looking for reviewer victims.
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
Flags: needinfo?(bugs)
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.
Attachment #8689889 - Flags: review+
Flags: needinfo?(bugs)
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.
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/
Attachment #8694859 - Flags: review+
Attachment #8761183 - Flags: review+
Attachment #8761184 - Flags: review+
Attachment #8761185 - Flags: review+
Attachment #8761186 - Flags: review+
Attachment #8761187 - Flags: review+
Attachment #8761188 - Flags: review+
Attachment #8763464 - Flags: review?(padenot)
Attachment #8763465 - Flags: review?(padenot)
Attachment #8763466 - Flags: review?(padenot)
Attachment #8763467 - Flags: review?(padenot)
Attachment #8763468 - Flags: review?(padenot)
Attachment #8763469 - Flags: review?(padenot)
Attachment #8763470 - Flags: review?(padenot)
Attachment #8763471 - Flags: review?(padenot)
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/1-2/
Comment on attachment 8694859 [details]
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.

Manually carrying forward r=jesup (see bug 1280858).
Attachment #8694859 - Flags: review+
Attachment #8761183 - Flags: review+
Attachment #8761184 - Flags: review+
Attachment #8761185 - Flags: review+
Attachment #8761186 - Flags: review+
Attachment #8761187 - Flags: review+
Attachment #8761188 - Flags: review+
Are there tests for this ?
Flags: needinfo?(jib)
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 ?
Attachment #8763464 - Flags: review?(padenot) → review+
Attachment #8763465 - Flags: review?(padenot)
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.
Attachment #8763467 - Flags: review?(padenot) → review+
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?
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.
Attachment #8763468 - Flags: review?(padenot)
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.
Attachment #8763469 - Flags: review?(padenot)
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.
Attachment #8763470 - Flags: review?(padenot)
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.
Attachment #8763471 - Flags: review?(padenot) → review+
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.
Attachment #8763466 - Flags: review?(padenot) → review+
This patch set would benefit from more splitting between functional changes and cleanups, and from more comments and descriptive commit messages, I think.
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.
Flags: needinfo?(jib)
See Also: → 1281866
Depends on: 1281866
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).
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.
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.
Review commit: https://reviewboard.mozilla.org/r/62102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62102/
Attachment #8767669 - Flags: review?(padenot)
Attachment #8689889 - Flags: review+
Attachment #8694859 - Flags: review+
Attachment #8761183 - Flags: review+
Attachment #8761184 - Flags: review+
Attachment #8761185 - Flags: review+
Attachment #8761186 - Flags: review+
Attachment #8761187 - Flags: review+
Attachment #8761188 - Flags: review+
Attachment #8763465 - Flags: review?(padenot)
Attachment #8763468 - Flags: review?(padenot)
Attachment #8763469 - Flags: review?(padenot)
Attachment #8763470 - Flags: review?(padenot)
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/2-3/
Attachment #8763465 - Flags: review?(padenot)
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
Comment on attachment 8767669 [details]
Bug 1213517 - optimize for maintenance of constraints (member pointer approach).

https://reviewboard.mozilla.org/r/62102/#review59040
Attachment #8767669 - Flags: review?(padenot) → review+
Attachment #8763468 - Flags: review?(padenot) → review+
Comment on attachment 8763468 [details]
Bug 1213517 - Only restart camera if net settings actually change.

https://reviewboard.mozilla.org/r/59660/#review59042
Attachment #8763469 - Flags: review?(padenot) → review+
Comment on attachment 8763469 [details]
Bug 1213517 - Consolidate camera Allocate's and Restart's constraints logic.

https://reviewboard.mozilla.org/r/59662/#review59044
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
Attachment #8763470 - Flags: review?(padenot) → review+
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/
Attachment #8763465 - Flags: review?(padenot)
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/
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/
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/
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/
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/
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/
Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

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

comment 112 through comment 115 all show the same change, while comment 116 through comment 119 are duds.
Comment on attachment 8763465 [details]
Bug 1213517 - Report correct constraint in OverconstrainedError when constraints conflict directly.

https://reviewboard.mozilla.org/r/59654/#review59352
Attachment #8763465 - Flags: review?(padenot) → review+
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.
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/4-5/
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/
Attachment #8763469 - Flags: review+
Attachment #8763470 - Flags: review+
Attachment #8763471 - Flags: review+
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/
Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/5-6/
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/
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/
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/
Gah! All this moz review noise! I just rebased the patches, and forgot one r? so it spammed twice. :(
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.
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.
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
Attachment #8768892 - Flags: review?(rjesup) → review+
Attachment #8768893 - Flags: review?(rjesup) → review+
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.
Attachment #8768894 - Flags: review?(rjesup) → review+
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/
Attachment #8761186 - Flags: review?(rjesup)
Attachment #8768892 - Flags: review+ → review?(rjesup)
Attachment #8768893 - Flags: review+ → review?(rjesup)
Attachment #8768894 - Flags: review+ → review?(rjesup)
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
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/
Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/5-6/
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/
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/
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/
Duplicate of this bug: 1213397
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.
Attachment #8768893 - Flags: review?(rjesup) → review+
Comment on attachment 8768892 [details]
Bug 1213517 - Make applyConstraints() and getSettings() do nothing after stop and shutdown.

https://reviewboard.mozilla.org/r/62894/#review61080
Attachment #8768892 - Flags: review?(rjesup) → review+
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.
Blocks: 1286945
Comment on attachment 8768894 [details]
Bug 1213517 - Take highest ideal value from competing width, height and frameRate.

https://reviewboard.mozilla.org/r/62898/#review61358
Attachment #8768894 - Flags: review?(rjesup) → review+
Thanks! Mind re-r+-ing comment 163? Then I can autoland (once inbound re-opens).
Flags: needinfo?(rjesup)
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
Attachment #8761186 - Flags: review?(rjesup)
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/
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/6-7/
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/7-8/
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/7-8/
Comment on attachment 8761183 [details]
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58490/diff/6-7/
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/6-7/
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/6-7/
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/6-7/
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/6-7/
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/6-7/
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/5-6/
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/6-7/
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/4-5/
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/6-7/
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/6-7/
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/6-7/
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/6-7/
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/6-7/
Comment on attachment 8763471 [details]
Bug 1213517 - Wire up getSettings().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59666/diff/6-7/
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/2-3/
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/2-3/
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/2-3/
Rebased.
Flags: needinfo?(rjesup)
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f22693a94979
add webidl for track.getConstraints() and track.getSettings(). r=smaug
https://hg.mozilla.org/integration/autoland/rev/3764e2fac765
split off MediaTaskUtils.h from MediaUtils.h to shed dependencies. r=jesup
https://hg.mozilla.org/integration/autoland/rev/015cb0829df5
make track.applyConstraints() store constraints on success. r=jesup
https://hg.mozilla.org/integration/autoland/rev/0fc93ec9ccd7
make getUserMedia store initial constraints on resulting tracks. r=jesup
https://hg.mozilla.org/integration/autoland/rev/6ed917c7d94d
Introduce AllocationHandle to MediaEngine::Allocate(). r=jesup
https://hg.mozilla.org/integration/autoland/rev/6ee4fac755f4
Normalize all the constraints internally, not just some. r=jesup
https://hg.mozilla.org/integration/autoland/rev/e6ac385fd969
Add an un-flattened NormalizedConstraints type for downstream use. r=jesup
https://hg.mozilla.org/integration/autoland/rev/05b370d75989
Use NormalizedConstraints in low-level code. r=jesup
https://hg.mozilla.org/integration/autoland/rev/9701bb4384d8
Add a way to merge multiple NormalizedConstraints. r=jesup
https://hg.mozilla.org/integration/autoland/rev/6f025025d259
Consider competing constraints in getUserMedia+applyConstraints. r=jesup
https://hg.mozilla.org/integration/autoland/rev/c0c0d3eb6dc9
Consider competing required constraints with OverconstrainedError. r=padenot
https://hg.mozilla.org/integration/autoland/rev/e4829b553f9e
Report correct constraint in OverconstrainedError when constraints conflict directly. r=padenot
https://hg.mozilla.org/integration/autoland/rev/97cfe28779d9
optimize for maintenance of constraints (member pointer approach). r=padenot
https://hg.mozilla.org/integration/autoland/rev/85832c026282
Normalize even more of the constraints code. r=padenot
https://hg.mozilla.org/integration/autoland/rev/abf811706099
Lift correct constraint out of lower-level code for OverconstrainedError. r=padenot
https://hg.mozilla.org/integration/autoland/rev/697fac643b8e
Only restart camera if net settings actually change. r=padenot
https://hg.mozilla.org/integration/autoland/rev/70f85193f925
Consolidate camera Allocate's and Restart's constraints logic. r=padenot
https://hg.mozilla.org/integration/autoland/rev/77b16f74919f
Let cam access in competing tabs get closer to their ideals when a tab closes. r=padenot
https://hg.mozilla.org/integration/autoland/rev/eca5b39db393
Wire up getSettings(). r=padenot
https://hg.mozilla.org/integration/autoland/rev/97d42349adb9
Make applyConstraints() and getSettings() do nothing after stop and shutdown. r=jesup
https://hg.mozilla.org/integration/autoland/rev/6f3e405706ef
Clamp competing ideal values before considering them to avoid outliers distorting result. r=jesup
https://hg.mozilla.org/integration/autoland/rev/93b1deb435f2
Take highest ideal value from competing width, height and frameRate. r=jesup
hi jib, do you have any idea what made the bustage after this push of autoland? e.g.
https://treeherder.mozilla.org/logviewer.html#?job_id=638206&repo=autoland#L5431
Flags: needinfo?(jib)
Depends on: 1287378
My fault for doing a conservative try run. :( This patch set is tripping up static analysis in two places.

I have two fixes for this in bug 1287378, which should take care of the problem if it hasn't been backed out yet.
Flags: needinfo?(jib)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5890c3d8ac67
Backed out changeset 93b1deb435f2 for Static Checking Build bustage
https://hg.mozilla.org/integration/autoland/rev/a2b508faaa89
Backed out changeset 6f3e405706ef 
https://hg.mozilla.org/integration/autoland/rev/b97a43e6a167
Backed out changeset 97d42349adb9 
https://hg.mozilla.org/integration/autoland/rev/391b528627d5
Backed out changeset eca5b39db393 
https://hg.mozilla.org/integration/autoland/rev/e7455e8591c1
Backed out changeset 77b16f74919f 
https://hg.mozilla.org/integration/autoland/rev/ca6e59681605
Backed out changeset 70f85193f925 
https://hg.mozilla.org/integration/autoland/rev/cee5abc65e94
Backed out changeset 697fac643b8e 
https://hg.mozilla.org/integration/autoland/rev/0067bcf60359
Backed out changeset abf811706099 
https://hg.mozilla.org/integration/autoland/rev/91beab01e785
Backed out changeset 85832c026282 
https://hg.mozilla.org/integration/autoland/rev/1adb5905697d
Backed out changeset 97cfe28779d9 
https://hg.mozilla.org/integration/autoland/rev/75e965332b72
Backed out changeset e4829b553f9e 
https://hg.mozilla.org/integration/autoland/rev/f925108ff4d3
Backed out changeset c0c0d3eb6dc9 
https://hg.mozilla.org/integration/autoland/rev/4e9cca9e0124
Backed out changeset 6f025025d259 
https://hg.mozilla.org/integration/autoland/rev/aa39f9c1df60
Backed out changeset 9701bb4384d8 
https://hg.mozilla.org/integration/autoland/rev/41fa5df5762f
Backed out changeset 05b370d75989 
https://hg.mozilla.org/integration/autoland/rev/c9f03cb41197
Backed out changeset e6ac385fd969 
https://hg.mozilla.org/integration/autoland/rev/ebe5efee4d5e
Backed out changeset 6ee4fac755f4 
https://hg.mozilla.org/integration/autoland/rev/d4b46fc0c418
Backed out changeset 6ed917c7d94d 
https://hg.mozilla.org/integration/autoland/rev/97bad3c58567
Backed out changeset 0fc93ec9ccd7 
https://hg.mozilla.org/integration/autoland/rev/d40be578a737
Backed out changeset 015cb0829df5 
https://hg.mozilla.org/integration/autoland/rev/c292ec33315d
Backed out changeset 3764e2fac765 
https://hg.mozilla.org/integration/autoland/rev/d2021311169e
Backed out changeset f22693a94979
oh...sorry! I didn't see your comment, so had backed out your changesets...:(
Duplicate of this bug: 1287378
Comment on attachment 8771954 [details]
Bug 1213517 - Fix static analysis for bug 1213517 landing: make constructor explicit.

https://reviewboard.mozilla.org/r/64916/#review61942
Attachment #8771954 - Flags: review?(rjesup) → review+
Attachment #8771955 - Flags: review?(rjesup) → review+
Comment on attachment 8771955 [details]
Bug 1213517 - Fix static analysis for bug 1213517 landing: Avoid non-memmovable nsTArray<NormalizedConstraintSet>.

https://reviewboard.mozilla.org/r/64918/#review61948
Pushed by jbruaroey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5f0e605aeb4f
add webidl for track.getConstraints() and track.getSettings(). r=smaug
https://hg.mozilla.org/integration/autoland/rev/2e20119e43d4
split off MediaTaskUtils.h from MediaUtils.h to shed dependencies. r=jesup
https://hg.mozilla.org/integration/autoland/rev/02335d85ca40
make track.applyConstraints() store constraints on success. r=jesup
https://hg.mozilla.org/integration/autoland/rev/4df85217957d
make getUserMedia store initial constraints on resulting tracks. r=jesup
https://hg.mozilla.org/integration/autoland/rev/87361f380664
Introduce AllocationHandle to MediaEngine::Allocate(). r=jesup
https://hg.mozilla.org/integration/autoland/rev/cadeeb107695
Normalize all the constraints internally, not just some. r=jesup
https://hg.mozilla.org/integration/autoland/rev/84fb1be97595
Add an un-flattened NormalizedConstraints type for downstream use. r=jesup
https://hg.mozilla.org/integration/autoland/rev/c613c94abee0
Use NormalizedConstraints in low-level code. r=jesup
https://hg.mozilla.org/integration/autoland/rev/cfc68e9868d9
Add a way to merge multiple NormalizedConstraints. r=jesup
https://hg.mozilla.org/integration/autoland/rev/dbf1866063ce
Consider competing constraints in getUserMedia+applyConstraints. r=jesup
https://hg.mozilla.org/integration/autoland/rev/de8a576b136d
Consider competing required constraints with OverconstrainedError. r=padenot
https://hg.mozilla.org/integration/autoland/rev/7be03e7ea909
Report correct constraint in OverconstrainedError when constraints conflict directly. r=padenot
https://hg.mozilla.org/integration/autoland/rev/c83da3566e98
optimize for maintenance of constraints (member pointer approach). r=padenot
https://hg.mozilla.org/integration/autoland/rev/e202052952d5
Normalize even more of the constraints code. r=padenot
https://hg.mozilla.org/integration/autoland/rev/3110045d432e
Lift correct constraint out of lower-level code for OverconstrainedError. r=padenot
https://hg.mozilla.org/integration/autoland/rev/2a924630566f
Only restart camera if net settings actually change. r=padenot
https://hg.mozilla.org/integration/autoland/rev/62c5705fc282
Consolidate camera Allocate's and Restart's constraints logic. r=padenot
https://hg.mozilla.org/integration/autoland/rev/bf9c34d7ecdf
Let cam access in competing tabs get closer to their ideals when a tab closes. r=padenot
https://hg.mozilla.org/integration/autoland/rev/80cae085c9a5
Wire up getSettings(). r=padenot
https://hg.mozilla.org/integration/autoland/rev/5eb669a837c9
Make applyConstraints() and getSettings() do nothing after stop and shutdown. r=jesup
https://hg.mozilla.org/integration/autoland/rev/e08e367db0f1
Clamp competing ideal values before considering them to avoid outliers distorting result. r=jesup
https://hg.mozilla.org/integration/autoland/rev/de5e58e85231
Take highest ideal value from competing width, height and frameRate. r=jesup
https://hg.mozilla.org/integration/autoland/rev/2c32d3cb63d8
Fix static analysis for bug 1213517 landing: make constructor explicit. r=jesup
https://hg.mozilla.org/integration/autoland/rev/de179cfc917a
Fix static analysis for bug 1213517 landing: Avoid non-memmovable nsTArray<NormalizedConstraintSet>. r=jesup
https://hg.mozilla.org/mozilla-central/rev/5f0e605aeb4f
https://hg.mozilla.org/mozilla-central/rev/2e20119e43d4
https://hg.mozilla.org/mozilla-central/rev/02335d85ca40
https://hg.mozilla.org/mozilla-central/rev/4df85217957d
https://hg.mozilla.org/mozilla-central/rev/87361f380664
https://hg.mozilla.org/mozilla-central/rev/cadeeb107695
https://hg.mozilla.org/mozilla-central/rev/84fb1be97595
https://hg.mozilla.org/mozilla-central/rev/c613c94abee0
https://hg.mozilla.org/mozilla-central/rev/cfc68e9868d9
https://hg.mozilla.org/mozilla-central/rev/dbf1866063ce
https://hg.mozilla.org/mozilla-central/rev/de8a576b136d
https://hg.mozilla.org/mozilla-central/rev/7be03e7ea909
https://hg.mozilla.org/mozilla-central/rev/c83da3566e98
https://hg.mozilla.org/mozilla-central/rev/e202052952d5
https://hg.mozilla.org/mozilla-central/rev/3110045d432e
https://hg.mozilla.org/mozilla-central/rev/2a924630566f
https://hg.mozilla.org/mozilla-central/rev/62c5705fc282
https://hg.mozilla.org/mozilla-central/rev/bf9c34d7ecdf
https://hg.mozilla.org/mozilla-central/rev/80cae085c9a5
https://hg.mozilla.org/mozilla-central/rev/5eb669a837c9
https://hg.mozilla.org/mozilla-central/rev/e08e367db0f1
https://hg.mozilla.org/mozilla-central/rev/de5e58e85231
https://hg.mozilla.org/mozilla-central/rev/2c32d3cb63d8
https://hg.mozilla.org/mozilla-central/rev/de179cfc917a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1287854
Blocks: 1088621
No longer depends on: 1088621
No longer blocks: 1088621
Depends on: 1288338
New pages:

https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSupportedConstraints
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings
https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/applyConstraints
https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/getCapabilities (not part of this bug; drafted by mistake but it should be reasonably useful)

Heavily updated so almost new:

https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/getConstraints
https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack/getSettings

Updated pages:

https://developer.mozilla.org/en-US/docs/Web/API/Media_Streams_API
https://developer.mozilla.org/en-US/docs/Web/API/MediaStream
https://developer.mozilla.org/en-US/docs/Web/API/MediaStreamTrack
https://developer.mozilla.org/en-US/Firefox/Releases/50

There’s still work to do here:

The docs of MediaTrackConstraints are very incomplete; I need to write out how each constraint is written, and so forth.

The pages detailing each individual property are not written yet. The summaries, however, should be a good start for most people. I will work on the detail pages very soon.

And I need to finish writing the guide material about how to work with constrainable properties in the main Media Capture and Streams page.

So I’m leaving this doc-needed for the moment; but I wanted to note progress while I work on another doc bug for a bit.
The guide content about how to work with constraints is about 75% done now. I’m taking a time-out from finishing this to get some work done on DOM and other general API doc updates. I’ll be back to this soon; ideally by mid-to-late next week.
I’ve now documented into our reference the types used to build constraints:

https://developer.mozilla.org/en-US/docs/Web/API/LongRange
https://developer.mozilla.org/en-US/docs/Web/API/ConstrainLong
https://developer.mozilla.org/en-US/docs/Web/API/DoubleRange
https://developer.mozilla.org/en-US/docs/Web/API/ConstrainDouble
https://developer.mozilla.org/en-US/docs/Web/API/ConstrainDOMString
https://developer.mozilla.org/en-US/docs/Web/API/ConstrainBoolean

I think I’m back into guide material next. There’s more reference to do yet, with the properties of MediaTrackConstraints and MediaTrackSettings still needing to be fully documented, but I think getting the guide material finished up will be the next key step.
Depends on: 1307630
No longer depends on: 1307630
Depends on: 1308605
Fixed assorted little things here and there, including improving descriptions of many things, updating some out-of-date names here and there, and added the following pages to the docs:

https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/width
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/height
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/channelCount
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/sampleRate
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/sampleSize
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/echoCancellation
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/facingMode
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/deviceId
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/groupId
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/aspectRatio
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/frameRate
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/volume
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackConstraints/latency

https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/echoCancellation
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/channelCount
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/sampleRate
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/sampleSize
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/height
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/width
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/volume
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/latency
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/aspectRatio
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/frameRate
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/facingMode
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/groupId
https://developer.mozilla.org/en-US/docs/Web/API/MediaTrackSettings/deviceId

With that, and a few just-completed tweaks to other pages previously done, I am putting these docs to bed. Please let me know if there are any errors. Ideally, eventually we will have some more in-place examples rather than generally linking to the one large example, but that will do for now.
Depends on: 1335005
Depends on: 1341409
You need to log in before you can comment on or make changes to this bug.