A gUM-video's resolution settings may be incorrect

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
RESOLVED FIXED
Last year
Last year

People

(Reporter: pehrsons, Assigned: pehrsons)

Tracking

({regression})

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

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60+ fixed, firefox61+ fixed)

Details

Attachments

(2 attachments)

In bug 1434946 I added back setting the framerate for a video track's getSettings(). The setter was dispatched to main thread after StartCapture() had finished [1], which meant that there is now a race with DeliverFrame() for setting the track's resolution settings [2]. DeliverFrame() may sometimes run earlier and dispatch another setter for the track's resolution settings based on the scaled resolution of the actual frame.

If DeliverFrame wins, we end up with the wrong resolution settings (given by constraints) until the next input resolution change, or the next constraints change. For the latter the race happens again.

These settings are js-observable. This was caught by the mochitest in bug 1450954 that got backed out.


[1] https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#329-331
[2] https://searchfox.org/mozilla-central/rev/a30a60eadcff99d2a043241955d215dbeccad876/dom/media/webrtc/MediaEngineRemoteVideoSource.cpp#638-639
Priority: -- → P1
Assignee

Comment 1

Last year
This is going to be ugly, but will have to do for 60. Bug 1453269 will be the proper fix.
See Also: → 1453269
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

Last year
mozreview-review
Comment on attachment 8968561 [details]
Bug 1454625 - Schedule settings update before setting mImage.

https://reviewboard.mozilla.org/r/237248/#review243404
Attachment #8968561 - Flags: review?(padenot) → review+

Comment 6

Last year
mozreview-review
Comment on attachment 8968562 [details]
Bug 1454625 - Don't update settings from constraints if already updated from frame.

https://reviewboard.mozilla.org/r/237250/#review243408

This is crazy but will work.
Attachment #8968562 - Flags: review?(padenot) → review+
Assignee

Comment 7

Last year
(In reply to Paul Adenot (:padenot) from comment #6)
> Comment on attachment 8968562 [details]
> Bug 1454625 - Don't update settings from constraints if already updated from
> frame.
> 
> https://reviewboard.mozilla.org/r/237250/#review243408
> 
> This is crazy but will work.

I'm not particularly happy about it either, but after a couple of dirtier hacks this is the cleanest I came up with.

Comment 8

Last year
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/197abd266c8b
Schedule settings update before setting mImage. r=padenot
https://hg.mozilla.org/integration/autoland/rev/a3c07c7bfce3
Don't update settings from constraints if already updated from frame. r=padenot

Comment 9

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/197abd266c8b
https://hg.mozilla.org/mozilla-central/rev/a3c07c7bfce3
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8968561 [details]
Bug 1454625 - Schedule settings update before setting mImage.

This approval request applies to both patches on this bug

Approval Request Comment
[Feature/Bug causing the regression]: bug 1434946
[User impact if declined]: js observable regression in screen capture track settings
[Is this code covered by automated tests?]: yes, the test on bug 1450954 will catch this
[Has the fix been verified in Nightly?]: No, but this made the test from bug 1450954 green on try
[Needs manual test from QE? If yes, steps to reproduce]: No, the symptom is intermittent and doesn't repro reliably
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Simple and safe change
[String changes made/needed]: None
Attachment #8968561 - Flags: approval-mozilla-beta?
Comment on attachment 8968561 [details]
Bug 1454625 - Schedule settings update before setting mImage.

fix webrtc race, approved for 60.0b14
Attachment #8968561 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8968562 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.