Closed Bug 1454625 Opened 6 years ago Closed 6 years ago

A gUM-video's resolution settings may be incorrect

Categories

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

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + fixed
firefox61 + fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

(Keywords: regression)

Attachments

(2 files)

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
This is going to be ugly, but will have to do for 60. Bug 1453269 will be the proper fix.
See Also: → 1453269
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 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+
(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.
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
https://hg.mozilla.org/mozilla-central/rev/197abd266c8b
https://hg.mozilla.org/mozilla-central/rev/a3c07c7bfce3
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
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.