On Sun, Jul 07, 2013 at 10:22:23PM +0300, Henri Kemppainen wrote:
> So I needed to see my thoughts on paper but my desk was so full of stuff
> I couldn't make room for pen and paper. Instead I fired up Gimp, and
> drawing with the mouse worked fine until I realized it's next to impossible
> to draw diagonal lines that look like lines.
>
> Instead of straight lines, I got waves. The faster I draw the mouse, the
> deeper the waves. It looked like diagonal mouse motion generated a pair
> of pointer motion events, one for the X axis and another for Y. And Gimp
> smoothed that stairstep motion, resulting in waves. Xev(1) confirmed my
> hypothesis.
>
> It turns out wsmouse(4) is breaking the mouse input into multiple events.
> This isn't necessarily a bug, since it allows for a very small and simple
> event structure which works without modification as new properties (such
> as button states, axes, etc.) are added.
>
> Now wsmouse generates all the events it can in a loop before waking up
> the process that waits for these events. So on the receiving side (i.e.
> in the xenocara ws(4) driver) we can sum all the consecutive X and Y
> deltas from a single read() before issuing a pointer motion event. This
> eliminates the stairsteps as long as the events generated by wsmouse fit
> in the buffers involved.
>
> Other approaches would be either extending the event structure, or perhaps
> adding a new event type that somehow communicates the intended grouping
> for events that follow/precede (but ugh...). I felt the ws(4) fix was
> the least intrusive, and it appears to work just fine here. What do you
> think?
>
> This image (drawn in Gimp) illustrates the problem. The last two lines
> were drawn with my diff applied.
>
> http://guu.fi/mousebug.png
Thanks for the report and patch.
The issue that input drivers devices need high refresh frequency to be
able to achieve high-precision freehand drawing is quite well known¹.
I re-did a few experiments with and without your patch on my laptop
(with the Lenovo track point wihch is a relative device). I could not
reproduce the waves you're getting in Gimp, but without your patch
diagonals show indeed larger staircases effects. Under xfig(1) or
bitmap(1) the difference is much less noticeable. Clearly the
strategy used by applications to trac mouse motion for freehand
drawing are not the same.
I do fear that with some devices your patch will collapse too many
events and make it harder to follow small radius curves.
OTOH, I've always considered that sending many small steps events to
the application, and letting it merge them would generally be better,
even if they come with a too high latency. Having associated
kernel timestamps would help estimating the speed and get better
dynamic interpolation, but the X input driver model doesn't really
make that possible.
So I'd like to see a few more reports of whether that patch helps or
not.
-----
¹) For years X has tried to solve this issue by using asynchrous IO (ie a
signal handler for SIGIO) to handle input events as soon as
possible. Unfortunatly even though it sort-of worked, it let way too
much code run in the signal handler itself, and without major
architectural rework, it just can't work 100% reliably, so we ended up
disabling that code by default.
An approach using a separate thread for input was also tried, but the
overhead and complexity of proper locking between display drivers and
input drivers killed that idea too.
>
> Index: xenocara/driver/xf86-input-ws/src/ws.c
> ===================================================================
> RCS file: /cvs/xenocara/driver/xf86-input-ws/src/ws.c,v
> retrieving revision 1.57
> diff -u -p -r1.57 ws.c
> --- xenocara/driver/xf86-input-ws/src/ws.c 8 Jul 2012 14:22:03 -0000
> 1.57
> +++ xenocara/driver/xf86-input-ws/src/ws.c 7 Jul 2013 18:33:57 -0000
> @@ -474,7 +474,7 @@ wsReadInput(InputInfoPtr pInfo)
> {
> WSDevicePtr priv = (WSDevicePtr)pInfo->private;
> static struct wscons_event eventList[NUMEVENTS];
> - int n, c;
> + int n, c, dx, dy;
> struct wscons_event *event = eventList;
> unsigned char *pBuf;
>
> @@ -488,10 +488,11 @@ wsReadInput(InputInfoPtr pInfo)
> if (n == 0)
> return;
>
> + dx = dy = 0;
> n /= sizeof(struct wscons_event);
> while (n--) {
> int buttons = priv->lastButtons;
> - int dx = 0, dy = 0, dz = 0, dw = 0, ax = 0, ay = 0;
> + int dz = 0, dw = 0, ax = 0, ay = 0;
> int zbutton = 0, wbutton = 0;
>
> switch (event->type) {
> @@ -506,11 +507,11 @@ wsReadInput(InputInfoPtr pInfo)
> buttons));
> break;
> case WSCONS_EVENT_MOUSE_DELTA_X:
> - dx = event->value;
> + dx += event->value;
> DBG(4, ErrorF("Relative X %d\n", event->value));
> break;
> case WSCONS_EVENT_MOUSE_DELTA_Y:
> - dy = -event->value;
> + dy -= event->value;
> DBG(4, ErrorF("Relative Y %d\n", event->value));
> break;
> case WSCONS_EVENT_MOUSE_ABSOLUTE_X:
> @@ -548,14 +549,18 @@ wsReadInput(InputInfoPtr pInfo)
> }
> ++event;
>
> - if (dx || dy) {
> - if (wsWheelEmuFilterMotion(pInfo, dx, dy))
> + if ((dx || dy) && event->type != WSCONS_EVENT_MOUSE_DELTA_X &&
> + event->type != WSCONS_EVENT_MOUSE_DELTA_Y) {
> + int tmpx = dx, tmpy = dy;
> + dx = dy = 0;
> +
> + if (wsWheelEmuFilterMotion(pInfo, tmpx, tmpy))
> continue;
>
> /* relative motion event */
> DBG(3, ErrorF("postMotionEvent dX %d dY %d\n",
> - dx, dy));
> - xf86PostMotionEvent(pInfo->dev, 0, 0, 2, dx, dy);
> + tmpx, tmpy));
> + xf86PostMotionEvent(pInfo->dev, 0, 0, 2, tmpx, tmpy);
> }
> if (dz && priv->Z.negative != WS_NOMAP
> && priv->Z.positive != WS_NOMAP) {
> @@ -583,9 +588,9 @@ wsReadInput(InputInfoPtr pInfo)
> ay = tmp;
> }
> if (ax) {
> - dx = ax - priv->old_ax;
> + int xdelta = ax - priv->old_ax;
> priv->old_ax = ax;
> - if (wsWheelEmuFilterMotion(pInfo, dx, 0))
> + if (wsWheelEmuFilterMotion(pInfo, xdelta, 0))
> continue;
>
> /* absolute position event */
> @@ -593,15 +598,24 @@ wsReadInput(InputInfoPtr pInfo)
> xf86PostMotionEvent(pInfo->dev, 1, 0, 1, ax);
> }
> if (ay) {
> - dy = ay - priv->old_ay;
> + int ydelta = ay - priv->old_ay;
> priv->old_ay = ay;
> - if (wsWheelEmuFilterMotion(pInfo, 0, dy))
> + if (wsWheelEmuFilterMotion(pInfo, 0, ydelta))
> continue;
>
> /* absolute position event */
> DBG(3, ErrorF("postMotionEvent y %d\n", ay));
> xf86PostMotionEvent(pInfo->dev, 1, 1, 1, ay);
> }
> + }
> + if (dx || dy) {
> + if (wsWheelEmuFilterMotion(pInfo, dx, dy))
> + return;
> +
> + /* relative motion event */
> + DBG(3, ErrorF("postMotionEvent dX %d dY %d\n",
> + dx, dy));
> + xf86PostMotionEvent(pInfo->dev, 0, 0, 2, dx, dy);
> }
> return;
> }
>
>
>
--
Matthieu Herrb
