fix binding the root background pixmap in case of depth mismatch #984

absolutelynothelix · 2022-12-25T14:20:35Z

previously we were using atoms set by other utilities to obtain the root back pixmap and the root's visual for it. however, their depths may mismatch. in this case picom will fail to bind the root back pixmap what causes errors, warnings and weird behavior on all backends:

$ build/src/picom --config=/dev/null --backend=xrender
[ 12/25/2022 17:15:37.319 x_create_picture_with_pictfmt_and_pixmap ERROR ] failed to create picture (X error 8 MATCH request 139 minor 4 serial 2232)
[ 12/25/2022 17:15:37.320 root_damaged ERROR ] Failed to bind root back pixmap
[ 12/25/2022 17:15:37.320 x_create_picture_with_pictfmt_and_pixmap ERROR ] failed to create picture (X error 8 MATCH request 139 minor 4 serial 2249)
[ 12/25/2022 17:15:37.320 root_damaged ERROR ] Failed to bind root back pixmap
...
$ build/src/picom --config=/dev/null --backend=glx
[ 12/25/2022 17:15:46.335 x_log_error WARN ] X error 8 MATCH request 152 minor 22 serial 725
[ 12/25/2022 17:15:46.335 x_log_error WARN ] X error 9 DRAWABLE request 156 minor 4 serial 726
[ 12/25/2022 17:15:46.335 x_log_error WARN ] X error 161 GLX_BAD_PIXMAP request 152 minor 16 serial 0
[ 12/25/2022 17:15:46.335 x_log_error WARN ] X error 161 GLX_BAD_PIXMAP request 152 minor 16 serial 0
[ 12/25/2022 17:15:46.336 x_log_error WARN ] X error 161 GLX_BAD_PIXMAP request 152 minor 23 serial 740
[ 12/25/2022 17:15:46.336 x_log_error WARN ] X error 8 MATCH request 152 minor 22 serial 743
[ 12/25/2022 17:15:46.336 x_log_error WARN ] X error 9 DRAWABLE request 156 minor 4 serial 744
[ 12/25/2022 17:15:46.336 x_log_error WARN ] X error 161 GLX_BAD_PIXMAP request 152 minor 16 serial 0
...
[ 12/25/2022 17:15:47.528 x_log_error WARN ] X error 161 GLX_BAD_PIXMAP request 152 minor 16 serial 0
[ 12/25/2022 17:15:47.528 x_log_error WARN ] X error 161 GLX_BAD_PIXMAP request 152 minor 23 serial 899
$ build/src/picom --config=/dev/null --legacy-backends --backend=xrender
[ 12/25/2022 17:16:12.626 x_create_picture_with_pictfmt_and_pixmap ERROR ] failed to create picture (X error 8 MATCH request 139 minor 4 serial 2258)
[ 12/25/2022 17:16:12.628 x_log_error WARN ] X error 143 RENDER_PICTURE request 139 minor 8 serial 2260
[ 12/25/2022 17:16:12.636 x_log_error WARN ] X error 143 RENDER_PICTURE request 139 minor 8 serial 2315
[ 12/25/2022 17:16:12.660 x_log_error WARN ] X error 143 RENDER_PICTURE request 139 minor 8 serial 2353
...
$ build/src/picom --config=/dev/null --legacy-backends --backend=glx
[ 12/25/2022 17:16:18.352 x_create_picture_with_pictfmt_and_pixmap ERROR ] failed to create picture (X error 8 MATCH request 139 minor 4 serial 2290)
[ 12/25/2022 17:16:18.353 x_log_error WARN ] X error 8 MATCH request 152 minor 22 serial 2293
[ 12/25/2022 17:16:18.353 x_log_error WARN ] X error 9 DRAWABLE request 156 minor 4 serial 2294
[ 12/25/2022 17:16:18.353 x_log_error WARN ] X error 161 GLX_BAD_PIXMAP request 152 minor 16 serial 0
...
[ 12/25/2022 17:16:19.580 x_log_error WARN ] X error 161 GLX_BAD_PIXMAP request 152 minor 16 serial 0
[ 12/25/2022 17:16:19.581 x_log_error WARN ] X error 161 GLX_BAD_PIXMAP request 152 minor 23 serial 2852

now we're using the root back pixmap's depth to obtain a suitable visual for it.

