Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: next
Choose a base branch
from

Conversation

tryone144
Copy link
Collaborator

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: #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.

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.

Bind the mask before binding the shadow. Only bind when corner-radius is
set or the window is shaped.

related: 9d7cbe4
@codecov
Copy link

codecov bot commented Mar 13, 2023

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%) ⬆️
src/win.c 67.22% <27.77%> (-1.03%) ⬇️

... and 1 file with indirect coverage changes

@tryone144
Copy link
Collaborator Author

@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 tryone144 requested a review from yshui March 13, 2023 00:08
@yshui
Copy link
Owner

yshui commented Mar 29, 2023

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.

@absolutelynothelix
Copy link
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Picom crashes upon opening new firefox tab
3 participants