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 <[email protected]> > 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 <[email protected]> > --- > 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 <[email protected]> > 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 <[email protected]> > --- > 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 <[email protected]> > 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 <[email protected]> > --- > 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 <[email protected]> > 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 <[email protected]> > --- > 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 <[email protected]> > 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 <[email protected]> > --- > 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 > > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