i've tested feh, hsetroot and nitrogen and they're all setting the right depth for the root back pixmap. however, at least xfdesktop (a part of the xfce desktop environment) always sets root back pixmap's depth to 32, when common root's depth is 24.

we could report this behavior upstream, but:

we're not sure if xfdesktop is completely wrong;
we're not sure if xfdesktop is the only one who does this;
there is a decent workaround for this and polybar already uses it.

if you think that this approach is wrong and there is a better way to workaround this, feel free to close this pull request. otherwise, feel free to request changes, i'll see what i can do.

related xfdesktop's code: https://github.com/xfce-mirror/xfdesktop/blob/f196763ba04211c7a3b21edba37c3b8f56b25516/src/xfce-desktop.c#L372-L374
related polybar's issue: polybar/polybar#2798

p.s. this pull request doesn't include a fix for legacy backends. it's an easy and pretty much the same fix for the legacy xrender backend, but legacy glx backend makes me doubt that fixing this worths the effort. if you want a fix for the legacy xrender backend included, hmu.

codecov · 2022-12-25T14:21:47Z

Codecov Report

Attention: 26 lines in your changes are missing coverage. Please review.

Comparison is base (90f5f4c) 36.46% compared to head (4a79e7b) 36.57%.

Additional details and impacted files

@@            Coverage Diff             @@
##             next     #984       /-   ##
==========================================
  Coverage   36.46%   36.57%    0.10%     
==========================================
  Files          50       50              
  Lines       11523    11533       10     
==========================================
  Hits         4202     4218       16     
  Misses       7321     7315       -6

Files	Coverage Δ
src/x.h	`87.50% <ø> (ø)`
src/x.c	`44.63% <0.00%> ( 0.09%)`	⬆️
src/picom.c	`62.15% <0.00%> ( 0.16%)`	⬆️
src/render.c	`0.57% <0.00%> (-0.01%)`	⬇️

... and 1 file with indirect coverage changes

yshui · 2022-12-25T19:59:16Z

src/picom.c

 			ps->root_image = ps->backend_data->ops->bind_pixmap(
-			    ps->backend_data, pixmap, x_get_visual_info(ps->c, ps->vis), false);
+			    ps->backend_data, pixmap,
+			    x_get_visual_info(ps->c, x_get_visual_for_depth(ps->c, r->depth)),


i had to do some research to figure out why we couldn't do the same as win_bind_pixmap and use the pixmap's visual directly.

turns out the visual is part of the window's attributes, and GetWindowAttributes doesn't work with a pixmap. can you add a comment to record this fact? thanks.

i've tried to add a descriptive comment of what we are doing and why.

yshui · 2022-12-25T20:05:37Z

In CreateWindow request, it is stated that:

If background-pixmap is given, it overrides the default background-pixmap. The background pixmap and the window must have the same root and the same depth (or a Match error results).

yshui · 2022-12-25T20:13:09Z

I can confirm ChangeWindowAttributes has this check as well. how is xfdesktop even able to set the background pixmap to a pixmap with a different depth?

absolutelynothelix · 2022-12-25T20:39:51Z

I can confirm ChangeWindowAttributes has this check as well. how is xfdesktop even able to set the background pixmap to a pixmap with a different depth?

xfce tricks ;)

i think that the root pixmap atom could point to any pixmap, not only the one that is associated with the root window (if i understand how it all works correctly). when setting background with hsetroot, for example, doing xprop on the desktop returns information about the root window. launching xfdesktop seems to create a kind of overlay window over the root window and xprop on the desktop returns information about it.

hsetroot, then xprop on the desktop: http://0x0.st/o56n.log
xfdesktop, then xprop on the desktop, then xprop -root: http://0x0.st/o565.log

i wonder what other des do to set the desktop background. i suspect it may be a common practice, except that they're setting visuals matching display's screen.

~~update: gnome seems to do the same thing.~~

yshui · 2022-12-25T22:07:51Z

Ah I suppose it sets _XROOTPMAP_ID to something that's not actually the background pixmap of the root window?

