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 <yshuiv7@gmail.com>

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, &reg_paint_in_bound, &reg_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(&reg_visible_local);
 			pixman_region32_fini(&reg_bound_local);
 		}

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.
 	///

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(&reg);
 
 	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;
 }
 

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,
+image_handle
-                               struct xvisual_info fmt, bool owned) {
+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) {
+		pixmap = img->pixmap;
-			xcb_free_pixmap(base->c->c, img->pixmap);
-		}
 		free(img);
 	}
+	return pixmap;
 }
 
 bool dummy_is_image_transparent(struct backend_base *base, image_handle image) {

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) {
+	if (p && *p != EGL_NO_IMAGE) {
-		eglDestroyImage(gd->display, p->image);
+		eglDestroyImage(gd->display, *p);
-		p->image = EGL_NO_IMAGE;
+		*p = EGL_NO_IMAGE;
-	}
-
-	if (p->owned) {
-		xcb_free_pixmap(base->c->c, p->pixmap);
-		p->pixmap = XCB_NONE;
 	}
 
 	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);
+	return NULL;
-	ret->owned = false;
-	ret->image = EGL_NO_IMAGE;
-	ret->pixmap = 0;
-	return ret;
 }
 
 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 = cmalloc(EGLImage);
-	eglpixmap->pixmap = pixmap;
+	*eglpixmap = eglCreateImage(gd->display, EGL_NO_CONTEXT, EGL_NATIVE_PIXMAP_KHR,
-	eglpixmap->image = eglCreateImage(gd->display, EGL_NO_CONTEXT, EGL_NATIVE_PIXMAP_KHR,
+	                            (EGLClientBuffer)(uintptr_t)pixmap, NULL);
-	                                  (EGLClientBuffer)(uintptr_t)pixmap, NULL);
-	eglpixmap->owned = owned;
 
-	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) {
+	if (eglpixmap && *eglpixmap) {
-		eglDestroyImage(gd->display, eglpixmap->image);
+		eglDestroyImage(gd->display, *eglpixmap);
 	}
 	free(eglpixmap);
-
-	if (owned) {
-		xcb_free_pixmap(base->c->c, pixmap);
-	}
 	free(wd);
 	return NULL;
 }

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) {

src/backend/gl/gl_common.h

@@ -4,6 +4,7 @@
 #include <epoxy/gl.h>
 #include <stdbool.h>
 #include <string.h>
+#include <xcb/xproto.h>
 
 #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);

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) {
+	if (p) {
-		glXDestroyPixmap(base->c->dpy, p->glpixmap);
+		glXDestroyPixmap(base->c->dpy, *p);
-		p->glpixmap = 0;
+		*p = 0;
-	}
-
-	if (p->owned) {
-		xcb_free_pixmap(base->c->c, p->pixmap);
-		p->pixmap = XCB_NONE;
 	}
 
 	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);
+	return NULL;
-	ret->owned = false;
-	ret->glpixmap = 0;
-	ret->pixmap = 0;
-	return ret;
 }
 
 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) {
+glx_bind_pixmap(backend_t *base, xcb_pixmap_t pixmap, struct xvisual_info fmt) {
-	struct _glx_pixmap *glxpixmap = NULL;
+	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 = cmalloc(GLXPixmap);
-	glxpixmap->pixmap = pixmap;
+	inner->pixmap = pixmap;
-	glxpixmap->glpixmap = glXCreatePixmap(base->c->dpy, fbconfig->cfg, pixmap, attrs);
+	*glxpixmap = glXCreatePixmap(base->c->dpy, fbconfig->cfg, pixmap, attrs);
-	glxpixmap->owned = owned;
 
-	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) {
+	if (glxpixmap && *glxpixmap) {
-		glXDestroyPixmap(base->c->dpy, glxpixmap->glpixmap);
+		glXDestroyPixmap(base->c->dpy, *glxpixmap);
 	}
 	free(glxpixmap);
-
-	if (owned) {
-		xcb_free_pixmap(base->c->c, pixmap);
-	}
 	free(wd);
 	return NULL;
 }

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;
+	// Whether we allocated it this pixmap.
-	// or not, e.g. this is a named pixmap of a X window.
+	// or not, i.e. this pixmap is passed in via xrender_bind_pixmap
-	bool owned;
+	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,
+static image_handle
-                                        struct xvisual_info fmt, bool owned) {
+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;
 }
 

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,

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 =
+	w->win_image = b->ops->bind_pixmap(b, pixmap, x_get_visual_info(b->c, w->a.visual));
-	    b->ops->bind_pixmap(b, pixmap, x_get_visual_info(b->c, w->a.visual), true);
 	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;
 	}