Skip to content

wayland: add support for tabel input #16276

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jp7677
Copy link

@jp7677 jp7677 commented Apr 22, 2025

This is a draft PR about adding support for tablet/stylus input for Wayland. The motivation behind this is that this PR lets me control mpv on a Gnome Wayland session with a tablet/stylus as I'm usually using a tablet as a full mouse replacement. Gnome Desktop unfortunately doesn't offer a mouse fallback for tablet input when a client doesn't support the tablet protocol, so a stylus can't interact with the mpv window there at all.

This is still early draft, but functional (tested on Gnome and labwc/wlroots) except for drag&drop like operations. It still needs polishing, some refactoring to avoid code duplication (cursor visibility) and splitting commits and better commit messages, but I wanted to create this PR in this state for general feedback. The main question is: is there interest in tablet/stylus support and does this PR has a chance of getting merged? The tablet protocol is not that simple and a lot of boilerplate code is needed for just a few basic handlers. I hope not, but I would understand if this is too much for an edge case.

Thanks for creating and maintaining mpv and looking forward to some general early feedback!

@jp7677 jp7677 force-pushed the wayland-tablet-protocol branch 2 times, most recently from 36504fa to 92498d3 Compare April 22, 2025 06:27
@llyyr
Copy link
Contributor

llyyr commented Apr 22, 2025

Gnome Desktop unfortunately doesn't offer a mouse fallback for tablet input when a client doesn't support the tablet protocol, so a stylus can't interact with the mpv window there at all.

Is this something GNOME won't implement or something that simply has never been reported? sway falls back to pointer events if a client doesn't support touch or tablet protocols, it would be much simpler to add this fallback in Mutter than go around fixing every single client

@jp7677
Copy link
Author

jp7677 commented Apr 22, 2025

Is this something GNOME won't implement or something that simply has never been reported? sway falls back to pointer events if a client doesn't support touch or tablet protocols, it would be much simpler to add this fallback in Mutter than go around fixing every single client

Looking at https://gitlab.gnome.org/GNOME/mutter/-/issues/2226 from three years ago, I think it is unfortunately the former, at least at that time.

PS: I agree by the way that mouse emulation in the compositor would be better, Labwc did the same as Sway for tablet input and that feels much more robust to me (that is, the Labwc implementation, I never looked that detailed at Sway and can't comment on how well it works there).

@kasper93
Copy link
Member

kasper93 commented Apr 22, 2025

sway falls back to pointer events if a client doesn't support touch or tablet protocols,

Note that it is completely broken in mpv, at least for osc.lua. Sway reset pointer position to "real" mouse pos when all fingers leave the touch screen. By the time osc.lua processes the touch, the pointer is in another place. Making all buttons non-
clickable in practice.

And there is no good way to "fix" or workaround that. Because we have no idea if the move was real or it was emulated switch to mouse.

So proper native touch support might be the only way forward.

Copy link

github-actions bot commented Apr 22, 2025

@llyyr
Copy link
Contributor

llyyr commented Apr 22, 2025

So proper native touch support might be the only way forward.

We already have native touch support, this is about tablet support. And sway translating tablet events to pointer events works perfectly in mpv. I can't say for touch events as I don't have a touch device, and would need to rip out touch support from mpv to even test it.

@kasper93
Copy link
Member

Doesn't work on my end.

@llyyr
Copy link
Contributor

llyyr commented Apr 22, 2025

Doesn't work on my end.

What doesn't work? Touch or tablet? If touch doesn't work that's a mpv bug, or a bug with sway/wlroots sending us touch events (not one with translating touch to pointer events).

edit: I checked and it's a sway bug with touch input, works on river.

@mahkoh
Copy link
Contributor

mahkoh commented Apr 25, 2025

sway falls back to pointer events if a client doesn't support touch or tablet protocols

That cannot work. Let's say an application has no tablet support and uses libdecor to draw window decorations, which (let's say) also doesn't support tablets. The user can now drag the window by its decorations in sway because sway sends emulated pointer events. But then the application adds support for tablet events and now the user can no longer interact with the window decorations because sway no longer sends emulated events and libdecor still doesn't support tablet events.

