From 9332cba8df8c3137d8d690e8001878811283c3bd Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Tue, 31 Mar 2020 05:01:14 +0100 Subject: [PATCH] core: delayed handling of root ConfigureNotify Previously, root ConfigureNotify is handled immediately, by resetting the backend, which in turn releases all the window images. This puts all the windows into a state where they don't have images attached, which they really should be in when the screen is redirected. (To expand a little, a window without images should only exist if: * It's an unmanaged window. * Screen is unredirected.) Normally, this kind of window could be fine, as the next render phase will re-acquire images for them. However, if a window in this state is destroyed with fading enabled, then the render phase won't try to acquire images for it, causing it to go into the main rendering function without images attached, and trigger an assertion. This commit delays the handling of root ConfigureNotify until the render phase. This way, the images will be immediately re-acquired after they are released, thus prevent this problem from happening. Also adds a testcase for this. Fixes #357 Signed-off-by: Yuxuan Shui --- src/event.c | 2 +- src/picom.c | 441 +++++++++++++++++++----------------- src/picom.h | 9 +- tests/configs/issue357.conf | 3 + tests/run_tests.sh | 1 + tests/testcases/issue357.py | 34 +++ 6 files changed, 272 insertions(+), 218 deletions(-) create mode 100644 tests/configs/issue357.conf create mode 100755 tests/testcases/issue357.py diff --git a/src/event.c b/src/event.c index 9662767..2a9d1b0 100644 --- a/src/event.c +++ b/src/event.c @@ -250,7 +250,7 @@ static inline void ev_configure_notify(session_t *ps, xcb_configure_notify_event log_debug("{ send_event: %d, id: %#010x, above: %#010x, override_redirect: %d }", ev->event, ev->window, ev->above_sibling, ev->override_redirect); if (ev->window == ps->root) { - configure_root(ps, ev->width, ev->height); + set_root_flags(ps, ROOT_FLAGS_CONFIGURED); } else { configure_win(ps, ev); } diff --git a/src/picom.c b/src/picom.c index 008a99b..d97f10c 100644 --- a/src/picom.c +++ b/src/picom.c @@ -98,6 +98,7 @@ const char *const BACKEND_STRS[] = {[BKEND_XRENDER] = "xrender", session_t *ps_g = NULL; void set_root_flags(session_t *ps, uint64_t flags) { + log_debug("Setting root flags: %lu", flags); ps->root_flags |= flags; ps->pending_updates = true; } @@ -400,6 +401,222 @@ xcb_window_t find_client_win(session_t *ps, xcb_window_t w) { return ret; } +/** + * Rebuild cached screen_reg. + */ +static void rebuild_screen_reg(session_t *ps) { + get_screen_region(ps, &ps->screen_reg); +} + +/** + * Rebuild shadow_exclude_reg. + */ +static void rebuild_shadow_exclude_reg(session_t *ps) { + bool ret = parse_geometry(ps, ps->o.shadow_exclude_reg_str, &ps->shadow_exclude_reg); + if (!ret) + exit(1); +} + +/// Free up all the images and deinit the backend +static void destroy_backend(session_t *ps) { + win_stack_foreach_managed_safe(w, &ps->window_stack) { + // Wrapping up fading in progress + if (win_skip_fading(ps, w)) { + // `w` is freed by win_skip_fading + continue; + } + + if (ps->backend_data) { + if (w->state == WSTATE_MAPPED) { + win_release_images(ps->backend_data, w); + } else { + assert(!w->win_image); + assert(!w->shadow_image); + } + } + free_paint(ps, &w->paint); + } + + if (ps->backend_data && ps->root_image) { + ps->backend_data->ops->release_image(ps->backend_data, ps->root_image); + ps->root_image = NULL; + } + + if (ps->backend_data) { + // deinit backend + if (ps->backend_blur_context) { + ps->backend_data->ops->destroy_blur_context( + ps->backend_data, ps->backend_blur_context); + ps->backend_blur_context = NULL; + } + ps->backend_data->ops->deinit(ps->backend_data); + ps->backend_data = NULL; + } +} + +static bool initialize_blur(session_t *ps) { + struct kernel_blur_args kargs; + struct gaussian_blur_args gargs; + struct box_blur_args bargs; + + void *args = NULL; + switch (ps->o.blur_method) { + case BLUR_METHOD_BOX: + bargs.size = ps->o.blur_radius; + args = (void *)&bargs; + break; + case BLUR_METHOD_KERNEL: + kargs.kernel_count = ps->o.blur_kernel_count; + kargs.kernels = ps->o.blur_kerns; + args = (void *)&kargs; + break; + case BLUR_METHOD_GAUSSIAN: + gargs.size = ps->o.blur_radius; + gargs.deviation = ps->o.blur_deviation; + args = (void *)&gargs; + break; + default: return true; + } + + ps->backend_blur_context = ps->backend_data->ops->create_blur_context( + ps->backend_data, ps->o.blur_method, args); + return ps->backend_blur_context != NULL; +} + +/// Init the backend and bind all the window pixmap to backend images +static bool initialize_backend(session_t *ps) { + if (ps->o.experimental_backends) { + assert(!ps->backend_data); + // Reinitialize win_data + assert(backend_list[ps->o.backend]); + ps->backend_data = backend_list[ps->o.backend]->init(ps); + if (!ps->backend_data) { + log_fatal("Failed to initialize backend, aborting..."); + quit(ps); + return false; + } + ps->backend_data->ops = backend_list[ps->o.backend]; + + if (!initialize_blur(ps)) { + log_fatal("Failed to prepare for background blur, aborting..."); + ps->backend_data->ops->deinit(ps->backend_data); + ps->backend_data = NULL; + quit(ps); + return false; + } + + // window_stack shouldn't include window that's + // not in the hash table at this point. Since + // there cannot be any fading windows. + HASH_ITER2(ps->windows, _w) { + if (!_w->managed) { + continue; + } + auto w = (struct managed_win *)_w; + assert(w->state == WSTATE_MAPPED || w->state == WSTATE_UNMAPPED); + if (w->state == WSTATE_MAPPED) { + // We need to reacquire image + log_debug("Marking window %#010x (%s) for update after " + "redirection", + w->base.id, w->name); + if (w->shadow) { + struct color c = { + .red = ps->o.shadow_red, + .green = ps->o.shadow_green, + .blue = ps->o.shadow_blue, + .alpha = ps->o.shadow_opacity, + }; + win_bind_shadow(ps->backend_data, w, c, + ps->gaussian_map); + } + + w->flags |= WIN_FLAGS_PIXMAP_STALE; + ps->pending_updates = true; + } + } + } + + // The old backends binds pixmap lazily, nothing to do here + return true; +} + +/// Handle configure event of the root window +static void configure_root(session_t *ps) { + auto r = XCB_AWAIT(xcb_get_geometry, ps->c, ps->root); + if (!r) { + log_fatal("Failed to fetch root geometry"); + abort(); + } + + log_info("Root configuration changed, new geometry: %dx%d", r->width, r->height); + bool has_root_change = false; + if (ps->redirected) { + // On root window changes + if (ps->o.experimental_backends) { + assert(ps->backend_data); + has_root_change = ps->backend_data->ops->root_change != NULL; + } else { + // Old backend can handle root change + has_root_change = true; + } + + if (!has_root_change) { + // deinit/reinit backend and free up resources if the backend + // cannot handle root change + destroy_backend(ps); + } + free_paint(ps, &ps->tgt_buffer); + } + + ps->root_width = r->width; + ps->root_height = r->height; + + rebuild_screen_reg(ps); + rebuild_shadow_exclude_reg(ps); + + // Invalidate reg_ignore from the top + auto top_w = win_stack_find_next_managed(ps, &ps->window_stack); + if (top_w) { + rc_region_unref(&top_w->reg_ignore); + top_w->reg_ignore_valid = false; + } + + if (ps->redirected) { + for (int i = 0; i < ps->ndamage; i++) { + pixman_region32_clear(&ps->damage_ring[i]); + } + ps->damage = ps->damage_ring + ps->ndamage - 1; +#ifdef CONFIG_OPENGL + // GLX root change callback + if (BKEND_GLX == ps->o.backend && !ps->o.experimental_backends) { + glx_on_root_change(ps); + } +#endif + if (has_root_change) { + if (ps->backend_data != NULL) { + ps->backend_data->ops->root_change(ps->backend_data, ps); + } + // Old backend's root_change is not a specific function + } else { + if (!initialize_backend(ps)) { + log_fatal("Failed to re-initialize backend after root " + "change, aborting..."); + ps->quit = true; + // TODO only event handlers should request ev_break, + // otherwise it's too hard to keep track of what can break + // the event loop + ev_break(ps->loop, EVBREAK_ALL); + return; + } + + // Re-acquire the root pixmap. + root_damaged(ps); + } + force_repaint(ps); + } + return; +} + static void handle_root_flags(session_t *ps) { if ((ps->root_flags & ROOT_FLAGS_SCREEN_CHANGE) != 0) { if (ps->o.xinerama_shadow_crop) { @@ -415,6 +632,11 @@ static void handle_root_flags(session_t *ps) { } ps->root_flags &= ~(uint64_t)ROOT_FLAGS_SCREEN_CHANGE; } + + if ((ps->root_flags & ROOT_FLAGS_CONFIGURED) != 0) { + configure_root(ps); + ps->root_flags &= ~(uint64_t)ROOT_FLAGS_CONFIGURED; + } } static struct managed_win *paint_preprocess(session_t *ps, bool *fade_running) { @@ -646,216 +868,6 @@ static struct managed_win *paint_preprocess(session_t *ps, bool *fade_running) { return bottom; } -/** - * Rebuild cached screen_reg. - */ -static void rebuild_screen_reg(session_t *ps) { - get_screen_region(ps, &ps->screen_reg); -} - -/** - * Rebuild shadow_exclude_reg. - */ -static void rebuild_shadow_exclude_reg(session_t *ps) { - bool ret = parse_geometry(ps, ps->o.shadow_exclude_reg_str, &ps->shadow_exclude_reg); - if (!ret) - exit(1); -} - -/// Free up all the images and deinit the backend -static void destroy_backend(session_t *ps) { - win_stack_foreach_managed_safe(w, &ps->window_stack) { - // Wrapping up fading in progress - if (win_skip_fading(ps, w)) { - // `w` is freed by win_skip_fading - continue; - } - - if (ps->backend_data) { - if (w->state == WSTATE_MAPPED) { - win_release_images(ps->backend_data, w); - } else { - assert(!w->win_image); - assert(!w->shadow_image); - } - } - free_paint(ps, &w->paint); - } - - if (ps->backend_data && ps->root_image) { - ps->backend_data->ops->release_image(ps->backend_data, ps->root_image); - ps->root_image = NULL; - } - - if (ps->backend_data) { - // deinit backend - if (ps->backend_blur_context) { - ps->backend_data->ops->destroy_blur_context( - ps->backend_data, ps->backend_blur_context); - ps->backend_blur_context = NULL; - } - ps->backend_data->ops->deinit(ps->backend_data); - ps->backend_data = NULL; - } -} - -static bool initialize_blur(session_t *ps) { - struct kernel_blur_args kargs; - struct gaussian_blur_args gargs; - struct box_blur_args bargs; - - void *args = NULL; - switch (ps->o.blur_method) { - case BLUR_METHOD_BOX: - bargs.size = ps->o.blur_radius; - args = (void *)&bargs; - break; - case BLUR_METHOD_KERNEL: - kargs.kernel_count = ps->o.blur_kernel_count; - kargs.kernels = ps->o.blur_kerns; - args = (void *)&kargs; - break; - case BLUR_METHOD_GAUSSIAN: - gargs.size = ps->o.blur_radius; - gargs.deviation = ps->o.blur_deviation; - args = (void *)&gargs; - break; - default: return true; - } - - ps->backend_blur_context = ps->backend_data->ops->create_blur_context( - ps->backend_data, ps->o.blur_method, args); - return ps->backend_blur_context != NULL; -} - -/// Init the backend and bind all the window pixmap to backend images -static bool initialize_backend(session_t *ps) { - if (ps->o.experimental_backends) { - assert(!ps->backend_data); - // Reinitialize win_data - assert(backend_list[ps->o.backend]); - ps->backend_data = backend_list[ps->o.backend]->init(ps); - if (!ps->backend_data) { - log_fatal("Failed to initialize backend, aborting..."); - quit(ps); - return false; - } - ps->backend_data->ops = backend_list[ps->o.backend]; - - if (!initialize_blur(ps)) { - log_fatal("Failed to prepare for background blur, aborting..."); - ps->backend_data->ops->deinit(ps->backend_data); - ps->backend_data = NULL; - quit(ps); - return false; - } - - // window_stack shouldn't include window that's - // not in the hash table at this point. Since - // there cannot be any fading windows. - HASH_ITER2(ps->windows, _w) { - if (!_w->managed) { - continue; - } - auto w = (struct managed_win *)_w; - assert(w->state == WSTATE_MAPPED || w->state == WSTATE_UNMAPPED); - if (w->state == WSTATE_MAPPED) { - // We need to reacquire image - log_debug("Marking window %#010x (%s) for update after " - "redirection", - w->base.id, w->name); - if (w->shadow) { - struct color c = { - .red = ps->o.shadow_red, - .green = ps->o.shadow_green, - .blue = ps->o.shadow_blue, - .alpha = ps->o.shadow_opacity, - }; - win_bind_shadow(ps->backend_data, w, c, - ps->gaussian_map); - } - - w->flags |= WIN_FLAGS_PIXMAP_STALE; - ps->pending_updates = true; - } - } - } - - // The old backends binds pixmap lazily, nothing to do here - return true; -} - -/// Handle configure event of a root window -void configure_root(session_t *ps, int width, int height) { - log_info("Root configuration changed, new geometry: %dx%d", width, height); - bool has_root_change = false; - if (ps->redirected) { - // On root window changes - if (ps->o.experimental_backends) { - assert(ps->backend_data); - has_root_change = ps->backend_data->ops->root_change != NULL; - } else { - // Old backend can handle root change - has_root_change = true; - } - - if (!has_root_change) { - // deinit/reinit backend and free up resources if the backend - // cannot handle root change - destroy_backend(ps); - } - free_paint(ps, &ps->tgt_buffer); - } - - ps->root_width = width; - ps->root_height = height; - - rebuild_screen_reg(ps); - rebuild_shadow_exclude_reg(ps); - - // Invalidate reg_ignore from the top - auto top_w = win_stack_find_next_managed(ps, &ps->window_stack); - if (top_w) { - rc_region_unref(&top_w->reg_ignore); - top_w->reg_ignore_valid = false; - } - - if (ps->redirected) { - for (int i = 0; i < ps->ndamage; i++) { - pixman_region32_clear(&ps->damage_ring[i]); - } - ps->damage = ps->damage_ring + ps->ndamage - 1; -#ifdef CONFIG_OPENGL - // GLX root change callback - if (BKEND_GLX == ps->o.backend && !ps->o.experimental_backends) { - glx_on_root_change(ps); - } -#endif - if (has_root_change) { - if (ps->backend_data != NULL) { - ps->backend_data->ops->root_change(ps->backend_data, ps); - } - // Old backend's root_change is not a specific function - } else { - if (!initialize_backend(ps)) { - log_fatal("Failed to re-initialize backend after root " - "change, aborting..."); - ps->quit = true; - // TODO only event handlers should request ev_break, - // otherwise it's too hard to keep track of what can break - // the event loop - ev_break(ps->loop, EVBREAK_ALL); - return; - } - - // Re-acquire the root pixmap. - root_damaged(ps); - } - force_repaint(ps); - } - return; -} - void root_damaged(session_t *ps) { if (ps->root_tile_paint.pixmap) { free_root_tile(ps); @@ -1378,12 +1390,15 @@ static void handle_pending_updates(EV_P_ struct session *ps) { free(r); } + // Handle screen changes + // This HAS TO be called before refresh_stale_images, as handle_root_flags + // could call configure_root, which will release images and mark them + // stale. + handle_root_flags(ps); + // Refresh pixmaps and shadows refresh_stale_images(ps); - // Handle screen changes - handle_root_flags(ps); - e = xcb_request_check(ps->c, xcb_ungrab_server_checked(ps->c)); if (e) { log_fatal_x_error(e, "failed to ungrab x server"); diff --git a/src/picom.h b/src/picom.h index 45acdc3..fb64fda 100644 --- a/src/picom.h +++ b/src/picom.h @@ -25,7 +25,11 @@ #include "win.h" #include "x.h" -enum root_flags { ROOT_FLAGS_SCREEN_CHANGE = 1 }; +enum root_flags { + ROOT_FLAGS_SCREEN_CHANGE = 1, // Received RandR screen change notify, we + // use this to track refresh rate changes + ROOT_FLAGS_CONFIGURED = 2 // Received configure notify on the root window +}; // == Functions == // TODO move static inline functions that are only used in picom.c, into @@ -40,9 +44,6 @@ uint32_t determine_evmask(session_t *ps, xcb_window_t wid, win_evmode_t mode); xcb_window_t find_client_win(session_t *ps, xcb_window_t w); -/// Handle configure event of a root window -void configure_root(session_t *ps, int width, int height); - void circulate_win(session_t *ps, xcb_circulate_notify_event_t *ce); void update_refresh_rate(session_t *ps); diff --git a/tests/configs/issue357.conf b/tests/configs/issue357.conf new file mode 100644 index 0000000..00c3ba6 --- /dev/null +++ b/tests/configs/issue357.conf @@ -0,0 +1,3 @@ +fading = true; +fade-in-step = 1; +fade-out-step = 0.01; diff --git a/tests/run_tests.sh b/tests/run_tests.sh index 9f64442..89b3ab5 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -4,6 +4,7 @@ exe=$(realpath $1) cd $(dirname $0) ./run_one_test.sh $exe configs/empty.conf testcases/basic.py +./run_one_test.sh $exe configs/issue357.conf testcases/issue357.py ./run_one_test.sh $exe configs/issue239.conf testcases/issue239.py ./run_one_test.sh $exe configs/issue239_2.conf testcases/issue239_2.py ./run_one_test.sh $exe configs/issue239_3.conf testcases/issue239_3.py diff --git a/tests/testcases/issue357.py b/tests/testcases/issue357.py new file mode 100755 index 0000000..0d1859d --- /dev/null +++ b/tests/testcases/issue357.py @@ -0,0 +1,34 @@ +#!/usr/bin/env python + +import xcffib.xproto as xproto +import xcffib +import time +from common import set_window_name, trigger_root_configure + +conn = xcffib.connect() +setup = conn.get_setup() +root = setup.roots[0].root +visual = setup.roots[0].root_visual +depth = setup.roots[0].root_depth + +# issue 239 is caused by a window gaining a shadow during its fade-out transition +wid = conn.generate_id() +print("Window 1: ", hex(wid)) + +# Create a window +conn.core.CreateWindowChecked(depth, wid, root, 0, 0, 100, 100, 0, xproto.WindowClass.InputOutput, visual, 0, []).check() + +# Set Window name so it doesn't get a shadow +set_window_name(conn, wid, "Test window 1") + +# Map the window, causing picom to unredirect +print("mapping 1") +conn.core.MapWindowChecked(wid).check() +time.sleep(0.5) + +trigger_root_configure(conn) + +# Destroy the windows +conn.core.DestroyWindowChecked(wid).check() + +time.sleep(1)