[PATCH xserver 16/17] Input: Add initial multitouch support from Xi 2.1

[Peter Hutterer]

On Thu, Jan 06, 2011 at 05:57:45PM +0000, Daniel Stone wrote:
> > >  /**
> > > + * 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.

that sounds like an excellent comment to put in the code...

> > > +    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?

event? do you man macro? if so, yes, but see your BitIsOn comment, if that
works.

> > > +    /* 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()?!

GetDeepestWindow(sprite)->drawable.id would be acceptable I think.

> > > +    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.

fwiw, DeliverTouchEvents would be good enough, given that's what the others
use too. but either way is fine.

> 
> > > +        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 (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?!

heh, good point.

> > > +        /* 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.

drivers _must_ have tracking anyway, so they know when a new touchpoint was
created. we could have xf86CreateTouchEvent() that only sets up the
server-internal mapping. so a driver would do

if (new_touch)
   xf86CreateTouchEvent(non-monotonic-touch-id)
xf86PostTouchEvent(non-monotonic-touch-id)

which would avoid any touch id tracking other than what the hardware does in
the driver. plus, bonus point, if a driver re-uses a tracking ID but didn't
send the TouchEnd event you can emulate one in xf86CreateTouchEvent() before
you get ID collision.

does this make sense? just thinking aloud here.

> > > +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.

i strongly recommend against having the array size in a separate struct, one
that we may at any time not even have a pointer to. just add a size field.

> > > +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.

already replied in the other email, leave it like this, thanks. maybe "axis
number of x axis" though. silly as it sounds, it actually read like a
copy/paste error when I looked at this first. the code clarifies it, but the
commend could easily be expanded.

Cheers,
  Peter
> 
> > 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