[PATCH 3/3] dix: support the transformation matrix for relative devices.

[Simon Thum]

On 06/03/2011 03:27 AM, Peter Hutterer wrote:
> On Thu, Jun 02, 2011 at 03:44:15PM +0200, Simon Thum wrote:
>> On 05/31/2011 05:57 AM, Peter Hutterer wrote:
>>> The transformation matrix we previously stored was a scaled matrix based on
>>> the axis ranges of the device. For relative movements, the scaling is not
>>> required (or desired).
>> Well, also any non-zero translating component will kill the mouse. That
>> also should be zeroed out in the relative transform matrix (which I'd
>> name after the purpose). In principle that matrix would be 2x2.
> 
> Then we need to zero it out on-the-fly since a device may have the same
> transformation matrix (for rotation for example) but the coordinates it
> sends may be either abs or relative.
I'd probably also do it that way, but I think there's even a more
elegant solution: Instead of

(x y 1) * matrix

you could do

(x y 0) * matrix

That should work equally well. Of course, if you have to split abs/rel
matrices for other reasons, that's overkill.

> 
>> Now for scaling or rotation you'll need sub-pixel processing, or it will
>> only work properly with orthogonal angles and integer scale. Also I'd
>> say the transform should be applied after acceleration, but arguments
>> can be made for any order. 
> 
> I think having the acceleration apply after transformation ensures
> consistency - the acceleration of the "left" movement of the pointer is
> always the same. I can be convinced otherwise though.
In principle I'd also prefer that order. My arguments were based on ease
of implementation, see below.

> 
> also, aiui, if the transformation applies before accel, we can merge this
> patch - the issues pointed out below apply to "accel before transform"
> only.
If that was the case you'd have simply seen my reviewed-by ;)

First off, you're discarding sub-pixel information that doesn't make it
into the (integer) output. That's wrong by itself, e.g. scaling down
cancels slow motion, you can't really rotate by small angles, etc.

> 
>> My argument would be that acceleration has
>> been built on the assumption that the input "is" integer mickeys more or
>> less straight from the device, and the remainder is more or less private
>> stuff. Without descending into qualia considerations, there are three
>> solutions:
> 
> correct me if I'm wrong here but the current accel code seems to also assume
> a unified resolution. the higher the device's resolution, the higher the
> mickeys (and thus the acceleration) but I didn't see anything in the accel
> code that takes this into account. this may be something worth looking into
> as well.
Constant deceleration and/or velocity scaling serve exactly that
purpose. CD helps if you really want to slow down the pointer, VS can
adapt to resolution. I know I mostly portrayed VS it as a tool to adapt
to reporting rate differences, but that really is only the
"stay-compatible" use case, which should be largely irrelevant by now.
However, CD reduces both the estimate and final pointer speed, so it's a
sorta catch-all setting.

During HAL times, I tried to get both mouse resolution and reporting
rate available from HAL, but I got ignored ;( Based on that we could
compute a "suitable" VS, perhaps even including display resolution. I
think MS does that, although I don't really see the point.

I think "my" wiki page has a scenario for high-resolution mice, plus
some users asked for that. Perhaps the man page can be improved.

>  
>> 1 ) Base accel on float mickeys/deltas, and choose any order
>> 2 ) First accel, then transform valuator + remainder (without setting
>> the latter again, i.e. you need one more row of remainders and base your
>> events on that)
>> 3 ) Make the valuator masks double (or add FP val masks), and expose
>> them to drivers and accel and you-name-it.
> 
> I believe Daniel has a set of patches to do exactly that but they haven't
> been on the list yet.
I failed to look at them, TB just shows me a mailto: url.

> 
>> OK, 3) is actually 1) on steroids, but it's unavoidable in the long run,
>> if you ask me. 2) is the fix for now.
>>
>> So the most viable order would be 2-1-3 (too lazy to reshuffle). In any
>> case I'd feel better if you didn't push this patch. I think I'm
>> available enough these times to at least support that roadmap.
> 
> The big advantage we have with properties is that we are not locked into a
> single format. We can switch the matrix to support float format in the
> future while still supporting an integer matrix as well.
The problem isn't the matrix, it's retaining sub-pixel information (see
above). This would be non-existent if valuators were floats, so we could
choose any order. Other possibilities would be fixed-point valuators,
combined int/remainder valuators, or what not.

My point is that accel-before-transform is easier since accel produces a
non-integer outcome, and transform can use that. The converse isn't
true, at least at the moment.

My reasons for keeping velocity estimates based on raw integer data are
twofold; first, it's easier to cache directions (avoiding atan2).
Second, the smoothing feature depends on being handed true mickeys.

Both aren't very strong reasons, but I'd rather keep than drop these.
What I'd drop are the schemes - and your transform would do away the
last reason to keep them - enabling us to do the following data flow:

1) derive velocity estimate from the raw data (that's read-only)
2) transform relative
3) multiply in acceleration

