[Patch xserver] acceleration cleanup
Peter Hutterer
peter.hutterer at who-t.net
Wed Feb 16 16:54:21 PST 2011
merged 1/4 and 3/4 into -next, skipping them in this reply. It'd be really
helpful if you could send the messages one-by-one though.
patch 4/4 still has a comment typo I've pointed out before - should the valuator
mask really be called first_valuator?? I'm not sure what has changed why in
the other patches, please always send a changelog of what changed over the
last version of the patch.
There are a few comments on the patches in
http://lists.freedesktop.org/archives/xorg-devel/2011-February/019033.html
maybe that email disappeared or got swamped out of your inbox?
Either way, I don't know whether the comments have been addressed or if not,
why not. The code looks better though, still some comments inside.
On Tue, Feb 15, 2011 at 12:40:35AM +0100, Simon Thum wrote:
> From e8758c7e77c4d5e501b4bcaf0622a4f3c9824253 Mon Sep 17 00:00:00 2001
> From: Simon Thum <simon.thum at gmx.de>
> Date: Sun, 5 Sep 2010 18:10:42 +0200
> Subject: [PATCH 2/4] dix: refactor predictable scheme initialization
>
> This intends to clean up the predictable accel struct
> from purely scheme-related things like input properties,
> as they would be useless in other use cases such
> as wheel acceleration.
>
> Signed-off-by: Simon Thum <simon.thum at gmx.de>
> ---
> dix/ptrveloc.c | 75 ++++++++++++++++++++++++++++++++++-----------------
> include/ptrveloc.h | 13 ++++++---
> 2 files changed, 59 insertions(+), 29 deletions(-)
>
> diff --git a/dix/ptrveloc.c b/dix/ptrveloc.c
> index 1b9c81b..5416ef7 100644
> --- a/dix/ptrveloc.c
> +++ b/dix/ptrveloc.c
> @@ -30,6 +30,7 @@
> #include <ptrveloc.h>
> #include <exevents.h>
> #include <X11/Xatom.h>
> +#include <os.h>
>
> #include <xserver-properties.h>
>
> @@ -68,9 +69,9 @@ SimpleSmoothProfile(DeviceIntPtr dev, DeviceVelocityPtr vel, float velocity,
> static PointerAccelerationProfileFunc
> GetAccelerationProfile(DeviceVelocityPtr vel, int profile_num);
> static BOOL
> -InitializePredictableAccelerationProperties(DeviceIntPtr dev);
> +InitializePredictableAccelerationProperties(DeviceIntPtr, long*);
> static BOOL
> -DeletePredictableAccelerationProperties(DeviceIntPtr dev);
> +DeletePredictableAccelerationProperties(DeviceIntPtr, long*);
>
> /*#define PTRACCEL_DEBUGGING*/
>
> @@ -87,6 +88,9 @@ DeletePredictableAccelerationProperties(DeviceIntPtr dev);
> /* some int which is not a profile number */
> #define PROFILE_UNINITIALIZE (-100)
>
> +/* number of properties for predictable acceleration */
> +#define NPROPS_PREDICTABLE_ACCEL 4
> +
>
> /**
> * Init DeviceVelocity struct so it should match the average case
> @@ -125,17 +129,24 @@ FreeVelocityData(DeviceVelocityPtr vel){
> */
> Bool
> InitPredictableAccelerationScheme(DeviceIntPtr dev,
> - ValuatorAccelerationPtr protoScheme) {
> + ValuatorAccelerationPtr protoScheme) {
> DeviceVelocityPtr vel;
> ValuatorAccelerationRec scheme;
> + PredictableAccelSchemePtr schemeData;
> scheme = *protoScheme;
> vel = calloc(1, sizeof(DeviceVelocityRec));
> - if (!vel)
> - return FALSE;
> + schemeData = calloc(1, sizeof(PredictableAccelSchemeRec));
> + if (!vel || !schemeData)
> + return FALSE;
> InitVelocityData(vel);
> - scheme.accelData = vel;
> + schemeData->vel = vel;
> + schemeData->prop_handlers = calloc(NPROPS_PREDICTABLE_ACCEL,
> + sizeof(long));
please no magic lengths arrays, schemeData should have length field for
prop_handlers. this goes for all APIs related to that, they should take a
length field for sanity checks.
> + if (!schemeData->prop_handlers)
> + return FALSE;
> + scheme.accelData = schemeData;
> dev->valuator->accelScheme = scheme;
> - InitializePredictableAccelerationProperties(dev);
> + InitializePredictableAccelerationProperties(dev, schemeData->prop_handlers);
> return TRUE;
> }
>
> @@ -146,14 +157,24 @@ InitPredictableAccelerationScheme(DeviceIntPtr dev,
> void
> AccelerationDefaultCleanup(DeviceIntPtr dev)
> {
> - /*sanity check*/
> - if( dev->valuator->accelScheme.AccelSchemeProc == acceleratePointerPredictable
> - && dev->valuator->accelScheme.accelData != NULL){
> + DeviceVelocityPtr vel = GetDevicePredictableAccelData(dev);
> + long* prop_handlers;
> + if (vel) {
> + /* the proper guarantee would be that we're not inside of
> + * AccelSchemeProc(), but that seems impossible. Schemes don't get
> + * schwitched often anyway.
> + */
> + OsBlockSignals();
> dev->valuator->accelScheme.AccelSchemeProc = NULL;
> - FreeVelocityData(dev->valuator->accelScheme.accelData);
> + FreeVelocityData(vel);
> + free(vel);
> + prop_handlers = ((PredictableAccelSchemePtr)
> + dev->valuator->accelScheme.accelData)->prop_handlers;
> + DeletePredictableAccelerationProperties(dev, prop_handlers);
> + free(prop_handlers);
> free(dev->valuator->accelScheme.accelData);
> dev->valuator->accelScheme.accelData = NULL;
> - DeletePredictableAccelerationProperties(dev);
> + OsReleaseSignals();
> }
> }
>
> @@ -345,26 +366,30 @@ AccelInitScaleProperty(DeviceIntPtr dev, DeviceVelocityPtr vel)
> return XIRegisterPropertyHandler(dev, AccelSetScaleProperty, NULL, NULL);
> }
>
> -BOOL
> -InitializePredictableAccelerationProperties(DeviceIntPtr dev)
> +static BOOL
> +InitializePredictableAccelerationProperties(
> + DeviceIntPtr dev,
> + long* prop_handlers)
> {
> DeviceVelocityPtr vel = GetDevicePredictableAccelData(dev);
>
> if(!vel)
> return FALSE;
>
> - vel->prop_handlers[0] = AccelInitProfileProperty(dev, vel);
> - vel->prop_handlers[1] = AccelInitDecelProperty(dev, vel);
> - vel->prop_handlers[2] = AccelInitAdaptDecelProperty(dev, vel);
> - vel->prop_handlers[3] = AccelInitScaleProperty(dev, vel);
> + prop_handlers[0] = AccelInitProfileProperty(dev, vel);
> + prop_handlers[1] = AccelInitDecelProperty(dev, vel);
> + prop_handlers[2] = AccelInitAdaptDecelProperty(dev, vel);
> + prop_handlers[3] = AccelInitScaleProperty(dev, vel);
>
> return TRUE;
> }
>
> BOOL
> -DeletePredictableAccelerationProperties(DeviceIntPtr dev)
> +DeletePredictableAccelerationProperties(
> + DeviceIntPtr dev,
> + long* prop_handlers)
> {
> - DeviceVelocityPtr vel;
> + DeviceVelocityPtr vel;
> Atom prop;
> int i;
>
> @@ -379,8 +404,8 @@ DeletePredictableAccelerationProperties(DeviceIntPtr dev)
>
> vel = GetDevicePredictableAccelData(dev);
> for (i = 0; vel && i < NPROPS_PREDICTABLE_ACCEL; i++)
> - if (vel->prop_handlers[i])
> - XIUnregisterPropertyHandler(dev, vel->prop_handlers[i]);
> + if (prop_handlers[i])
> + XIUnregisterPropertyHandler(dev, prop_handlers[i]);
reset the prop handlers to 0? not needed afaict, but good form nonetheless.
> return TRUE;
> }
> @@ -397,8 +422,7 @@ InitTrackers(DeviceVelocityPtr vel, int ntracker)
> return;
> }
> free(vel->tracker);
> - vel->tracker = (MotionTrackerPtr)malloc(ntracker * sizeof(MotionTracker));
> - memset(vel->tracker, 0, ntracker * sizeof(MotionTracker));
> + vel->tracker = (MotionTrackerPtr)calloc(ntracker, sizeof(MotionTracker));
> vel->num_tracker = ntracker;
> }
>
> @@ -1026,7 +1050,8 @@ GetDevicePredictableAccelData(
> acceleratePointerPredictable &&
> dev->valuator->accelScheme.accelData != NULL){
>
> - return (DeviceVelocityPtr)dev->valuator->accelScheme.accelData;
> + return ((PredictableAccelSchemePtr)
> + dev->valuator->accelScheme.accelData)->vel;
> }
> return NULL;
> }
> diff --git a/include/ptrveloc.h b/include/ptrveloc.h
> index 8c59c03..94cf8c9 100644
> --- a/include/ptrveloc.h
> +++ b/include/ptrveloc.h
> @@ -62,9 +62,6 @@ typedef struct _MotionTracker {
> int dir; /* initial direction bitfield */
> } MotionTracker, *MotionTrackerPtr;
>
> -/* number of properties for predictable acceleration */
> -#define NPROPS_PREDICTABLE_ACCEL 4
> -
> /**
> * Contains all data needed to implement mouse ballistics
> */
> @@ -91,9 +88,17 @@ typedef struct _DeviceVelocityRec {
> struct { /* to be able to query this information */
> int profile_number;
> } statistics;
> - long prop_handlers[NPROPS_PREDICTABLE_ACCEL];
> } DeviceVelocityRec, *DeviceVelocityPtr;
>
> +/**
> + * contains the run-time data for the predictable scheme, that is, a
> + * DeviceVelocityPtr and the property handlers.
> + */
> +typedef struct _PredictableAccelSchemeRec {
> + DeviceVelocityPtr vel;
> + long* prop_handlers;
> +} PredictableAccelSchemeRec, *PredictableAccelSchemePtr;
> +
> extern _X_EXPORT void
> InitVelocityData(DeviceVelocityPtr vel);
>
> --
> 1.7.3.4
>
> From 53ccbcb1052754ea85b667d4ef7197dccffca621 Mon Sep 17 00:00:00 2001
> From: Simon Thum <simon.thum at gmx.de>
> Date: Thu, 3 Feb 2011 21:52:24 +0100
> Subject: [PATCH 4/4] dix: update pointer acceleration code to use ValuatorMask
>
> Signed-off-by: Simon Thum <simon.thum at gmx.de>
> ---
> dix/getevents.c | 24 ++---------
> dix/ptrveloc.c | 111 ++++++++++++++++++++++++----------------------------
> include/input.h | 10 ++---
> include/ptrveloc.h | 12 +++---
> 4 files changed, 65 insertions(+), 92 deletions(-)
>
> diff --git a/dix/getevents.c b/dix/getevents.c
> index 794df42..1ccddfa 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -791,17 +791,14 @@ moveRelative(DeviceIntPtr dev, int *x, int *y, ValuatorMask *mask)
> * Accelerate the data in valuators based on the device's acceleration scheme.
> *
> * @param dev The device which's pointer is to be moved.
> - * @param first The first valuator in @valuators
> - * @param num Total number of valuators in @valuators.
> - * @param valuators Valuator data for each axis between @first and
> - * @first+ at num.
> + * @param valuators Valuator mask
> * @param ms Current time.
> */
> static void
> -accelPointer(DeviceIntPtr dev, int first, int num, int *valuators, CARD32 ms)
> +accelPointer(DeviceIntPtr dev, ValuatorMask* valuators, CARD32 ms)
> {
> if (dev->valuator->accelScheme.AccelSchemeProc)
> - dev->valuator->accelScheme.AccelSchemeProc(dev, first, num, valuators, ms);
> + dev->valuator->accelScheme.AccelSchemeProc(dev, valuators, ms);
> }
>
> /**
> @@ -1169,20 +1166,7 @@ GetPointerEvents(EventList *events, DeviceIntPtr pDev, int type, int buttons,
> moveAbsolute(pDev, &x, &y, &mask);
> } else {
> if (flags & POINTER_ACCELERATE) {
> - /* FIXME: Pointer acceleration only requires X and Y values. This
> - * should be converted to masked valuators. */
> - int vals[2];
> - vals[0] = valuator_mask_isset(&mask, 0) ?
> - valuator_mask_get(&mask, 0) : 0;
> - vals[1] = valuator_mask_isset(&mask, 1) ?
> - valuator_mask_get(&mask, 1) : 0;
> - accelPointer(pDev, 0, 2, vals, ms);
> -
> - if (valuator_mask_isset(&mask, 0))
> - valuator_mask_set(&mask, 0, vals[0]);
> - if (valuator_mask_isset(&mask, 1))
> - valuator_mask_set(&mask, 1, vals[1]);
> -
> + accelPointer(pDev, &mask, ms);
> /* The pointer acceleration code modifies the fractional part
> * in-place, so we need to extract this information first */
> x_frac = pDev->last.remainder[0];
> diff --git a/dix/ptrveloc.c b/dix/ptrveloc.c
> index 56ea809..0313061 100644
> --- a/dix/ptrveloc.c
> +++ b/dix/ptrveloc.c
> @@ -1068,18 +1068,15 @@ GetDevicePredictableAccelData(
> void
> acceleratePointerPredictable(
> DeviceIntPtr dev,
> - int first_valuator,
> - int num_valuators,
> - int *valuators,
> - int evtime)
> + ValuatorMask* val,
> + CARD32 evtime)
whoah. how did the int → CARD32 change sneak into this patch. Should be a
separate patch, IMO. though tbh I don't see the point, if we have different
sizes for ints here then we are really in trouble.
> {
> float fdx, fdy, tmp, mult; /* no need to init */
> - int dx = 0, dy = 0;
> - int *px = NULL, *py = NULL;
> + int dx = 0, dy = 0, tmpi;
> DeviceVelocityPtr velocitydata = GetDevicePredictableAccelData(dev);
> Bool soften = TRUE;
>
> - if (!num_valuators || !valuators || !velocitydata)
> + if (!velocitydata)
> return;
>
> if (velocitydata->statistics.profile_number == AccelProfileNone &&
> @@ -1087,13 +1084,12 @@ acceleratePointerPredictable(
> return; /*we're inactive anyway, so skip the whole thing.*/
> }
>
> - if (first_valuator == 0) {
> - dx = valuators[0];
> - px = &valuators[0];
> + if (valuator_mask_isset(val, 0)) {
> + dx = valuator_mask_get(val, 0);
> }
> - if (first_valuator <= 1 && num_valuators >= (2 - first_valuator)) {
> - dy = valuators[1 - first_valuator];
> - py = &valuators[1 - first_valuator];
> +
> + if (valuator_mask_isset(val, 1)) {
> + dy = valuator_mask_get(val, 1);
> }
>
> if (dx || dy){
> @@ -1107,7 +1103,7 @@ acceleratePointerPredictable(
> mult = ComputeAcceleration (dev, velocitydata,
> dev->ptrfeed->ctrl.threshold,
> (float)dev->ptrfeed->ctrl.num /
> - (float)dev->ptrfeed->ctrl.den);
> + (float)dev->ptrfeed->ctrl.den);
>
> if(mult != 1.0f || velocitydata->const_acceleration != 1.0f) {
> ApplySofteningAndConstantDeceleration( velocitydata,
> @@ -1122,13 +1118,15 @@ acceleratePointerPredictable(
> * process each axis conditionally, there's no danger
> * of a toggling remainder. Its lack of guarantees likely
> * makes it faster on the average target. */
> - *px = lrintf(tmp);
> - dev->last.remainder[0] = tmp - (float)*px;
> + tmpi = lrintf(tmp);
> + valuator_mask_set(val, 0, tmpi);
> + dev->last.remainder[0] = tmp - (float)tmpi;
> }
> if (dy) {
> tmp = mult * fdy + dev->last.remainder[1];
> - *py = lrintf(tmp);
> - dev->last.remainder[1] = tmp - (float)*py;
> + tmpi = lrintf(tmp);
> + valuator_mask_set(val, 1, tmpi);
> + dev->last.remainder[1] = tmp - (float)tmpi;
> }
> DebugAccelF("pos (%i | %i) remainders x: %.3f y: %.3f delta x:%.3f y:%.3f\n",
> *px, *py, dev->last.remainder[0], dev->last.remainder[1], fdx, fdy);
> @@ -1149,25 +1147,18 @@ acceleratePointerPredictable(
> void
> acceleratePointerLightweight(
> DeviceIntPtr dev,
> - int first_valuator,
> - int num_valuators,
> - int *valuators,
> - int ignored)
> + ValuatorMask* val,
> + CARD32 ignored)
> {
> - float mult = 0.0;
> - int dx = 0, dy = 0;
> - int *px = NULL, *py = NULL;
> -
> - if (!num_valuators || !valuators)
> - return;
> + float mult = 0.0, tmpf;
> + int dx = 0, dy = 0, tmpi;
>
> - if (first_valuator == 0) {
> - dx = valuators[0];
> - px = &valuators[0];
> + if (valuator_mask_isset(val, 0)) {
> + dx = valuator_mask_get(val, 0);
> }
> - if (first_valuator <= 1 && num_valuators >= (2 - first_valuator)) {
> - dy = valuators[1 - first_valuator];
> - py = &valuators[1 - first_valuator];
> +
> + if (valuator_mask_isset(val, 1)) {
> + dy = valuator_mask_get(val, 1);
> }
>
> if (!dx && !dy)
> @@ -1177,24 +1168,24 @@ acceleratePointerLightweight(
> /* modeled from xf86Events.c */
> if (dev->ptrfeed->ctrl.threshold) {
> if ((abs(dx) + abs(dy)) >= dev->ptrfeed->ctrl.threshold) {
> - dev->last.remainder[0] = ((float)dx *
> - (float)(dev->ptrfeed->ctrl.num)) /
> - (float)(dev->ptrfeed->ctrl.den) +
> - dev->last.remainder[0];
> - if (px) {
> - *px = (int)dev->last.remainder[0];
> - dev->last.remainder[0] = dev->last.remainder[0] -
> - (float)(*px);
> + tmpf = ((float)dx *
> + (float)(dev->ptrfeed->ctrl.num)) /
> + (float)(dev->ptrfeed->ctrl.den) +
> + dev->last.remainder[0];
> + if (dx) {
> + tmpi = (int) tmpf;
> + valuator_mask_set(val, 0, tmpi);
> + dev->last.remainder[0] = tmpf - (float)tmpi;
> }
>
> - dev->last.remainder[1] = ((float)dy *
> - (float)(dev->ptrfeed->ctrl.num)) /
> - (float)(dev->ptrfeed->ctrl.den) +
> - dev->last.remainder[1];
> - if (py) {
> - *py = (int)dev->last.remainder[1];
> - dev->last.remainder[1] = dev->last.remainder[1] -
> - (float)(*py);
> + tmpf = ((float)dy *
> + (float)(dev->ptrfeed->ctrl.num)) /
> + (float)(dev->ptrfeed->ctrl.den) +
> + dev->last.remainder[1];
> + if (dy) {
> + tmpi = (int) tmpf;
> + valuator_mask_set(val, 1, tmpi);
> + dev->last.remainder[1] = tmpf - (float)tmpi;
tabs → spaces please
if using vim,
set listchars=tab:>-,trail:~
set list
helps tremendously with this
> }
> }
> }
> @@ -1204,18 +1195,18 @@ acceleratePointerLightweight(
> (float)(dev->ptrfeed->ctrl.den) - 1.0) /
> 2.0) / 2.0;
> if (dx) {
> - dev->last.remainder[0] = mult * (float)dx +
> - dev->last.remainder[0];
> - *px = (int)dev->last.remainder[0];
> - dev->last.remainder[0] = dev->last.remainder[0] -
> - (float)(*px);
> + tmpf = mult * (float)dx +
> + dev->last.remainder[0];
> + tmpi = (int) tmpf;
> + valuator_mask_set(val, 0, tmpi);
> + dev->last.remainder[0] = tmpf - (float)tmpi;
> }
> if (dy) {
> - dev->last.remainder[1] = mult * (float)dy +
> - dev->last.remainder[1];
> - *py = (int)dev->last.remainder[1];
> - dev->last.remainder[1] = dev->last.remainder[1] -
> - (float)(*py);
> + tmpf = mult * (float)dy +
> + dev->last.remainder[1];
> + tmpi = (int)tmpf;
> + valuator_mask_set(val, 1, tmpi);
> + dev->last.remainder[1] = tmpf - (float)tmpi;
> }
> }
> }
> diff --git a/include/input.h b/include/input.h
> index ab58a15..3b2652c 100644
> --- a/include/input.h
> +++ b/include/input.h
> @@ -106,6 +106,8 @@ typedef struct _ClassesRec *ClassesPtr;
> typedef struct _SpriteRec *SpritePtr;
> typedef union _GrabMask GrabMask;
>
> +typedef struct _ValuatorMask ValuatorMask;
> +
> typedef struct _EventList {
> xEvent* event;
> int evlen; /* length of allocated memory for event in bytes. This is not
> @@ -142,10 +144,8 @@ typedef void (*DeviceUnwrapProc)(
> /* pointer acceleration handling */
> typedef void (*PointerAccelSchemeProc)(
> DeviceIntPtr /*pDev*/,
> - int /*first_valuator*/,
> - int /*num_valuators*/,
> - int* /*valuators*/,
> - int /*evtime*/);
> + ValuatorMask* /*first_valuator*/,
first_valuator?
Cheers,
Peter
> + CARD32 /*evtime*/);
>
> typedef void (*DeviceCallbackProc)(
> DeviceIntPtr /*pDev*/);
> @@ -163,8 +163,6 @@ typedef struct _DeviceRec {
> Bool on; /* used by DDX to keep state */
> } DeviceRec, *DevicePtr;
>
> -typedef struct _ValuatorMask ValuatorMask;
> -
> typedef struct {
> int click, bell, bell_pitch, bell_duration;
> Bool autoRepeat;
> diff --git a/include/ptrveloc.h b/include/ptrveloc.h
> index 94cf8c9..6411a2c 100644
> --- a/include/ptrveloc.h
> +++ b/include/ptrveloc.h
> @@ -1,6 +1,6 @@
> /*
> *
> - * Copyright ?? 2006-2009 Simon Thum simon dot thum at gmx dot de
> + * Copyright ?? 2006-2011 Simon Thum simon dot thum at gmx dot de
> *
> * Permission is hereby granted, free of charge, to any person obtaining a
> * copy of this software and associated documentation files (the "Software"),
> @@ -25,7 +25,7 @@
> #ifndef POINTERVELOCITY_H
> #define POINTERVELOCITY_H
>
> -#include <input.h> /* DeviceIntPtr */
> +#include <input.h>
>
> /* constants for acceleration profiles */
>
> @@ -133,11 +133,11 @@ InitPredictableAccelerationScheme(DeviceIntPtr dev,
> struct _ValuatorAccelerationRec* protoScheme);
>
> extern _X_INTERNAL void
> -acceleratePointerPredictable(DeviceIntPtr dev, int first_valuator,
> - int num_valuators, int *valuators, int evtime);
> +acceleratePointerPredictable(DeviceIntPtr dev, ValuatorMask* val,
> + CARD32 evtime);
>
> extern _X_INTERNAL void
> -acceleratePointerLightweight(DeviceIntPtr dev, int first_valuator,
> - int num_valuators, int *valuators, int ignored);
> +acceleratePointerLightweight(DeviceIntPtr dev, ValuatorMask* val,
> + CARD32 evtime);
>
> #endif /* POINTERVELOCITY_H */
> --
> 1.7.3.4
>
More information about the xorg-devel
mailing list