1747857 - Partial present video poster glitch

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect, P3)

Description

[Darkspirit]

Attachment: 2021-12-29_04-43-38.mp4

Gnome Xwayland, Debian Testing, Intel

This seems to be an earlier partial present regression that was caused by bug 1704956 and not by bug 1706488 (bug 1712969).

STR: Hover the video shortly.
Expected: When unhovering the video, the video should no longer be visible. Only the image should be visible.
Actual: The glitch occurs at the top or at the bottom of the video player: A part of the video remains visible. It may even consist of multiple stripes with correct content in-between.

mozregression --good 2021-01-20 --bad 2021-04-20 --pref gfx.webrender.all:true -a https://www.dw.com/en/sudan-gold-mine-collapse-kills-dozens/a-60281945

31:54.34 INFO: Last good revision: 0639a51e1f164a99e5be8d056e6a9bfb29242c7a
31:54.34 INFO: First bad revision: 2df08c0b8938005b211e18fa85c7696a758cdc5c
31:54.34 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0639a51e1f164a99e5be8d056e6a9bfb29242c7a&tochange=2df08c0b8938005b211e18fa85c7696a758cdc5c

2df08c0b8938005b211e18fa85c7696a758cdc5c Glenn Watson — Bug 1704956 - Support non-opaque compositor surfaces. r=gfx-reviewers,lsalzman

Reproducible with:

• EGL (screencast): mozregression --launch 2021-12-28 --pref gfx.webrender.all:true gfx.x11-egl.force-enabled:true -a https://www.dw.com/en/sudan-gold-mine-collapse-kills-dozens/a-60281945
• GLX: mozregression --launch 2021-12-28 --pref gfx.webrender.all:true gfx.x11-egl.force-disabled:true -a https://www.dw.com/en/sudan-gold-mine-collapse-kills-dozens/a-60281945

GLX can be fixed with gfx.webrender.allow-partial-present-buffer-age:false or with gfx.webrender.max-partial-present-rects:0:

• mozregression --launch 2021-12-28 --pref gfx.webrender.all:true gfx.x11-egl.force-disabled:true gfx.webrender.allow-partial-present-buffer-age:false -a https://www.dw.com/en/sudan-gold-mine-collapse-kills-dozens/a-60281945
• mozregression --launch 2021-12-28 --pref gfx.webrender.all:true gfx.x11-egl.force-disabled:true gfx.webrender.max-partial-present-rects:0 -a https://www.dw.com/en/sudan-gold-mine-collapse-kills-dozens/a-60281945

In contrast to bug 1712969 and its duplicates, gfx.webrender.allow-partial-present-buffer-age:false seems to help a bit on EGL: The glitch occurs, but quickly disappears.
mozregression --launch 2021-12-28 --pref gfx.webrender.all:true gfx.x11-egl.force-enabled:true gfx.webrender.allow-partial-present-buffer-age:false -a https://www.dw.com/en/sudan-gold-mine-collapse-kills-dozens/a-60281945

gfx.webrender.max-partial-present-rects:0 fixes EGL:
mozregression --launch 2021-12-28 --pref gfx.webrender.all:true gfx.x11-egl.force-enabled:true gfx.webrender.max-partial-present-rects:0 -a https://www.dw.com/en/sudan-gold-mine-collapse-kills-dozens/a-60281945

Comment 1

[Glenn Watson [:gw]]

I can reproduce this locally, and see what is happening. When the video surface gets removed the calculated dirty rect that gets passed to the renderer doesn't include this information.

Comment 2

[Glenn Watson [:gw]]

Attachment: Bug 1747857 - Invalidate partial dirty rects when surface counts change

This is a sufficient fix to handle this case, we should definitely
invalidate the dirty rect validity if the compositor surface count
has changed.

There are likely other cases where the surface count remains the same
but we also need to invalidate dirty rect validity (we can handle
these as follow ups and/or as we encounter other cases).

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2984af6718ee
Invalidate partial dirty rects when surface counts change r=gfx-reviewers,aosmond

Comment 4

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2984af6718ee

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. Might be worth nominating for ESR also as it grafts cleanly there as well.

Comment 6

[Glenn Watson [:gw]]

Comment on attachment 9258255 [details]
Bug 1747857 - Invalidate partial dirty rects when surface counts change

Beta/Release Uplift Approval Request

• User impact if declined: Visual glitches in some cases
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: As per bug report
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Patch is small, shouldn't affect other code paths
• String changes made/needed:

Comment 7

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9258255 [details]
Bug 1747857 - Invalidate partial dirty rects when surface counts change

Approved for 97.0b3.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bedda2136312

Comment 9

[Monica Chiorean]

I was able to reproduce the issue on Linux 20.4 using build 97.0a1 (20211228034259) and link from description.
Verified as fixed on Linux 20.4 using 98.0a1 (20220112213002). Pending verification on Beta 97.0b3 once available.

Comment 11

[Monica Chiorean]

(In reply to Monica Chiorean from comment #9)

I was able to reproduce the issue on Linux 20.4 using build 97.0a1 (20211228034259) and link from description.
Verified as fixed on Linux 20.4 using 98.0a1 (20220112213002). Pending verification on Beta 97.0b3 once available.

Verified as fixed on Linux 20.4 using Beta 97.0b3(20220113185849).

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Is this worth nominating for ESR?

Comment 13

[Glenn Watson [:gw]]

Comment on attachment 9258255 [details]
Bug 1747857 - Invalidate partial dirty rects when surface counts change

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Small patch that grafts cleanly, fixes a visual glitch when playing videos on systems with partial present (linux, android, non-native compositors).
• User impact if declined: Visual glitches per above
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small patch, simple fix

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9258255 [details]
Bug 1747857 - Invalidate partial dirty rects when surface counts change

Approved for 91.7esr.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr91/rev/6bbe658037c7

Comment 16

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Hello! Verified on 91.7.0esr(20220228225400) on Linux x86_64(Ubuntu 20.04), works as expected.