If that coincides with daniel's patches (disclaimer: I didn't look at
them), we've gotten away with one API bump.

Sounds fine?

Cheers,

Simon


> 
> Cheers,
>   Peter
>  
>>> Store two separate matrices, one scaled, one unmodified. Depending on the
>>> type of movement, apply the respective matrix.
>>>
>>> This breaks the DeviceIntRec ABI
>>>
>>> Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
>>> ---
>>>  dix/devices.c      |    4 ++--
>>>  dix/getevents.c    |   26 +++++++++++++++++++++++++-
>>>  include/inputstr.h |    5 ++++-
>>>  3 files changed, 31 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/dix/devices.c b/dix/devices.c
>>> index 0ccf252..4e4b702 100644
>>> --- a/dix/devices.c
>>> +++ b/dix/devices.c
>>> @@ -125,14 +125,14 @@ DeviceSetTransform(DeviceIntPtr dev, float *transform)
>>>          for (x=0; x<3; x++)
>>>              dev->transform.m[y][x] = *transform++;
>>>  
>>> -    pixman_f_transform_multiply(&dev->transform, &scale, &dev->transform);
>>> +    pixman_f_transform_multiply(&dev->scale, &scale, &dev->transform);
>>>  
>>>      /* scale */
>>>      pixman_f_transform_init_scale(&scale, 1.0 / sx, 1.0 / sy);
>>>      scale.m[0][2] = -dev->valuator->axes[0].min_value / sx;
>>>      scale.m[1][2] = -dev->valuator->axes[1].min_value / sy;
>>>  
>>> -    pixman_f_transform_multiply(&dev->transform, &dev->transform, &scale);
>>> +    pixman_f_transform_multiply(&dev->scale, &dev->scale, &scale);
>>>  }
>>>  
>>>  /**
>>> diff --git a/dix/getevents.c b/dix/getevents.c
>>> index 1352a81..26c7f5d 100644
>>> --- a/dix/getevents.c
>>> +++ b/dix/getevents.c
>>> @@ -1066,6 +1066,28 @@ transform(struct pixman_f_transform *m, int *x, int *y)
>>>      *y = lround(p.v[1]);
>>>  }
>>>  
>>> +
>>> +static void
>>> +transformRelative(DeviceIntPtr dev, ValuatorMask *mask)
>>> +{
>>> +    int x, y;
>>> +
>>> +    x = valuator_mask_isset(mask, 0) ? valuator_mask_get(mask, 0) : 0;
>>> +    y = valuator_mask_isset(mask, 1) ? valuator_mask_get(mask, 1) : 0;
>>> +
>>> +    transform(&dev->transform, &x, &y);
>>> +
>>> +    if (x)
>>> +        valuator_mask_set(mask, 0, x);
>>> +    else
>>> +        valuator_mask_unset(mask, 0);
>>> +
>>> +    if (y)
>>> +        valuator_mask_set(mask, 1, y);
>>> +    else
>>> +        valuator_mask_unset(mask, 1);
>>> +}
>>> +
>>>  static void
>>>  transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
>>>  {
>>> @@ -1076,7 +1098,7 @@ transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
>>>      oy = y = valuator_mask_isset(mask, 1) ? valuator_mask_get(mask, 1) :
>>>                                              dev->last.valuators[1];
>>>  
>>> -    transform(&dev->transform, &x, &y);
>>> +    transform(&dev->scale, &x, &y);
>>>  
>>>      if (valuator_mask_isset(mask, 0) || ox != x)
>>>          valuator_mask_set(mask, 0, x);
>>> @@ -1200,6 +1222,8 @@ GetPointerEvents(InternalEvent *events, DeviceIntPtr pDev, int type, int buttons
>>>          transformAbsolute(pDev, &mask);
>>>          moveAbsolute(pDev, &x, &y, &mask);
>>>      } else {
>>> +        transformRelative(pDev, &mask);
>>> +
>>>          if (flags & POINTER_ACCELERATE) {
>>>              accelPointer(pDev, &mask, ms);
>>>              /* The pointer acceleration code modifies the fractional part
>>> diff --git a/include/inputstr.h b/include/inputstr.h
>>> index 00f72c2..b917f6c 100644
>>> --- a/include/inputstr.h
>>> +++ b/include/inputstr.h
>>> @@ -530,8 +530,11 @@ typedef struct _DeviceIntRec {
>>>          XIPropertyHandlerPtr handlers; /* NULL-terminated */
>>>      } properties;
>>>  
>>> -    /* coordinate transformation matrix for absolute input devices */
>>> +    /* coordinate transformation matrix */
>>>      struct pixman_f_transform transform;
>>> +    /* scale matrix for absolute devices, this is the combined matrix of
>>> +       [1/scale] . [transform] . [scale]. See DeviceSetTransform */
>>> +    struct pixman_f_transform scale;
>>>  
>>>      /* XTest related master device id */
>>>      int xtest_master_id;
>>
>