Closed Bug 1213517 Opened 9 years ago Closed 9 years ago

Implement MediaStreamTrack.getConstraints() + getSettings()

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
platform-rel --- ?
firefox44 --- affected
firefox50 --- fixed
Blocking Flags:
backlog webrtc/webaudio+

People

(Reporter: jib, Assigned: jib)

References

(Blocks 3 open bugs,
URL
)

Details

(Keywords: dev-doc-complete, DevAdvocacy, Whiteboard: [DevRel:P2])

Attachments

(24 files)

Bug 1213517 - add webidl for track.getConstraints() and track.getSettings().
40 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1213517 - split off MediaTaskUtils.h from MediaUtils.h to shed dependencies.
40 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1213517 - make track.applyConstraints() store constraints on success.
40 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1213517 - make getUserMedia store initial constraints on resulting tracks.
40 bytes, text/x-review-board-request
Details
Bug 1213517 - Introduce AllocationHandle to MediaEngine::Allocate().
58 bytes, text/x-review-board-request
Details
Bug 1213517 - Normalize all the constraints internally, not just some.
58 bytes, text/x-review-board-request
Details
Bug 1213517 - Add an un-flattened NormalizedConstraints type for downstream use.
58 bytes, text/x-review-board-request
Details
Bug 1213517 - Use NormalizedConstraints in low-level code.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1213517 - Add a way to merge multiple NormalizedConstraints.
58 bytes, text/x-review-board-request
Details
Bug 1213517 - Consider competing constraints in getUserMedia+applyConstraints.
58 bytes, text/x-review-board-request
Details
Bug 1213517 - Consider competing required constraints with OverconstrainedError.
58 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1213517 - Report correct constraint in OverconstrainedError when constraints conflict directly.
58 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1213517 - Normalize even more of the constraints code.
58 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1213517 - Lift correct constraint out of lower-level code for OverconstrainedError.
58 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1213517 - Only restart camera if net settings actually change.
58 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1213517 - Consolidate camera Allocate's and Restart's constraints logic.
58 bytes, text/x-review-board-request
Details
Bug 1213517 - Let cam access in competing tabs get closer to their ideals when a tab closes.
58 bytes, text/x-review-board-request
Details
Bug 1213517 - Wire up getSettings().
58 bytes, text/x-review-board-request
Details
Bug 1213517 - optimize for maintenance of constraints (member pointer approach).
58 bytes, text/x-review-board-request
padenot
: review+
Details
Bug 1213517 - Make applyConstraints() and getSettings() do nothing after stop and shutdown.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1213517 - Clamp competing ideal values before considering them to avoid outliers distorting result.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1213517 - Take highest ideal value from competing width, height and frameRate.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1213517 - Fix static analysis for bug 1213517 landing: make constructor explicit.
58 bytes, text/x-review-board-request
jesup
: review+
Details
Bug 1213517 - Fix static analysis for bug 1213517 landing: Avoid non-memmovable nsTArray<NormalizedConstraintSet>.
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.
URL: https://jsfiddle.net/7udgdbbu/
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/
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...:(
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: 9 years ago
status-firefox50: --- → fixed
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.
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 1335005
Depends on: 1341409
Depends on: 1436104

MediaStreamTrack.getSettings() recently (around Firefox 91.0) stopped returning width and height for video tracks. This data can be obtained from the element's videoWidth and videoHeight fields, however other browsers also return it reliably on getSettings() and Firefox used to.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: