Bug 428499

Summary: Frame callbacks not send on 'empty' commits
Product: [Plasma] kwin Reporter: Robert Mader <robert.mader>
Component: wayland-genericAssignee: KWin default assignee <kwin-bugs-null>
Status: RESOLVED FIXED    
Severity: normal CC: argymeg, david, kde, nate, physkets, seqularise
Priority: HI Keywords: wayland
Version: unspecified   
Target Milestone: ---   
Platform: Other   
OS: Linux   
Latest Commit: Version Fixed In: 5.21
Sentry Crash Report:

Description Robert Mader 2020-10-31 10:33:21 UTC
SUMMARY
The Firefox Wayland backend uses a rather unconventional but, according to spec and with a clear consensus of Weston, Gnome and wlroots devs, valid method to use frame callbacks: it tries to emulate vsync by constantly requesting frame callbacks in a dedicated loop for the main surface - even though it may not trigger any repaints (i.e. doesn't attach new buffers nor submits any damage).

KWin currently gets stuck when this happens (as did other compositors until they got fixed :) )

STEPS TO REPRODUCE
1. start Firefox (preferably >=83) with the Wayland backend enabled (`MOZ_ENABLE_WAYLAND=1`)
2. enable `widget.wayland_vsync.enabled` (and preferably `gfx.webrender.all`) in `about:config`
3. restart the browser

OBSERVED RESULT
Firefox does not update its content

EXPECTED RESULT
Firefox should work pretty much as normal


SOFTWARE/OS VERSIONS
KDE Plasma Version: has been confirmed for up to 5.20


ADDITIONAL INFORMATION
Firefox bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1616894
Comment 1 Vlad Zahorodnii 2020-11-02 08:04:35 UTC
Thank you for reporting this issue. Indeed, currently, kwin won't repaint a window unless it's been damaged.

Also, if you discover a bug in kwin/wayland in the future, can you please use official channels to report it? It looks this issue has been known for almost half a year and we are made aware of it only now. :(
Comment 2 Robert Mader 2020-11-02 08:32:40 UTC
Yeah, the correct forwarding of bugs it an area for improvement :/ Now that I have an account here I'll try to do that more regularly.

For FF<->Gnome and FF<->wlroots it appears to work mostly by having some accidental overlap between devs who work on both projects. There is, however, a list of known KWin-only bugs at https://bugzilla.mozilla.org/show_bug.cgi?id=1609115 - maybe one of you finds time to go through them and check if there's still something actionable?

Concerning this bug I'd like to highlight the Mutter solution (https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1218) which decoupled frame callback emission from drawing all together. There's a list of scenarios where things can go wrong otherwise - for example if damage only happens in a part of a surface that's off-screen (see https://gitlab.gnome.org/GNOME/mutter/-/issues/817#note_738633). Just so you have that in mind when drafting a solution.
Comment 3 Robert Mader 2020-12-03 00:07:41 UTC
Some more notes here: we just enabled the frame callback based vsync emulation by default on the Wayland backend in Firefox 85. The Wayland backend is not enabled by default yet and 85 will only released in the end of January, however users explicitly enabling it and running nightly or soon beta will run into this issue. Therefore I personally am quite eager to see it resolved :)

The benefit of using frame callbacks are twofold: on one hand we can run at refresh rate on non-60Hz monitors, on the other we can stop rendering when the compositors stops sending callbacks, for example when the window is completely covered, reducing power consumption. So overall quite desirable.

I'm do not know how kwin handles callbacks, but I'd like to point out to how mutter initially handled this: https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/839/diffs?commit_id=d49d10b14f4e0fa80e6867979b26fab383610b39

While the solution is not optimal (and thus was later replaced with a more comprehensive solution mentioned above already), something similar might work for kwin as well - and in mutter it was only 4 lines.

Hoping you find time and energy to look into this soon, best regards
Comment 4 Vlad Zahorodnii 2020-12-15 10:07:44 UTC
Heh, in our cases, it will be a bit more involved. The compositor tries to avoid repainting as much as possible. In particular, if no damage has occurred, nothing will be repainted, no page flip will be scheduled, etc.

With the ongoing compositing timing rework, most of those issues will be addressed. So, it should be a rather trivial change.

