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

Wayland: problems with color cursors with semi transparency #4856

Closed
ernstp opened this issue Oct 20, 2021 · 8 comments
Closed

Wayland: problems with color cursors with semi transparency #4856

ernstp opened this issue Oct 20, 2021 · 8 comments
Assignees
Milestone

Comments

@ernstp
Copy link

ernstp commented Oct 20, 2021

When loading a ColorCursor with semi transparent elements they get blended with white (?) when over the SDL window when running with the SDL Wayland backend. But not over the rest of the Gnome Wayland desktop.
When using the x11 backend on Wayland the problem does not appear.

I first found it in a game but I reproduced it with the example program from https://wiki.libsdl.org/SDL_CreateColorCursor and this fantastic cursor:
cursor.bmp.gz

It doesn't seem to be affected by EGL_PRESENT_OPAQUE_EXT and allow_transparent etc.

Tested with SDL built from master ("Fixed grab handling when focus changes..."), mutter 40.5-1ubuntu2, mesa 21.3.0~rc1, 5.15-rc3.
AMDGPU, Radeon 6800 XT.

You can also reproduce this with Stellaris and forcing wayland.

@ernstp
Copy link
Author

ernstp commented Oct 20, 2021

Same with kernel 5.13 and Plasma Wayland.
Plasma Wayland + videodriver Wayland
bild

Plasma Wayland + videodriver X11.
bild

@sulix
Copy link
Contributor

sulix commented Oct 21, 2021

Huh, on my system (KWin+Mesa/Intel), this works weirdly as well. There's a white-coloured fringe around the cursor under wayland, and the whole cursor is blue, not green, (and seems to only have 1-bit alpha) under XWayland.

However, under nVidia with KWin, it works perfectly. Though, nVidia/Wayland (at least under KWin with EGLStreams) uses software cursors as a fallback.

So it seems like this is probably an issue with the graphics drivers as much as SDL. Then again, it looks like Xwayland is doing basically the same thing as SDL — creating an SHM surface of format WL_SHM_ARGB8888 and memcpy-ing the pointer data in:
https://gitlab.freedesktop.org/xorg/xserver/-/blob/master/hw/xwayland/xwayland-cursor.c
(It does also use zwp_tablet_tool_v2_set_cursor, but my guess is that should only affect tablets?)

@flibitijibibo
Copy link
Collaborator

Our implementation, for reference: https://github.com/libsdl-org/SDL/blob/main/src/video/wayland/SDL_waylandmouse.c

Basically the whole mouse file is for cursors and nothing else, should be able to scroll from top to bottom with little other code getting in the way.

@flibitijibibo flibitijibibo self-assigned this Oct 22, 2021
@flibitijibibo flibitijibibo added this to the 2.0.18 milestone Oct 22, 2021
@sulix
Copy link
Contributor

sulix commented Oct 22, 2021

Turns out this is just a premultiplied-alpha issue: wayland seems to require premultiplied alpha for its various alpha-blended formats.

KMSDRM also has to handle this for its cursor implementation, and has an existing helper function:
https://github.com/libsdl-org/SDL/blob/main/src/video/kmsdrm/SDL_kmsdrmmouse.c#L67

I have a hacky implemention which works here, using a variant of that helper: I'll tidy it up and send in a PR. It may be worth properly sharing the helper function between KMSDRM and Wayland (and any other target which may end up needing premultiplied alpha somewhere). I don't like how KMSDRM's version does a number of seemingly needless copies, then premultiplies it in-place, too, though hopefully cursor creation isn't anyone's performance bottleneck.

sulix added a commit to sulix/SDL that referenced this issue Oct 22, 2021
It turns out that Wayland's WL_SHM_FORMAT_ARGB8888 format (and, indeed,
all wayland RGBA formats) should be treated as premultiplied. SDL
surfaces tend not to be premultiplied, and this is assumed by other
backends when dealing with cursors.

This change premultiplies the cursor surface in Wayland_CreateCursor().
A helper function -- premultiply_surface_alpha_ARGB8888 -- is created to
do the conversion, based somewhat on the e similar
legacy_alpha_premultiply_ARGB8888 function in KMSDRM, which has the same
requirement to have premultiplied alpha for the cursor.

This should fix libsdl-org#4856
@slouken
Copy link
Collaborator

slouken commented Oct 22, 2021

Shared code for premultiplied alpha in src/video seems like a good idea.

