[PATCH xserver 16/17] Input: Add initial multitouch support from Xi 2.1
Daniel Stone
daniel at fooishbar.org
Thu Jan 6 09:57:45 PST 2011
Hi,
On Thu, Jan 06, 2011 at 10:18:00AM +1000, Peter Hutterer wrote:
> On Tue, Dec 28, 2010 at 05:58:07PM +0000, Daniel Stone wrote:
> > Xi 2.1 adds TouchClasses to devices, as well as TouchBegin, TouchMotion
> > and TouchEnd events, to allow support for multiple touchpoints on a
> > single device.
> >
> > Signed-off-by: Daniel Stone <daniel at fooishbar.org>
> > Co-authored-by: Chase Douglas <chase.douglas at canonical.com>
>
> urgh, you need to start adding comments on what changed in respect to the
> last patch.
Sorry, yes - will do in future.
> reviewed bottum up this time to prevent the last hunks from not getting any
> fresh-brain review, but it might also explain comments lacking context as
> you go down this email :)
Heh.
> > /**
> > + * Ensure a window trace is present in ti->sprite, constructing one for
> > + * TouchBegin events.
> > + */
> > +static Bool
> > +EnsureTouchSprite(DeviceIntPtr sourcedev, TouchPointInfoPtr ti, DeviceEvent *ev)
> > +{
> > + TouchClassPtr t = sourcedev->touch;
> > + SpritePtr sprite = &ti->sprite;
> > + int i;
> > +
> > + if (ev->type != ET_TouchBegin)
> > + {
> > + if (ev->type == ET_TouchMotion && sprite->spriteTraceGood <= 0)
> > + return FALSE;
>
> shouldn't this check for ET_TouchEnd too?
Not really. It's a bit more subtle than it needs to be -
ProcessTouchEnd _must_ call FinishTouchPoint for TouchEnd events, even
if there's no sprite. A sprite trace can be empty if nothing has
selected for it (or the selections have disappeared), and nothing has
grabbed for it, or the grabs have disappeared, or the grab owners have
all rejected the touch. So when this happens, we want to throw motion
events on the floor, but continue processing TouchEnd events.
> > + /* Change the device ID in the event to reflect who we're delivering
> > + * to. */
>
> a bit confusing imo, maybe "to reflect the device we're delivering the event
> for".
Sure, done.
> > + free(xi2);
> > + xi2 = NULL;
> > + ev->deviceid = deliverdev->id;
> > + err = EventToXI2((InternalEvent *) ev, &xi2);
> > + if (err != Success)
> > + {
> > + ErrorF("[Xi] %s: XI2 conversion failed in ProcessTouchEvent (%d)\n",
>
> typo "failed in DeliverTouchGrabEvents"
Fixed.
> > + for (i = sprite->spriteTraceGood; i >= 0; i--)
> > + {
> > + /* First check whether anyone has selected for touch events on this
> > + * window at all. */
> > + win = sprite->spriteTrace[i];
> > + inputMasks = wOtherInputMasks(win);
> > + if (!inputMasks)
> > + continue;
> > + iclients = inputMasks->inputClients;
> > + if (!iclients)
> > + continue;
> > +#define test_xi2_mask(x, d, b, f) ((x)->xi2mask[d][b] & f)
>
> this seems like something that's useful in other functions as well.
Do you want it introduced and exported in another event, with all other
users converted?
> > + ev->deviceid = deliverdev->id;
> > + err = EventToXI2((InternalEvent *) ev, &xi2);
> > + if (err != Success)
> > + {
> > + ErrorF("[Xi] %s: XI2 conversion failed in ProcessTouchEvent "
>
> same typo here
Fixed.
> > + "(%d)\n", sourcedev->name, err);
> > + return;
> > + }
> > + FixUpEventFromWindow(sprite, xi2, win, child, FALSE);
> > +
> > + /* See comment in grab-delivery TouchBegin branch. */
> > + if (ev->type != ET_TouchBegin) {
>
> \n before {
Oops, I thought I'd been fairly militant about this. :\ Fixed.
> > + /* We handle deliveries to MDs through the SD, rather than copying
> > + * the event and processing it twice. */
> > + if (IsMaster(sourcedev))
> > + return;
>
> please expand this comment on the SD/MD handling. it's not specified in the
> protocol IIRC so it's an implementation detail and the intended behaviour
> should be documented somewhere. would make the sourcedev, deliverdev,
> masterdev, etc. juggling easier to understand in DeliverTouch*.
Sure, done.
> > + /* Now deliver first to grabbing clients, then to clients with an
> > + * applicable event selection. */
> > + child = sprite->spriteTrace[sprite->spriteTraceGood - 1]->drawable.id;
>
> this cries out for a macro with some nice, comforting, heart-warming name..
GetIDOfDeepestWindowInSpriteTrace()?!
> > + DeliverTouchGrabEvents(sourcedev, ti, ev, child);
> > + DeliverTouchSelectionEvents(sourcedev, ti, ev, child);
>
> I don't think using "Selection" is a good name choise. everywhere else we
> either use "mask" or nothing (DeliverGrabbedEvent vs DeliverDeviceEvents)..
> "Selection" is imo too close to the copy/paste method and every time I read
> it somewhere it throws me out for a second.
TBH, I still don't think that clears anything up (being that grabs have
masks too, bitmasks in general which are surely much more applicable to
event delivery than copy & paste, etc), but I've changed it to
DeliverTouchMaskEvents anyway.
> > + flags |= XITouchOwnerAccepted;
> > + nev = GetTouchEvents(events, dev, stuff->touchid, mask, 0, flags);
>
> FALSE instead of 0 please
Oops, yes.
> > + if (nev == 0)
> > + return BadAlloc;
> > + for (i = 0; i < nev; i++)
> > + {
> > + ev = (DeviceEvent *)((events + i)->event);
> > + mieqProcessDeviceEvent(dev, (InternalEvent *) ev, NULL);
> > + }
>
> this could be interesting. you're injecting into the event stream here
> directly, so there's a race condition where you end up with touch motion
> events after sending a touch end event. note that when the request arrives,
> a touch motion event may still be in the EQ, so we need some handling in
> ProcessTouchEvent to discard those that are XITouchOwnerAccepted when they
> go through the processing path.
If a TouchEnd event arrives and isn't processed before the
TouchOwnerAccepted, then I fail to see how that would break anything.
If the TouchOwnerAccepted arrives after the TouchEnd event, then that's
also not a problem because of the ti->pending_finish handling which
doesn't _actually_ finish a touch event on TouchEnd until all grab
owners have either accepted or rejected the touch.
> > if ((stuff->grab_type == XIGrabtypeEnter ||
> > - stuff->grab_type == XIGrabtypeFocusIn) && stuff->detail != 0)
> > + stuff->grab_type == XIGrabtypeFocusIn ||
> > + stuff->grab_type == XIGrabtypeTouchBegin) &&
> > + stuff->detail != 0)
>
>
> indentation
Yarr.
> > +int
> > +ListTouchInfo(DeviceIntPtr dev, xXITouchInfo *touch)
> > +{
> > + touch->type = XITouchClass;
> > + touch->length = sizeof(xXITouchInfo) >> 2;
> > + touch->sourceid = dev->id;
> > + touch->mode = dev->touch->mode;
> > + touch->num_touches = dev->touch->num_touches;
> > +
> > + return touch->length << 2;
>
> return touch->length * 4 for consistency with the other List*Info.
>
> > +int
> > +ListTouchValuatorInfo(DeviceIntPtr dev, xXITouchValuatorInfo* val,
> > + int axisnumber)
> > +{
> > + TouchClassPtr t = dev->touch;
> > +
> > + val->type = XITouchValuatorClass;
> > + val->length = sizeof(xXITouchValuatorInfo) >> 2;
> > + val->sourceid = dev->id;
> > + val->number = axisnumber;
> > + val->label = t->axes[axisnumber].label;
> > + val->min.integral = t->axes[axisnumber].min_value;
> > + val->min.frac = 0;
> > + val->max.integral = t->axes[axisnumber].max_value;
> > + val->max.frac = 0;
> > + val->resolution = t->axes[axisnumber].resolution;
> > +
> > + return val->length << 2;
>
> return val->length * 4 for consistency with the other List*Info.
Done.
> > + if (BitIsOn(bits, XI_TouchBegin))
> > + {
> > + OtherInputMasks *inputMasks = wOtherInputMasks(win);
> > + InputClients *iclient = NULL;
> > + if (inputMasks)
> > + iclient = inputMasks->inputClients;
> > + for (; iclient; iclient = iclient->next)
> > + {
> > + if (CLIENT_ID(iclient->resource) == client->index)
> > + continue;
> > + if (BitIsOn(iclient->xi2mask[evmask->deviceid],
> > + XI_TouchBegin) ||
> > + BitIsOn(iclient->xi2mask[XIAllDevices],
> > + XI_TouchBegin) ||
> > + (dev && (IsMaster(dev) || dev->u.master) &&
> > + BitIsOn(iclient->xi2mask[XIAllMasterDevices],
> > + XI_TouchBegin)))
> > + return BadAccess;
> > + }
> > + }
> > + }
> > +
>
> please split this out into a helper function. it's self-contained anyway.
Come to think of it, why am I not using BitIsOn rather than the
test_xi2_mask macro?!
> > + touch = calloc(1,
> > + sizeof(TouchClassRec) +
> > + num_axes * sizeof(TouchAxisInfoRec) +
> > + max_touches * sizeof(TouchPointInfoRec) +
> > + max_touches * num_axes * sizeof(int));
>
> can we make this distinct pointers instead? i hate this approach when it's
> not on the wire. especially since a few lines later the sprite trace on the
> touch sprites is malloced anywya, so now you have a nice mix malloc and
> non-malloc that's not really documented anywhere.
Fair enough.
> > /**
> > + * Get events for a touch. Generates a TouchBegin event if end is not set and
> > + * the touch id is not active. Generates a TouchMotion event if end is not set
> > + * and the touch id is active. Generates a TouchEnd event if end is set and the
> > + * touch id is active.
> > + *
> > + * events is not NULL-terminated; the return value is the number of events.
> > + * The DDX is responsible for allocating the event structure in the first
> > + * place via GetMaximumEventsNum(), and for freeing it.
> > + */
> > +int
> > +GetTouchEvents(EventList *events, DeviceIntPtr pDev, unsigned int touchid,
> > + const ValuatorMask *mask_in, Bool end, uint32_t flags)
>
> i suppose a new flag define for pointer emulation would be useful, even if
> it doesn't do anything yet. this way, we don't have to break the abi again
> when we know what to do with pointer emulation.
'k.
> > + /* Touch IDs must increase monotonically.
> > + * XXX: Deal with this so the drivers don't have to. */
>
> change the xf86 api to xf86CreateTouchEvent() for the first (returning the
> touchid) and then xf86PostTouchEvent() for subsequent calls.
Fair enough, though this does require slightly more active tracking in
the driver, and I'd rather have more dumb than more smart drivers.
Hence the comment: if the touchid isn't monotonically increasing, I'd
prefer to just have a server-internal mapping, rather than drivers
having to reinvent the same thing over and over.
> > + else {
> > + x = pDev->spriteInfo->sprite->hotPhys.x;
> > + y = pDev->spriteInfo->sprite->hotPhys.y;
>
> if pointer emulation is enabled, this needs to call moveAbsolute and
> positionSprite, aside from calling GPE somewhere too. please add a fixme so
> that at least we remember that.
Sure.
> > + while (j < ti->num_listeners - 1)
> > + {
> > + ti->listeners[j] = ti->listeners[j + 1];
> > + break;
> > + }
>
> this can't be right. either the break is out of place here or the while
> should be an if(). a memmove should do here, though I suppose the compiler
> will do the right thing either way.
Christ on a bike. Yes, the break should get removed.
> > + ti->num_listeners--;
> > + ti->num_grabs--;
> > +
> > + ProcessInputEvents();
> > + nev = GetTouchEvents(events, dev, ti->id, mask, 0, 0);
>
> parameter 5 should be FALSE, not 0.
Yep.
> > /**
> > + * Returns the event type to match when comparing grabs.
> > + */
> > +static uint32_t
>
> pGrab->type is CARD8 and XI_TouchBegin must be uint16 (XGE provides for has
> 16 bit event types), so the uint32_t seems to provide size precision where
> none is needed. a simple int would do, I think.
>
> > @@ -261,6 +333,8 @@ GrabMatchesSecond(GrabPtr pFirstGrab, GrabPtr pSecondGrab, Bool ignoreDevice)
> > unsigned int any_modifier = (pFirstGrab->grabtype == GRABTYPE_XI2) ?
> > (unsigned int)XIAnyModifier :
> > (unsigned int)AnyModifier;
> > + uint32_t first_type = GetGrabEventMatch(pFirstGrab);
> > + uint32_t second_type = GetGrabEventMatch(pSecondGrab);
>
> same here with uint32_t.
Oops.
> > @@ -548,3 +548,61 @@ CountBits(const uint8_t *mask, int len)
> >
> > return ret;
> > }
> > +
> > +int
> > +FindTouchPoint(DeviceIntPtr dev, unsigned int touchid)
>
> why not return a TouchInfoRec*/NULL here instead of the index? afaict, the
> return value is only used to index into the array later, so we might as well
> return the pointer directly. same goes for CreateTouchPoint.
Fair.
> > + ti->active = FALSE;
> > + ti->pending_finish = FALSE;
> > + ti->num_grabs = 0;
> > + ti->sprite.spriteTraceGood = 0;
> > + free(ti->listeners);
> > + ti->listeners = NULL;
> > + ti->num_listeners = 0;
>
> I admit I got a bit lost here, but - don't we need to reset the valuators
> somewhere here as well?
Yeah, we do.
> > +#include <X11/extensions/XI2proto.h>
>
> if this is for the XI_TouchMotion, it should include XI.h instead.
Fair enough.
> > +typedef struct _TouchPointInfo {
> > + Bool active; /* whether or not the touch is active */
> > + Bool pending_finish; /* true if the touch is physically inactive
> > + * but still owned by a grab */
> > + uint32_t id;
> > + SpriteRec sprite; /* window trace for delivery */
> > + int *valuators; /* last-recorded valuators */
>
> how big is valuators?
> wouldn't hurt to have a nvaluators field here, or just use a valuator mask?
> right now, if I just get passed a TPI I wouldn't know the size of the array.
TouchClassRec::num_axes.
> > +typedef struct _TouchAxisInfo {
> > + int resolution;
> > + int min_value;
> > + int max_value;
> > + Atom label;
> > +} TouchAxisInfoRec, *TouchAxisInfoPtr;
>
> s/tab/spaces please
Sure.
> > +typedef struct _TouchClassRec {
> > + TouchAxisInfoPtr axes;
> > + unsigned short num_axes;
> > + TouchPointInfoPtr touches;
> > + unsigned short num_touches;
> > + CARD8 mode; /* ::XIDirectTouch, XIDependentTouch */
> > + unsigned int last_touchid;
> > + int x_axis; /* axis number */
> > + int y_axis; /* axis number */
>
> axis number?? is this to hold the axis index for the x/y axis?
Yeah, it is.
> why not force the implementation to have x/y on 0/1, that's what we do for
> pointer events too.
*shrug*, it was from Chase's original patch, but to be honest I don't
really see the harm in being slightly more flexible.
> I don't see anything fundamentally wrong with the patch and I think my
> comments are more on style and general cleanups. I guess there will still be
> a bit of churn on the patch though. and I haven't actually tested it yet
> either.
Ha. Well, make sure you test the latest stuff out of my branch on
people.fd.o, which is what's in Chase's PPA, and what the Ubuntu guys
are testing against.
Cheers,
Daniel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg-devel/attachments/20110106/79220cd9/attachment.pgp>
More information about the xorg-devel
mailing list