Closed
Bug 1438134
Opened 7 years ago
Closed 7 years ago
Failed applyConstraints may still change resolution
Categories
(Core :: WebRTC: Audio/Video, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla61
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)
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
Assignee | ||
Updated•7 years ago
|
Rank: 15
Priority: -- → P2
Assignee | ||
Updated•7 years ago
|
Keywords: regression
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
mozreview-review |
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-
Comment 3•7 years ago
|
||
Fix optional for 59 per the triage team.
Assignee | ||
Comment 4•7 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•7 years ago
|
||
Comment 10•7 years ago
|
||
mozreview-review |
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 11•7 years ago
|
||
mozreview-review |
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 12•7 years ago
|
||
mozreview-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 13•7 years ago
|
||
mozreview-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 14•7 years ago
|
||
mozreview-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 15•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 16•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 17•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 22•7 years ago
|
||
I don't think this is worth uplifting. It's pretty minor.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 25•7 years ago
|
||
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
Comment 26•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/162cbccb6cc8
https://hg.mozilla.org/mozilla-central/rev/44e300270943
https://hg.mozilla.org/mozilla-central/rev/8138123384a6
https://hg.mozilla.org/mozilla-central/rev/87e505c96a0a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•