1454625 - A gUM-video's resolution settings may be incorrect

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

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

Comment 1

[Andreas Pehrson [:pehrsons]]

This is going to be ugly, but will have to do for 60. Bug 1453269 will be the proper fix.

Comment 2

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1454625 - Schedule settings update before setting mImage.

Review commit: https://reviewboard.mozilla.org/r/237248/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237248/

Comment 3

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1454625 - Don't update settings from constraints if already updated from frame.

Review commit: https://reviewboard.mozilla.org/r/237250/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/237250/

Comment 4

[Andreas Pehrson [:pehrsons]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/237250/diff/1-2/

Comment 5

[Paul Adenot (:padenot)]

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

https://reviewboard.mozilla.org/r/237248/#review243404

Comment 6

[Paul Adenot (:padenot)]

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.

Comment 7

[Andreas Pehrson [:pehrsons]]

(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

[Pulsebot]

Pushed by [e-mail]:
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

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/197abd266c8b
https://hg.mozilla.org/mozilla-central/rev/a3c07c7bfce3

Comment 10

[Andreas Pehrson [:pehrsons]]

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

Comment 11

[Julien Cristau [:jcristau]]

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

fix webrtc race, approved for 60.0b14

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/7e549c71e606
https://hg.mozilla.org/releases/mozilla-beta/rev/3d90a56e773b