core: don't call schedule_render too early

I mistakenly assumed that PresentCompleteNotify event signifies the end
of a vblank (or the start of scanout). But actually this event can in
theory in sent at any point during a vblank, with its timestamp pointing
to when the end of vblank is. (that's why we often find the timestamp to
be in the future).

Add a delay so schedule_render is actually called at the end of vblank,
so it doesn't mistakenly think the render is too slow to complete.

Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

src/common.h

@@ -147,6 +147,8 @@ typedef struct session {
 	ev_timer fade_timer;
 	/// Use an ev_timer callback for drawing
 	ev_timer draw_timer;
+	/// Timer for the end of each vblanks. Used for calling schedule_render.
+	ev_timer vblank_timer;
 	/// Called every time we have timeouts or new data on socket,
 	/// so we can be sure if xcb read from X socket at anytime during event
 	/// handling, we will not left any event unhandled in the queue

src/event.c

@@ -716,7 +716,8 @@ void ev_handle(session_t *ps, xcb_generic_event_t *ev) {
 	// XXX redraw needs to be more fine grained
 	queue_redraw(ps);
 
-	// the events sent from SendEvent will be ignored
+	// We intentionally ignore events sent via SendEvent. Those events has the 8th bit
+	// of response_type set, meaning they will match none of the cases below.
 	switch (ev->response_type) {
 	case FocusIn: ev_focus_in(ps, (xcb_focus_in_event_t *)ev); break;
 	case FocusOut: ev_focus_out(ps, (xcb_focus_out_event_t *)ev); break;

src/picom.c

@@ -1515,13 +1515,8 @@ handle_present_complete_notify(session_t *ps, xcb_present_complete_notify_event_
 
 	struct timespec now;
 	clock_gettime(CLOCK_MONOTONIC, &now);
-	uint64_t now_usec = (uint64_t)(now.tv_sec * 1000000 + now.tv_nsec / 1000);
+	auto now_us = (int64_t)(now.tv_sec * 1000000L + now.tv_nsec / 1000);
-	uint64_t drift;
+	auto drift = iabs((int64_t)cne->ust - now_us);
-	if (cne->ust > now_usec) {
-		drift = cne->ust - now_usec;
-	} else {
-		drift = now_usec - cne->ust;
-	}
 
 	if (ps->last_msc_instant != 0) {
 		auto frame_count = cne->msc - ps->last_msc;
@@ -1537,14 +1532,28 @@ handle_present_complete_notify(session_t *ps, xcb_present_complete_notify_event_
 		// align with the monotonic clock. If not, disable frame pacing because we
 		// can't schedule frames reliably.
 		log_error("Temporal anomaly detected, frame pacing disabled. (Are we "
-		          "running inside a time namespace?), %" PRIu64 " %" PRIu64,
+		          "running inside a time namespace?), %" PRIi64 " %" PRIu64,
-		          now_usec, ps->last_msc_instant);
+		          now_us, ps->last_msc_instant);
 		ps->frame_pacing = false;
 	}
 	ps->last_msc_instant = cne->ust;
 	ps->last_msc = cne->msc;
 	if (ps->redraw_needed) {
-		schedule_render(ps, true);
+		if (now_us > (int64_t)cne->ust) {
+			schedule_render(ps, true);
+		} else {
+			// Wait until the end of the current vblank to call
+			// schedule_render. If we call schedule_render too early, it can
+			// mistakenly think the render missed the vblank, and doesn't
+			// schedule render for the next vblank, causing frame drops.
+			log_trace("The end of this vblank is %" PRIi64 " us into the "
+			          "future",
+			          (int64_t)cne->ust - now_us);
+			assert(!ev_is_active(&ps->vblank_timer));
+			ev_timer_set(&ps->vblank_timer,
+			             ((double)cne->ust - (double)now_us) / 1000000.0, 0);
+			ev_timer_start(ps->loop, &ps->vblank_timer);
+		}
 	}
 }
 
@@ -1823,6 +1832,13 @@ static void draw_callback(EV_P_ ev_timer *w, int revents) {
 	}
 }
 
+static void schedule_render_callback(EV_P_ ev_timer *w, int revents attr_unused) {
+	session_t *ps = session_ptr(w, vblank_timer);
+	ev_timer_stop(EV_A_ w);
+
+	schedule_render(ps, true);
+}
+
 static void x_event_callback(EV_P attr_unused, ev_io *w, int revents attr_unused) {
 	session_t *ps = (session_t *)w;
 	xcb_generic_event_t *ev = xcb_poll_for_event(ps->c.c);
@@ -2419,6 +2435,7 @@ static session_t *session_init(int argc, char **argv, Display *dpy,
 	ev_io_start(ps->loop, &ps->xiow);
 	ev_init(&ps->unredir_timer, tmout_unredir_callback);
 	ev_init(&ps->draw_timer, draw_callback);
+	ev_init(&ps->vblank_timer, schedule_render_callback);
 
 	ev_init(&ps->fade_timer, fade_timer_callback);
 

src/utils.h

@@ -125,14 +125,22 @@ safe_isnan(double a) {
  * @param max maximum value
  * @return normalized value
  */
-static inline int attr_const normalize_i_range(int i, int min, int max) {
+static inline int attr_const attr_unused normalize_i_range(int i, int min, int max) {
-	if (i > max)
+	if (i > max) {
 		return max;
-	if (i < min)
+	}
+	if (i < min) {
 		return min;
+	}
 	return i;
 }
 
+/// Generic integer abs()
+#define iabs(val)                                                                        \
+	({                                                                               \
+		__auto_type __tmp = (val);                                               \
+		__tmp > 0 ? __tmp : -__tmp;                                              \
+	})
 #define min2(a, b) ((a) > (b) ? (b) : (a))
 #define max2(a, b) ((a) > (b) ? (a) : (b))
 #define min3(a, b, c) min2(a, min2(b, c))
@@ -149,10 +157,12 @@ static inline int attr_const normalize_i_range(int i, int min, int max) {
  * @return normalized value
  */
 static inline double attr_const normalize_d_range(double d, double min, double max) {
-	if (d > max)
+	if (d > max) {
 		return max;
-	if (d < min)
+	}
+	if (d < min) {
 		return min;
+	}
 	return d;
 }
 
@@ -162,7 +172,7 @@ static inline double attr_const normalize_d_range(double d, double min, double m
  * @param d double value to normalize
  * @return normalized value
  */
-static inline double attr_const normalize_d(double d) {
+static inline double attr_const attr_unused normalize_d(double d) {
 	return normalize_d_range(d, 0.0, 1.0);
 }