core: make sure we don't sleep on events, again

Once again we found ourselves going into sleep while there are X events
in libxcb's buffer.

This time it's because both vblank_handle_x_events and x_poll_for_events
can read from the X connection. This is introduced as part of commit
d617d0ba4f. Before that, we were only calling xcb_poll_for_queued_events
after vblank_handle_x_events.

By changing that to a xcb_poll_for_events (and later to
x_poll_for_events, which calls xcb_poll_for_events), we could read
additional vblank events into the buffer. These events will not be
handled because vblank_handle_x_events has already be called, so we will
be going into sleep with these events in the buffer. Since vblank events
driver the frame render loop, this causes indefinitely long delay
between frames.

Related-to: #1345 #1330
Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>

src/picom.c

@@ -1451,31 +1451,48 @@ static void unredirect(session_t *ps) {
 static void handle_x_events(struct session *ps) {
 	bool wm_was_consistent = wm_is_consistent(ps->wm);
 
-	if (ps->vblank_scheduler) {
-		vblank_handle_x_events(ps->vblank_scheduler);
+	while (true) {
+		// Flush because if we go into sleep when there is still requests
+		// in the outgoing buffer, they will not be sent for an indefinite
+		// amount of time. Use XFlush here too, we might still use some
+		// Xlib functions because OpenGL.
+		//
+		// Also note, `xcb_flush`/`XFlush` may _read_ more events from the server
+		// (yes, this is ridiculous, I know). But since we are polling for events
+		// in a loop, this should be fine - if we read events here, they will be
+		// handled below; and if some requests is sent later in this loop, which
+		// means some events must have been received, we will loop once more and
+		// get here to flush them.
+		XFlush(ps->c.dpy);
+		xcb_flush(ps->c.c);
+
+		// We have to check for vblank events (i.e. special xcb events) and normal
+		// events in a loop. This is because both `xcb_poll_for_event` and
+		// `xcb_poll_for_special_event` will read data from the X connection and
+		// put it in a buffer. So whichever one we call last, say
+		// `xcb_poll_for_special_event`, will read data into the buffer that might
+		// contain events that `xcb_poll_for_event` should handle, and vice versa.
+		// This causes us to go into sleep with events in the buffer.
+		//
+		// We have to keep calling both of them until neither of them return any
+		// events.
+		bool has_events = false;
+		if (ps->vblank_scheduler) {
+			has_events = vblank_handle_x_events(ps->vblank_scheduler) ==
+			             VBLANK_HANDLE_X_EVENTS_OK;
+		}
+
+		xcb_generic_event_t *ev;
+		while ((ev = x_poll_for_event(&ps->c))) {
+			has_events = true;
+			ev_handle(ps, (xcb_generic_event_t *)ev);
+			free(ev);
+		};
+
+		if (!has_events) {
+			break;
+		}
 	}
-
-	// Flush because if we go into sleep when there is still requests in the
-	// outgoing buffer, they will not be sent for an indefinite amount of
-	// time. Use XFlush here too, we might still use some Xlib functions
-	// because OpenGL.
-	//
-	// Also note, after we have flushed here, we won't flush again in this
-	// function before going into sleep. This is because `xcb_flush`/`XFlush`
-	// may _read_ more events from the server (yes, this is ridiculous, I
-	// know). And we can't have that, see the comments above this function.
-	//
-	// This means if functions called ev_handle need to send some events,
-	// they need to carefully make sure those events are flushed, one way or
-	// another.
-	XFlush(ps->c.dpy);
-	xcb_flush(ps->c.c);
-
-	xcb_generic_event_t *ev;
-	while ((ev = x_poll_for_event(&ps->c))) {
-		ev_handle(ps, (xcb_generic_event_t *)ev);
-		free(ev);
-	};
 	int err = xcb_connection_has_error(ps->c.c);
 	if (err) {
 		log_fatal("X11 server connection broke (error %d)", err);

src/vblank.c

@@ -69,7 +69,7 @@ struct vblank_scheduler_ops {
 	bool (*init)(struct vblank_scheduler *self);
 	void (*deinit)(struct vblank_scheduler *self);
 	bool (*schedule)(struct vblank_scheduler *self);
-	bool (*handle_x_events)(struct vblank_scheduler *self);
+	enum vblank_handle_x_events_result (*handle_x_events)(struct vblank_scheduler *self);
 };
 
 static void
@@ -444,10 +444,14 @@ static void handle_present_complete_notify(struct present_vblank_scheduler *self
 	ev_timer_start(self->base.loop, &self->callback_timer);
 }
 
-static bool handle_present_events(struct vblank_scheduler *base) {
+static enum vblank_handle_x_events_result
+handle_present_events(struct vblank_scheduler *base) {
 	auto self = (struct present_vblank_scheduler *)base;
 	xcb_present_generic_event_t *ev;
+	enum vblank_handle_x_events_result result = VBLANK_HANDLE_X_EVENTS_NO_EVENTS;
 	while ((ev = (void *)xcb_poll_for_special_event(base->c->c, self->event))) {
+		result = VBLANK_HANDLE_X_EVENTS_OK;
+
 		if (ev->event != self->event_id) {
 			// This event doesn't have the right event context, it's not meant
 			// for us.
@@ -460,7 +464,7 @@ static bool handle_present_events(struct vblank_scheduler *base) {
 	next:
 		free(ev);
 	}
-	return true;
+	return result;
 }
 
 static const struct vblank_scheduler_ops vblank_scheduler_ops[LAST_VBLANK_SCHEDULER] = {
@@ -571,11 +575,11 @@ vblank_scheduler_new(struct ev_loop *loop, struct x_connection *c, xcb_window_t
 	return self;
 }
 
-bool vblank_handle_x_events(struct vblank_scheduler *self) {
+enum vblank_handle_x_events_result vblank_handle_x_events(struct vblank_scheduler *self) {
 	assert(self->type < LAST_VBLANK_SCHEDULER);
 	auto fn = vblank_scheduler_ops[self->type].handle_x_events;
 	if (fn != NULL) {
 		return fn(self);
 	}
-	return true;
+	return VBLANK_HANDLE_X_EVENTS_NO_EVENTS;
 }

src/vblank.h

@@ -29,6 +29,12 @@ enum vblank_callback_action {
 	VBLANK_CALLBACK_DONE,
 };
 
+enum vblank_handle_x_events_result {
+	VBLANK_HANDLE_X_EVENTS_OK,
+	VBLANK_HANDLE_X_EVENTS_ERROR,
+	VBLANK_HANDLE_X_EVENTS_NO_EVENTS,
+};
+
 typedef enum vblank_callback_action (*vblank_callback_t)(struct vblank_event *event,
                                                          void *user_data);
 
@@ -47,4 +53,4 @@ vblank_scheduler_new(struct ev_loop *loop, struct x_connection *c, xcb_window_t
                      enum vblank_scheduler_type type, bool use_realtime_scheduling);
 void vblank_scheduler_free(struct vblank_scheduler *);
 
-bool vblank_handle_x_events(struct vblank_scheduler *self);
+enum vblank_handle_x_events_result vblank_handle_x_events(struct vblank_scheduler *self);