win: rework how events for unmapped windows are handled #546
Conversation
|
@tryone144 would appreciate if you can give this a look, thanks. |
Codecov Report
@@ Coverage Diff @@
## next #546 +/- ##
==========================================
+ Coverage 37.76% 38.21% +0.44%
==========================================
Files 46 46
Lines 9055 9056 +1
==========================================
+ Hits 3420 3461 +41
+ Misses 5635 5595 -40
|
|
Observed a crash on this. investigating... Update: fixed. |
yshui
force-pushed
the
window-state-update
branch
4 times, most recently
from
e855586
to
5286bdb
Compare
There was a problem hiding this comment.
Added some comments to clarify. But looks good overall.
- Some questions left regarding
destroy_backend(). - We could add assertion checks to, e.g.,
win_on_win_size_change()to be only called on "mapped" windows like already done inwin_set_shadow(). - Is the xinerama screen properly initialized in
fill_winor by a configure event, or does this introduce a regression? - Todo note in
map_win_start()regardingwin_on_factor_change().
src/picom.c
Outdated
| if (w->state == WSTATE_MAPPED) { | ||
| win_release_images(ps->backend_data, w); | ||
| } else { | ||
| // Unmapped windows could still have shadow images. | ||
| // And they might have image stale flags set by events. | ||
| assert(!w->win_image); | ||
| assert(!w->shadow_image); | ||
| win_clear_flags( | ||
| w, WIN_FLAGS_PIXMAP_STALE | WIN_FLAGS_SHADOW_STALE); | ||
| win_release_images(ps->backend_data, w); |
There was a problem hiding this comment.
I think mapped windows can also have stale shadows (related to #395 and #479 (comment)) letting win_release_images() fail. Back then I couldn't reproduce this artificially, though.
There was a problem hiding this comment.
Interesting, I wonder why. I think destroy_backend can only be reached after the critical section, so the stale flags should already be handled for mapped windows.
There was a problem hiding this comment.
paint_preprocess()->unredirect()->destroy_backend()is called after the critical section before rendering and should be safe.session_destroy()->unredirect()->destroy_backend()is called inmainaftersession_run()returns. Are we safe there?handle_root_flags()->configure_root()->destroy_backend()is called IN the critical section BEFORE the stale flags have been handled. This might fail if pixmaps get invalidated during a root-change (i.e. screen size changes?). I have a testcase for that, but I can't get the timing right to trigger this.
There was a problem hiding this comment.
You are right, 2 and 3 are not safe.
I don't see a good solution beside always clear the stale flags, so that's what I'll do.
There was a problem hiding this comment.
Can you post your testcase for 3 somewhere? Thanks.
There was a problem hiding this comment.
Can you post your testcase for 3 somewhere? Thanks.
https://gist.github.com/tryone144/1103f5107448fecf5e7e02a6cf4ffb02
There was a problem hiding this comment.
Hmm, looking at the code, I think IMAGES_STALE flags are only set by win_process_update_flags, and immediately handled later in win_process_image_flags; except for one case, where map_win_start is called in handle_new_windows.
That seems to be the only case where it's possible for destroy_backend to see a image stale flag.
There was a problem hiding this comment.
Looks like with this PR, we don't have to call map_win_start in handle_new_windows anymore. So we can eliminate case 3 (if my previous point is correct).
There was a problem hiding this comment.
@tryone144 i'm going to merge this for now.
yshui
force-pushed
the
window-state-update
branch
from
5286bdb
to
063a352
Compare
| // TODO(yshui) can we just replace calls below with win_on_factor_change? | ||
|
|
||
| // Update window focus state | ||
| win_update_focused(ps, w); |
There was a problem hiding this comment.
this is called in win_process_update_flags -> win_on_factor_change
|
I deleted two more update calls from |
yshui
force-pushed
the
window-state-update
branch
2 times, most recently
from
3f48ea6
to
f1da2c2
Compare
|
@tryone144 Actually, my impression about what caused #525 is wrong. It's not a window received an update when it's unmapped, it happens because a window's pixmap is invalidated, then the window immediately got unmapped. This PR should still fix the problem though. |
Right. It's just my impression of what happens first was wrong. |
Reference: #546 (comment) Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
Make unmapped window events work mostly like a mapped window, except flags set on unmapped windows aren't processed until the window is mapped. Hopefully this unifies some of the code paths and reduce corner cases. Should fix #525 Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
While working on this PR, I introduced a bug where shadow images for unmapped windows aren't properly recreated after unredirect/redirect. The shadow image is freed during unredirect, OTOH redirect only set IMAGE_STALE flags for mapped window, thus the shadow images for unmapped windows will be missing. This bug is already fixed in the previous commit. But the testcase is good to keep nonetheless. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
Reference: #546 (comment) Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
yshui
force-pushed
the
window-state-update
branch
from
bdf5770
to
e49ef2c
Compare
Set WIN_FLAGS_MAPPED instead, it will be handled later. Previously, we call map_win_start because we need the geometry of the window, which is updated in map_win_start. Now, we get the geometry in fill_win, so map_win_start is not needed anymore. Eliminate a case where destroy_backend could see IMAGES_STALE flags set on windows. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
yshui
force-pushed
the
window-state-update
branch
from
3cac24c
to
60f733d
Compare
After 60f733d, we can be sure stale flags won't be seen in destroy_backend. Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
Reference: yshui#546 (comment) Signed-off-by: Yuxuan Shui <yshuiv7@gmail.com>
Make unmapped window events work mostly like a mapped window, except
flags set on unmapped windows aren't processed until the window is
mapped.
Hopefully this unifies some of the code paths and reduce corner cases.
Should fix #525
Signed-off-by: Yuxuan Shui yshuiv7@gmail.com
TODO