[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