From 467e46c077a5b7e38aca28e8a43da9bd393713ff Mon Sep 17 00:00:00 2001 From: Michael Stapelberg Date: Sun, 5 Jul 2020 09:05:16 +0200 Subject: [PATCH] avoid pixmap allocations in the redraw path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before this change, i3lock could become unusable on systems where pixmap allocations take long. I don’t know precisely why, but slow pixmap allocations is a symptom that my computer sometimes exhibits, apparently when low on GPU memory. In that situation, duplicate key press events would be processed (apparently received from X11!), making correct password entry impossible. --- i3lock.c | 3 ++- unlock_indicator.c | 49 +++++++++++++++++++++++++++++++--------------- unlock_indicator.h | 5 ++++- 3 files changed, 39 insertions(+), 18 deletions(-) diff --git a/i3lock.c b/i3lock.c index 134fdda..bd5008d 100644 --- a/i3lock.c +++ b/i3lock.c @@ -1221,7 +1221,8 @@ int main(int argc, char *argv[]) { free(image_raw_format); /* Pixmap on which the image is rendered to (if any) */ - xcb_pixmap_t bg_pixmap = draw_image(last_resolution); + xcb_pixmap_t bg_pixmap = create_bg_pixmap(conn, screen, last_resolution, color); + draw_image(bg_pixmap, last_resolution); xcb_window_t stolen_focus = find_focused_window(conn, screen->root); diff --git a/unlock_indicator.c b/unlock_indicator.c index d6237ed..b677a90 100644 --- a/unlock_indicator.c +++ b/unlock_indicator.c @@ -86,8 +86,7 @@ auth_state_t auth_state; * resolution and returns it. * */ -xcb_pixmap_t draw_image(uint32_t *resolution) { - xcb_pixmap_t bg_pixmap = XCB_NONE; +void draw_image(xcb_pixmap_t bg_pixmap, uint32_t *resolution) { const double scaling_factor = get_dpi_value() / 96.0; int button_diameter_physical = ceil(scaling_factor * BUTTON_DIAMETER); DEBUG("scaling_factor is %.f, physical diameter is %d px\n", @@ -95,7 +94,7 @@ xcb_pixmap_t draw_image(uint32_t *resolution) { if (!vistype) vistype = get_root_visual_type(screen); - bg_pixmap = create_bg_pixmap(conn, screen, resolution, color); + /* Initialize cairo: Create one in-memory surface to render the unlock * indicator on, create one XCB surface to actually draw (one or more, * depending on the amount of screens) unlock indicators on. */ @@ -105,6 +104,19 @@ xcb_pixmap_t draw_image(uint32_t *resolution) { cairo_surface_t *xcb_output = cairo_xcb_surface_create(conn, bg_pixmap, vistype, resolution[0], resolution[1]); cairo_t *xcb_ctx = cairo_create(xcb_output); + /* After the first iteration, the pixmap will still contain the previous + * contents. Explicitly clear the entire pixmap with the background color + * first to get back into a defined state: */ + char strgroups[3][3] = {{color[0], color[1], '\0'}, + {color[2], color[3], '\0'}, + {color[4], color[5], '\0'}}; + uint32_t rgb16[3] = {(strtol(strgroups[0], NULL, 16)), + (strtol(strgroups[1], NULL, 16)), + (strtol(strgroups[2], NULL, 16))}; + cairo_set_source_rgb(xcb_ctx, rgb16[0] / 255.0, rgb16[1] / 255.0, rgb16[2] / 255.0); + cairo_rectangle(xcb_ctx, 0, 0, resolution[0], resolution[1]); + cairo_fill(xcb_ctx); + if (img) { if (!tile) { cairo_set_source_surface(xcb_ctx, img, 0, 0); @@ -119,16 +131,6 @@ xcb_pixmap_t draw_image(uint32_t *resolution) { cairo_fill(xcb_ctx); cairo_pattern_destroy(pattern); } - } else { - char strgroups[3][3] = {{color[0], color[1], '\0'}, - {color[2], color[3], '\0'}, - {color[4], color[5], '\0'}}; - uint32_t rgb16[3] = {(strtol(strgroups[0], NULL, 16)), - (strtol(strgroups[1], NULL, 16)), - (strtol(strgroups[2], NULL, 16))}; - cairo_set_source_rgb(xcb_ctx, rgb16[0] / 255.0, rgb16[1] / 255.0, rgb16[2] / 255.0); - cairo_rectangle(xcb_ctx, 0, 0, resolution[0], resolution[1]); - cairo_fill(xcb_ctx); } if (unlock_indicator && @@ -329,7 +331,18 @@ xcb_pixmap_t draw_image(uint32_t *resolution) { cairo_surface_destroy(output); cairo_destroy(ctx); cairo_destroy(xcb_ctx); - return bg_pixmap; +} + +static xcb_pixmap_t bg_pixmap = XCB_NONE; + +/* + * Releases the current background pixmap so that the next redraw_screen() call + * will allocate a new one with the updated resolution. + * + */ +void free_bg_pixmap(void) { + xcb_free_pixmap(conn, bg_pixmap); + bg_pixmap = XCB_NONE; } /* @@ -338,12 +351,16 @@ xcb_pixmap_t draw_image(uint32_t *resolution) { */ void redraw_screen(void) { DEBUG("redraw_screen(unlock_state = %d, auth_state = %d)\n", unlock_state, auth_state); - xcb_pixmap_t bg_pixmap = draw_image(last_resolution); + if (bg_pixmap == XCB_NONE) { + DEBUG("allocating pixmap for %d x %d px\n", last_resolution[0], last_resolution[1]); + bg_pixmap = create_bg_pixmap(conn, screen, last_resolution, color); + } + + draw_image(bg_pixmap, last_resolution); xcb_change_window_attributes(conn, win, XCB_CW_BACK_PIXMAP, (uint32_t[1]){bg_pixmap}); /* XXX: Possible optimization: Only update the area in the middle of the * screen instead of the whole screen. */ xcb_clear_area(conn, 0, win, 0, 0, last_resolution[0], last_resolution[1]); - xcb_free_pixmap(conn, bg_pixmap); xcb_flush(conn); } diff --git a/unlock_indicator.h b/unlock_indicator.h index 93c1de8..581d028 100644 --- a/unlock_indicator.h +++ b/unlock_indicator.h @@ -1,6 +1,8 @@ #ifndef _UNLOCK_INDICATOR_H #define _UNLOCK_INDICATOR_H +#include + typedef enum { STATE_STARTED = 0, /* default state */ STATE_KEY_PRESSED = 1, /* key was pressed, show unlock indicator */ @@ -19,7 +21,8 @@ typedef enum { STATE_I3LOCK_LOCK_FAILED = 4, /* i3lock failed to load */ } auth_state_t; -xcb_pixmap_t draw_image(uint32_t* resolution); +void free_bg_pixmap(void); +void draw_image(xcb_pixmap_t bg_pixmap, uint32_t* resolution); void redraw_screen(void); void clear_indicator(void);