sulix added a commit to sulix/SDL that referenced this issue Oct 22, 2021
It turns out that Wayland's WL_SHM_FORMAT_ARGB8888 format (and, indeed,
all wayland RGBA formats) should be treated as premultiplied. SDL
surfaces tend not to be premultiplied, and this is assumed by other
backends when dealing with cursors.

This change premultiplies the cursor surface in Wayland_CreateCursor().
A helper function -- premultiply_surface_alpha_ARGB8888 -- is created to
do the conversion, based somewhat on the e similar
legacy_alpha_premultiply_ARGB8888 function in KMSDRM, which has the same
requirement to have premultiplied alpha for the cursor.

This should fix libsdl-org#4856
sulix added a commit to sulix/SDL that referenced this issue Oct 22, 2021
It turns out that Wayland's WL_SHM_FORMAT_ARGB8888 format (and, indeed,
all wayland RGBA formats) should be treated as premultiplied. SDL
surfaces tend not to be premultiplied, and this is assumed by other
backends when dealing with cursors.

This change premultiplies the cursor surface in Wayland_CreateCursor().
A helper function -- premultiply_surface_alpha_ARGB8888 -- is created to
do the conversion, based somewhat on the e similar
legacy_alpha_premultiply_ARGB8888 function in KMSDRM, which has the same
requirement to have premultiplied alpha for the cursor.

This should fix libsdl-org#4856
@sulix
Copy link
Contributor

sulix commented Oct 22, 2021

I've pushed a commit which just adds another conversion function: this works well enough for me, but does reveal that nVidia/KWin/Wayland actually doesn't want premultiplied alpha. This seems to be a bug on KWin or nVidia's side, though. Interestingly, the same issue appears (regardless of this patch) with XWayland on nVidia, and XWayland on Intel/Mesa is broken, too.

Shared code for premultiplied alpha in src/video seems like a good idea.

I'm looking at this at the moment. The only really "nice" way of doing this would be to add a "premultiplied" flag to SDL_PixelFormat, but that's an awful lot of work to do properly. In the meantime, I'm looking at just having a general "surface pixels → buffer full of premultiplied ARGB8888" function in SDL_pixel.c. (It might fit slightly better in SDL_surface.c, but there's no corresponding internal header, and SDL_pixel_c.h has a bunch of "misc functions").

sulix added a commit to sulix/SDL that referenced this issue Oct 22, 2021
It turns out that Wayland's WL_SHM_FORMAT_ARGB8888 format (and, indeed,
all wayland RGBA formats) should be treated as premultiplied. SDL
surfaces tend not to be premultiplied, and this is assumed by other
backends when dealing with cursors.

This change premultiplies the cursor surface in Wayland_CreateCursor()
using the new SDL_PremultiplySurfaceAlphaToARGB8888(). In so doing, it
also adds support for a wider range of input surfaces, including those
with non-ARGB8888 pixel formats, and those which don't have
pitch==width.

This should fix libsdl-org#4856
@sulix
Copy link
Contributor

sulix commented Oct 22, 2021

Opened PR #4864, which adds a helper function, and uses it for Wayland and KMSDRM cursors, both of which now work fine on my machine. It also loosens the requirements around the exact format the surfaces are passed in, though I don't think this isn't strictly necessary, as conversion seem to be being done elsewhere.

With that change, this is fixed with me on everything I've tried but nVidia/KWin, which ends up with double-multiplied alpha (a "dark" halo around the cursor). This is less visually disturbing than the previous issue, though, and is almost certainly an nVidia or KWin bug.

@ernstp
Copy link
Author

ernstp commented Oct 22, 2021

Stellaris cursor looks fine on Mutter now, thanks!

flibitijibibo pushed a commit to flibitijibibo/SDL that referenced this issue Oct 25, 2021
It turns out that Wayland's WL_SHM_FORMAT_ARGB8888 format (and, indeed,
all wayland RGBA formats) should be treated as premultiplied. SDL
surfaces tend not to be premultiplied, and this is assumed by other
backends when dealing with cursors.

This change premultiplies the cursor surface in Wayland_CreateCursor()
using the new SDL_PremultiplySurfaceAlphaToARGB8888(). In so doing, it
also adds support for a wider range of input surfaces, including those
with non-ARGB8888 pixel formats, and those which don't have
pitch==width.

This should fix libsdl-org#4856
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

No branches or pull requests

4 participants