picom icon indicating copy to clipboard operation
picom copied to clipboard

Delay (re-)creation of the mask image, uncouple from shadow image

Open tryone144 opened this issue 1 year ago • 4 comments

Move the creation / binding of mask images into the critical section. Delay binding and release with WINDOW_FLAGS_MASK_STALE (part of WINDOW_FLAGS_IMAGES_STALE) analog to the handling of shadow images.

No longer bind the mask image when creating the shadow as the mask might not have changed and therefore not been released before. (Previously, the mask might have been already created in paint_all_new)

Bind the mask before binding the shadow, so we can use it for shadow creating. Only bind when corner-radius is set or the window is shaped.

should fix: https://github.com/yshui/picom/issues/1029


This fixes the assertion failure in win_bind_mask() to protect against leaking a mask image.

  • When a window is created without shadow but rounded corners, paint_all_new() will create and bind a (new) mask image.
  • Once the window receives shadow, a new shadow image is created.
  • If the window has rounded corners, first a (new) mask image is bound, and then the shadow created and bound based on that mask (if supported by the backend).
  • This triggers the assertion, since the mask image is already bound (in paint_all_new())

Can be reproduced with:

$ picom --config /dev/null --backend glx --shadow --shadow-exclude focused --corner-radius 15

Currently, no automated test can be written for this case, because the dummy backend does not support creating the shadow image from a mask image and therefore will never go through the affected code path.

tryone144 avatar Mar 13 '23 00:03 tryone144

Codecov Report

Merging #1035 (932bbab) into next (cee1287) will decrease coverage by 0.09%. The diff coverage is 27.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #1035      +/-   ##
==========================================
- Coverage   37.82%   37.74%   -0.09%     
==========================================
  Files          48       48              
  Lines       10844    10862      +18     
==========================================
- Hits         4102     4100       -2     
- Misses       6742     6762      +20     
Impacted Files Coverage Δ
src/backend/backend.c 59.78% <ø> (+0.10%) :arrow_up:
src/win.c 67.22% <27.77%> (-1.03%) :arrow_down:

... and 1 file with indirect coverage changes

codecov[bot] avatar Mar 13 '23 00:03 codecov[bot]

@yshui I am not sure if there are pressing reasons to delay the update of the mask image similar to the shadow image in the critical section. If not, we might want to keep the mask creation in paint_all_new and instead just check whether the mask image already exists when creating the shadow.

tryone144 avatar Mar 13 '23 00:03 tryone144

Hmm, creating the mask image in critical section makes me a bit uneasy. In the critical section we grab the X server and pause its processing of other client's requests. Creating the mask image can take a (relatively) long time, as it needs to render the mask, it's probably not a good idea to grab the server for that long.

yshui avatar Mar 29 '23 14:03 yshui

looks good, builds and works for me. the only thing i'd personally change is to rename corner_radius_new to new_corner_radius as it sounds better.

absolutelynothelix avatar Apr 13 '23 18:04 absolutelynothelix