[PATCH] dix: Save touchpoint last coordinates before transform. #49347

[Peter Hutterer]

On Sun, Nov 18, 2012 at 11:29:41PM -0500, Yuly Novikov wrote:
> Well then, out of the 3 possible options, which one do you prefer:
> 1. Storing X and Y before transform in addition to valuators.
> 2. Storing only the valuators, but pre-transform.

this one ^^
so essentially your patch, but without the removal of the valuator mask

Cheers,
   Peter

> 3. Storing the post transform valuators as now, but inverse transform
> them and transform again each time.
> I don't really like option 3, since not every transformation is
> invertible and there will be an error if transformation is changed.
> My preference is option 2.
> What do you think? Am I right in interpreting your comment that your
> preference is option 1?
> 
> Regards,
> Yuly
> 
> On Sun, Nov 18, 2012 at 10:32 PM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > sorry about the delay, I wanted to get a test-case for this first to
> > reliably reproduce it and that took me a while.
> >
> > On Thu, Nov 08, 2012 at 05:13:39PM -0500, Yuly Novikov wrote:
> >> A touchpoint used to store the entire valuators structure,
> >> and coordinates there were stored post-transformation,
> >> which resulted in Coordinate Transformation Matrix
> >> being applied multiple times to the last coordinates,
> >> in the case when only pressure changes in the last touch event.
> >>
> >> This commit gets rid of the valuators, as only X and Y coordinates
> >> are actually used, and stores only the relevant values.
> >> The variable name was changed to indicate
> >> that the values are stored before transformation,
> >> to avoid confusion with last.valuators, which are after transformation.
> >
> > NACK to that part, the reason we use the ValuatorMask here is because this
> > way we always deal with valuator masks when dealing with input coordinates.
> > last.valuator not being a VM yet is a leftover from the time before the VMs
> > were introduced.
> >
> > please leave that as-is, even if it feels clunky.
> >
> > Cheers,
> >    Peter
> >
> >>
> >> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=49347
> >>
> >> Signed-off-by: Yuly Novikov <ynovikov at chromium.org>
> >
> >> ---
> >>  dix/devices.c      |    2 --
> >>  dix/getevents.c    |   19 ++++++-------------
> >>  dix/touch.c        |    1 -
> >>  include/inputstr.h |    3 +--
> >>  4 files changed, 7 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/dix/devices.c b/dix/devices.c
> >> index 613323f..d23b1b4 100644
> >> --- a/dix/devices.c
> >> +++ b/dix/devices.c
> >> @@ -967,8 +967,6 @@ CloseDevice(DeviceIntPtr dev)
> >>      free(dev->deviceGrab.sync.event);
> >>      free(dev->config_info);     /* Allocated in xf86ActivateDevice. */
> >>      free(dev->last.scroll);
> >> -    for (j = 0; j < dev->last.num_touches; j++)
> >> -        free(dev->last.touches[j].valuators);
> >>      free(dev->last.touches);
> >>      dev->config_info = NULL;
> >>      dixFreePrivates(dev->devPrivates, PRIVATE_DEVICE);
> >> diff --git a/dix/getevents.c b/dix/getevents.c
> >> index 8b4379d..fe60ac3 100644
> >> --- a/dix/getevents.c
> >> +++ b/dix/getevents.c
> >> @@ -1951,32 +1951,25 @@ GetTouchEvents(InternalEvent *events, DeviceIntPtr dev, uint32_t ddx_touchid,
> >>      default:
> >>          return 0;
> >>      }
> >> -    if (t->mode == XIDirectTouch && !(flags & TOUCH_CLIENT_ID)) {
> >> -        if (!valuator_mask_isset(&mask, 0))
> >> -            valuator_mask_set_double(&mask, 0,
> >> -                                     valuator_mask_get_double(touchpoint.ti->
> >> -                                                              valuators, 0));
> >> -        if (!valuator_mask_isset(&mask, 1))
> >> -            valuator_mask_set_double(&mask, 1,
> >> -                                     valuator_mask_get_double(touchpoint.ti->
> >> -                                                              valuators, 1));
> >> -    }
> >>
> >>      /* Get our screen event co-ordinates (root_x/root_y/event_x/event_y):
> >>       * these come from the touchpoint in Absolute mode, or the sprite in
> >>       * Relative. */
> >>      if (t->mode == XIDirectTouch) {
> >> -        transformAbsolute(dev, &mask);
> >>
> >>          if (!(flags & TOUCH_CLIENT_ID)) {
> >> -            for (i = 0; i < valuator_mask_size(&mask); i++) {
> >> +            for (i = 0; i < 2; i++) {
> >>                  double val;
> >>
> >>                  if (valuator_mask_fetch_double(&mask, i, &val))
> >> -                    valuator_mask_set_double(touchpoint.ti->valuators, i, val);
> >> +                    touchpoint.ti->last_raw_axes[i] = val;
> >> +                else
> >> +                    valuator_mask_set_double(&mask, i,
> >> +                                             touchpoint.ti->last_raw_axes[i]);
> >>              }
> >>          }
> >>
> >> +        transformAbsolute(dev, &mask);
> >>          clipAbsolute(dev, &mask);
> >>      }
> >>      else {
> >> diff --git a/dix/touch.c b/dix/touch.c
> >> index 5f77be5..5074ec7 100644
> >> --- a/dix/touch.c
> >> +++ b/dix/touch.c
> >> @@ -226,7 +226,6 @@ void
> >>  TouchInitDDXTouchPoint(DeviceIntPtr dev, DDXTouchPointInfoPtr ddxtouch)
> >>  {
> >>      memset(ddxtouch, 0, sizeof(*ddxtouch));
> >> -    ddxtouch->valuators = valuator_mask_new(dev->valuator->numAxes);
> >>  }
> >>
> >>  Bool
> >> diff --git a/include/inputstr.h b/include/inputstr.h
> >> index 5a38924..c06f9fe 100644
> >> --- a/include/inputstr.h
> >> +++ b/include/inputstr.h
> >> @@ -330,8 +330,7 @@ typedef struct _DDXTouchPointInfo {
> >>      Bool active;                /* whether or not the touch is active */
> >>      uint32_t ddx_id;            /* touch ID given by the DDX */
> >>      Bool emulate_pointer;
> >> -
> >> -    ValuatorMask *valuators;    /* last recorded axis values */
> >> +    double last_raw_axes[2];    /* last X/Y axis valuator data as posted */
> >>  } DDXTouchPointInfoRec;
> >>
> >>  typedef struct _TouchClassRec {
> >> --
> >> 1.7.7.3
> >>