GNOME Bugzilla – Bug 787665
gdk_frame_clock_get_frame_time is irregular and causing stuttering
Last modified: 2018-02-15 08:20:14 UTC
gdk_frame_clock_get_frame_time is irregular and causing stuttering I noticed stuttering in gnome-maps, using the default CLUTTER_BACKEND=gdk where it relies on the GDK frame clock: "paint" g_get_monotonic_time +17278μs, gdk_frame_clock_get_frame_time +17278μs "paint" g_get_monotonic_time +17449μs, gdk_frame_clock_get_frame_time +17426μs "paint" g_get_monotonic_time +17620μs, gdk_frame_clock_get_frame_time +17600μs "paint" g_get_monotonic_time +17633μs, gdk_frame_clock_get_frame_time +17677μs "paint" g_get_monotonic_time +17230μs, gdk_frame_clock_get_frame_time +17192μs "paint" g_get_monotonic_time +17223μs, gdk_frame_clock_get_frame_time +17275μs "paint" g_get_monotonic_time +18004μs, gdk_frame_clock_get_frame_time +17962μs "paint" g_get_monotonic_time +17979μs, gdk_frame_clock_get_frame_time +17935μs "paint" g_get_monotonic_time +17617μs, gdk_frame_clock_get_frame_time +17708μs "paint" g_get_monotonic_time +16808μs, gdk_frame_clock_get_frame_time +16764μs "paint" g_get_monotonic_time +16840μs, gdk_frame_clock_get_frame_time +16860μs I also noticed that using CLUTTER_BACKEND=wayland has no such problem and is visibly smoother.
Created attachment 359778 [details] [review] Fix-irregular-gdk_frame_clock_get_frame_time.patch BEFORE gdkgears: 58 FPS and visibly stuttering gnome-maps: "paint" g_get_monotonic_time +17278μs, gdk_frame_clock_get_frame_time +17278μs "paint" g_get_monotonic_time +17449μs, gdk_frame_clock_get_frame_time +17426μs "paint" g_get_monotonic_time +17620μs, gdk_frame_clock_get_frame_time +17600μs "paint" g_get_monotonic_time +17633μs, gdk_frame_clock_get_frame_time +17677μs "paint" g_get_monotonic_time +17230μs, gdk_frame_clock_get_frame_time +17192μs "paint" g_get_monotonic_time +17223μs, gdk_frame_clock_get_frame_time +17275μs "paint" g_get_monotonic_time +18004μs, gdk_frame_clock_get_frame_time +17962μs "paint" g_get_monotonic_time +17979μs, gdk_frame_clock_get_frame_time +17935μs "paint" g_get_monotonic_time +17617μs, gdk_frame_clock_get_frame_time +17708μs "paint" g_get_monotonic_time +16808μs, gdk_frame_clock_get_frame_time +16764μs "paint" g_get_monotonic_time +16840μs, gdk_frame_clock_get_frame_time +16860μs AFTER gdkgears: 60 FPS and visibly smoother gnome-maps: "paint" g_get_monotonic_time +17938μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +15612μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +17740μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +15659μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +16490μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +17859μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +15660μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +16684μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +17739μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +15276μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +16927μs, gdk_frame_clock_get_frame_time +16667μs "paint" g_get_monotonic_time +17384μs, gdk_frame_clock_get_frame_time +16667μs
The frame clock is fed by the compositor, whereas the Clutter backend for Wayland uses a fixed internal 16.67 msec timer. The interface between Clutter, GDK, and Clutter-GTK is not extremely well-behaved; there is a lot of impedance mismatch as to when things need to be redrawn.
I know, I fixed it. Although I think I can do a better patch (coming soon). By some strange coincidence I was working on Mir frame clocks for the first part of this year so all these issues seem quite familiar.
Created attachment 359786 [details] [review] Fix-irregular-gdk_frame_clock_get_frame_time-v2.patch Tidied up some edge cases.
The simplest way to verify the fix is to run tests/gdkgears and you might see something like what I see: Before: 57-58 FPS and visibly stuttering After: 60 FPS and smooth
Created attachment 359839 [details] [review] Fix-irregular-gdk_frame_clock_get_frame_time-v3.patch v3: Fixed up use of the "current" frame timings. In v2 they were retrieved slightly too late after being zeroed, so the fallback refresh interval of 16667us was getting used. Now we get the more correct refresh interval from the previous frame where it's not zero (16680us for 59.95Hz).
Would be nice to get an opinion from Owen, since he wrote most of this code.
Review of attachment 359839 [details] [review]: ::: gdk/gdkframeclockidle.c @@ +352,3 @@ if (priv->freeze_count == 0) { + gint64 frame_interval = FRAME_INTERVAL; My main objection to this is that you're artificially synchronising to 60 fps, but there's no provision for displays that are refreshing at something that is not ~60 Hz. I mean: it works *in practice*, for most systems, but it's not really future-proof. Ideally, you'd want to keep track of the previous frame(s) timings and try to match up to those, instead of hardcoding 16667 microseconds. Ostensibly, we could even think about degrading the clock step to (adjusted) 45/30/15 fps if the previous frame took more than the budget allocated to it.
On Wayland we could also implement a FrameClock subclass using the presentation_time extension : http://ppaalanen.blogspot.com/2015/02/weston-repaint-scheduling.html
Emmanuele: That is incorrect. The FRAME_INTERVAL=16667 macro is only used as a fallback. Which is why it is already defined and used in the surrounding code - not something I introduced. The correct frame interval is indeed detected from the backend here: + if (prev_timings && prev_timings->refresh_interval) + frame_interval = prev_timings->refresh_interval; and you can see it's working here: gnome-maps on a 59.95Hz monitor: "paint" g_get_monotonic_time +18228μs, gdk_frame_clock_get_frame_time +16680μs "paint" g_get_monotonic_time +15010μs, gdk_frame_clock_get_frame_time +16680μs "paint" g_get_monotonic_time +17134μs, gdk_frame_clock_get_frame_time +16680μs As for downclocking to lower frame rates, that too is already implemented: + if (frame_time_error >= frame_interval) + priv->frame_time = reset_frame_time; So I agree with your concerns and but had already resolved them :) --- Lionel: Presentation time is already used. See 'compute_min_next_frame_time' in the same source file. And yes I'm familiar with the weston repaint scheduling as I implemented the same thing in Mir a few years ago: http://bazaar.launchpad.net/~mir-team/mir/development-branch/revision/2719 Although NVIDIA and John Carmack had the same idea too: https://www.khronos.org/registry/OpenGL/extensions/NV/GLX_NV_delay_before_swap.txt --- P.S. I will be travelling for the next week, so sorry in advance for the lack of communication.
(In reply to Daniel van Vugt from comment #10) > Lionel: > > Presentation time is already used. See 'compute_min_next_frame_time' in the > same source file. And yes I'm familiar with the weston repaint scheduling as > I implemented the same thing in Mir a few years ago: FWIW, neither mutter nor gtk+ implement the wp_presentation protocol: https://cgit.freedesktop.org/wayland/wayland-protocols/tree/stable/presentation-time/presentation-time.xml Which I think is what Lionel is talking about. Without this, all presentation times are entirely made up by the client.
I'm happy to not worry about discussing presentation time here. An accurate presentation time mostly just helps to tweak latency. It has nothing to do with smoothness, and smoothness is what this bug is about. Optimal smoothness only requires that you know the (maximum) refresh rate of the physical display with some precision, and you match it.
(In reply to Emmanuele Bassi (:ebassi) from comment #8) > it's not really future-proof. > > Ideally, you'd want to keep track of the previous frame(s) timings and try > to match up to those, instead of hardcoding 16667 microseconds. Additional notes (other than the fact that 16667 is not hardcoded): Actually future-proof is supporting dynamic frequency displays (GSync, FreeSync, and some anonymous laptop LCDs) and always trying to get animations up to the highest frequency of the display. In order to do that you need to aim for the reported (maximum) refresh rate of a monitor, which will often be a shorter interval than the difference between previous frames. So be careful to never measure frame interval as just the difference between presentation times. You could get stuck at a sub-optimal frame rate. > > Ostensibly, we could even think about degrading the clock step to (adjusted) > 45/30/15 fps if the previous frame took more than the budget allocated to it. Actually, for optimal smoothness you need to do the opposite. And the existing frame clock logic kind of has the right idea already. If the previous frame took more than one frame to render then the right answer is to start rendering the next one with zero delay. The goal then is to limit the number of consecutive missed frames to one, by starting on the next one earlier. Incidentally, this is why triple buffering is consistently smoother than double buffering, because it does just that if when the producer on the frame queue fails to keep up with the consumer (the display). Although it's possible to achieve the same catch-up algorithm without the constant lag of triple buffering, which is what we did in Mir.
gdkgears is still the simplest way to test the patch as you can see the difference in both animation smoothness and FPS numbers there. Also, here's another way to write the test results that might make more sense: BEFORE gdkgears: 58 FPS and visibly stuttering gnome-maps on a 59.95Hz monitor: "paint" gdk_frame_clock_get_frame_time +17278μs "paint" gdk_frame_clock_get_frame_time +17426μs "paint" gdk_frame_clock_get_frame_time +17600μs ^^^ misses about 5% of frames, or stutters 3 times per sec ^^^ AFTER gdkgears: 60 FPS and smoother gnome-maps on a 59.95Hz monitor: "paint" gdk_frame_clock_get_frame_time +16680μs "paint" gdk_frame_clock_get_frame_time +16680μs "paint" gdk_frame_clock_get_frame_time +16680μs ^^^ misses no frames ^^^
fwiw, I've asked Owen to review this, since the frameclock is mainly his work.
would it be okay to cherry-pick this to gtk3?
Please do. I developed the fix against gtk3 in the first place. Although I understand people wanting to be cautious and testing it in gtk4 for some time.
An ack from a GTK maintainer would be good - I'm happy to do the actual commit.
I think we can safely backport this too. There should not be much difference.
Pushed to gtk-3-22 too now
Thanks. It's worth remembering the difference between 58 FPS and 60 FPS... 58 FPS stutters twice every second 60 FPS never stutters (on a 60Hz display) When you think of it like that then it's easier to understand there's a significant visual improvement.