GNOME Bugzilla – Bug 724442
Touchscreen does not work
Last modified: 2014-06-04 21:55:52 UTC
When running a wayland session input from the touchscreen is ignored / has no effect.
Created attachment 275161 [details] [review] wayland: Implement the server side bits of wl_touch_interface Clutter touch events are translated into events being sent down the interface resource, with the exception of FRAME/CANCEL events, which are handled directly via an evdev event filter. The seat now announces invariably the WL_SEAT_CAPABILITY_TOUCH capability, this should be eventually updated as devices come and go. The creation of MetaWaylandTouchSurface structs is dynamic, attached to the lifetime of first/last touch on the client surface, and only if the surface requests the wl_touch interface. MetaWaylandTouchInfo structs are created to track individual touches, and are locked to a single MetaWaylandTouchSurface (the implicit grab surface) determined on CLUTTER_TOUCH_BEGIN.
*** Bug 728057 has been marked as a duplicate of this bug. ***
Review of attachment 275161 [details] [review]: Looks mostly OK. Fairly modern, so you wrote this recently. Yay! Haven't tested yet since my touch device isn't fully ready for Wayland support yet. (files are still copying, only a few more hours to go!) Preliminary review. ::: src/wayland/meta-wayland-touch.c @@ +32,3 @@ +#include <fcntl.h> +#include <unistd.h> +#include <sys/mman.h> Some of these includes look unused. fnctl, mmap, errno. Could you clean them up? @@ +62,3 @@ + +static void +_touch_surface_free (gpointer data) No need for the underscores at the beginning of method names. @@ +173,3 @@ + { + ClutterActor *actor = + CLUTTER_ACTOR (meta_window_get_compositor_private (surface->window)); I know you copied this from meta_wayland_pointer_get_relative_coordinates, but it was wrong there too. This should simply be: clutter_actor_transform_stage_point (surface->surface_actor, ...); I'll fix it later today. @@ +186,3 @@ +void +meta_wayland_touch_update (MetaWaylandTouch *touch, + const ClutterEvent *event) Why is this in touch_update and not touch_handle_event? touch_update is early dispatch, while touch_handle_event happens after the compositor has a chance to filter out the event, like with grabs. It seems like most of this code should be in touch_handle_event, since we don't want n_processed_events to increase rapidly while we have a grab while never being flushed out. (The "update" vs. "handle_event" naming is quite poor, especially when we have TOUCH_UPDATE events. Suggestions welcome.) @@ +207,3 @@ + return; + + touch_info = _touch_get_info (touch, sequence, TRUE); Will TOUCH_BEGIN ever reuse the same slot? I'm not quite sure how this is architected. If not, please just do the creation here. @@ +211,3 @@ + } + else + touch_info = _touch_get_info (touch, sequence, FALSE); Will this ever return NULL? Wouldn't it be invalid to get a TOUCH_UPDATE / TOUCH_END for a sequence that never got a begin? @@ +228,3 @@ + _touch_get_relative_coordinates (touch, touch_info->touch_surface->surface, + event, &touch_info->x, &touch_info->y); + touch_info->updated = TRUE; Nothing ever sets this back to FALSE, and there's no way I can tell for a touch info to be created outside of this method, so I'm not quite sure when we'll ever have a non-updated touch info here. @@ +340,3 @@ + surfaces = s = _touch_get_surfaces (touch, TRUE); + + while (s) for (s = surface; s; s = s->next) @@ +349,3 @@ + l = &touch_surface->resource_list; + wl_resource_for_each(resource, l) + wl_touch_send_frame (resource); Please always put braces around wl_resource_for_each loops. @@ +352,3 @@ + } + + g_list_free (surfaces); I assume that we were meant to mark all touch infos as not updated anymore here, and wait for the next frame. @@ +454,3 @@ + case LIBINPUT_EVENT_TOUCH_FRAME: + touch->n_frame_events = touch->n_touch_events; + touch->n_touch_events = 0; I think the handling here is wrong. Maybe it will never show up in practice, but if we get two frame events before processing events (e.g. the master clock hasn't ticked) this could go awry. If it's really unlikely, just mark it as an "XXX" with a comment. @@ +456,3 @@ + touch->n_touch_events = 0; + break; + case LIBINPUT_EVENT_TOUCH_CANCEL: I couldn't find anything in libinput that sent a LIBINPUT_EVENT_TOUCH_CANCEL. The Wayland protocol says that it's used for the compositor when it recognizes a gesture or takes a grab. Is this eventually going to be sent by libinput in the future, or was it a mistake that crept into libinput's API? How does Clutter send CLUTTER_EVENT_TOUCH_CANCEL events right now? From XI2's TouchCancel event? It seems like we should have a meta_wayland_touch_cancel (); in this case when we steal a gesture for ourselves or take a grab.
Review of attachment 275161 [details] [review]: ::: src/wayland/meta-wayland-touch.c @@ +456,3 @@ + touch->n_touch_events = 0; + break; + case LIBINPUT_EVENT_TOUCH_CANCEL: Originally _CANCEL came from Wayland wl_touch where its purpose indeed was to cancel in case a gesture etc was recognized. I did leave it there intentionally as I'm not sure such features will be out of scope for regular touch devices (it is in-scope for touchpad for example, but there it won't need any cancel event).
Created attachment 275197 [details] [review] wayland: Implement the server side bits of wl_touch_interface Clutter touch events are translated into events being sent down the interface resource, with the exception of FRAME/CANCEL events, which are handled directly via an evdev event filter. The seat now announces invariably the WL_SEAT_CAPABILITY_TOUCH capability, this should be eventually updated as devices come and go. The creation of MetaWaylandTouchSurface structs is dynamic, attached to the lifetime of first/last touch on the client surface, and only if the surface requests the wl_touch interface. MetaWaylandTouchInfo structs are created to track individual touches, and are locked to a single MetaWaylandTouchSurface (the implicit grab surface) determined on CLUTTER_TOUCH_BEGIN.
Thanks for the early comments :), some replies below (In reply to comment #3) > ::: src/wayland/meta-wayland-touch.c > @@ +32,3 @@ > +#include <fcntl.h> > +#include <unistd.h> > +#include <sys/mman.h> > > Some of these includes look unused. fnctl, mmap, errno. Could you clean them > up? Indeed, done > > @@ +62,3 @@ > + > +static void > +_touch_surface_free (gpointer data) > > No need for the underscores at the beginning of method names. Fixed throughout the file > > @@ +173,3 @@ > + { > + ClutterActor *actor = > + CLUTTER_ACTOR (meta_window_get_compositor_private (surface->window)); > > I know you copied this from meta_wayland_pointer_get_relative_coordinates, but > it was wrong there too. This should simply be: > > clutter_actor_transform_stage_point (surface->surface_actor, ...); > > I'll fix it later today. Ah, that looks better :), changed that > > @@ +186,3 @@ > +void > +meta_wayland_touch_update (MetaWaylandTouch *touch, > + const ClutterEvent *event) > > Why is this in touch_update and not touch_handle_event? > > touch_update is early dispatch, while touch_handle_event happens after the > compositor has a chance to filter out the event, like with grabs. It seems like > most of this code should be in touch_handle_event, since we don't want > n_processed_events to increase rapidly while we have a grab while never being > flushed out. These counters are admittedly a bit of a hack... in order to circumvent the fact that clutter events are queued, while we process frame events in the filter before these are even queued. I've let that stay on update() so far, as it should happen invariable as events get, and not conditionally if eg. grabbed by a gesture on handle_event(). > > (The "update" vs. "handle_event" naming is quite poor, especially when we have > TOUCH_UPDATE events. Suggestions welcome.) > > @@ +207,3 @@ > + return; > + > + touch_info = _touch_get_info (touch, sequence, TRUE); > > Will TOUCH_BEGIN ever reuse the same slot? I'm not quite sure how this is > architected. If not, please just do the creation here. Hmm, yeah, missed that on the updated patch, in practice slots are reused, but the previous info is gone when the previous touch was released. > > @@ +211,3 @@ > + } > + else > + touch_info = _touch_get_info (touch, sequence, FALSE); > > Will this ever return NULL? Wouldn't it be invalid to get a TOUCH_UPDATE / > TOUCH_END for a sequence that never got a begin? It could if the touch gets cancelled, I preferred to have it safe against untracked touches. > > @@ +228,3 @@ > + _touch_get_relative_coordinates (touch, touch_info->touch_surface->surface, > + event, &touch_info->x, &touch_info->y); > + touch_info->updated = TRUE; > > Nothing ever sets this back to FALSE, and there's no way I can tell for a touch > info to be created outside of this method, so I'm not quite sure when we'll > ever have a non-updated touch info here. Doh, good catch it's now unset in touch_get_surfaces() (either called for cancel or frame events) > > @@ +340,3 @@ > + surfaces = s = _touch_get_surfaces (touch, TRUE); > + > + while (s) > > for (s = surface; s; s = s->next) > > @@ +349,3 @@ > + l = &touch_surface->resource_list; > + wl_resource_for_each(resource, l) > + wl_touch_send_frame (resource); > > Please always put braces around wl_resource_for_each loops. > > @@ +352,3 @@ > + } > + > + g_list_free (surfaces); > > I assume that we were meant to mark all touch infos as not updated anymore > here, and wait for the next frame. unsetting the updated field was indeed missing, the rest is handled in handle_frame_event() > > @@ +454,3 @@ > + case LIBINPUT_EVENT_TOUCH_FRAME: > + touch->n_frame_events = touch->n_touch_events; > + touch->n_touch_events = 0; > > I think the handling here is wrong. Maybe it will never show up in practice, > but if we get two frame events before processing events (e.g. the master clock > hasn't ticked) this could go awry. If it's really unlikely, just mark it as an > "XXX" with a comment. In practice it seems libinput compresses things so you only get a touch event set on libinput_dispatch(), so shouldn't be a problem, but as said, this frame event management is quite hacky... would be better when libinput is used directly > > @@ +456,3 @@ > + touch->n_touch_events = 0; > + break; > + case LIBINPUT_EVENT_TOUCH_CANCEL: > > I couldn't find anything in libinput that sent a LIBINPUT_EVENT_TOUCH_CANCEL. > The Wayland protocol says that it's used for the compositor when it recognizes > a gesture or takes a grab. Is this eventually going to be sent by libinput in > the future, or was it a mistake that crept into libinput's API? > > How does Clutter send CLUTTER_EVENT_TOUCH_CANCEL events right now? From XI2's > TouchCancel event? In X11, that event is not sent in practice... the concept of cancellation comes with XI_TouchOwnership, and neither Clutter nor GTK+ dared to open that can of worms. > > It seems like we should have a meta_wayland_touch_cancel (); in this case when > we steal a gesture for ourselves or take a grab. Indeed, but I've left that out so far, I would like to eventually work on a place to hook gestures on mutter that'd work for x11 and wayland
(In reply to comment #4) > > Originally _CANCEL came from Wayland wl_touch where its purpose indeed was to > cancel in case a gesture etc was recognized. I did leave it there intentionally > as I'm not sure such features will be out of scope for regular touch devices > (it is in-scope for touchpad for example, but there it won't need any cancel > event). One place I thought it would/should be called is eg. when the caller does libinput_suspend() and there's touches being tracked, but OTOH libinput is quite stateless in that you only read status from the events it hands to you, and compositors should already be undoing key/button state for other devices too if that happens.
(In reply to comment #4) > Originally _CANCEL came from Wayland wl_touch where its purpose indeed was to > cancel in case a gesture etc was recognized. I did leave it there intentionally > as I'm not sure such features will be out of scope for regular touch devices > (it is in-scope for touchpad for example, but there it won't need any cancel > event). So, can you imagine any device that would send a _CANCEL event?
(In reply to comment #8) > (In reply to comment #4) > > Originally _CANCEL came from Wayland wl_touch where its purpose indeed was to > > cancel in case a gesture etc was recognized. I did leave it there intentionally > > as I'm not sure such features will be out of scope for regular touch devices > > (it is in-scope for touchpad for example, but there it won't need any cancel > > event). > > So, can you imagine any device that would send a _CANCEL event? It would not be the device that sends the _CANCEL event, but libinput doing gesture detection and as a result sending such an event.
Hm, is gesture recognition in-scope for libinput? I always thought it was simply a low-level device handling library, with the bulk of its features being things like pointer acceleration.
(In reply to comment #10) > Hm, is gesture recognition in-scope for libinput? I always thought it was > simply a low-level device handling library, with the bulk of its features being > things like pointer acceleration. For certain devices it certainly is, for example touchpads. It already supports two finger drag gesture and taps etc. What was uncertain whether regular touch devices generating touch events would ever be in scope for doing gesture detection. I'm not saying it should be, but only explaining why the enum was left where it is.
Created attachment 276987 [details] [review] wayland: Implement the server side bits of wl_touch_interface Clutter touch events are translated into events being sent down the interface resource, with the exception of FRAME/CANCEL events, which are handled directly via an evdev event filter. The seat now announces invariably the WL_SEAT_CAPABILITY_TOUCH capability, this should be eventually updated as devices come and go. The creation of MetaWaylandTouchSurface structs is dynamic, attached to the lifetime of first/last touch on the client surface, and only if the surface requests the wl_touch interface. MetaWaylandTouchInfo structs are created to track individual touches, and are locked to a single MetaWaylandTouchSurface (the implicit grab surface) determined on CLUTTER_TOUCH_BEGIN.
Review of attachment 276987 [details] [review]: ::: src/wayland/meta-wayland-touch.c @@ +71,3 @@ +touch_surface_ref (MetaWaylandTouchSurface *surface) +{ + g_atomic_int_inc (&surface->ref_count); mutter isn't thread-safe, so we don't need atomics. @@ +102,3 @@ + if (touch_info->touch_surface == touch_surface) + g_hash_table_iter_remove (&iter); + } Maybe check that the touch surface was removed from the hash table at this point? @@ +299,3 @@ + } + + g_hash_table_remove (touch->touches, clutter_event_get_event_sequence (event)); sequence? @@ +484,3 @@ + touch->display = display; + touch->touch_surfaces = g_hash_table_new_full (NULL, NULL, NULL, + (GDestroyNotify) touch_surface_free); I think it's weird that no matter how references are on the object, removing it from the hashtable destroys it, but the code seems to be intentionally set up this way. It does mean that we can't use g_hash_table_steal or anything like that. ::: src/wayland/meta-wayland-touch.h @@ +44,3 @@ +}; + +struct _MetaWaylandTouchSurface Is there any reason this is public? It seems this can just be internal to meta-wayland-touch.c @@ +53,3 @@ +}; + +struct _MetaWaylandTouchInfo Same.
Created attachment 277904 [details] [review] wayland: Implement the server side bits of wl_touch_interface Clutter touch events are translated into events being sent down the interface resource, with the exception of FRAME/CANCEL events, which are handled directly via an evdev event filter. The seat now announces invariably the WL_SEAT_CAPABILITY_TOUCH capability, this should be eventually updated as devices come and go. The creation of MetaWaylandTouchSurface structs is dynamic, attached to the lifetime of first/last touch on the client surface, and only if the surface requests the wl_touch interface. MetaWaylandTouchInfo structs are created to track individual touches, and are locked to a single MetaWaylandTouchSurface (the implicit grab surface) determined on CLUTTER_TOUCH_BEGIN.
The last patch contains all suggested fixes, just minor comments below (In reply to comment #13) > @@ +484,3 @@ > + touch->display = display; > + touch->touch_surfaces = g_hash_table_new_full (NULL, NULL, NULL, > + (GDestroyNotify) > touch_surface_free); > > I think it's weird that no matter how references are on the object, removing it > from the hashtable destroys it, but the code seems to be intentionally set up > this way. It does mean that we can't use g_hash_table_steal or anything like > that. Yeah, the hashtable was meant to have the ownership on the MetaWayland. I've changed that so it's unref() which really frees things when the refcount drops to 0, but things still kind of rely on having the surface removed from the hashtable when the last touch is lifted, and that's controlled through the refcount.
I'd be fine with having the touch take ownership, and renaming the refcount to something like "touch_count", with a comment explaining the scheme.
erm, having the hash table take ownership, rather.
I've finally changed back memory ownership to the hashtable, renamed ref/unref to increment/decrement_touch(), and added further comments in touch_surface_decrement_touch() and touch_handle_surface_destroy() about the memory management detail. This is now in master. Attachment 277904 [details] pushed as 2250865 - wayland: Implement the server side bits of wl_touch_interface