@jp7677 jp7677 force-pushed the wayland-tablet-protocol branch 3 times, most recently from f331ef9 to b3db60b Compare April 25, 2025 17:09
@jp7677
Copy link
Author

jp7677 commented Apr 25, 2025

I think this is pretty much ready from my side, moving the window by dragging the window with the stylus works now and with that I think the stylus support is solid. That said, I think I got the wiring correctly, but a keen eye is very welcome, especially for shutting things down. Let me know what you think and if it's worth it.

@mahkoh I contributed to the tablet support for labwc, which also uses mouse emulation for clients that do not support the tablet protocol. If you have a case with a reproduction scenario where it doesn't or shouldn't work, could you open an issue there since I would be curious to play with it.

@jp7677 jp7677 marked this pull request as ready for review April 25, 2025 17:21
@mahkoh
Copy link
Contributor

mahkoh commented Apr 25, 2025

If you have a case with a reproduction scenario where it doesn't or shouldn't work

What I described above should be easy enough to reproduce in any application that uses both libdecor and supports tablet input. You might have to enable CSD in your compositor for libdecor to do anything.

@jp7677 jp7677 force-pushed the wayland-tablet-protocol branch from b3db60b to 0661c5e Compare April 25, 2025 18:53
@jp7677 jp7677 marked this pull request as draft April 26, 2025 08:47
@jp7677 jp7677 force-pushed the wayland-tablet-protocol branch 4 times, most recently from 0f9c657 to efdd136 Compare April 26, 2025 17:25
@jp7677 jp7677 force-pushed the wayland-tablet-protocol branch 2 times, most recently from 53ceec3 to 051710c Compare April 27, 2025 06:46
@jp7677
Copy link
Author

jp7677 commented Apr 27, 2025

CI for the latest pipeline on master fails with the same errors like the latest pipeline for this PR, so I don’t think this is on me.

@llyyr
Copy link
Contributor

llyyr commented Apr 27, 2025

CI for the latest pipeline on master fails with the same errors like the latest pipeline for this PR, so I don’t think this is on me.

Flaky windows CI is new mpv contributor hazing ritual don't worry

@kasper93
Copy link
Member

kasper93 commented Apr 27, 2025

CI for the latest pipeline on master fails with the same errors like the latest pipeline for this PR, so I don’t think this is on me.

Flaky windows CI is new mpv contributor hazing ritual don't worry

I don't understand, why is this Windows related?

EDIT:

Build with GCC 15 is fixed in #16293

@jp7677
Copy link
Author

jp7677 commented May 13, 2025

Thanks for the comments, I’ve rework the code accordingly.
What do you prefer w.r.t to resolving the discussions here? Should I close the discussions once it has been reworked to my best knowledge or do you prefer to leave resolving to the original poster?

player/command.c Outdated
Comment on lines 3110 to 3112
node_map_add_int64(&node, "x", xs);
node_map_add_int64(&node, "y", ys);
node_map_add_flag(&node, "tool-in-proximity", tool_in_proximity);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also include tool down/up and button states here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I couldn't find any examples for button states, I ended up with a structure like this:

{"x":0,"y":0,"tool-in-proximity":false,"tool-tip":"up","tool-stylus-btn1":"released","tool-stylus-btn2":"released","tool-stylus-btn3":"released"}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Playing a bit with the lua side, is it correct that observing a sub property doesn't work?

This snippet doesn't yield a callback: mp.observe_property("tablet-pos/tool-tip", "string", on_tablet_tool_tip_change)

This one: mp.observe_property("tablet-pos", "native", on_tablet_change) does work, but I think only when x/y have changed, not when only a button press has occurred.

Copy link
Member

@kasper93 kasper93 May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should call mp_notify or mp_notify_property at appropriate place to trigger client observers.

Copy link
Author

@jp7677 jp7677 May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I've added a notify_tablet_update() (similar to touch) to trigger an update to tablet-pos when any of its properties have changed. Still looking for way to make e.g. mp.observe_property("tablet-pos/tool-tip",... work.