that's evil :'(

absolutelynothelix · 2022-12-26T00:47:32Z

Ah I suppose it sets _XROOTPMAP_ID to something that's not actually the background pixmap of the root window?

that's evil :'(

i believe that xfdesktop creates it's desktop window, manages it and sets _XROOTPMAP_ID to it's pixmap. and i assume that other desktop environments act the same, while utilities like feh, hsetroot, nitrogen, etc. are managing the real root window. xfdesktop is indeed evil, see #982 :D

absolutelynothelix · 2022-12-26T12:47:19Z

makes sense.

$ xrestop -b -m 1
...
2 - Desktop ( PID: 7652 ):
	res_base      : 0x1200000
...
$ xprop -root _XROOTPMAP_ID
_XROOTPMAP_ID(PIXMAP): pixmap id # 0x120000f

absolutelynothelix · 2022-12-27T20:03:51Z

i thought that we could compare the root back pixmap's depth and the root depth and use the root visual if they match. but imho it adds complexity to the code and i doubt that we're winning anything because root_damaged isn't called that often. also we anyway have to get pixmap's geometry to obtain it's depth. however, if you really want me to use this approach, hmu and i'll use it.

yshui · 2023-01-13T16:48:06Z

thanks. there is one more thing i am worried about.

a visual with the matching depth is not necessarily the right visual. e.g. 32-bit depth can be ARGB8, or RGB10A2. is there no chance of fixing this on xfdesktop's side?

obviously there is no standard/specification to say who is actually correct here, but i can't think of a non-hacky way of fixing this on our side.

absolutelynothelix · 2023-01-13T17:43:52Z

a visual with the matching depth is not necessarily the right visual. e.g. 32-bit depth can be ARGB8, or RGB10A2.

that's a good point. but if the xcb_depth_visuals_iterator function iterates from the first visual (in terms of id) it's very unlikely because it seems that the available visuals are sorted from the most common to the most exotic (iirc the default rgb24 visual is the first available visual and the default argb32 visual is like the 10th available visual).

is there no chance of fixing this on xfdesktop's side?

we could try to report this behavior upstream but you've reminded me about a funny xfdesktop's option when setting a background called transparent. if you set a 1920x1080 wallpaper with this option it will be set just fine but if you then set a, say, 640x480 wallpaper, it will overlap the previous one. i believe it's transparent to the root window, so you can set a wallpaper with hsetroot then with xfdesktop and you'll see both of them (if the wallpaper set with xfdesktop doesn't completely cover the wallpaper set with hsetroot, ofc). that's a weird feature, but it exists.

i can't think of a non-hacky way of fixing this on our side

in the end, this fix reduces the chance of a root back pixmap binding failure. if you really want to eliminate it you may ping-pong a visual between x_get_visual_for_depth and x_get_visual_info unless you're sure that you got the right visual, but, as you've said, it's hacky (and, probably, not very performant, but, as i said before, suitable visuals are usually the first available visuals).

yshui · 2023-01-13T17:52:28Z

it's very unlikely because it seems that the available visuals are sorted from the most common to the most exotic

I don't want to rely on this assumption.

a funny xfdesktop's option when setting a background: transparent.

I think this is irrelevant. What they put in _XROOTPMAP_ID has to be the background pixmap of the root window. xfdesktop is arguably breaking the contract here.

until you're sure that you got a right visual

There's no way for picom to know. The pixmap might bind successfully but will look wrong. (like if you bind a ARGB8 pixmap with a RGB10A2 visual)

absolutelynothelix · 2023-01-13T18:08:40Z

I don't want to rely on this assumption.

ikr, you can never trust the x.org.

What they put in _XROOTPMAP_ID has to be the background pixmap of the root window. xfdesktop is arguably breaking the contract here.

unfortunately, looks like it's not only xfce. talking about the "contract": is _XROOTPMAP_ID a standard?

There's no way for picom to know. The pixmap might bind successfully but will look wrong. (like if you bind a ARGB8 pixmap with a RGB10A2 visual)

there is a way. we won't rely on the pixmap binding failure as a search criteria for the right visual. instead we could

get visual info for the root visual using the x_get_visual_info function so we will know the component sizes and order;
iterate over available visuals for the root back pixmap's depth, get their details with x_get_visual_info as well and compare them to the root visual details.

that's what i've meant with

ping-pong a visual between x_get_visual_for_depth and x_get_visual_info

yshui · 2023-01-13T23:51:04Z

looks like it's not only xfce.

there are more 😱 ?!

is _XROOTPMAP_ID a standard?

unfortunately no.

instead we could [snip]

no this isn't reliable either. when the pixmap is detached from the root window, we can't assume anything. the component size can be different too.

absolutelynothelix · 2023-01-14T15:54:04Z

there are more 😱 ?!

i've tested some desktop environments and here are the results:
"the blessed" - mate, gnome - they set both _XROOTPMAP_ID and ESETROOT_PMAP_ID to the same value which seems to point to a "good" pixmap (not owned by a particular process)
"the neutral" - kde plasma 5, moksha, pantheon - they don't set any atoms pointing to the root pixmap at all
"the cursed" - lxqt - ???

p.s. sorry about the last force pushes to both of my open pull requests, i'm learning git :D

it returns the first found visual for the given depth

absolutelynothelix · 2024-02-01T04:27:04Z

rebased on top of the latest commit in the next branch and refactored.

x: add the x_get_visual_for_depth function

it returns the first found visual for the given depth

picom: fix binding the root background pixmap in case of depth mismatch

if the root background pixmap's depth doesn't match the root window's
depth, find a suitable visual for the root background pixmap's depth and
use it instead of the root window's visual

render: fix binding the root background pixmap in case of depth mismatch

fix the same issue in the legacy backends, see the previous commit for
details. this commit also removes the x_validate_pixmap function because
it was used only in the get_root_tile function and the fix pretty much
implies and embeds it.

src/picom.c

if the root background pixmap's depth doesn't match the root window's depth, find a suitable visual for the root background pixmap's depth and use it instead of the root window's visual

fix the same issue in the legacy backends, see the previous commit for details. this commit also removes the x_validate_pixmap function because it was used only in the get_root_tile function and the fix pretty much implies and embeds it.

yshui

lgtm

yshui reviewed Dec 25, 2022

View reviewed changes

absolutelynothelix force-pushed the fix-binding-root-back-pixmap branch from 07f9516 to 64683b7 Compare December 26, 2022 10:22

yshui mentioned this pull request Dec 27, 2022

glx backend segfaults when killing xfdesktop #982

Closed

absolutelynothelix force-pushed the fix-binding-root-back-pixmap branch from 64683b7 to c2cc510 Compare December 30, 2022 02:35

absolutelynothelix marked this pull request as draft January 11, 2023 11:37

absolutelynothelix force-pushed the fix-binding-root-back-pixmap branch from c2cc510 to ee80fc8 Compare January 12, 2023 10:59

absolutelynothelix changed the title ~~picom: fix binding root back pixmap in case of depth mismatch~~ fix binding root back pixmap in case of depth mismatch Jan 12, 2023

absolutelynothelix changed the title ~~fix binding root back pixmap in case of depth mismatch~~ fix root back pixmap binding in case of depth mismatch Jan 12, 2023

absolutelynothelix marked this pull request as ready for review January 12, 2023 11:22

absolutelynothelix force-pushed the fix-binding-root-back-pixmap branch from ee80fc8 to 1f12f87 Compare January 14, 2023 14:32

This was referenced Jan 22, 2023

picom fails to reinitialize itself on config update on xfce #392

Closed

Picom exits on configuration file change #421

Closed

Cannot build picom #908

Closed

absolutelynothelix closed this Feb 23, 2023

absolutelynothelix deleted the fix-binding-root-back-pixmap branch February 23, 2023 01:20

absolutelynothelix restored the fix-binding-root-back-pixmap branch February 23, 2023 01:20

absolutelynothelix reopened this Feb 23, 2023

absolutelynothelix force-pushed the fix-binding-root-back-pixmap branch from 1f12f87 to faa92d9 Compare February 23, 2023 01:31

absolutelynothelix changed the title ~~fix root back pixmap binding in case of depth mismatch~~ fix binding root back pixmap in case of depth mismatch Feb 23, 2023

absolutelynothelix mentioned this pull request Apr 9, 2023

Picom crashes on Intel GPU when I change the config file #1045

Closed

absolutelynothelix marked this pull request as draft June 17, 2023 14:15

absolutelynothelix mentioned this pull request Dec 29, 2023

x_create_picture_with_pictfmt_and_pixmap ERROR, root_damaged ERROR #1159

Closed

x: add the x_get_visual_for_depth function

4401666

it returns the first found visual for the given depth

absolutelynothelix force-pushed the fix-binding-root-back-pixmap branch from faa92d9 to 18cb511 Compare February 1, 2024 04:21

absolutelynothelix changed the title ~~fix binding root back pixmap in case of depth mismatch~~ fix binding the root background pixmap in case of depth mismatch Feb 1, 2024

absolutelynothelix marked this pull request as ready for review February 1, 2024 04:27

absolutelynothelix requested a review from yshui February 1, 2024 04:27

yshui reviewed Feb 1, 2024

View reviewed changes

src/picom.c Outdated Show resolved Hide resolved

absolutelynothelix added 2 commits February 2, 2024 02:04

picom: fix binding the root background pixmap in case of depth mismatch

a655730

if the root background pixmap's depth doesn't match the root window's depth, find a suitable visual for the root background pixmap's depth and use it instead of the root window's visual

absolutelynothelix force-pushed the fix-binding-root-back-pixmap branch from 18cb511 to 4a79e7b Compare February 1, 2024 23:06

yshui approved these changes Feb 2, 2024

View reviewed changes

yshui merged commit bbc657e into yshui:next Feb 2, 2024
16 checks passed

absolutelynothelix deleted the fix-binding-root-back-pixmap branch February 2, 2024 10:45

yshui mentioned this pull request Feb 9, 2024

Unexpectedbalck root screen as well a 100% CPU load #1150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix binding the root background pixmap in case of depth mismatch #984

fix binding the root background pixmap in case of depth mismatch #984

absolutelynothelix commented Dec 25, 2022 •

edited

Loading

codecov bot commented Dec 25, 2022 •

edited

Loading

yshui Dec 25, 2022

absolutelynothelix Dec 26, 2022 •

edited

Loading

yshui commented Dec 25, 2022

yshui commented Dec 25, 2022

absolutelynothelix commented Dec 25, 2022 •

edited

Loading

yshui commented Dec 25, 2022

absolutelynothelix commented Dec 26, 2022 •

edited

Loading

absolutelynothelix commented Dec 26, 2022 •

edited

Loading

absolutelynothelix commented Dec 27, 2022 •

edited

Loading

yshui commented Jan 13, 2023

absolutelynothelix commented Jan 13, 2023 •

edited

Loading

yshui commented Jan 13, 2023

absolutelynothelix commented Jan 13, 2023

yshui commented Jan 13, 2023

absolutelynothelix commented Jan 14, 2023 •

edited

Loading

absolutelynothelix commented Feb 1, 2024

yshui left a comment

fix binding the root background pixmap in case of depth mismatch #984

fix binding the root background pixmap in case of depth mismatch #984

Conversation

absolutelynothelix commented Dec 25, 2022 • edited Loading

codecov bot commented Dec 25, 2022 • edited Loading

Codecov Report

yshui Dec 25, 2022

Choose a reason for hiding this comment

absolutelynothelix Dec 26, 2022 • edited Loading

Choose a reason for hiding this comment

yshui commented Dec 25, 2022

yshui commented Dec 25, 2022

absolutelynothelix commented Dec 25, 2022 • edited Loading

yshui commented Dec 25, 2022

absolutelynothelix commented Dec 26, 2022 • edited Loading

absolutelynothelix commented Dec 26, 2022 • edited Loading

absolutelynothelix commented Dec 27, 2022 • edited Loading

yshui commented Jan 13, 2023

absolutelynothelix commented Jan 13, 2023 • edited Loading

yshui commented Jan 13, 2023

absolutelynothelix commented Jan 13, 2023

yshui commented Jan 13, 2023

absolutelynothelix commented Jan 14, 2023 • edited Loading

absolutelynothelix commented Feb 1, 2024

x: add the x_get_visual_for_depth function

picom: fix binding the root background pixmap in case of depth mismatch

render: fix binding the root background pixmap in case of depth mismatch

yshui left a comment

Choose a reason for hiding this comment

absolutelynothelix commented Dec 25, 2022 •

edited

Loading

codecov bot commented Dec 25, 2022 •

edited

Loading

absolutelynothelix Dec 26, 2022 •

edited

Loading

absolutelynothelix commented Dec 25, 2022 •

edited

Loading

absolutelynothelix commented Dec 26, 2022 •

edited

Loading

absolutelynothelix commented Dec 26, 2022 •

edited

Loading

absolutelynothelix commented Dec 27, 2022 •

edited

Loading

absolutelynothelix commented Jan 13, 2023 •

edited

Loading

absolutelynothelix commented Jan 14, 2023 •

edited

Loading