From d4e76b271acd46b0587f739776efecf48d91553f Mon Sep 17 00:00:00 2001 From: Yuxuan Shui Date: Mon, 18 Nov 2019 22:34:05 +0000 Subject: [PATCH] win: delayed release of shadow image Previously win_set_shadow tries to release the shadow image when turning off shadow for a window. When shadow is turned off _immediately_ after it's turned on, picom won't have a chance to handle the delayed creation of the shadow before win_set_shadow tries to release the shadow image, causing a assertion failure because win_set_shadow tried to release a non-existing image. This commit makes releasing the shadow image delayed as well. In theory, we could check the STALE flag in win_set_shadow before release the image, but that duplicates the logic that is already in win_process_flags. Signed-off-by: Yuxuan Shui --- src/win.c | 47 +++++++++++++++------- tests/run_tests.sh | 1 + tests/testcases/issue239_3.py | 2 + tests/testcases/issue239_3_norefresh.py | 52 +++++++++++++++++++++++++ 4 files changed, 89 insertions(+), 13 deletions(-) create mode 100755 tests/testcases/issue239_3_norefresh.py diff --git a/src/win.c b/src/win.c index 7ec5ff3..42d9c28 100644 --- a/src/win.c +++ b/src/win.c @@ -695,34 +695,55 @@ static void win_set_shadow(session_t *ps, struct managed_win *w, bool shadow_new log_debug("Updating shadow property of window %#010x (%s) to %d", w->base.id, w->name, shadow_new); + if (w->state == WSTATE_UNMAPPED) { + // No need to add damage or update shadow + // Unmapped window shouldn't have any images + w->shadow = shadow_new; + assert(!w->shadow_image); + assert(!w->win_image); + assert(w->flags & WIN_FLAGS_IMAGES_NONE); + return; + } + + // Keep a copy of window extent before the shadow change. Will be used for + // calculation of damaged region region_t extents; pixman_region32_init(&extents); win_extents(w, &extents); + // Apply the shadow change w->shadow = shadow_new; + // Add damage for shadow change + // Window extents need update on shadow state change // Shadow geometry currently doesn't change on shadow state change // calc_shadow_geometry(ps, w); - // Mark the old extents as damaged if the shadow is removed - if (!w->shadow) { - add_damage(ps, &extents); - win_release_shadow(ps->backend_data, w); - } - pixman_region32_clear(&extents); - // Mark the new extents as damaged if the shadow is added + // Note: because the release and creation of the shadow images are delayed. When + // multiple shadow changes happen in a row, without rendering phase between them, + // there could be a stale shadow image attached to the window even if w->shadow + // was previously false. And vice versa. So we check the STALE flag before + // asserting the existence of the shadow image. if (w->shadow) { + // Mark the new extents as damaged if the shadow is added + assert(!w->shadow_image || (w->flags & WIN_FLAGS_SHADOW_STALE)); + pixman_region32_clear(&extents); win_extents(w, &extents); add_damage_from_win(ps, w); - if (w->state != WSTATE_UNMAPPED) { - assert(!w->shadow_image); - // Delayed creation of shadow image - w->flags |= WIN_FLAGS_SHADOW_STALE; - ps->pending_updates = true; - } + } else { + // Mark the old extents as damaged if the shadow is removed + assert(w->shadow_image || (w->flags & WIN_FLAGS_SHADOW_STALE)); + add_damage(ps, &extents); } + pixman_region32_fini(&extents); + + // Delayed update of shadow image + // By setting WIN_FLAGS_SHADOW_STALE, we ask win_process_flags to re-create or + // release the shaodw in based on whether w->shadow is set. + w->flags |= WIN_FLAGS_SHADOW_STALE; + ps->pending_updates = true; } /** diff --git a/tests/run_tests.sh b/tests/run_tests.sh index fd499b9..9f64442 100755 --- a/tests/run_tests.sh +++ b/tests/run_tests.sh @@ -7,3 +7,4 @@ cd $(dirname $0) ./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 +./run_one_test.sh $exe configs/issue239_3.conf testcases/issue239_3_norefresh.py diff --git a/tests/testcases/issue239_3.py b/tests/testcases/issue239_3.py index 9e9bc2f..c3d6e15 100755 --- a/tests/testcases/issue239_3.py +++ b/tests/testcases/issue239_3.py @@ -36,6 +36,8 @@ print("set new name") win_name = "NoShadow" conn.core.ChangePropertyChecked(xproto.PropMode.Replace, wid, name_atom, str_type_atom, 8, len(win_name), win_name).check() +time.sleep(0.5) + # Set the Window name so it gets a shadow print("set new name") win_name = "YesShadow" diff --git a/tests/testcases/issue239_3_norefresh.py b/tests/testcases/issue239_3_norefresh.py new file mode 100755 index 0000000..9e9bc2f --- /dev/null +++ b/tests/testcases/issue239_3_norefresh.py @@ -0,0 +1,52 @@ +#!/usr/bin/env python + +import xcffib.xproto as xproto +import xcffib +import time + +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 id is ", 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 +name = "_NET_WM_NAME" +name_atom = conn.core.InternAtom(True, len(name), name).reply().atom +str_type = "STRING" +str_type_atom = conn.core.InternAtom(True, len(str_type), str_type).reply().atom + +win_name = "YesShadow" +conn.core.ChangePropertyChecked(xproto.PropMode.Replace, wid, name_atom, str_type_atom, 8, len(win_name), win_name).check() + +# Map the window +print("mapping") +conn.core.MapWindowChecked(wid).check() + +time.sleep(0.5) + +print("set new name") +win_name = "NoShadow" +conn.core.ChangePropertyChecked(xproto.PropMode.Replace, wid, name_atom, str_type_atom, 8, len(win_name), win_name).check() + +# Set the Window name so it gets a shadow +print("set new name") +win_name = "YesShadow" +conn.core.ChangePropertyChecked(xproto.PropMode.Replace, wid, name_atom, str_type_atom, 8, len(win_name), win_name).check() + +time.sleep(0.5) + +# Unmap the window +conn.core.UnmapWindowChecked(wid).check() + +time.sleep(0.5) + +# Destroy the window +conn.core.DestroyWindowChecked(wid).check()