[GDK3] GdkFrameClockIdle: fix frame time calculation

TLDR

This MR fixes 2 issues:

1. The frame clock time is sometimes drifting into the future (#2433 (closed)).
2. Skips of single frames were not immediately handled.

Frame clock drift

Background

In order to maintain smooth animations, the delta between the frame clock time and g_get_monotonic_time() should stay more or less constant. Furthermore, with the current codebase, if this delta becomes too big it can cause various issues.

One such issue, related to the frame clock drifting into the future, was #1612 (closed). In this case the GtkProgressBar animation stopped entirely. It was fixed by !731 (merged). However the fix turned out not to be complete, as the author of !731 (merged) himself suspected. Even with !731 (merged), the frame clock time can still sometimes drift into the future.

Description

If gdk_frame_clock_paint_idle() is called more than once per frame on average, frame_time - the value returned by gdk_frame_clock_get_frame_time() - would be advanced too fast and drift into the future.

Conversely, if gdk_frame_clock_paint_idle() is called less than once per frame on average, this would lead to frame_time advancing too slowly and drifting into the past. This can happen, for example, when one or more frames are skipped due to a compositor stall.

The original fix by @vanvugt (commit c6901a8b) was probably meant to capture both "too slow" as well as "too fast" frame clock time, however it only caught situations where the frame clock was falling behind the current time.

This MR handles a frame clock time drift in both directions. This is done by comparing the "smoothest" frame_time with the current time rather than with the reset time.

Explanation of the fix

This is the code before this MR, taken from the function gdk_frame_clock_paint_idle():

smoothest_frame_time = priv->frame_time + frame_interval;
reset_frame_time = compute_frame_time (clock_idle);
frame_time_error = ABS (reset_frame_time - smoothest_frame_time);
if (frame_time_error >= frame_interval)
  priv->frame_time = reset_frame_time;
else
  priv->frame_time = smoothest_frame_time;

We can see that

1. smoothest_frame_time = frame_time + frame_interval, by definition.
2. reset_frame_time > frame_time, because compute_frame_time() returns a value that is guaranteed to be bigger than frame_time.

Subtracting (1) from (2) we get that the following is always true (notice the - before frame_interval):

3. reset_frame_time - smoothest_frame_time > -frame_interval

Since frame_time_error is set to the absolute value of the left side of (3), it follows that the condition of the if statement - i.e. frame_time_error >= frame_interval - will only be true if

4. reset_frame_time - smoothest_frame_time > +frame_interval.

So the ABS() function didn't do anything here. Furthermore, in that case reset_frame_time must be bigger than smoothest_frame_time, which is itself bigger than frame_time by definition. In such a situation compute_frame_time() simply returns the current time. So here reset_frame_time is equal to the current time. We can now rephrase equation (4):

5. current time > smoothest_frame_time + frame_interval.

Equation (5) means that the condition in the original code is true only when smoothest_frame_time is more than a frame_interval in the past. That means that the original condition only caught "too slow" frame clock time.

The new code compares smoothest_frame_time with the current time rather than with reset_frame_time. That way both 'too slow' as well as 'too fast' frame clock time is handled. This also seems more in line with the intent of the original fix, since the usage of ABS() is now meaningful.

Handling skipped frames

A single frame skip is unfortunately relatively common. It can happen when the client doesn't receive a frame callback for a certain frame (because of a compositor stall or whatever), it might happen if rendering a frame takes too much time, and there might be other reasons. When a frame is skipped, gdk_frame_clock_paint_idle() is not called and so the frame clock time is not updated. That means that frame_time is falling behind the current time with every skipped frame.

The original fix from commit c6901a8b handled such situations. However sometimes it failed to detect single frame skips right away. To understand why, consider that the frame clock callback is not called exactly at frame intervals. A little less, or a little more, than an exact multiple of a full display frame interval might pass between consecutive calls.

Because of that, when we skip a single frame, frame_time_error - the delta between the smoothest_frame_time and the current time - is sometimes a little less than a full frame interval. That means that the original condition, which compares frame_time_error with a full frame interval, might sometimes not immediately detect a frame skip. Rather it would detect the skip a few frames later, when frame_time_error finally happens to be a little more than a frame interval.

The following log snippet explains the situation. A frame skip occurred on the line from 23:53:44.707, however the deltas were smaller than the frame interval (which is 16667 microseconds) until the line from 23:53:44.774, where the skip was finally detected and handled.

Gdk-Message: 23:53:44.607: current_time - smoothest_frame_time -513
Gdk-Message: 23:53:44.624: current_time - smoothest_frame_time -479
Gdk-Message: 23:53:44.640: current_time - smoothest_frame_time -511
Gdk-Message: 23:53:44.657: current_time - smoothest_frame_time -151
Gdk-Message: 23:53:44.674: current_time - smoothest_frame_time -272
Gdk-Message: 23:53:44.707: current_time - smoothest_frame_time 16475  # A frame was skipped here
Gdk-Message: 23:53:44.724: current_time - smoothest_frame_time 16347
Gdk-Message: 23:53:44.740: current_time - smoothest_frame_time 16358
Gdk-Message: 23:53:44.757: current_time - smoothest_frame_time 16417
Gdk-Message: 23:53:44.774: current_time - smoothest_frame_time 16680  # The above skip was only detected here
Gdk-Message: 23:53:44.774: *** RESET *** frame < current
Gdk-Message: 23:53:44.790: current_time - smoothest_frame_time -542
Gdk-Message: 23:53:44.807: current_time - smoothest_frame_time -771
Gdk-Message: 23:53:44.824: current_time - smoothest_frame_time -343
Gdk-Message: 23:53:44.840: current_time - smoothest_frame_time -773
Gdk-Message: 23:53:44.856: current_time - smoothest_frame_time -1035

In such a situation, when a frame was skipped we sometimes got 2 jank frames: 1 in the original frame skip, and the other when we finally detect the skip and reset the frame time. With this fix we'd only get a single jank frame (the original frame skip).

The following log snippet demonstrates the fix. A frame skip occurred on the line from 13:46:03.593. The skip was detected and handled immediately, so only a single jank frame occurred.

Gdk-Message: 13:46:03.509: current_time - smoothest_frame_time -241
Gdk-Message: 13:46:03.526: current_time - smoothest_frame_time 246
Gdk-Message: 13:46:03.543: current_time - smoothest_frame_time 317
Gdk-Message: 13:46:03.560: current_time - smoothest_frame_time 1218
Gdk-Message: 13:46:03.593: current_time - smoothest_frame_time 17552 # A frame was skipped and handled immediately
Gdk-Message: 13:46:03.593: *** RESET *** frame < current
Gdk-Message: 13:46:03.610: current_time - smoothest_frame_time -73
Gdk-Message: 13:46:03.626: current_time - smoothest_frame_time -360
Gdk-Message: 13:46:03.643: current_time - smoothest_frame_time 115

In order to detect this scenario, the new code resets the frame time when the delta between 'smoothest_frame_time` and the current time is more than half a frame interval, as opposed to more than a full frame interval.