WIP: xwayland: Compute and set screen size and position
Unlike XRandR, the Wayland output layouts may have outputs placed at negative offsets.
Make sure to compute the size and also the actual location of the screen based on all outputs size and position.
Closes: #899
Merge request reports
Activity
Who in their right mind would have negative wl_output/xdg_output offsets? O_o
Sway allows for that, and there is nothing in Wayland protocols that prevents having output location in the negative range.
I suspect anyone who does will eventually see interesting issues with "smart" X11 clients putting stuff "offscreen" (or so they thought).
Yes, I meant to mention that in the issue, we can fix input but that won't help with apps trying to do the right thing in keeping their popup windows within the screen boundaries.
Nevertheless that works surprisingly well, or at least not as bad as one might think.
added 19 commits
-
35b569ee...d4faab87 - 17 commits from branch
xorg:master - b1f59dd7 - xwayland/input: Remove pointer valuator limits
- ef0b6e29 - xwayland: Do not limit pointer location
-
35b569ee...d4faab87 - 17 commits from branch
I think it makes sense to leave the pointer limits to the Wayland compositor because the Wayland compositor manages the pointer, Xwayland is just a Wayland client and as such has no control over the real pointer location.
So having limits in Xwayland is useless at best and can cause all sort of issues if the output layout does not match the usual patterns found in the X11 world.
I am not saying that placing outputs in the negative is a good idea though, as the X11 clients may try to keep their windows in the positive range (e.g. gtk popup windows), or place windows at a negative position trying to hide the window, but from an input point of view, we should not limit the pointer in Xwayland.
Edited by Olivier Fourdanmentioned in issue #962 (closed)
added 13 commits
-
ef0b6e29...5684d436 - 10 commits from branch
xorg:master - 98dd21b4 - dix: Set sprite's minimum limits
- cb62e426 - xwayland/input: Remove pointer valuator limits
- 469a964e - xwayland: Compute and set screen size and position
Toggle commit list-
ef0b6e29...5684d436 - 10 commits from branch
Right, different but much better approach IMHO.
Instead of removing all contraint and limits arbitrarily, make sure the actual Wayland output size and position are taken into account for the screen size and location, which determines the sprite limits.
That fixes the input events being discarded when Sway places its output in the negative range, while keeping the pointer constraints when set so we do not break with !414 (merged) and reintroduce #962 (closed)
Edited by Olivier Fourdanadded 27 commits
-
469a964e...6ef1b010 - 24 commits from branch
xorg:master - ca8d5a10 - dix: Set sprite's minimum limits
- 5729e506 - xwayland/input: Remove pointer valuator limits
- 707b98bf - xwayland: Compute and set screen size and position
Toggle commit list-
469a964e...6ef1b010 - 24 commits from branch
Tried this patch, but I can't get clicks to go through where screen coordinates are negative. Tested with Sway and
GDK_BACKEND=x11 gnome-calculator.xrandrcorrectly reports a negative screen position.Edited by Simon Seradded 7 commits
-
529e2f4f...5dc16f6f - 4 commits from branch
xorg:master - d98eac1a - dix: Set sprite's minimum limits
- fd383921 - xwayland/input: Remove pointer valuator limits
- 63170759 - xwayland: Compute and set screen size and position
Toggle commit list-
529e2f4f...5dc16f6f - 4 commits from branch
@emersion, it works fine here, can you please retest or give some info on your setup?
I rechecked and it works fine with negative values here, using https://gitlab.freedesktop.org/ofourdan/xserver/-/tree/xwayland-limitless.
140 140 141 141 /* Valuators */ 142 142 InitValuatorAxisStruct(device, 0, axes_labels[0], 143 0, 0xFFFF, 10000, 0, 10000, Absolute); 143 NO_AXIS_LIMITS, NO_AXIS_LIMITS, 10000, 0, 10000, Absolute); 144 144 InitValuatorAxisStruct(device, 1, axes_labels[1], 145 0, 0xFFFF, 10000, 0, 10000, Absolute); 145 NO_AXIS_LIMITS, NO_AXIS_LIMITS, 10000, 0, 10000, Absolute); this doesn't look right. The valuators are always in device units and the mapping is handled by the X server. Think about it: if you had a Wacom tablet with an axis range of 0-0xffff you wouldn't update the device's firmware to reach your output that happens to be sitting in the wrong spot :)
Does this have any effect? Because if so, the actual bug is in the server's screen mapping somewhere, not in xwayland.
Yes, this is required to get the button events coords within the negative range.
Without this, if the Wayland outputs are placed in the negative range, the coordinates reported on
ButtonPressandButtonReleaseare clamped within the axis range of 0-0xffff, and therefore remain positive.So maybe this is something in
QueuePointerEventswrtButtonPress/ButtonReleaseevents that clamps/transforms the coordinates wrong?Or rather in fill_pointer_events()...
Sorry it's been 2 months since I last investigate that...
So the issue with button
ButtonPress/ButtonReleaseevents is that we reuse the last position and that one gets clamped by the axis range for absolute motion events, as seen by placing a breakpoint onclipAxis().Backtrace
Thread 1 "Xwayland" hit Breakpoint 1, clipAxis (axisNum=axisNum@entry=1, val=val@entry=0x7fffa1e54ba8, pDev=, pDev=) at getevents.c:642 642 if (axisNum >= pDev->valuator->numAxes) (gdb) step 645 axis = pDev->valuator->axes + axisNum; (gdb) step 648 if (axis->max_value <= axis->min_value) (gdb) p *axis $4 = {resolution = 10000, min_resolution = 0, max_resolution = 10000, min_value = 0, max_value = 65535, label = 244, mode = 1 '\001', scroll = {type = SCROLL_TYPE_NONE, increment = 0, flags = 0}} (gdb) step 651 if (*val < axis->min_value) (gdb) p *axis $4 = {resolution = 10000, min_resolution = 0, max_resolution = 10000, min_value = 0, max_value = 65535, label = 244, mode = 1 '\001', scroll = {type = SCROLL_TYPE_NONE, increment = 0, flags = 0}} (gdb) p *val $5 = -3222.1866666666665 (gdb) bt #0 clipAxis (axisNum=axisNum@entry=1, val=val@entry=0x7fffa1e54ba8, pDev=, pDev=) at getevents.c:651 #1 0x0000000000566ac0 in clipAbsolute (dev=dev@entry=0x164bf70, mask=mask@entry=0x7fffa1e54c90) at getevents.c:728 #2 0x000000000056759c in fill_pointer_events (events=0x7f71d3946c20, events@entry=0x7f71d3946010, pDev=pDev@entry=0x164bf70, type=6, buttons=buttons@entry=0, ms=ms@entry=952078231, flags=flags@entry=20, mask_in=0x7fffa1e54fa0) at getevents.c:1404 #3 0x0000000000568c80 in GetPointerEvents (events=0x7f71d3946010, pDev=pDev@entry=0x164bf70, type=, type@entry=6, buttons=buttons@entry=0, flags=flags@entry=20, mask_in=mask_in@entry=0x7fffa1e554d0) at getevents.c:1691 #4 0x0000000000569270 in QueuePointerEvents (device=0x164bf70, type=type@entry=6, buttons=buttons@entry=0, flags=flags@entry=20, mask=mask@entry=0x7fffa1e554d0) at getevents.c:1290 #5 0x000000000042f7fc in dispatch_pointer_motion_event (xwl_seat=0x164ba50) at xwayland-input.c:532 #6 0x00007f71d41d3af0 in ffi_call_unix64 () from /lib64/libffi.so.6 #7 0x00007f71d41d32ab in ffi_call () from /lib64/libffi.so.6 #8 0x00007f71d4c7deb2 in wl_closure_invoke (closure=0x13b2dc0, flags=1, target=, opcode=5, data=) at ../../../../src/gnome/wayland/src/connection.c:1018 #9 0x00007f71d4c7a91a in dispatch_event (display=display@entry=0xdcf620, queue=, queue=) at ../../../../src/gnome/wayland/src/wayland-client.c:1452 #10 0x00007f71d4c7be6c in dispatch_queue (queue=0xdcf6f0, display=0xdcf620) at ../../../../src/gnome/wayland/src/wayland-client.c:1598 #11 wl_display_dispatch_queue_pending (display=0xdcf620, queue=0xdcf6f0) at ../../../../src/gnome/wayland/src/wayland-client.c:1840 #12 0x00007f71d4c7bebc in wl_display_dispatch_pending (display=) at ../../../../src/gnome/wayland/src/wayland-client.c:1903 #13 0x000000000043083b in xwl_read_events (xwl_screen=0xc54270) at xwayland-screen.c:436 #14 xwl_read_events (xwl_screen=0xc54270) at xwayland-screen.c:423 #15 0x0000000000586351 in ospoll_wait (ospoll=0xc49bf0, timeout=) at ospoll.c:657 #16 0x0000000000580213 in WaitForSomething (are_ready=0) at WaitFor.c:208 #17 0x0000000000550713 in Dispatch () at ../include/list.h:220 #18 0x0000000000554824 in dix_main (argc=10, argv=0x7fffa1e56928, envp=) at main.c:272 #19 0x00007f71d4585042 in __libc_start_main () from /lib64/libc.so.6 #20 0x000000000042b2de in _start ()fwiw, that's a backtrace from a motion event, not a button event but that shouldn't matter here.
Digging into this: If either
(POINTER_SCREEN | POINTER_DESKTOP)is set, the value in the mask is transformed from the desktop range to the range of the device, seescale_from_screen(). WherePOINTER_SCREENis set thescreen->xis added to the value but otherwise we take the value as-is.So provided that
screenInfo.xandscreenInfo.widthare set correctly, the value in the mask should never be outside the axis ranges anyway andclipAxis()is a noop. Could it be as simple as that vale being incorrect?fwiw, that's a backtrace from a motion event, not a button event but that shouldn't matter here.
Yep, that's normal, we don't get to this on button events.
So provided that
screenInfo.xandscreenInfo.widthare set correctly, the value in the mask should never be outside the axis ranges anyway andclipAxis()is a noop. Could it be as simple as that value being incorrect?This is handled by another commit in this MR and the
screenInfovalues are all correct (I checked),screenInfo.yis negative as it should.The thing I fail to understand here is, those axis ranges (that we set to
0 - 0xFFFF), aren't we expecting the actual given axis values to fall within that range? Because what we get from the compositor with an output placed at negative coordinates is actually negative values, which fall outside of the0 - 0xFFFFrange (hence the patch).Two things I noted while debugging this:
-
s/POINTER_SCREEN/POINTER_DESKTOP/is required indispatch_pointer_motion_event()because the coordinates Wayland uses aren't per-screen but desktop-wide (per-screen coords cannot be negative since they're relative to the origin) -
output_get_new_size()is buggy, it needs to work likeupdate_desktop_dimensions(), i.e. calculate the maximum extents of all screens and then usewidth=xmax-xminto accommodate for a truly arbitrary x/y width/height combination.
-
Right, I'll address those in a separate merge request, considering these are not directly related to the original issue at stake here (#899).
changed this line in version 8 of the diff
Summary: this cannot work in X because the input system prevents it.
Test setup here: 1920x1080@0,0 and 1920x1200@1920,-120. A problematic coordinate is for example 2000/-50 (top/left of right monitor).
X has two event coordinates:
rootY(global) andeventY(within window). A window has a positionwinY. The area of all screens is described inscreenInfo.x/yandscreenInfo.width/height.Xwayland currently passes the 2000/-50 coordinate into the DIX which will scale that to the
screenInfo.x/y/width/heightand provide us with positiverootX/rootYcoordinates within the desktop extents. So 2000/-50 turns into 2000/70. On delivery to the client,eventYis calculated asrootY - winYwhich is wrong for negative window offsets. For a fullscreen window on the right monitor, 2000/-50 would become event coordinates 80/190 (instead of the correct 80/70).To fix this, we would need someone motivated enough to fine-comb the server for every instance of where this matters and replace this with
rootY - (winY - screenInfo.y). This is unrealistic.And root coordinates have to be positive. sway allows both outputs to be offset at -1200, so if we allow for negative root coordinates our coordinate range would be
[-1200, 0]. Good luck finding and fixing every single X client that cannot handle that range, the Server Response on connection does not provide an x/y offset for the screens so we can assume nothing expects this.So our options are:
- finding and fixing every single instance where the window offset is being used
- finding and fixing every client that cannot handle a negative root coordinate range
Neither of these options are realistic.
Thanks @whot for your review!
So this will be a “close wontfix”, but what about the two other commits in this merge request, are they worth keeping? (my take is they aren't, but asking here nevertheless).
More precisely, I am thinking of commit d98eac1a
Honestly, I don't know. It looks correct but tbh I think the only way to check whether it has side-effects is to set up an old-style multihead setup (two
ScreenRecs) wherepScreen->x > 0and then check if something blows up.63170759 is wrong (well, the existing code already is) - that's the one that needs to have the extents calculated first and then work backwards towards the x + width from that, see !395 (comment 555615)
So I fixed
output_get_new_size()to compute the screen size correctly even with the negative offset (I checked, the computed values are now computed correctly as per the setup).I also changed to
POINTER_DESKTOP.But overall it actually makes things actually worse with outputs placed in the negative range, the input reporting stops too high now:
A picture is worth a thousand words...
Even though the screen size and position are correct, (0,-120) - [3840×1200].
So things just get worse with this IMHO.
Note: the branch has been updated with those changes, to make it easier to test
Edited by Olivier FourdanAh! Wait! I understand!
If we ignore the actual screen position then it works... I mean, not with Sway and negative output locations, but events are reported correctly.
Basically, doing:
update_screen_size(xwl_output, 0, 0, x2 - x1, y2 - y1);instead of:
update_screen_size(xwl_output, x1, y1, x2 - x1, y2 - y1);Branch updated with this.
So now, events are correctly reported in the positive range and not at all in the negative.
IOW, this MR changes nothing (neither it fixes the original issue nor it breaks more things
IIRC from yesterday the reported limit is because you're getting leave events at that line.
root_ywas correct butevent_yends up beingroot_y + 120and that generates theLeaveNotifywhen you get to that position. That's the bit that needs to be fixed by the server by fixing every use ofpWin->drawable.y.@whot Sorry for responding to this old thread (I can't really think of any better place to bring this up), but I think there's another way this could work:
This only seems like a problem if you're directly passing negative coordinates from XWayland, but couldn't you do the same kind of normalization around the desktop extents directly in XWayland? So that XWayland only ever hands off positive coordinates?
I think it would be a better solution to support negative coordinates directly in X, but like you mentioned that would be unrealistic. This would be a bit of a "hack", but it'd only need to touch code in XWayland, and would better align with the expectations of the protocol. The main downside being that coordinates in Wayland wouldn't always have the same values as coordinates in X (but as long as everything is piped through the Wayland protocol and not some outside channel, this shouldn't be an issue).
I'm basically doing the same thing right now manually as a workaround. (Finding the minX, minY and shifting all output coordinates from the difference with 0,0 when negative).
Edited by Kira BruneauI'm going to punt this question to @ofourdan because tbh, I have zero recollection of this MR, on re-reading this it was all new to me
Everything is possible of course, this is software...
However, if that means changing half of the DIX code to accommodate a somehow peculiar configuration in the Wayland compositor, it might be easier and more time effective to adjust the Wayland compositor setup. That's the reason why I basically gave up on this MR.
But if you reckon this can be achieved (reasonably easily), feel free to pick up from where I left it and build upon it
added 15 commits
-
63170759...d35f6833 - 12 commits from branch
xorg:master - f6015bca - dix: Set sprite's minimum limits
- 6d85028a - xwayland: Compute and set screen size and position
- 3d091785 - xwayland: Use desktop coordinates for pointer events
Toggle commit list-
63170759...d35f6833 - 12 commits from branch
