From c51d446854e310999d41e53cd43de1ed47b70ebe Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 8 Apr 2024 09:02:55 +0100 Subject: [PATCH] backend: move pixmap ownership management out of the backends Caller of `bind_pixmap` generally knows if the pixmap is owned or not, they are fully able to decide if the pixmap should be freed, so let them do it. Signed-off-by: Yuxuan Shui --- src/backend/backend.c | 4 ++- src/backend/backend.h | 9 +++--- src/backend/backend_common.c | 8 ++++-- src/backend/dummy/dummy.c | 15 +++++----- src/backend/gl/egl.c | 52 +++++++++++---------------------- src/backend/gl/gl_common.c | 6 +++- src/backend/gl/gl_common.h | 4 ++- src/backend/gl/glx.c | 54 +++++++++++------------------------ src/backend/xrender/xrender.c | 28 +++++++++++------- src/picom.c | 2 +- src/win.c | 19 ++++++++---- 11 files changed, 94 insertions(+), 107 deletions(-) diff --git a/src/backend/backend.c b/src/backend/backend.c index 7a99ecd0..7db4f8ec 100644 --- a/src/backend/backend.c +++ b/src/backend/backend.c @@ -484,7 +484,9 @@ bool paint_all_new(session_t *ps, struct managed_win *const t) { ps->backend_data->ops->compose( ps->backend_data, new_img, window_coord, w->mask_image, window_coord, ®_paint_in_bound, ®_visible); - ps->backend_data->ops->release_image(ps->backend_data, new_img); + auto pixmap = + ps->backend_data->ops->release_image(ps->backend_data, new_img); + CHECK(pixmap == XCB_NONE); pixman_region32_fini(®_visible_local); pixman_region32_fini(®_bound_local); } diff --git a/src/backend/backend.h b/src/backend/backend.h index bc9fdd1c..0a90a91f 100644 --- a/src/backend/backend.h +++ b/src/backend/backend.h @@ -208,13 +208,11 @@ struct backend_operations { * @param backend_data backend data * @param pixmap X pixmap to bind * @param fmt information of the pixmap's visual - * @param owned whether the ownership of the pixmap is transferred to the - * backend. * @return backend specific image handle for the pixmap. May be * NULL. */ image_handle (*bind_pixmap)(backend_t *backend_data, xcb_pixmap_t pixmap, - struct xvisual_info fmt, bool owned); + struct xvisual_info fmt); /// Create a shadow context for rendering shadows with radius `radius`. /// Default implementation: default_create_shadow_context @@ -268,7 +266,10 @@ struct backend_operations { /// Free resources associated with an image data structure /// /// @param image the image to be released, cannot be NULL. - void (*release_image)(backend_t *backend_data, image_handle image) attr_nonnull(1, 2); + /// @return if this image is created by `bind_pixmap`, the X pixmap; 0 + /// otherwise. + xcb_pixmap_t (*release_image)(backend_t *backend_data, image_handle image) + attr_nonnull(1, 2); /// Create a shader object from a shader source. /// diff --git a/src/backend/backend_common.c b/src/backend/backend_common.c index adc14152..723c091e 100644 --- a/src/backend/backend_common.c +++ b/src/backend/backend_common.c @@ -309,9 +309,12 @@ image_handle default_render_shadow(backend_t *backend_data, int width, int heigh auto visual = x_get_visual_for_standard(backend_data->c, XCB_PICT_STANDARD_ARGB_32); auto ret = backend_data->ops->bind_pixmap( - backend_data, shadow, x_get_visual_info(backend_data->c, visual), true); + backend_data, shadow, x_get_visual_info(backend_data->c, visual)); x_free_picture(backend_data->c, pict); x_free_picture(backend_data->c, shadow_pixel); + if (!ret) { + xcb_free_pixmap(backend_data->c->c, shadow); + } return ret; } @@ -326,7 +329,8 @@ backend_render_shadow_from_mask(backend_t *backend_data, int width, int height, pixman_region32_fini(®); auto shadow = backend_data->ops->shadow_from_mask(backend_data, mask, sctx, color); - backend_data->ops->release_image(backend_data, mask); + auto pixmap = backend_data->ops->release_image(backend_data, mask); + CHECK(pixmap == XCB_NONE); return shadow; } diff --git a/src/backend/dummy/dummy.c b/src/backend/dummy/dummy.c index f43729dd..c96e7181 100644 --- a/src/backend/dummy/dummy.c +++ b/src/backend/dummy/dummy.c @@ -85,8 +85,8 @@ bool dummy_blur(struct backend_base *backend_data attr_unused, double opacity at return true; } -image_handle dummy_bind_pixmap(struct backend_base *base, xcb_pixmap_t pixmap, - struct xvisual_info fmt, bool owned) { +image_handle +dummy_bind_pixmap(struct backend_base *base, xcb_pixmap_t pixmap, struct xvisual_info fmt) { auto dummy = (struct dummy_data *)base; struct dummy_image *img = NULL; HASH_FIND_INT(dummy->images, &pixmap, img); @@ -100,28 +100,27 @@ image_handle dummy_bind_pixmap(struct backend_base *base, xcb_pixmap_t pixmap, img->transparent = fmt.alpha_size != 0; img->refcount = ccalloc(1, int); *img->refcount = 1; - img->owned = owned; HASH_ADD_INT(dummy->images, pixmap, img); return (image_handle)img; } -void dummy_release_image(backend_t *base, image_handle image) { +xcb_pixmap_t dummy_release_image(backend_t *base, image_handle image) { auto dummy = (struct dummy_data *)base; if ((struct backend_image *)image == &dummy->mask) { - return; + return XCB_NONE; } auto img = (struct dummy_image *)image; + xcb_pixmap_t pixmap = XCB_NONE; assert(*img->refcount > 0); (*img->refcount)--; if (*img->refcount == 0) { HASH_DEL(dummy->images, img); free(img->refcount); - if (img->owned) { - xcb_free_pixmap(base->c->c, img->pixmap); - } + pixmap = img->pixmap; free(img); } + return pixmap; } bool dummy_is_image_transparent(struct backend_base *base, image_handle image) { diff --git a/src/backend/gl/egl.c b/src/backend/gl/egl.c index 981aedfe..09dcf45a 100644 --- a/src/backend/gl/egl.c +++ b/src/backend/gl/egl.c @@ -23,12 +23,6 @@ #include "utils.h" #include "x.h" -struct egl_pixmap { - EGLImage image; - xcb_pixmap_t pixmap; - bool owned; -}; - struct egl_data { struct gl_data gl; EGLDisplay display; @@ -65,16 +59,11 @@ const char *eglGetErrorString(EGLint error) { */ static void egl_release_image(backend_t *base, struct gl_texture *tex) { struct egl_data *gd = (void *)base; - struct egl_pixmap *p = tex->user_data; + EGLImage *p = tex->user_data; // Release binding - if (p->image != EGL_NO_IMAGE) { - eglDestroyImage(gd->display, p->image); - p->image = EGL_NO_IMAGE; - } - - if (p->owned) { - xcb_free_pixmap(base->c->c, p->pixmap); - p->pixmap = XCB_NONE; + if (p && *p != EGL_NO_IMAGE) { + eglDestroyImage(gd->display, *p); + *p = EGL_NO_IMAGE; } free(p); @@ -110,11 +99,7 @@ void egl_deinit(backend_t *base) { } static void *egl_decouple_user_data(backend_t *base attr_unused, void *ud attr_unused) { - auto ret = cmalloc(struct egl_pixmap); - ret->owned = false; - ret->image = EGL_NO_IMAGE; - ret->pixmap = 0; - return ret; + return NULL; } static bool egl_set_swap_interval(int interval, EGLDisplay dpy) { @@ -250,9 +235,9 @@ end: } static image_handle -egl_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, struct xvisual_info fmt, bool owned) { +egl_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, struct xvisual_info fmt) { struct egl_data *gd = (void *)base; - struct egl_pixmap *eglpixmap = NULL; + EGLImage *eglpixmap = NULL; auto r = xcb_get_geometry_reply(base->c->c, xcb_get_geometry(base->c->c, pixmap), NULL); @@ -273,20 +258,19 @@ egl_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, struct xvisual_info fmt, b log_debug("depth %d", fmt.visual_depth); inner->y_inverted = true; + inner->pixmap = pixmap; - eglpixmap = cmalloc(struct egl_pixmap); - eglpixmap->pixmap = pixmap; - eglpixmap->image = eglCreateImage(gd->display, EGL_NO_CONTEXT, EGL_NATIVE_PIXMAP_KHR, - (EGLClientBuffer)(uintptr_t)pixmap, NULL); - eglpixmap->owned = owned; + eglpixmap = cmalloc(EGLImage); + *eglpixmap = eglCreateImage(gd->display, EGL_NO_CONTEXT, EGL_NATIVE_PIXMAP_KHR, + (EGLClientBuffer)(uintptr_t)pixmap, NULL); - if (eglpixmap->image == EGL_NO_IMAGE) { + if (*eglpixmap == EGL_NO_IMAGE) { log_error("Failed to create eglpixmap for pixmap %#010x: %s", pixmap, eglGetErrorString(eglGetError())); goto err; } - log_trace("EGLImage %p", eglpixmap->image); + log_trace("EGLImage %p", *eglpixmap); // Create texture inner->user_data = eglpixmap; @@ -297,20 +281,16 @@ egl_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, struct xvisual_info fmt, b wd->dim = 0; wd->inner->refcount = 1; glBindTexture(GL_TEXTURE_2D, inner->texture); - glEGLImageTargetTexStorageEXT(GL_TEXTURE_2D, eglpixmap->image, NULL); + glEGLImageTargetTexStorageEXT(GL_TEXTURE_2D, *eglpixmap, NULL); glBindTexture(GL_TEXTURE_2D, 0); gl_check_err(); return (image_handle)wd; err: - if (eglpixmap && eglpixmap->image) { - eglDestroyImage(gd->display, eglpixmap->image); + if (eglpixmap && *eglpixmap) { + eglDestroyImage(gd->display, *eglpixmap); } free(eglpixmap); - - if (owned) { - xcb_free_pixmap(base->c->c, pixmap); - } free(wd); return NULL; } diff --git a/src/backend/gl/gl_common.c b/src/backend/gl/gl_common.c index 1aeb0f63..3522026c 100644 --- a/src/backend/gl/gl_common.c +++ b/src/backend/gl/gl_common.c @@ -762,15 +762,19 @@ static void gl_release_image_inner(backend_t *base, struct gl_texture *inner) { gl_check_err(); } -void gl_release_image(backend_t *base, image_handle image) { +xcb_pixmap_t gl_release_image(backend_t *base, image_handle image) { auto wd = (struct backend_image *)image; auto inner = (struct gl_texture *)wd->inner; inner->refcount--; assert(inner->refcount >= 0); + + xcb_pixmap_t pixmap = XCB_NONE; if (inner->refcount == 0) { + pixmap = inner->pixmap; gl_release_image_inner(base, inner); } free(wd); + return pixmap; } void *gl_create_window_shader(backend_t *backend_data attr_unused, const char *source) { diff --git a/src/backend/gl/gl_common.h b/src/backend/gl/gl_common.h index af188569..7b63e2ab 100644 --- a/src/backend/gl/gl_common.h +++ b/src/backend/gl/gl_common.h @@ -4,6 +4,7 @@ #include #include #include +#include #include "backend/backend.h" #include "backend/backend_common.h" @@ -83,6 +84,7 @@ struct gl_texture { GLuint texture; int width, height; bool y_inverted; + xcb_pixmap_t pixmap; // Textures for auxiliary uses. GLuint auxiliary_texture[2]; @@ -165,7 +167,7 @@ GLuint gl_new_texture(GLenum target); bool gl_image_op(backend_t *base, enum image_operations op, image_handle image, const region_t *reg_op, const region_t *reg_visible, void *arg); -void gl_release_image(backend_t *base, image_handle image); +xcb_pixmap_t gl_release_image(backend_t *base, image_handle image); image_handle gl_make_mask(backend_t *base, geometry_t size, const region_t *reg); image_handle gl_clone(backend_t *base, image_handle image, const region_t *reg_visible); diff --git a/src/backend/gl/glx.c b/src/backend/gl/glx.c index 4201a492..d3b894a2 100644 --- a/src/backend/gl/glx.c +++ b/src/backend/gl/glx.c @@ -36,12 +36,6 @@ #include "win.h" #include "x.h" -struct _glx_pixmap { - GLXPixmap glpixmap; - xcb_pixmap_t pixmap; - bool owned; -}; - struct _glx_data { struct gl_data gl; xcb_window_t target_win; @@ -169,23 +163,18 @@ bool glx_find_fbconfig(struct x_connection *c, struct xvisual_info m, * Free a glx_texture_t. */ static void glx_release_image(backend_t *base, struct gl_texture *tex) { - struct _glx_pixmap *p = tex->user_data; + GLXPixmap *p = tex->user_data; // Release binding - if (p->glpixmap && tex->texture) { + if (p && tex->texture) { glBindTexture(GL_TEXTURE_2D, tex->texture); - glXReleaseTexImageEXT(base->c->dpy, p->glpixmap, GLX_FRONT_LEFT_EXT); + glXReleaseTexImageEXT(base->c->dpy, *p, GLX_FRONT_LEFT_EXT); glBindTexture(GL_TEXTURE_2D, 0); } // Free GLX Pixmap - if (p->glpixmap) { - glXDestroyPixmap(base->c->dpy, p->glpixmap); - p->glpixmap = 0; - } - - if (p->owned) { - xcb_free_pixmap(base->c->c, p->pixmap); - p->pixmap = XCB_NONE; + if (p) { + glXDestroyPixmap(base->c->dpy, *p); + *p = 0; } free(p); @@ -217,11 +206,7 @@ void glx_deinit(backend_t *base) { } static void *glx_decouple_user_data(backend_t *base attr_unused, void *ud attr_unused) { - auto ret = cmalloc(struct _glx_pixmap); - ret->owned = false; - ret->glpixmap = 0; - ret->pixmap = 0; - return ret; + return NULL; } static bool glx_set_swap_interval(int interval, Display *dpy, GLXDrawable drawable) { @@ -381,8 +366,8 @@ end: } static image_handle -glx_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, struct xvisual_info fmt, bool owned) { - struct _glx_pixmap *glxpixmap = NULL; +glx_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, struct xvisual_info fmt) { + GLXPixmap *glxpixmap = NULL; auto gd = (struct _glx_data *)base; // Retrieve pixmap parameters, if they aren't provided if (fmt.visual_depth > OPENGL_MAX_DEPTH) { @@ -453,17 +438,16 @@ glx_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, struct xvisual_info fmt, b inner->y_inverted = fbconfig->y_inverted; - glxpixmap = cmalloc(struct _glx_pixmap); - glxpixmap->pixmap = pixmap; - glxpixmap->glpixmap = glXCreatePixmap(base->c->dpy, fbconfig->cfg, pixmap, attrs); - glxpixmap->owned = owned; + glxpixmap = cmalloc(GLXPixmap); + inner->pixmap = pixmap; + *glxpixmap = glXCreatePixmap(base->c->dpy, fbconfig->cfg, pixmap, attrs); - if (!glxpixmap->glpixmap) { + if (!*glxpixmap) { log_error("Failed to create glpixmap for pixmap %#010x", pixmap); goto err; } - log_trace("GLXPixmap %#010lx", glxpixmap->glpixmap); + log_trace("GLXPixmap %#010lx", *glxpixmap); // Create texture inner->user_data = glxpixmap; @@ -471,20 +455,16 @@ glx_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, struct xvisual_info fmt, b inner->has_alpha = fmt.alpha_size != 0; wd->inner->refcount = 1; glBindTexture(GL_TEXTURE_2D, inner->texture); - glXBindTexImageEXT(base->c->dpy, glxpixmap->glpixmap, GLX_FRONT_LEFT_EXT, NULL); + glXBindTexImageEXT(base->c->dpy, *glxpixmap, GLX_FRONT_LEFT_EXT, NULL); glBindTexture(GL_TEXTURE_2D, 0); gl_check_err(); return (image_handle)wd; err: - if (glxpixmap && glxpixmap->glpixmap) { - glXDestroyPixmap(base->c->dpy, glxpixmap->glpixmap); + if (glxpixmap && *glxpixmap) { + glXDestroyPixmap(base->c->dpy, *glxpixmap); } free(glxpixmap); - - if (owned) { - xcb_free_pixmap(base->c->c, pixmap); - } free(wd); return NULL; } diff --git a/src/backend/xrender/xrender.c b/src/backend/xrender/xrender.c index 4b961564..e27fbf9e 100644 --- a/src/backend/xrender/xrender.c +++ b/src/backend/xrender/xrender.c @@ -86,9 +86,9 @@ struct xrender_image_data_inner { int width, height; xcb_visualid_t visual; uint8_t depth; - // Whether we own this image, e.g. we allocated it; - // or not, e.g. this is a named pixmap of a X window. - bool owned; + // Whether we allocated it this pixmap. + // or not, i.e. this pixmap is passed in via xrender_bind_pixmap + bool internal_pixmap; }; struct xrender_rounded_rectangle_cache { @@ -523,8 +523,8 @@ xrender_blur(backend_t *backend_data, double opacity, void *ctx_, image_handle m return true; } -static image_handle xrender_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, - struct xvisual_info fmt, bool owned) { +static image_handle +xrender_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, struct xvisual_info fmt) { xcb_generic_error_t *e; auto r = xcb_get_geometry_reply(base->c->c, xcb_get_geometry(base->c->c, pixmap), &e); if (!r) { @@ -544,9 +544,9 @@ static image_handle xrender_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, xcb_render_create_picture_value_list_t pic_attrs = {.repeat = XCB_RENDER_REPEAT_NORMAL}; inner->pict = x_create_picture_with_visual_and_pixmap( base->c, fmt.visual, pixmap, XCB_RENDER_CP_REPEAT, &pic_attrs); - inner->owned = owned; inner->visual = fmt.visual; inner->refcount = 1; + inner->internal_pixmap = false; img->base.inner = (struct backend_image_inner_base *)inner; img->base.opacity = 1; @@ -560,13 +560,17 @@ static image_handle xrender_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, } return (image_handle)img; } -static void +static xcb_pixmap_t xrender_release_image_inner(backend_t *base, struct xrender_image_data_inner *inner) { x_free_picture(base->c, inner->pict); - if (inner->owned) { + if (inner->internal_pixmap) { xcb_free_pixmap(base->c->c, inner->pixmap); + inner->pixmap = XCB_NONE; } + + auto ret = inner->pixmap; free(inner); + return ret; } static void @@ -584,16 +588,18 @@ xrender_release_rounded_corner_cache(backend_t *base, } } -static void xrender_release_image(backend_t *base, image_handle image) { +static xcb_pixmap_t xrender_release_image(backend_t *base, image_handle image) { auto img = (struct xrender_image *)image; + xcb_pixmap_t pixmap = XCB_NONE; xrender_release_rounded_corner_cache(base, img->rounded_rectangle); img->rounded_rectangle = NULL; img->base.inner->refcount -= 1; if (img->base.inner->refcount == 0) { - xrender_release_image_inner( + pixmap = xrender_release_image_inner( base, (struct xrender_image_data_inner *)img->base.inner); } free(img); + return pixmap; } static void xrender_deinit(backend_t *backend_data) { @@ -716,7 +722,7 @@ xrender_new_inner(backend_t *base, int w, int h, xcb_visualid_t visual, uint8_t new_inner->visual = visual; new_inner->depth = depth; new_inner->refcount = 1; - new_inner->owned = true; + new_inner->internal_pixmap = true; return new_inner; } diff --git a/src/picom.c b/src/picom.c index a39c9b8d..92d7d135 100644 --- a/src/picom.c +++ b/src/picom.c @@ -1192,7 +1192,7 @@ void root_damaged(session_t *ps) { free(r); ps->root_image = ps->backend_data->ops->bind_pixmap( - ps->backend_data, pixmap, x_get_visual_info(&ps->c, visual), false); + ps->backend_data, pixmap, x_get_visual_info(&ps->c, visual)); if (ps->root_image) { ps->backend_data->ops->set_image_property( ps->backend_data, IMAGE_PROPERTY_EFFECTIVE_SIZE, diff --git a/src/win.c b/src/win.c index 46aa6ebb..0f8d8237 100644 --- a/src/win.c +++ b/src/win.c @@ -310,25 +310,34 @@ static inline void win_release_pixmap(backend_t *base, struct managed_win *w) { log_debug("Releasing pixmap of window %#010x (%s)", w->base.id, w->name); assert(w->win_image); if (w->win_image) { - base->ops->release_image(base, w->win_image); + auto pixmap = base->ops->release_image(base, w->win_image); w->win_image = NULL; // Bypassing win_set_flags, because `w` might have been destroyed w->flags |= WIN_FLAGS_PIXMAP_NONE; + if (pixmap != XCB_NONE) { + xcb_free_pixmap(base->c->c, pixmap); + } } } static inline void win_release_shadow(backend_t *base, struct managed_win *w) { log_debug("Releasing shadow of window %#010x (%s)", w->base.id, w->name); if (w->shadow_image) { assert(w->shadow); - base->ops->release_image(base, w->shadow_image); + auto pixmap = base->ops->release_image(base, w->shadow_image); w->shadow_image = NULL; + if (pixmap != XCB_NONE) { + xcb_free_pixmap(base->c->c, pixmap); + } } } static inline void win_release_mask(backend_t *base, struct managed_win *w) { if (w->mask_image) { - base->ops->release_image(base, w->mask_image); + auto pixmap = base->ops->release_image(base, w->mask_image); w->mask_image = NULL; + if (pixmap != XCB_NONE) { + xcb_free_pixmap(base->c->c, pixmap); + } } } @@ -344,10 +353,10 @@ static inline bool win_bind_pixmap(struct backend_base *b, struct managed_win *w return false; } log_debug("New named pixmap for %#010x (%s) : %#010x", w->base.id, w->name, pixmap); - w->win_image = - b->ops->bind_pixmap(b, pixmap, x_get_visual_info(b->c, w->a.visual), true); + w->win_image = b->ops->bind_pixmap(b, pixmap, x_get_visual_info(b->c, w->a.visual)); if (!w->win_image) { log_error("Failed to bind pixmap"); + xcb_free_pixmap(b->c->c, pixmap); win_set_flags(w, WIN_FLAGS_IMAGE_ERROR); return false; }