Closed Bug 1438134 Opened 7 years ago Closed 7 years ago

Failed applyConstraints may still change resolution

Categories

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

Product:
Core
Component:
WebRTC: Audio/Video
Version:
59 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- unaffected
firefox59 --- wontfix
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

(Keywords: regression)

Attachments

(4 files)

Bug 1438134 - Apply new capability only after stopping capture.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1438134 - Make the return value of MediaEngineSource::Reconfigure well defined.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1438134 - MozPromisify ApplyConstraints internals.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1438134 - Resolve ApplyConstraints and end track on unexpected error.
59 bytes, text/x-review-board-request
jib
: review+
Details
If we call applyConstraints on a camera's video track and restarting the camera fails, we will already have applied the new setting, and there's a low-risk race where a frame may end up at the new resolution even though applyConstraints() is rejected. [1] In beta 59 there's even one failure mode where we could end up applying the new resolution, reject the applyConstraints(), and leave the camera running. [2] [1] https://searchfox.org/mozilla-central/rev/d03ad8843e3bf2e856126bc53b0475c595e5183b/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#384,389 [2] https://dxr.mozilla.org/mozilla-beta/rev/7590a5f25ea3eab7a8bf1ed118f394f6a01ea335/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#392
Rank: 15
Priority: -- → P2
Keywords: regression
Comment on attachment 8950917 [details] Bug 1438134 - Apply new capability only after stopping capture. https://reviewboard.mozilla.org/r/220174/#review226328 I don't think this is the right fix. See comment. ::: dom/media/webrtc/MediaEngineRemoteVideoSource.cpp:383 (Diff revision 1) > + { > + MutexAutoLock lock(mMutex); > + // Start() applies mCapability on the device. > + mCapability = newCapability; > + } > > - rv = Start(nullptr); > + if (started) { > + nsresult rv = Start(nullptr); > if (NS_WARN_IF(NS_FAILED(rv))) { > return rv; If the goal is that failed applyConstraints must not change resolution, then this is insufficient. We still set constraints here if Start() fails. Should we set them back if Start() fails? If we set them back, should we attempt to restart the camera again with the old constraints, in case that works better, to not bail with a broken camera? The bigger problem here I think is the spec [1] has no accomodation for failing applyConstraints with an interrupted camera, and the only allowed error is OverconstrainedError. The applyConstraints algorithm doesn't actually talk about not breaking the camera at all, it's merely concerned with whether the constraints are valid or not. This makes sense, since applyConstraints is expected to be a no-foul failure (as if nothing happened). Given this, I think the right solution here may be to do what we do on unmute: If Start()ing (or Stop()ing)) the camera fails, let applyConstraints succeed, but end the track (firing the ended event). [1] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-constrainablepattern-applyconstraints
Attachment #8950917 - Flags: review?(jib) → review-
Fix optional for 59 per the triage team.
status-firefox59: affected → fix-optional
Comment on attachment 8950917 [details] Bug 1438134 - Apply new capability only after stopping capture. https://reviewboard.mozilla.org/r/220174/#review226328 > If the goal is that failed applyConstraints must not change resolution, then this is insufficient. > > We still set constraints here if Start() fails. Should we set them back if Start() fails? > > If we set them back, should we attempt to restart the camera again with the old constraints, in case that works better, to not bail with a broken camera? > > The bigger problem here I think is the spec [1] has no accomodation for failing applyConstraints with an interrupted camera, and the only allowed error is OverconstrainedError. > > The applyConstraints algorithm doesn't actually talk about not breaking the camera at all, it's merely concerned with whether the constraints are valid or not. This makes sense, since applyConstraints is expected to be a no-foul failure (as if nothing happened). > > Given this, I think the right solution here may be to do what we do on unmute: If Start()ing (or Stop()ing)) the camera fails, let applyConstraints succeed, but end the track (firing the ended event). > > [1] https://w3c.github.io/mediacapture-main/getusermedia.html#dom-constrainablepattern-applyconstraints I think this is irrelevant to this patch. After a failed reconfigure -- how would one make it start again? Perhaps it's possible with `track.enabled = false; track.enabled = true;`? What I'm trying to prevent is that a frame at the new resolution comes through before we have even tried to stop the device. That would look weird, especially if restarting the device later fails and renders it useless for some reason. We currently raise an InternalError if Reconfigure() fails due to another reason than a constraint. That's not what I'm challenging, but sure, I think you're right about that being the proper behavior. I can add a patch to remove that path and make it resolve the promise and end the track.
Comment on attachment 8953424 [details] Bug 1438134 - Make the return value of MediaEngineSource::Reconfigure well defined. https://reviewboard.mozilla.org/r/222656/#review228632 Code analysis found 1 defect in this patch: - 1 defect found by clang-tidy You can run this analysis locally with: - `./mach static-analysis check path/to/file.cpp` (C/C++) If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx ::: dom/media/webrtc/MediaEngineWebRTCAudio.cpp:250 (Diff revision 1) > - return ReevaluateAllocation(aHandle, &constraints, aPrefs, aDeviceId, > + nsresult rv = ReevaluateAllocation(aHandle, &constraints, aPrefs, aDeviceId, > - aOutBadConstraint); > + aOutBadConstraint); > + if (NS_FAILED(rv)) { > + if (aOutBadConstraint) { > + return NS_ERROR_INVALID_ARG; > + } else { Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]
Comment on attachment 8950917 [details] Bug 1438134 - Apply new capability only after stopping capture. https://reviewboard.mozilla.org/r/220174/#review232910 Agree it's marginally preferable to set constraints ass close as possible to Reconfigure(), to limit the possibility of (new) resize parameters getting out of sync with chosen device settings. r=me contingent on opening a new bug to address the arguably larger isssue that applyConstraints() probably shouldn't fail at all over operational errors like this, but instead succeed and fire ended on the track (the only fatal error the spec allows on this method is OverconstrainedError).
Attachment #8950917 - Flags: review?(jib) → review+
Comment on attachment 8953424 [details] Bug 1438134 - Make the return value of MediaEngineSource::Reconfigure well defined. https://reviewboard.mozilla.org/r/222656/#review232916
Attachment #8953424 - Flags: review?(jib) → review+
Comment on attachment 8953425 [details] Bug 1438134 - MozPromisify ApplyConstraints internals. https://reviewboard.mozilla.org/r/222658/#review232942 ::: dom/media/MediaManager.cpp:1262 (Diff revision 1) > - aConstraints, aCallerType); > + nsMainThreadPtrHandle<PledgeVoid> pledge( > + new nsMainThreadPtrHolder<PledgeVoid>( > + "LocalTrackSource::ApplyConstraints::ResultPledge", > + do_AddRef(p))); Shouldn't need nsMainThreadPtrHandle<> here, since `pledge` never leaves the main thread. ::: dom/media/MediaManager.cpp:1277 (Diff revision 1) > + return; > + } > + > + pledge->Resolve(false); > + }, > + [pledge, window = nsCOMPtr<nsPIDOMWindowInner>(aWindow), Perhaps the old code was overly cautious, but it didn't keep the window alive over the runnable, whereas this does. Can you show some precedence for this pattern? ::: dom/media/MediaManager.cpp:1278 (Diff revision 1) > + } > + > + pledge->Resolve(false); > + }, > + [pledge, window = nsCOMPtr<nsPIDOMWindowInner>(aWindow), > + listener = mListener, trackID = mTrackID] listener and trackID appear unused. Remove? ::: dom/media/MediaManager.cpp:4420 (Diff revision 1) > - if (rv == NS_ERROR_NOT_AVAILABLE && !badConstraint) { > + if (rv == NS_ERROR_INVALID_ARG && !badConstraint) { > nsTArray<RefPtr<MediaDevice>> devices; > devices.AppendElement(device); > badConstraint = MediaConstraintsHelper::SelectSettings( > NormalizedConstraints(c), devices, isChrome); > } > - NS_DispatchToMainThread(NewRunnableFrom([id, windowId, rv, > - badConstraint]() mutable { > + > + if (rv == NS_ERROR_INVALID_ARG) { Nit: combine ifs
Attachment #8953425 - Flags: review?(jib) → review+
Comment on attachment 8953426 [details] Bug 1438134 - Resolve ApplyConstraints and end track on unexpected error. https://reviewboard.mozilla.org/r/222660/#review233364 Lgtm.
Attachment #8953426 - Flags: review?(jib) → review+
Comment on attachment 8953425 [details] Bug 1438134 - MozPromisify ApplyConstraints internals. https://reviewboard.mozilla.org/r/222658/#review232942 > listener and trackID appear unused. Remove? Nm. Used in next patch.
See Also: → 1445891
Comment on attachment 8953425 [details] Bug 1438134 - MozPromisify ApplyConstraints internals. https://reviewboard.mozilla.org/r/222658/#review232942 > Perhaps the old code was overly cautious, but it didn't keep the window alive over the runnable, whereas this does. Can you show some precedence for this pattern? I'll make this a weak pointer instead, which gives us the old lifetime properties back.
(In reply to Jan-Ivar Bruaroey [:jib] (needinfo? me) from comment #11) > r=me contingent on opening a new bug to address the arguably larger isssue > that applyConstraints() probably shouldn't fail at all over operational > errors like this, but instead succeed and fire ended on the track (the only > fatal error the spec allows on this method is OverconstrainedError). Hang on, this is what the last patch already does. All jolly then, I'll close bug 1445891.
I don't think this is worth uplifting. It's pretty minor.
status-firefox59: fix-optional → wontfix
status-firefox60: affected → wontfix
status-firefox61: --- → affected
Pushed by pehrsons@gmail.com: https://hg.mozilla.org/integration/autoland/rev/162cbccb6cc8 Apply new capability only after stopping capture. r=jib https://hg.mozilla.org/integration/autoland/rev/44e300270943 Make the return value of MediaEngineSource::Reconfigure well defined. r=jib https://hg.mozilla.org/integration/autoland/rev/8138123384a6 MozPromisify ApplyConstraints internals. r=jib https://hg.mozilla.org/integration/autoland/rev/87e505c96a0a Resolve ApplyConstraints and end track on unexpected error. r=jib
Depends on: 1452031
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: