Failed applyConstraints may still change resolution

RESOLVED FIXED in Firefox 61

Status

()

defect
P2
normal
Rank:
15
RESOLVED FIXED
Last year
Last year

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

({regression})

59 Branch
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox58 unaffected, firefox59 wontfix, firefox60 wontfix, firefox61 fixed)

Details

Attachments

(4 attachments)

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

Last year
Rank: 15
Priority: -- → P2
Assignee

Updated

Last year
Keywords: regression
Comment hidden (mozreview-request)

Comment 2

Last year
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-
Fix optional for 59 per the triage team.
Assignee

Comment 4

Last year
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 10

Last year
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 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 15

Last year
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

Updated

Last year
See Also: → 1445891
Assignee

Comment 16

Last year
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.
(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)
I don't think this is worth uplifting. It's pretty minor.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 25

Last year
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
Assignee

Updated

Last year
Depends on: 1452031
You need to log in before you can comment on or make changes to this bug.