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: set_cursor_visibility doesn't respect fractional scaling (erroring when < 1.0). #12309

Closed
eddyb opened this issue Sep 1, 2023 · 3 comments

Comments

@eddyb
Copy link

eddyb commented Sep 1, 2023

Important Information

Provide following Information:

  • mpv version: 0.36.0
  • Linux Distribution and Version: NixOS 23.11 (23.11pre520402.e7f38be3775b)
  • Source of the mpv binary: nixpkgs unstable (NixOS/nixpkgs@e7f38be)
  • If known which version of mpv introduced the problem: 0.36 (wayland: add wp-fractional-scale-v1 support #10943)
  • Window Manager and version: Plasma 5 (kwin_wayland 5.27.7)
  • GPU driver and version: Mesa 23.1.5 (RADV)

Reproduction steps

There may be visual issues at all scales, but to get the error:

  1. in a wayland compositor with fractional scaling, set least one monitor to below 1.0
    (I use 0.75 on 1440p side monitors, with the primary 4K monitor kept at 1.0)
  2. run mpv on a monitor with scale below 1.0 (or move its window there)
    (I used mpv --no-config --pause av://lavfi:color below for the repro)
  3. move the cursor onto the mpv window

Expected behavior

mpv should be able to show/hide the cursor correctly.

Actual behavior

mpv causes this wayland error and quits:

wl_surface@9: error 0: buffer scale must be at least one (0 specified)

[vo/gpu/wayland] Error occurred on the display fd, closing

Passing --cursor-autohide=always bypasses the problem, but it's not a great workaround since it makes operating the OSD difficult (it does, however, make me strongly suspect set_cursor_visibility - see below).


This appears to be the relevant code:

mpv/video/out/wayland_common.c

Lines 1749 to 1764 in 998c3a1

static int set_cursor_visibility(struct vo_wayland_state *wl, bool on)
{
wl->cursor_visible = on;
if (on) {
if (wl->cursor_shape_device) {
set_cursor_shape(wl);
} else {
if (spawn_cursor(wl))
return VO_FALSE;
struct wl_cursor_image *img = wl->default_cursor->images[0];
struct wl_buffer *buffer = wl_cursor_image_get_buffer(img);
if (!buffer)
return VO_FALSE;
wl_pointer_set_cursor(wl->pointer, wl->pointer_id, wl->cursor_surface,
img->hotspot_x/wl->scaling, img->hotspot_y/wl->scaling);
wl_surface_set_buffer_scale(wl->cursor_surface, wl->scaling);

Using wl->scaling (a double) as an integer will round towards 0, and produce 0 from e.g. 0.75.

The only other caller of wl_surface_set_buffer_scale checks for fractional scaling:

mpv/video/out/wayland_common.c

Lines 1828 to 1831 in 998c3a1

static void set_surface_scaling(struct vo_wayland_state *wl)
{
if (wl->fractional_scale_manager)
return;

Log file

Output of WAYLAND_DEBUG=1 mpv --no-config --pause av://lavfi:color -v -v >log.txt 2>&1: https://0x0.st/Hpj0.txt

@eddyb eddyb added the os:linux label Sep 1, 2023
@Dudemanguy
Copy link
Member

Dudemanguy commented Sep 1, 2023

I intentionally didn't use fractional scaling on the cursor since it's really stupid. Cursor themes only come in a select few sizes and the behavior with integer scaling is bad too. Cursor shape solves this so you shouldn't have this problem there (the compositor picks the cursor). However, truncating to 0 on the fallback code is definitely bad. It should be at least 1.

Does this patch fix it?

diff --git a/video/out/wayland_common.c b/video/out/wayland_common.c
index 8179436c21..3d97e677c2 100644
--- a/video/out/wayland_common.c
+++ b/video/out/wayland_common.c
@@ -1759,9 +1759,10 @@ static int set_cursor_visibility(struct vo_wayland_state *wl, bool on)
             struct wl_buffer *buffer = wl_cursor_image_get_buffer(img);
             if (!buffer)
                 return VO_FALSE;
+            int scale = MPMAX(wl->scaling, 1);
             wl_pointer_set_cursor(wl->pointer, wl->pointer_id, wl->cursor_surface,
-                                  img->hotspot_x/wl->scaling, img->hotspot_y/wl->scaling);
-            wl_surface_set_buffer_scale(wl->cursor_surface, wl->scaling);
+                                  img->hotspot_x / scale, img->hotspot_y / scale);
+            wl_surface_set_buffer_scale(wl->cursor_surface, scale);
             wl_surface_attach(wl->cursor_surface, buffer, 0, 0);
             wl_surface_damage_buffer(wl->cursor_surface, 0, 0, img->width, img->height);
         }

@Dudemanguy
Copy link
Member

Sorry totally forgot about this... give me a second.

@eddyb
Copy link
Author

eddyb commented Oct 30, 2023

Sorry totally forgot about this... give me a second.

Same, I'm really sorry, completely lost track of it!
I meant to try it out but shortly afterwards I realized that Chrome is intensely broken for scales below 1.0, and I switched everything from forcing a font DPI of 165 or so (of my 1.0 scale at the time 4K monitor, with the other monitors being scale 0.75 to account for their reduced density), to "96" """DPI""" + fractional scaling.

You can even reproduce their bugginess in a vacuum (as long as you're on Wayland):

$ google-chrome-stable --version
Google Chrome 118.0.5993.88
$ google-chrome-stable --disable-features=WaylandFractionalScaleV1 --force-device-scale-factor=0.99
$ google-chrome-stable --disable-features=WaylandFractionalScaleV1 --force-device-scale-factor=1.01

1.01 will look very close to 1.00, and interacting with the UI works as expected, but 0.99 has a lot of deviation in e.g. click hit-testing, almost like 0.99 fails to be applied and the scale used is more like, idk, 1.5, but 0.99 is still recorded somewhere and used to e.g. map cursor coordinates from Wayland.
(if I had more energy and time I would try to track it down and report/submit a patch to them but... not worth it)

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

No branches or pull requests

2 participants