screencopy: Handle shm copy in commit event handler

This simplifies the implementation considerably.

include/wlr/types/wlr_screencopy_v1.h

@@ -52,7 +52,6 @@ struct wlr_screencopy_frame_v1 {
 	struct wl_listener buffer_destroy;
 
 	struct wlr_output *output;
-	struct wl_listener output_precommit;
 	struct wl_listener output_commit;
 	struct wl_listener output_destroy;
 	struct wl_listener output_enable;

types/wlr_screencopy_v1.c

@@ -21,7 +21,6 @@ struct screencopy_damage {
 	struct pixman_region32 damage;
 	struct wl_listener output_precommit;
 	struct wl_listener output_destroy;
-	uint32_t last_commit_seq;
 };
 
 static const struct zwlr_screencopy_frame_v1_interface frame_impl;
@@ -44,13 +43,6 @@ static void screencopy_damage_accumulate(struct screencopy_damage *damage) {
 	struct pixman_region32 *region = &damage->damage;
 	struct wlr_output *output = damage->output;
 
-	/* This check is done so damage that has been added and cleared in the
-	 * frame precommit handler is not added again after it has been handled.
-	 */
-	if (damage->last_commit_seq == output->commit_seq) {
-		return;
-	}
-
 	if (output->pending.committed & WLR_OUTPUT_STATE_DAMAGE) {
 		// If the compositor submitted damage, copy it over
 		pixman_region32_union(region, region, &output->pending.damage);
@@ -62,8 +54,6 @@ static void screencopy_damage_accumulate(struct screencopy_damage *damage) {
 		pixman_region32_union_rect(region, region, 0, 0,
 			output->width, output->height);
 	}
-
-	damage->last_commit_seq = output->commit_seq;
 }
 
 static void screencopy_damage_handle_output_precommit(
@@ -98,7 +88,6 @@ static struct screencopy_damage *screencopy_damage_create(
 	}
 
 	damage->output = output;
-	damage->last_commit_seq = output->commit_seq - 1;
 	pixman_region32_init_rect(&damage->damage, 0, 0, output->width,
 		output->height);
 	wl_list_insert(&client->damages, &damage->link);
@@ -154,7 +143,6 @@ static void frame_destroy(struct wlr_screencopy_frame_v1 *frame) {
 		}
 	}
 	wl_list_remove(&frame->link);
-	wl_list_remove(&frame->output_precommit.link);
 	wl_list_remove(&frame->output_commit.link);
 	wl_list_remove(&frame->output_destroy.link);
 	wl_list_remove(&frame->output_enable.link);
@@ -200,38 +188,13 @@ static void frame_send_ready(struct wlr_screencopy_frame_v1 *frame,
 		tv_sec_hi, tv_sec_lo, when->tv_nsec);
 }
 
-static void frame_handle_output_precommit(struct wl_listener *listener,
+static bool frame_shm_copy(struct wlr_screencopy_frame_v1 *frame,
-		void *_data) {
+		uint32_t *flags) {
-	struct wlr_screencopy_frame_v1 *frame =
+	struct wl_shm_buffer *shm_buffer = frame->shm_buffer;
-		wl_container_of(listener, frame, output_precommit);
-	struct wlr_output_event_precommit *event = _data;
 	struct wlr_output *output = frame->output;
 	struct wlr_renderer *renderer = wlr_backend_get_renderer(output->backend);
 	assert(renderer);
 
-	if (!(output->pending.committed & WLR_OUTPUT_STATE_BUFFER)) {
-		return;
-	}
-
-	struct wl_shm_buffer *shm_buffer = frame->shm_buffer;
-	if (shm_buffer == NULL) {
-		return;
-	}
-
-	if (frame->with_damage) {
-		struct screencopy_damage *damage =
-			screencopy_damage_get_or_create(frame->client, output);
-		if (damage) {
-			screencopy_damage_accumulate(damage);
-			if (!pixman_region32_not_empty(&damage->damage)) {
-				return;
-			}
-		}
-	}
-
-	wl_list_remove(&frame->output_precommit.link);
-	wl_list_init(&frame->output_precommit.link);
-
 	int x = frame->box.x;
 	int y = frame->box.y;
 
@@ -244,23 +207,16 @@ static void frame_handle_output_precommit(struct wl_listener *listener,
 	wl_shm_buffer_begin_access(shm_buffer);
 	void *data = wl_shm_buffer_get_data(shm_buffer);
 	uint32_t renderer_flags = 0;
-	bool ok = wlr_renderer_read_pixels(renderer, drm_format, &renderer_flags,
+	bool ok;
-		stride, width, height, x, y, 0, 0, data);
+	ok = wlr_renderer_begin_with_buffer(renderer, output->front_buffer);
-	uint32_t flags = renderer_flags & WLR_RENDERER_READ_PIXELS_Y_INVERT ?
+	ok = ok && wlr_renderer_read_pixels(renderer, drm_format,
+		&renderer_flags, stride, width, height, x, y, 0, 0, data);
+	wlr_renderer_end(renderer);
+	*flags = renderer_flags & WLR_RENDERER_READ_PIXELS_Y_INVERT ?
 		ZWLR_SCREENCOPY_FRAME_V1_FLAGS_Y_INVERT : 0;
 	wl_shm_buffer_end_access(shm_buffer);
 
-	if (!ok) {
+	return ok;
-		wlr_log(WLR_ERROR, "Failed to read pixels from renderer");
-		zwlr_screencopy_frame_v1_send_failed(frame->resource);
-		frame_destroy(frame);
-		return;
-	}
-
-	zwlr_screencopy_frame_v1_send_flags(frame->resource, flags);
-	frame_send_damage(frame);
-	frame_send_ready(frame, event->when);
-	frame_destroy(frame);
 }
 
 static bool blit_dmabuf(struct wlr_renderer *renderer,
@@ -298,6 +254,27 @@ error_src_tex:
 	return false;
 }
 
+static bool frame_dma_copy(struct wlr_screencopy_frame_v1 *frame,
+		uint32_t *flags) {
+	struct wlr_dmabuf_v1_buffer *dma_buffer = frame->dma_buffer;
+	struct wlr_output *output = frame->output;
+	struct wlr_renderer *renderer = wlr_backend_get_renderer(output->backend);
+	assert(renderer);
+
+	// TODO: add support for copying regions with DMA-BUFs
+	if (frame->box.x != 0 || frame->box.y != 0 ||
+			output->width != frame->box.width ||
+			output->height != frame->box.height) {
+		return false;
+	}
+
+	bool ok = blit_dmabuf(renderer, dma_buffer, output->front_buffer);
+	*flags = dma_buffer->attributes.flags & WLR_DMABUF_ATTRIBUTES_FLAGS_Y_INVERT ?
+		ZWLR_SCREENCOPY_FRAME_V1_FLAGS_Y_INVERT : 0;
+
+	return ok;
+}
+
 static void frame_handle_output_commit(struct wl_listener *listener,
 		void *data) {
 	struct wlr_screencopy_frame_v1 *frame =
@@ -305,14 +282,14 @@ static void frame_handle_output_commit(struct wl_listener *listener,
 	struct wlr_output_event_commit *event = data;
 	struct wlr_output *output = frame->output;
 	struct wlr_renderer *renderer = wlr_backend_get_renderer(output->backend);
+	uint32_t flags;
 	assert(renderer);
 
 	if (!(event->committed & WLR_OUTPUT_STATE_BUFFER)) {
 		return;
 	}
 
-	struct wlr_dmabuf_v1_buffer *dma_buffer = frame->dma_buffer;
+	if (!frame->shm_buffer && !frame->dma_buffer) {
-	if (dma_buffer == NULL) {
 		return;
 	}
 
@@ -327,20 +304,8 @@ static void frame_handle_output_commit(struct wl_listener *listener,
 	wl_list_remove(&frame->output_commit.link);
 	wl_list_init(&frame->output_commit.link);
 
-	// TODO: add support for copying regions with DMA-BUFs
+	bool ok = frame->shm_buffer ?
-	if (frame->box.x != 0 || frame->box.y != 0 ||
+		frame_shm_copy(frame, &flags) : frame_dma_copy(frame, &flags);
-			output->width != frame->box.width ||
-			output->height != frame->box.height) {
-		zwlr_screencopy_frame_v1_send_failed(frame->resource);
-		frame_destroy(frame);
-		return;
-	}
-
-	bool ok = output->front_buffer && blit_dmabuf(renderer, dma_buffer,
-			output->front_buffer);
-	uint32_t flags = dma_buffer->attributes.flags & WLR_DMABUF_ATTRIBUTES_FLAGS_Y_INVERT ?
-		ZWLR_SCREENCOPY_FRAME_V1_FLAGS_Y_INVERT : 0;
-
 	if (!ok) {
 		zwlr_screencopy_frame_v1_send_failed(frame->resource);
 		frame_destroy(frame);
@@ -457,8 +422,7 @@ static void frame_handle_copy(struct wl_client *wl_client,
 		return;
 	}
 
-	if (!wl_list_empty(&frame->output_precommit.link) ||
+	if (frame->shm_buffer != NULL || frame->dma_buffer != NULL) {
-			frame->shm_buffer != NULL || frame->dma_buffer != NULL) {
 		wl_resource_post_error(frame->resource,
 			ZWLR_SCREENCOPY_FRAME_V1_ERROR_ALREADY_USED,
 			"frame already used");
@@ -468,9 +432,6 @@ static void frame_handle_copy(struct wl_client *wl_client,
 	frame->shm_buffer = shm_buffer;
 	frame->dma_buffer = dma_buffer;
 
-	wl_signal_add(&output->events.precommit, &frame->output_precommit);
-	frame->output_precommit.notify = frame_handle_output_precommit;
-
 	wl_signal_add(&output->events.commit, &frame->output_commit);
 	frame->output_commit.notify = frame_handle_output_commit;
 
@@ -575,7 +536,6 @@ static void capture_output(struct wl_client *wl_client,
 
 	wl_list_insert(&client->manager->frames, &frame->link);
 
-	wl_list_init(&frame->output_precommit.link);
 	wl_list_init(&frame->output_commit.link);
 	wl_list_init(&frame->output_enable.link);
 	wl_list_init(&frame->output_destroy.link);