[RFC] touch fixes for high-resolution devices
Peter Hutterer
peter.hutterer at who-t.net
Tue Nov 6 22:37:30 PST 2012
On Thu, Oct 18, 2012 at 02:13:03AM -0400, Thomas Jaeger wrote:
> The resolution of wacom touch screen devices is different from the
> screen resolution, which results in incorrect pointer movement as
> described in [1] and [2]. This is caused by calls to GetTouchEvents
> during touch event processing, which has unintended side effects such as
> moving the sprite or setting the last position valuators. The touch
> code calls GetTouchEvents in two places: (1) When replaying events and
> (2) when generating TouchEnd events that don't correspond to a user action.
> In case (1) the only purpose of this call is to generate a possible DCCE
> event. But since GetTouchEvents operates under the assumption that the
> device is a slave device, no event is generated for master devices.
> Patch 3 factors out the generation of the DCCE event (fixing the issue
> just described) and then replays the actual event. A remaining problem
> is that the DCCE event is passed to DeliverTouchEvents, which ignores
> the event as far as I can tell.
> In case (2), a touch event needs to be generated from a TouchPointInfo.
> GetTouchEvents already has lots code conditional on whether it is
> called from the driver or from the touch processing code, which could be
> extended further to handle this case correctly, but at this point, it
> seems simpler to split it into two separate functions, which is what
> patch 4 does.
>
> I've been testing these patches for the last few days and I haven't
> noticed any additional issues.
fwiw, I can confirm this patch set fixes at least two cases:
- cursor jumps after using a touch device
- coordinates in event history replaying.
the current server has the values slightly off in the subpixels, this set
fixes that (with a modification, see below).
so, happy with the patch set in general, I have a few comments though.
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=55477
> [2] http://lists.x.org/archives/xorg-devel/2012-October/033914.html
> From ba12ba280990a89e85a2e2516f855f4fca42956e Mon Sep 17 00:00:00 2001
> From: Thomas Jaeger <ThJaeger at gmail.com>
> Date: Sat, 13 Oct 2012 22:39:27 -0400
> Subject: [PATCH 1/5] remove init_event
>
> The function is identical to init_device_event from inpututils.c with
> the first two arguments swapped.
>
> Signed-off-by: Thomas Jaeger <ThJaeger at gmail.com>
> ---
> dix/getevents.c | 13 +------------
> 1 file changed, 1 insertion(+), 12 deletions(-)
>
> diff --git a/dix/getevents.c b/dix/getevents.c
> index 71d83c4..492579d 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -164,17 +164,6 @@ key_autorepeats(DeviceIntPtr pDev, int key_code)
> }
>
> static void
> -init_event(DeviceIntPtr dev, DeviceEvent *event, Time ms)
> -{
> - memset(event, 0, sizeof(DeviceEvent));
> - event->header = ET_Internal;
> - event->length = sizeof(DeviceEvent);
> - event->time = ms;
> - event->deviceid = dev->id;
> - event->sourceid = dev->id;
> -}
> -
> -static void
> init_touch_ownership(DeviceIntPtr dev, TouchOwnershipEvent *event, Time ms)
> {
> memset(event, 0, sizeof(TouchOwnershipEvent));
> @@ -1914,7 +1903,7 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
> event = &events->device_event;
> num_events++;
>
> - init_event(dev, event, ms);
> + init_device_event(event, dev, ms);
> /* if submitted for master device, get the sourceid from there */
> if (flags & TOUCH_CLIENT_ID) {
> event->sourceid = touchpoint.dix_ti->sourceid;
> --
> 1.7.10.4
>
> From 975304f3fe106a79c7834daba409b9481320ca2f Mon Sep 17 00:00:00 2001
> From: Thomas Jaeger <ThJaeger at gmail.com>
> Date: Sat, 13 Oct 2012 22:43:26 -0400
> Subject: [PATCH 2/5] Update the MD's position when a touch event is received
>
> Signed-off-by: Thomas Jaeger <ThJaeger at gmail.com>
> ---
> dix/getevents.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/dix/getevents.c b/dix/getevents.c
> index 492579d..6c592ec 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -1993,6 +1993,14 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
> if (emulate_pointer)
> storeLastValuators(dev, &mask, 0, 1, devx, devy);
>
> + /* Update the MD's co-ordinates, which are always in desktop space. */
> + if (emulate_pointer && !IsMaster(dev) && !IsFloating(dev)) {
> + DeviceIntPtr master = GetMaster(dev, MASTER_POINTER);
> +
> + master->last.valuators[0] = screenx;
> + master->last.valuators[1] = screeny;
> + }
> +
> event->root = scr->root->drawable.id;
>
> event_set_root_coordinates(event, screenx, screeny);
> --
> 1.7.10.4
>
> From 287a2a6495c039147b4449ca66672dbb4d60ad68 Mon Sep 17 00:00:00 2001
> From: Thomas Jaeger <ThJaeger at gmail.com>
> Date: Sat, 13 Oct 2012 22:51:24 -0400
> Subject: [PATCH 3/5] Don't use GetTouchEvents when replaying events
>
> GetTouchEvents has plenty of side effects such as moving the pointer or
> updating the master device, which we don't want to happen when
> replaying. The only reason for calling it was to generate a DCCE event,
> but GetTouchEvents doesn't even do that right (we might need a DCCE
> event even when replaying a master event, or clients could interpret
> valuator data incorrectly).
>
> This discussion is moot at the moment anyway, since DeliverTouchEvents
> doesn't appear to deliver DCCE events.
>
> Signed-off-by: Thomas Jaeger <ThJaeger at gmail.com>
> ---
> dix/touch.c | 61 ++++++++++++++++++++++++-------------------------------
> include/input.h | 3 +++
> 2 files changed, 30 insertions(+), 34 deletions(-)
>
> diff --git a/dix/touch.c b/dix/touch.c
> index 497ad7d..34ed240 100644
> --- a/dix/touch.c
> +++ b/dix/touch.c
> @@ -463,45 +463,14 @@ TouchEventHistoryPush(TouchPointInfoPtr ti, const DeviceEvent *ev)
> void
> TouchEventHistoryReplay(TouchPointInfoPtr ti, DeviceIntPtr dev, XID resource)
> {
> - InternalEvent *tel;
> - ValuatorMask *mask;
> - int i, nev;
> - int flags;
> + int i;
>
> if (!ti->history)
> return;
>
> - tel = InitEventList(GetMaximumEventsNum());
> - mask = valuator_mask_new(0);
> -
> - valuator_mask_set_double(mask, 0, ti->history[0].valuators.data[0]);
> - valuator_mask_set_double(mask, 1, ti->history[0].valuators.data[1]);
> -
> - flags = TOUCH_CLIENT_ID | TOUCH_REPLAYING;
> - if (ti->emulate_pointer)
> - flags |= TOUCH_POINTER_EMULATED;
> - /* Generate events based on a fake touch begin event to get DCCE events if
> - * needed */
> - /* FIXME: This needs to be cleaned up */
> - nev = GetTouchEvents(tel, dev, ti->client_id, XI_TouchBegin, flags, mask);
> - for (i = 0; i < nev; i++) {
> - /* Send saved touch begin event */
> - if (tel[i].any.type == ET_TouchBegin) {
> - DeviceEvent *ev = &ti->history[0];
> - ev->flags |= TOUCH_REPLAYING;
> - DeliverTouchEvents(dev, ti, (InternalEvent*)ev, resource);
> - }
> - else {/* Send DCCE event */
> - tel[i].any.time = ti->history[0].time;
> - DeliverTouchEvents(dev, ti, tel + i, resource);
> - }
> - }
> -
> - valuator_mask_free(&mask);
> - FreeEventList(tel, GetMaximumEventsNum());
> + TouchDeliverDeviceClassesChangedEvent(ti, ti->history[0].time, resource);
>
> - /* First event was TouchBegin, already replayed that one */
> - for (i = 1; i < ti->history_elements; i++) {
> + for (i = 0; i < ti->history_elements; i++) {
> DeviceEvent *ev = &ti->history[i];
>
> ev->flags |= TOUCH_REPLAYING;
> @@ -509,6 +478,30 @@ TouchEventHistoryReplay(TouchPointInfoPtr ti, DeviceIntPtr dev, XID resource)
> }
> }
>
> +void
> +TouchDeliverDeviceClassesChangedEvent(TouchPointInfoPtr ti, Time time,
> + XID resource)
> +{
> + DeviceIntPtr dev;
> + int num_events = 0;
> + InternalEvent dcce;
> +
> + dixLookupDevice(&dev, ti->sourceid, serverClient, DixWriteAccess);
> +
> + if (!dev)
> + return;
> +
> + /* UpdateFromMaster generates at most one event */
> + UpdateFromMaster(&dcce, dev, DEVCHANGE_POINTER_EVENT, &num_events);
> + BUG_WARN(num_events > 1);
> +
> + if (num_events) {
> + dcce.any.time = time;
> + // FIXME: This doesn't do anything!
> + DeliverTouchEvents(dev, ti, &dcce, resource);
This should be delivered trough ProcessOtherEvents, more specifically
pumped into
dev->public.processInputProc(event, dev);
That should ensure that it is processed correctly. (I admit, at this point I
do not have a test for DCCE between history replaying)
> + }
> +}
> +
> Bool
> TouchBuildDependentSpriteTrace(DeviceIntPtr dev, SpritePtr sprite)
> {
> diff --git a/include/input.h b/include/input.h
> index 5747f3c..6d46f5d 100644
> --- a/include/input.h
> +++ b/include/input.h
> @@ -579,6 +579,9 @@ extern int TouchListenerAcceptReject(DeviceIntPtr dev, TouchPointInfoPtr ti,
> int listener, int mode);
> extern int TouchAcceptReject(ClientPtr client, DeviceIntPtr dev, int mode,
> uint32_t touchid, Window grab_window, XID *error);
> +extern void TouchDeliverDeviceClassesChangedEvent(TouchPointInfoPtr ti,
> + Time time, XID resource);
> +
>
> /* misc event helpers */
> extern Mask GetEventMask(DeviceIntPtr dev, xEvent *ev, InputClientsPtr clients);
> --
> 1.7.10.4
>
> From 638754745e9e14b657a730242df3ebf632ca8f85 Mon Sep 17 00:00:00 2001
> From: Thomas Jaeger <ThJaeger at gmail.com>
> Date: Sat, 13 Oct 2012 23:08:27 -0400
> Subject: [PATCH 4/5] Don't use GetTouchEvents in EmitTouchEnd
>
> As before GetTouchEvents causes unwanted side effects. Add a new
> function GetDixTouchEnd, which generates a touch event from the touch
> point. We fill in the event's screen coordinates from the MD's current
> sprite position.
>
> Signed-off-by: Thomas Jaeger <ThJaeger at gmail.com>
> ---
> Xi/exevents.c | 18 ++++--------------
> dix/getevents.c | 31 +++++++++++++++++++++++++++++++
> include/input.h | 5 +++++
> 3 files changed, 40 insertions(+), 14 deletions(-)
>
> diff --git a/Xi/exevents.c b/Xi/exevents.c
> index 4cbeb37..dea536f 100644
> --- a/Xi/exevents.c
> +++ b/Xi/exevents.c
> @@ -1066,24 +1066,14 @@ ActivateEarlyAccept(DeviceIntPtr dev, TouchPointInfoPtr ti)
> static void
> EmitTouchEnd(DeviceIntPtr dev, TouchPointInfoPtr ti, int flags, XID resource)
> {
> - InternalEvent *tel = InitEventList(GetMaximumEventsNum());
> - ValuatorMask *mask = valuator_mask_new(2);
> - int i, nev;
> -
> - valuator_mask_set_double(mask, 0,
> - valuator_mask_get_double(ti->valuators, 0));
> - valuator_mask_set_double(mask, 1,
> - valuator_mask_get_double(ti->valuators, 1));
> + InternalEvent event;
>
> flags |= TOUCH_CLIENT_ID;
> if (ti->emulate_pointer)
> flags |= TOUCH_POINTER_EMULATED;
> - nev = GetTouchEvents(tel, dev, ti->client_id, XI_TouchEnd, flags, mask);
> - for (i = 0; i < nev; i++)
> - DeliverTouchEvents(dev, ti, tel + i, resource);
> -
> - valuator_mask_free(&mask);
> - FreeEventList(tel, GetMaximumEventsNum());
> + TouchDeliverDeviceClassesChangedEvent(ti, GetTimeInMillis(), resource);
> + GetDixTouchEnd(&event, dev, ti, flags);
> + DeliverTouchEvents(dev, ti, &event, resource);
> }
>
> /**
> diff --git a/dix/getevents.c b/dix/getevents.c
> index 6c592ec..c48bbdb 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -2021,6 +2021,37 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
> return num_events;
> }
>
> +void
> +GetDixTouchEnd(InternalEvent *ievent, DeviceIntPtr dev, TouchPointInfoPtr ti,
> + uint32_t flags)
> +{
> + ScreenPtr scr = dev->spriteInfo->sprite->hotPhys.pScreen;
> + DeviceEvent *event = &ievent->device_event;
> + CARD32 ms = GetTimeInMillis();
> +
> + BUG_WARN(!dev->enabled);
> +
> + init_device_event(event, dev, ms);
> +
> + event->sourceid = ti->sourceid;
> + event->type = ET_TouchEnd;
> +
> + event->root = scr->root->drawable.id;
> +
> + /* Get screen event coordinates from the sprite. Is this really the best
> + * we can do? */
> + event_set_root_coordinates(event,
> + dev->spriteInfo->sprite->hotPhys.x,
> + dev->spriteInfo->sprite->hotPhys.y);
use dev->last.valuators[0] here instead of the sprite coordinates. this
maintains subpixel accuracy and then passes my tests too.
Cheers,
Peter
> + event->touchid = ti->client_id;
> + event->flags = flags;
> +
> + if (flags & TOUCH_POINTER_EMULATED) {
> + event->flags |= TOUCH_POINTER_EMULATED;
> + event->detail.button = 1;
> + }
> +}
> +
> /**
> * Synthesize a single motion event for the core pointer.
> *
> diff --git a/include/input.h b/include/input.h
> index 6d46f5d..3ad2cb3 100644
> --- a/include/input.h
> +++ b/include/input.h
> @@ -465,6 +465,11 @@ extern int GetTouchOwnershipEvents(InternalEvent *events,
> TouchPointInfoPtr ti,
> uint8_t mode, XID resource, uint32_t flags);
>
> +extern void GetDixTouchEnd(InternalEvent *ievent,
> + DeviceIntPtr dev,
> + TouchPointInfoPtr ti,
> + uint32_t flags);
> +
> extern _X_EXPORT int GetProximityEvents(InternalEvent *events,
> DeviceIntPtr pDev,
> int type, const ValuatorMask *mask);
> --
> 1.7.10.4
>
> From 590410e453bf952bd5a20089072793029eedef12 Mon Sep 17 00:00:00 2001
> From: Thomas Jaeger <ThJaeger at gmail.com>
> Date: Sat, 13 Oct 2012 23:18:50 -0400
> Subject: [PATCH 5/5] Simplify GetTouchEvents
>
> With only one callee left, we are free to assume that
> !(flags & TOUCH_CLIENT_ID)
>
> Signed-off-by: Thomas Jaeger <ThJaeger at gmail.com>
> ---
> dix/getevents.c | 66 ++++++++++++++-----------------------------------------
> 1 file changed, 17 insertions(+), 49 deletions(-)
>
> diff --git a/dix/getevents.c b/dix/getevents.c
> index c48bbdb..7d42f56 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -1832,10 +1832,7 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
> int i;
> int num_events = 0;
> RawDeviceEvent *raw;
> - union touch {
> - TouchPointInfoPtr dix_ti;
> - DDXTouchPointInfoPtr ti;
> - } touchpoint;
> + DDXTouchPointInfoPtr ti;
> int need_rawevent = TRUE;
> Bool emulate_pointer = FALSE;
> int client_id = 0;
> @@ -1854,37 +1851,15 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
>
> /* Find and/or create the DDX touch info */
>
> - if (flags & TOUCH_CLIENT_ID) { /* A DIX-submitted TouchEnd */
> - touchpoint.dix_ti = TouchFindByClientID(dev, ddx_touchid);
> - BUG_RETURN_VAL(!touchpoint.dix_ti, 0);
> -
> - if (!mask_in ||
> - !valuator_mask_isset(mask_in, 0) ||
> - !valuator_mask_isset(mask_in, 1)) {
> - ErrorF
> - ("[dix] dix-submitted events must have x/y valuator information.\n");
> - return 0;
> - }
> -
> - need_rawevent = FALSE;
> - client_id = touchpoint.dix_ti->client_id;
> - }
> - else { /* a DDX-submitted touch */
> -
> - touchpoint.ti =
> - TouchFindByDDXID(dev, ddx_touchid, (type == XI_TouchBegin));
> - if (!touchpoint.ti) {
> - ErrorFSigSafe("[dix] %s: unable to %s touch point %u\n", dev->name,
> - type == XI_TouchBegin ? "begin" : "find", ddx_touchid);
> - return 0;
> - }
> - client_id = touchpoint.ti->client_id;
> + ti = TouchFindByDDXID(dev, ddx_touchid, (type == XI_TouchBegin));
> + if (!ti) {
> + ErrorFSigSafe("[dix] %s: unable to %s touch point %u\n", dev->name,
> + type == XI_TouchBegin ? "begin" : "find", ddx_touchid);
> + return 0;
> }
> + client_id = ti->client_id;
>
> - if (!(flags & TOUCH_CLIENT_ID))
> - emulate_pointer = touchpoint.ti->emulate_pointer;
> - else
> - emulate_pointer = ! !(flags & TOUCH_POINTER_EMULATED);
> + emulate_pointer = ti->emulate_pointer;
>
> if (!IsMaster(dev))
> events =
> @@ -1904,11 +1879,6 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
> num_events++;
>
> init_device_event(event, dev, ms);
> - /* if submitted for master device, get the sourceid from there */
> - if (flags & TOUCH_CLIENT_ID) {
> - event->sourceid = touchpoint.dix_ti->sourceid;
> - /* TOUCH_CLIENT_ID implies norawevent */
> - }
>
> switch (type) {
> case XI_TouchBegin:
> @@ -1933,20 +1903,20 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
> event->type = ET_TouchEnd;
> /* We can end the DDX touch here, since we don't use the active
> * field below */
> - if (!(flags & TOUCH_CLIENT_ID))
> - TouchEndDDXTouch(dev, touchpoint.ti);
> + TouchEndDDXTouch(dev, ti);
> break;
> default:
> return 0;
> }
> - if (t->mode == XIDirectTouch && !(flags & TOUCH_CLIENT_ID)) {
> +
> + if (t->mode == XIDirectTouch) {
> if (!valuator_mask_isset(&mask, 0))
> valuator_mask_set_double(&mask, 0,
> - valuator_mask_get_double(touchpoint.ti->
> + valuator_mask_get_double(ti->
> valuators, 0));
> if (!valuator_mask_isset(&mask, 1))
> valuator_mask_set_double(&mask, 1,
> - valuator_mask_get_double(touchpoint.ti->
> + valuator_mask_get_double(ti->
> valuators, 1));
> }
>
> @@ -1956,13 +1926,11 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
> if (t->mode == XIDirectTouch) {
> transformAbsolute(dev, &mask);
>
> - if (!(flags & TOUCH_CLIENT_ID)) {
> - for (i = 0; i < valuator_mask_size(&mask); i++) {
> - double val;
> + for (i = 0; i < valuator_mask_size(&mask); i++) {
> + double val;
>
> - if (valuator_mask_fetch_double(&mask, i, &val))
> - valuator_mask_set_double(touchpoint.ti->valuators, i, val);
> - }
> + if (valuator_mask_fetch_double(&mask, i, &val))
> + valuator_mask_set_double(ti->valuators, i, val);
> }
>
> clipAbsolute(dev, &mask);
> --
> 1.7.10.4
>
> _______________________________________________
> xorg-devel at lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
More information about the xorg-devel
mailing list