X server testing
Peter Hutterer
peter.hutterer at who-t.net
Thu Jun 25 17:35:55 PDT 2009
On Wed, Jun 24, 2009 at 09:00:37PM -0400, Thomas Jaeger wrote:
> Sorry, I forgot the attachment.
Merged, thanks.
>
> Thomas Jaeger wrote:
> > Peter Hutterer wrote:
> >> On Mon, Jun 22, 2009 at 07:13:12PM -0400, Thomas Jaeger wrote:
> >>
> >>> From 67076a1404cad4042e5cfc667d28f150e1663115 Mon Sep 17 00:00:00 2001
> >>> From: Thomas Jaeger <ThJaeger at gmail.com>
> >>> Date: Sat, 20 Jun 2009 20:17:41 -0400
> >>> Subject: [PATCH 4/4] dix: report subpixel coordinates for high-resolution devices
> >>>
> >>> ---
> >>> dix/getevents.c | 93 +++++++++++++++++++++++++++++++++++++-----------------
> >>> 1 files changed, 64 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/dix/getevents.c b/dix/getevents.c
> >>> index 25dd187..a64a0c7 100644
> >>> --- a/dix/getevents.c
> >>> +++ b/dix/getevents.c
> >>> @@ -240,10 +240,11 @@ CreateClassesChangedEvent(EventList* event,
> >>> * Rescale the coord between the two axis ranges.
> >>> */
> >>> static int
> >>> -rescaleValuatorAxis(int coord, AxisInfoPtr from, AxisInfoPtr to,
> >>> +rescaleValuatorAxis(int coord, float remainder, float *remainder_return, AxisInfoPtr from, AxisInfoPtr to,
> >> at this point I wonder whether we should just give in and work with floats.
> >> the reason why floats are separate are mainly because subpixels were
> >> tacked onto the current system, but keeping them separate everywhere seems
> >> to overly complicate things.
> >> would it make sense to just change the return value and coord to a float?
> >>
> >> this has a larger fallout, including switching dev->last.remainder over as
> >> well. The question is, is it worth the effort?
> >> AIUI floats are a problem on embedded platforms where they carry a
> >> significant performance hit. I'm not sure if the current implementation
> >> really avoids this issue. Most of the event processing deals with integers
> >> only, but during event generation where we do the pointer accel anyway I
> >> think splitting it up doesn't give us much benefit, does it?
> >
> > I've been playing with the idea of changing the types of various
> > parameter, but it never seems to work out very well: The X server uses
> > many different formats to represent valuator information internally, so
> > you end up converting back and forth no matter what format you choose.
> > The fact that every other function in dix/getevents.c modifiers the
> > valuator array and/or dev->last.valuator[i] in-place doesn't exactly
> > help either, this code could use a good refactoring. So I think dealing
> > with long parameter lists is the lesser evil, and I'm resubmitting the
> > patch with a few minor style cleanups and a fixed rescaleValuatorEvents
> > that actually takes the fractional value into account.
> >
> >>> @@ -745,26 +759,36 @@ accelPointer(DeviceIntPtr dev, int first, int num, int *valuators, CARD32 ms)
> >>> * @param dev The device to be moved.
> >>> * @param x Pointer to current x-axis value, may be modified.
> >>> * @param y Pointer to current y-axis value, may be modified.
> >>> + * @param x_frac
> >>> + * @param y_frac
> >> ^^ missing doc. I know it's just a few words, but..
> >>> * @param scr Screen the device's sprite is currently on.
> >>> * @param screenx Screen x coordinate the sprite is on after the update.
> >>> * @param screeny Screen y coordinate the sprite is on after the update.
> >>> + * @param screenx_frac
> >>> + * @param screeny_frac
> >> ^^ same thing here
> > This is what happens when you decide to come back later to fill in the
> > details and then never do, sorry.
> >
> >>> } else {
> >>> - if (flags & POINTER_ACCELERATE)
> >>> + if (flags & POINTER_ACCELERATE) {
> >>> accelPointer(pDev, first_valuator, num_valuators, valuators, ms);
> >>> + /* The pointer acceleration code modifies the fractional part
> >>> + * in-place, so we need to extract this information first */
> >> ^^^ stray tab
> > Sorry, I haven't managed to configure vim in such a way that it adopts
> > the indenting and space conventions of the code that it is looking at...
> >
> > Thanks,
> > Tom
>
> From fd863877e0667e2691fa7cadec65ac4cd5758502 Mon Sep 17 00:00:00 2001
> From: Thomas Jaeger <ThJaeger at gmail.com>
> Date: Sat, 20 Jun 2009 20:17:41 -0400
> Subject: [PATCH] dix: report subpixel coordinates for high-resolution devices
>
> ---
> dix/getevents.c | 92 +++++++++++++++++++++++++++++++++++++-----------------
> 1 files changed, 63 insertions(+), 29 deletions(-)
>
> diff --git a/dix/getevents.c b/dix/getevents.c
> index c510122..fcac056 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -240,10 +240,11 @@ CreateClassesChangedEvent(EventList* event,
> * Rescale the coord between the two axis ranges.
> */
> static int
> -rescaleValuatorAxis(int coord, AxisInfoPtr from, AxisInfoPtr to,
> +rescaleValuatorAxis(int coord, float remainder, float *remainder_return, AxisInfoPtr from, AxisInfoPtr to,
> int defmax)
> {
> - int fmin = 0, tmin = 0, fmax = defmax, tmax = defmax;
> + int fmin = 0, tmin = 0, fmax = defmax, tmax = defmax, coord_return;
> + float value;
>
> if(from && from->min_value < from->max_value) {
> fmin = from->min_value;
> @@ -254,14 +255,23 @@ rescaleValuatorAxis(int coord, AxisInfoPtr from, AxisInfoPtr to,
> tmax = to->max_value;
> }
>
> - if(fmin == tmin && fmax == tmax)
> + if(fmin == tmin && fmax == tmax) {
> + if (remainder_return)
> + *remainder_return = remainder;
> return coord;
> + }
>
> - if(fmax == fmin) /* avoid division by 0 */
> + if(fmax == fmin) { /* avoid division by 0 */
> + if (remainder_return)
> + *remainder_return = 0.0;
> return 0;
> + }
>
> - return lroundf(((float)(coord - fmin)) * (tmax - tmin) /
> - (fmax - fmin)) + tmin;
> + value = (coord + remainder - fmin) * (tmax - tmin) / (fmax - fmin) + tmin;
> + coord_return = lroundf(value);
> + if (remainder_return)
> + *remainder_return = value - coord_return;
> + return coord_return;
> }
>
> /**
> @@ -289,9 +299,11 @@ updateSlaveDeviceCoords(DeviceIntPtr master, DeviceIntPtr pDev)
>
> /* scale back to device coordinates */
> if(pDev->valuator->numAxes > 0)
> - pDev->last.valuators[0] = rescaleValuatorAxis(pDev->last.valuators[0], NULL, pDev->valuator->axes + 0, scr->width);
> + pDev->last.valuators[0] = rescaleValuatorAxis(pDev->last.valuators[0], pDev->last.remainder[0],
> + &pDev->last.remainder[0], NULL, pDev->valuator->axes + 0, scr->width);
> if(pDev->valuator->numAxes > 1)
> - pDev->last.valuators[1] = rescaleValuatorAxis(pDev->last.valuators[1], NULL, pDev->valuator->axes + 1, scr->height);
> + pDev->last.valuators[1] = rescaleValuatorAxis(pDev->last.valuators[1], pDev->last.remainder[1],
> + &pDev->last.remainder[0], NULL, pDev->valuator->axes + 1, scr->height);
>
> /* calculate the other axis as well based on info from the old
> * slave-device. If the old slave had less axes than this one,
> @@ -304,6 +316,8 @@ updateSlaveDeviceCoords(DeviceIntPtr master, DeviceIntPtr pDev)
> else
> pDev->last.valuators[i] =
> rescaleValuatorAxis(pDev->last.valuators[i],
> + pDev->last.remainder[i],
> + &pDev->last.remainder[i],
> lastSlave->valuator->axes + i,
> pDev->valuator->axes + i, 0);
> }
> @@ -410,7 +424,7 @@ GetMotionHistory(DeviceIntPtr pDev, xTimecoord **buff, unsigned long start,
> /* scale to screen coords */
> to = &core_axis;
> to->max_value = pScreen->width;
> - coord = rescaleValuatorAxis(coord, &from, to, pScreen->width);
> + coord = rescaleValuatorAxis(coord, 0.0, NULL, &from, to, pScreen->width);
>
> memcpy(corebuf, &coord, sizeof(INT16));
> corebuf++;
> @@ -421,7 +435,7 @@ GetMotionHistory(DeviceIntPtr pDev, xTimecoord **buff, unsigned long start,
> memcpy(&coord, icbuf++, sizeof(INT32));
>
> to->max_value = pScreen->height;
> - coord = rescaleValuatorAxis(coord, &from, to, pScreen->height);
> + coord = rescaleValuatorAxis(coord, 0.0, NULL, &from, to, pScreen->height);
> memcpy(corebuf, &coord, sizeof(INT16));
>
> } else if (IsMaster(pDev))
> @@ -456,7 +470,7 @@ GetMotionHistory(DeviceIntPtr pDev, xTimecoord **buff, unsigned long start,
> dflt = 0;
>
> /* scale from stored range into current range */
> - coord = rescaleValuatorAxis(coord, &from, to, 0);
> + coord = rescaleValuatorAxis(coord, 0.0, NULL, &from, to, 0);
> memcpy(ocbuf, &coord, sizeof(INT32));
> ocbuf++;
> }
> @@ -745,26 +759,36 @@ accelPointer(DeviceIntPtr dev, int first, int num, int *valuators, CARD32 ms)
> * @param dev The device to be moved.
> * @param x Pointer to current x-axis value, may be modified.
> * @param y Pointer to current y-axis value, may be modified.
> + * @param x_frac Fractional part of current x-axis value, may be modified.
> + * @param y_frac Fractional part of current y-axis value, may be modified.
> * @param scr Screen the device's sprite is currently on.
> * @param screenx Screen x coordinate the sprite is on after the update.
> * @param screeny Screen y coordinate the sprite is on after the update.
> + * @param screenx_frac Fractional part of screen x coordinate, as above.
> + * @param screeny_frac Fractional part of screen y coordinate, as above.
> */
> static void
> -positionSprite(DeviceIntPtr dev, int *x, int *y,
> - ScreenPtr scr, int *screenx, int *screeny)
> +positionSprite(DeviceIntPtr dev, int *x, int *y, float x_frac, float y_frac,
> + ScreenPtr scr, int *screenx, int *screeny, float *screenx_frac, float *screeny_frac)
> {
> int old_screenx, old_screeny;
>
> /* scale x&y to screen */
> - if (dev->valuator->numAxes > 0)
> - *screenx = rescaleValuatorAxis(*x, dev->valuator->axes + 0, NULL, scr->width);
> - else
> + if (dev->valuator->numAxes > 0) {
> + *screenx = rescaleValuatorAxis(*x, x_frac, screenx_frac,
> + dev->valuator->axes + 0, NULL, scr->width);
> + } else {
> *screenx = dev->last.valuators[0];
> + *screenx_frac = dev->last.remainder[0];
> + }
>
> - if (dev->valuator->numAxes > 1 )
> - *screeny = rescaleValuatorAxis(*y, dev->valuator->axes + 1, NULL, scr->height);
> - else
> + if (dev->valuator->numAxes > 1) {
> + *screeny = rescaleValuatorAxis(*y, y_frac, screeny_frac,
> + dev->valuator->axes + 1, NULL, scr->height);
> + } else {
> *screeny = dev->last.valuators[1];
> + *screeny_frac = dev->last.remainder[1];
> + }
>
> old_screenx = *screenx;
> old_screeny = *screeny;
> @@ -773,27 +797,31 @@ positionSprite(DeviceIntPtr dev, int *x, int *y,
> miPointerSetPosition(dev, screenx, screeny);
>
> if (dev->u.master) {
> - dev->u.master->last.valuators[0] = dev->last.valuators[0];
> - dev->u.master->last.valuators[1] = dev->last.valuators[1];
> + dev->u.master->last.valuators[0] = *screenx;
> + dev->u.master->last.valuators[1] = *screeny;
> + dev->u.master->last.remainder[0] = *screenx_frac;
> + dev->u.master->last.remainder[1] = *screeny_frac;
> }
>
> /* Crossed screen? Scale back to device coordiantes */
> if(*screenx != old_screenx)
> {
> scr = miPointerGetScreen(dev);
> - *x = rescaleValuatorAxis(*screenx, NULL,
> + *x = rescaleValuatorAxis(*screenx, *screenx_frac, &x_frac, NULL,
> dev->valuator->axes + 0, scr->width);
> }
> if(*screeny != old_screeny)
> {
> scr = miPointerGetScreen(dev);
> - *y = rescaleValuatorAxis(*screeny, NULL,
> + *y = rescaleValuatorAxis(*screeny, *screeny_frac, &y_frac, NULL,
> dev->valuator->axes + 1, scr->height);
> }
>
> /* dropy x/y (device coordinates) back into valuators for next event */
> dev->last.valuators[0] = *x;
> dev->last.valuators[1] = *y;
> + dev->last.remainder[0] = x_frac;
> + dev->last.remainder[1] = y_frac;
> }
>
> /**
> @@ -1018,6 +1046,7 @@ GetPointerEvents(EventList *events, DeviceIntPtr pDev, int type, int buttons,
> RawDeviceEvent *raw;
> int x = 0, y = 0, /* device coords */
> cx, cy; /* only screen coordinates */
> + float x_frac = 0.0, y_frac = 0.0, cx_frac, cy_frac;
> ScreenPtr scr = miPointerGetScreen(pDev);
>
> /* refuse events from disabled devices */
> @@ -1050,26 +1079,31 @@ GetPointerEvents(EventList *events, DeviceIntPtr pDev, int type, int buttons,
> {
>
> if (num_valuators >= 1 && first_valuator == 0)
> - valuators[0] = rescaleValuatorAxis(valuators[0], NULL,
> + valuators[0] = rescaleValuatorAxis(valuators[0], 0.0, &x_frac, NULL,
> pDev->valuator->axes + 0,
> scr->width);
> if (first_valuator <= 1 && num_valuators >= (2 - first_valuator))
> - valuators[1 - first_valuator] = rescaleValuatorAxis(valuators[1 - first_valuator], NULL,
> + valuators[1 - first_valuator] = rescaleValuatorAxis(valuators[1 - first_valuator], 0.0, &y_frac, NULL,
> pDev->valuator->axes + 1,
> scr->height);
> }
>
> moveAbsolute(pDev, &x, &y, first_valuator, num_valuators, valuators);
> } else {
> - if (flags & POINTER_ACCELERATE)
> + if (flags & POINTER_ACCELERATE) {
> accelPointer(pDev, first_valuator, num_valuators, valuators, 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];
> + y_frac = pDev->last.remainder[1];
> + }
> moveRelative(pDev, &x, &y, first_valuator, num_valuators, valuators);
> }
>
> set_raw_valuators(raw, first_valuator, num_valuators, valuators,
> raw->valuators.data);
>
> - positionSprite(pDev, &x, &y, scr, &cx, &cy);
> + positionSprite(pDev, &x, &y, x_frac, y_frac, scr, &cx, &cy, &cx_frac, &cy_frac);
> updateHistory(pDev, first_valuator, num_valuators, ms);
>
> /* Update the valuators with the true value sent to the client*/
> @@ -1102,8 +1136,8 @@ GetPointerEvents(EventList *events, DeviceIntPtr pDev, int type, int buttons,
>
> event->root_x = cx; /* root_x/y always in screen coords */
> event->root_y = cy;
> - event->root_x_frac = pDev->last.remainder[0];
> - event->root_y_frac = pDev->last.remainder[1];
> + event->root_x_frac = cx_frac;
> + event->root_y_frac = cy_frac;
>
> set_valuators(pDev, event, first_valuator, num_valuators, valuators);
>
> --
> 1.6.3.1
>
Cheers,
Peter
More information about the xorg-devel
mailing list