From 5f6b9e5620b8a8e9acaf97fca45aaa36b88747b5 Mon Sep 17 00:00:00 2001 From: Simon Ser Date: Wed, 6 Mar 2024 16:48:33 +0100 Subject: [PATCH] Replace wlr_client_buffer with wlr_surface_texture wlr_client_buffer has a bad design: just because it needs some ref'counting functionality, it's implemented as a wlr_buffer. Its purpose is to allow compositors to obtain a frozen wlr_texture to keep painting an old representation of the surface for an arbitrary amount of time (e.g. during layout reconfigurations). However a wlr_buffer lock means something else entirely, and abusing it for ref'counting a texture leads to various issues. In particular, wlr_surface checks the wlr_client_buffer lock count to decide whether to mutate a texture on commit. This falls apart if the buffer is locked for another purpose than freezing a surface texture (e.g. locked by another wlroots helper). Additionally, wlr_client_buffer forces backends to re-import and re-check the buffer each time the client commits, since DMA-BUF wlr_client_buffers are destroyed and re-created on each commit. wlr_client_buffer also doesn't properly forward all wlr_buffer operations to the source buffer: some are unimplemented, and the source buffer is not locked by wlr_client_buffer thus may be destroyed anytime. To fix all of this mess, introduce a new struct wlr_surface_texture whose purpose is just to track whether a wlr_texture is frozen. Compositors can lock the texture to freeze it. The wlr_buffer can be accessed from wlr_surface_texture.buffer, or at surface commit time from wlr_surface.current.buffer. --- include/wlr/types/wlr_compositor.h | 24 +++++-- types/scene/surface.c | 35 ++-------- types/wlr_compositor.c | 101 +++++++++++++++++++++++------ 3 files changed, 107 insertions(+), 53 deletions(-) diff --git a/include/wlr/types/wlr_compositor.h b/include/wlr/types/wlr_compositor.h index dc95b3037..b3ae0b488 100644 --- a/include/wlr/types/wlr_compositor.h +++ b/include/wlr/types/wlr_compositor.h @@ -119,16 +119,29 @@ struct wlr_surface_output { struct wl_listener destroy; }; +struct wlr_surface_texture { + struct wlr_texture *texture; + // The buffer this surface texture was created from. NULL if released. + struct wlr_buffer *buffer; + // True if the texture won't be mutated by client surface commits. + bool locked; + + // private state + + struct wl_listener buffer_release; + bool dropped; +}; + struct wlr_surface { struct wl_resource *resource; struct wlr_renderer *renderer; // may be NULL /** - * The surface's buffer, if any. A surface has an attached buffer when it + * The surface's texture, if any. A surface has an attached buffer when it * commits with a non-null buffer in its pending state. A surface will not - * have a buffer if it has never committed one, has committed a null buffer, - * or something went wrong with uploading the buffer. + * have a texture if it has never committed one, has committed a null + * buffer, or something went wrong with uploading the buffer. */ - struct wlr_client_buffer *buffer; + struct wlr_surface_texture *texture; /** * The last commit's buffer damage, in buffer-local coordinates. This * contains both the damage accumulated by the client via @@ -257,6 +270,9 @@ struct wlr_compositor { typedef void (*wlr_surface_iterator_func_t)(struct wlr_surface *surface, int sx, int sy, void *data); +void wlr_surface_texture_lock(struct wlr_surface_texture *surf_tex); +void wlr_surface_texture_unlock(struct wlr_surface_texture *surf_tex); + /** * Set the lifetime role for this surface. * diff --git a/types/scene/surface.c b/types/scene/surface.c index 11396e2fd..9567ee4bb 100644 --- a/types/scene/surface.c +++ b/types/scene/surface.c @@ -72,28 +72,6 @@ static void scene_surface_handle_surface_destroy( wlr_scene_node_destroy(&surface->buffer->node); } -// This is used for wlr_scene where it unconditionally locks buffers preventing -// reuse of the existing texture for shm clients. With the usage pattern of -// wlr_scene surface handling, we can mark its locked buffer as safe -// for mutation. -static void client_buffer_mark_next_can_damage(struct wlr_client_buffer *buffer) { - buffer->n_ignore_locks++; -} - -static void scene_buffer_unmark_client_buffer(struct wlr_scene_buffer *scene_buffer) { - if (!scene_buffer->buffer) { - return; - } - - struct wlr_client_buffer *buffer = wlr_client_buffer_get(scene_buffer->buffer); - if (!buffer) { - return; - } - - assert(buffer->n_ignore_locks > 0); - buffer->n_ignore_locks--; -} - static int min(int a, int b) { return a < b ? a : b; } @@ -148,13 +126,10 @@ static void surface_reconfigure(struct wlr_scene_surface *scene_surface) { wlr_scene_buffer_set_dest_size(scene_buffer, width, height); wlr_scene_buffer_set_transform(scene_buffer, state->transform); - scene_buffer_unmark_client_buffer(scene_buffer); - - if (surface->buffer) { - client_buffer_mark_next_can_damage(surface->buffer); - - wlr_scene_buffer_set_buffer_with_damage(scene_buffer, - &surface->buffer->base, &surface->buffer_damage); + if (surface->texture) { + scene_buffer_set_buffer_and_texture(scene_buffer, + surface->texture->buffer, surface->texture->texture, + &surface->buffer_damage); } else { wlr_scene_buffer_set_buffer(scene_buffer, NULL); } @@ -198,7 +173,7 @@ static bool scene_buffer_point_accepts_input(struct wlr_scene_buffer *scene_buff static void surface_addon_destroy(struct wlr_addon *addon) { struct wlr_scene_surface *surface = wl_container_of(addon, surface, addon); - scene_buffer_unmark_client_buffer(surface->buffer); + wlr_scene_buffer_set_buffer(surface->buffer, NULL); wlr_addon_finish(&surface->addon); diff --git a/types/wlr_compositor.c b/types/wlr_compositor.c index 791eb77cb..298a2eb37 100644 --- a/types/wlr_compositor.c +++ b/types/wlr_compositor.c @@ -53,6 +53,76 @@ static void pending_buffer_resource_handle_destroy(struct wl_listener *listener, set_pending_buffer_resource(surface, NULL); } +static void surface_texture_handle_buffer_release(struct wl_listener *listener, + void *data) { + struct wlr_surface_texture *surf_tex = wl_container_of(listener, surf_tex, buffer_release); + surf_tex->buffer = NULL; + wl_list_remove(&surf_tex->buffer_release.link); + wl_list_init(&surf_tex->buffer_release.link); +} + +static struct wlr_surface_texture *surface_texture_create(struct wlr_buffer *buffer, + struct wlr_renderer *renderer) { + struct wlr_texture *texture = wlr_texture_from_buffer(renderer, buffer); + if (texture == NULL) { + return NULL; + } + + struct wlr_surface_texture *surf_tex = calloc(1, sizeof(*surf_tex)); + if (surf_tex == NULL) { + wlr_texture_destroy(texture); + return NULL; + } + + surf_tex->buffer = buffer; + surf_tex->texture = texture; + + surf_tex->buffer_release.notify = surface_texture_handle_buffer_release; + wl_signal_add(&buffer->events.release, &surf_tex->buffer_release); + + return surf_tex; +} + +static void surface_texture_consider_destroy(struct wlr_surface_texture *surf_tex) { + if (!surf_tex->dropped || surf_tex->locked) { + return; + } + wl_list_remove(&surf_tex->buffer_release.link); + wlr_texture_destroy(surf_tex->texture); + free(surf_tex); +} + +static bool surface_texture_apply_damage(struct wlr_surface_texture *surf_tex, + struct wlr_buffer *next, const pixman_region32_t *damage) { + if (surf_tex->locked) { + return false; + } + return wlr_texture_update_from_buffer(surf_tex->texture, next, damage); +} + +static void surface_texture_drop(struct wlr_surface_texture *surf_tex) { + if (surf_tex == NULL) { + return; + } + assert(!surf_tex->dropped); + surf_tex->dropped = true; + surface_texture_consider_destroy(surf_tex); +} + +void wlr_surface_texture_lock(struct wlr_surface_texture *surf_tex) { + assert(!surf_tex->locked); + surf_tex->locked = true; +} + +void wlr_surface_texture_unlock(struct wlr_surface_texture *surf_tex) { + if (surf_tex == NULL) { + return; + } + assert(surf_tex->locked); + surf_tex->locked = false; + surface_texture_consider_destroy(surf_tex); +} + static void surface_handle_destroy(struct wl_client *client, struct wl_resource *resource) { struct wlr_surface *surface = wlr_surface_from_resource(resource); @@ -407,18 +477,16 @@ static void surface_state_move(struct wlr_surface_state *state, static void surface_apply_damage(struct wlr_surface *surface) { if (surface->current.buffer == NULL) { // NULL commit - if (surface->buffer != NULL) { - wlr_buffer_unlock(&surface->buffer->base); - } - surface->buffer = NULL; + surface_texture_drop(surface->texture); + surface->texture = NULL; surface->opaque = false; return; } surface->opaque = buffer_is_opaque(surface->current.buffer); - if (surface->buffer != NULL) { - if (wlr_client_buffer_apply_damage(surface->buffer, + if (surface->texture != NULL) { + if (surface_texture_apply_damage(surface->texture, surface->current.buffer, &surface->buffer_damage)) { wlr_buffer_unlock(surface->current.buffer); surface->current.buffer = NULL; @@ -430,18 +498,15 @@ static void surface_apply_damage(struct wlr_surface *surface) { return; } - struct wlr_client_buffer *buffer = wlr_client_buffer_create( - surface->current.buffer, surface->renderer); - - if (buffer == NULL) { + struct wlr_surface_texture *texture = + surface_texture_create(surface->current.buffer, surface->renderer); + if (texture == NULL) { wlr_log(WLR_ERROR, "Failed to upload buffer"); return; } - if (surface->buffer != NULL) { - wlr_buffer_unlock(&surface->buffer->base); - } - surface->buffer = buffer; + surface_texture_drop(surface->texture); + surface->texture = texture; } static void surface_update_opaque_region(struct wlr_surface *surface) { @@ -746,9 +811,7 @@ static void surface_handle_resource_destroy(struct wl_resource *resource) { pixman_region32_fini(&surface->buffer_damage); pixman_region32_fini(&surface->opaque_region); pixman_region32_fini(&surface->input_region); - if (surface->buffer != NULL) { - wlr_buffer_unlock(&surface->buffer->base); - } + surface_texture_drop(surface->texture); free(surface); } @@ -814,10 +877,10 @@ static struct wlr_surface *surface_create(struct wl_client *client, } struct wlr_texture *wlr_surface_get_texture(struct wlr_surface *surface) { - if (surface->buffer == NULL) { + if (surface->texture == NULL) { return NULL; } - return surface->buffer->texture; + return surface->texture->texture; } bool wlr_surface_has_buffer(struct wlr_surface *surface) {