Copy link
Author

@jp7677 jp7677 May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmh, running mp.observe_property("mouse-pos/x" doesn't yield anything either, so I tend to conclude that it currently just doesn't works like this.

That said, something like this still works for overriding tip and buttons (when additionally disabling tablet input with --input-tablet-emulate-mouse=no) which is actually really cool:

tablet_pos_tool_tip_old = ""
mp.observe_property("tablet-pos", "native", function(name, value)
        tablet_pos_tool_tip = value["tool-tip"]
        if (tablet_pos_tool_tip ~= tablet_pos_tool_tip_old and tablet_pos_tool_tip == "up") then
                mp.msg.log("info", "Tablet tool tip tap ..")
        end 
        tablet_pos_tool_tip_old	= tablet_pos_tool_tip
end)

@na-na-hi
Copy link
Contributor

What do you prefer w.r.t to resolving the discussions here? Should I close the discussions once it has been reworked to my best knowledge or do you prefer to leave resolving to the original poster?

Feel free to resolve mine. I don't have the privilege to resolve my own reviews.

@jp7677 jp7677 force-pushed the wayland-tablet-protocol branch 5 times, most recently from a73fc44 to bfe7eb2 Compare May 15, 2025 16:35
@kasper93
Copy link
Member

For me it works worse than emulated input. One issue is when you hover stylus over a OSC button, click on it, and keep stylus in the same location, any subsequent clicks on the same button doesn't work. No such issue on mpv master.

(not related to tablet issue, offtopic): and it still doesn't fix the issue which I'm most interested in, where touch input doesn't work in sway completely if you move mouse pointer in sway, because it resets touch pos to mouse pointer pos and it's not recognized as a click by the time it's processed, osc is pulling pos.

@jp7677
Copy link
Author

jp7677 commented May 17, 2025

For me it works worse than emulated input. One issue is when you hover stylus over a OSC button, click on it, and keep stylus in the same location, any subsequent clicks on the same button doesn't work. No such issue on mpv master.

I've tried to reproduce this on both Gnome/mutter and labwc, but repeated clicks do work fine on both. Do you see that issue in another compositor? I don't have a sway setup, so don't know about their tablet support.

PS: Thanks for trying this PR!

@kasper93
Copy link
Member

I've tried to reproduce this on both Gnome/mutter and labwc, but repeated clicks do work fine on both. Do you see that issue in another compositor? I don't have a sway setup, so don't know about their tablet support.

I tested the same on Gnome. I can also reproduce it here. Basically what I try to do is to click play/pause button, repellently. Without moving away stylus, so the cursor is still there. And what happens is that it's not registering taps. Oftentimes it finally register, but not sure what is the exact trigger, probably I move tip more or something like that.

@jp7677
Copy link
Author

jp7677 commented May 17, 2025

I've tried to reproduce this on both Gnome/mutter and labwc, but repeated clicks do work fine on both. Do you see that issue in another compositor? I don't have a sway setup, so don't know about their tablet support.

I tested the same on Gnome. I can also reproduce it here. Basically what I try to do is to click play/pause button, repellently. Without moving away stylus, so the cursor is still there. And what happens is that it's not registering taps. Oftentimes it finally register, but not sure what is the exact trigger, probably I move tip more or something like that.

Thanks. Strange, I just installed swaywm on my Fedora box and also on sway I can play/pause continuously just fine. Looking at the PR, the only thing that could explain this (without thinking of weird stuff), is your stylus going out of proximity. Could you try removing the proximity check at https://github.com/jp7677/mpv/blob/wayland-tablet-protocol/input/input.c#L1107 and https://github.com/jp7677/mpv/blob/wayland-tablet-protocol/input/input.c#L1118 ?
Gnome marks the stylus out of proximity when start dragging with the stylus, may be something like this gets triggered?

@kasper93
Copy link
Member

stylus is always in proximity and mp_input_tablet_tool_{up/down} is called on every tap. Still the command doesn't trigger and video doesn't pause. Though when in paused state it always unpause. Either way it seems that feed_key is done correctly.

I think this is the exactly same issue I described before regarding tablet mode. In more generic terms, mouse emulation is not playing nice with OSC, because of timing dependency. What happens is that scripts observe mouse button events and trigger action on that, but by the time it is processed the "emulated" state might not be consistent. I haven't debug it, but it looks like that's the case.

See mouse_hit_coords in osc.lua. It does hit test on current mouse pos, which may no longer be emulated one. Or basically up/down events are not in sync with emulated ones.

I think we should resolve this first, because basically this PR by adding emulation inside mpv, making experience worse than the emulation provided by the compositors themselves.

We probably should recognize in input.c when mouse emulation is happening and make sure it's not interrupted somehow.

I can debug it, if I find some time, but not much of it lately.

@jp7677
Copy link
Author

jp7677 commented May 18, 2025

stylus is always in proximity and mp_input_tablet_tool_{up/down} is called on every tap. Still the command doesn't trigger and video doesn't pause. Though when in paused state it always unpause. Either way it seems that feed_key is done correctly.

Thanks for confirming that at least I didn't messed up ;)

I think this is the exactly same issue I described before regarding tablet mode.

I guess you wanted to say "touch mode"?

See mouse_hit_coords in osc.lua. It does hit test on current mouse pos, which may no longer be emulated one. Or basically up/down events are not in sync with emulated ones.

So this is a timing issue and depends on machine speed/load/tablet/touch sample rate etc?

I think we should resolve this first, because basically this PR by adding emulation inside mpv, making experience worse than the emulation provided by the compositors themselves.

If compositors provide emulation, which isn't the case for Gnome/mutter, but I get your point.

I can debug it, if I find some time, but not much of it lately.

Thanks.

@jp7677 jp7677 force-pushed the wayland-tablet-protocol branch 2 times, most recently from 373b860 to f062f69 Compare May 25, 2025 18:13
jp7677 added 4 commits May 25, 2025 20:17
The tablet protocol is supported in the current winimum
wayland-protocols version, so we can always build this.
Note that this is still the unstable version, once the
minimum required wayland-protocol version is 1.35, the
stable version of protocol can be included.
Tablets require their own tablet manager and tablet
seats since multiple tablets/pens can be connected to
a single seat. So this is unfortunately a lot of
wiring.
This ensures that tablet objects are properly destroyed on
removal or shutdown, even if we are not really interested
in tablet events.
Setup tablet tool events, we want to use motion, tip and
button events later.
@jp7677 jp7677 force-pushed the wayland-tablet-protocol branch from f062f69 to 54aad7b Compare May 25, 2025 18:18
jp7677 added 6 commits May 28, 2025 18:44
This ensures that tablet pad objects are properly destroyed on
removal or shutdown, even if we are not (yet) interested in
tablet pad events.
This ensures that tablet pad objects that belong to a group,
thus rings and strips, are all properly destroyed on removal
or shutdown.

Note that event listeners for ring a strip are not added
since we are not (yet) interested in them and they are not
needed for proper destroy.
Pretty much copied from pointer cursors, including
the support for the cursor-shape protocol.
Upgrade tablet input to first class citizen similar like
touch input, but more simple since use case for now is
mouse emulation only.

Keep state for coordinates and in-proximity.
Introduce property for disabling tablet input.
Introduce Client-API property `tablet-pos`.
@jp7677 jp7677 force-pushed the wayland-tablet-protocol branch from 54aad7b to f9c441c Compare May 28, 2025 16:45
@jp7677
Copy link
Author

jp7677 commented May 28, 2025

Sorry, linting should be good now.

Edit: that one is not on me ;)

check xml................................................................Failed
- hook id: check-xml
- exit code: 1

TOOLS/mpv-osd-symbols.sfdir/font.props: Failed to xml parse (TOOLS/mpv-osd-symbols.sfdir/font.props:1:0: syntax error)

Also don’t think this one is caused by this PR:

231/236 mpv:libmpv / libmpv-test-multilang.mkv                    FAIL              2.99s   exit status 1

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.

6 participants