@Robert I have one question though, does Firefox continuously creates frame callbacks (even if nothing has been changed) or only on demand?
Comment 5 Vlad Zahorodnii 2020-12-15 10:07:58 UTC
in our case*
Comment 6 Robert Mader 2020-12-15 10:31:10 UTC
> does Firefox continuously creates frame callbacks (even if nothing has been changed) or only on demand?

Not completely continuous, but always if FF *may* repaint soon.

> The compositor tries to avoid repainting as much as possible.

Yeah, that was also a motivation in Mutter to move frame callback handling completely outside of the paint code eventually (https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1218).

Totally agree that this is quite involving to really get right :/

FTR.: the FF patch was pushed back at least another train, so will not ride to release before 86 in February.
Comment 7 Bug Janitor Service 2021-02-02 13:21:45 UTC
A possibly relevant merge request was started @ https://invent.kde.org/plasma/kwin/-/merge_requests/654
Comment 8 Vlad Zahorodnii 2021-02-02 14:00:52 UTC
Git commit 798cfae321c9cfac3b99638fbaf5b0a61713cfaf by Vlad Zahorodnii.
Committed on 02/02/2021 at 14:00.
Pushed by vladz into branch 'master'.

Introduce SurfaceInterface::hasFrameCallbacks()

The new method can be used to check if the surface has a pending frame
callback.

M  +5    -0    src/server/surface_interface.cpp
M  +1    -0    src/server/surface_interface.h

https://invent.kde.org/plasma/kwayland-server/commit/798cfae321c9cfac3b99638fbaf5b0a61713cfaf
Comment 9 Vlad Zahorodnii 2021-02-02 14:01:42 UTC
Git commit 6f5e8e7a6a56b26592d47c0b85e8d86b4c765874 by Vlad Zahorodnii.
Committed on 02/02/2021 at 14:01.
Pushed by vladz into branch 'Plasma/5.21'.

Introduce SurfaceInterface::hasFrameCallbacks()

The new method can be used to check if the surface has a pending frame
callback.


(cherry picked from commit 798cfae321c9cfac3b99638fbaf5b0a61713cfaf)

M  +5    -0    src/server/surface_interface.cpp
M  +1    -0    src/server/surface_interface.h

https://invent.kde.org/plasma/kwayland-server/commit/6f5e8e7a6a56b26592d47c0b85e8d86b4c765874
Comment 10 Vlad Zahorodnii 2021-02-03 06:50:26 UTC
Git commit 72fda78cf612bd4fd82132f381e74bcad70c7be7 by Vlad Zahorodnii.
Committed on 03/02/2021 at 06:50.
Pushed by vladz into branch 'master'.

wayland: Schedule repaints even on empty commits

If only frame callbacks have been committed, the compositor must still
schedule compositing.

M  +28   -0    scene.cpp
M  +2    -0    scene.h
M  +4    -20   subsurfacemonitor.cpp
M  +1    -0    subsurfacemonitor.h
M  +6    -0    toplevel.cpp
M  +2    -0    toplevel.h

https://invent.kde.org/plasma/kwin/commit/72fda78cf612bd4fd82132f381e74bcad70c7be7
Comment 11 Vlad Zahorodnii 2021-02-03 06:51:01 UTC
Git commit 256245cd5004f0e199ffc5f04585c92f8399c895 by Vlad Zahorodnii.
Committed on 03/02/2021 at 06:50.
Pushed by vladz into branch 'Plasma/5.21'.

wayland: Schedule repaints even on empty commits

If only frame callbacks have been committed, the compositor must still
schedule compositing.


(cherry picked from commit 72fda78cf612bd4fd82132f381e74bcad70c7be7)

M  +28   -0    scene.cpp
M  +2    -0    scene.h
M  +4    -20   subsurfacemonitor.cpp
M  +1    -0    subsurfacemonitor.h
M  +6    -0    toplevel.cpp
M  +2    -0    toplevel.h

https://invent.kde.org/plasma/kwin/commit/256245cd5004f0e199ffc5f04585c92f8399c895
Comment 12 Robert Mader 2021-02-03 23:21:06 UTC
Great, thanks a lot!