[PATCH 3/3] dix: support the transformation matrix for relative devices.
Peter Hutterer
peter.hutterer at who-t.net
Tue Jun 7 21:52:16 PDT 2011
On Sun, Jun 05, 2011 at 01:32:45PM +0200, Simon Thum wrote:
> 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.
I think they need to be split for the scaling anyway. but this would still
help with avoiding invalid client values, thanks.
> >> 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.
the problem with pointer acceleration is that the UI to tweak it is is
suboptimal. yes, there's xinput but we don't have a ubiquitous GUI tool for
a per-device CD. it'd be great if the default settings for each device would
adjust accordingly.
evdev does report resolution if the kernel provides it, but only on absolute
devices so far because that's all the kernel gives us.
> >> 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.
each email has a Message-ID header, that's what you can search on when
someone posts this.
> >> 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?
yes, because unless I misunderstood it again, we can apply daniel's
valuator_*_double patches and then just apply the transformation as
originally proposed in this patch :)
plus the velocity estimate, but that's hopefully the simple bit.
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;
> >>
> >
>
More information about the xorg-devel
mailing list