This MR fixes 2 issues:
- The frame clock time is sometimes drifting into the future (#2433 (closed)).
- Skips of single frames were not immediately handled.
Frame clock drift
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.
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
smoothest_frame_time = frame_time + frame_interval, by definition.
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):
reset_frame_time - smoothest_frame_time > -frame_interval
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
reset_frame_time - smoothest_frame_time > +frame_interval.
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):
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.