Bug 10324 - x&y axis not scaled when reporting extension motion events
x&y axis not scaled when reporting extension motion events
Status: RESOLVED FIXED
Product: xorg
Classification: Unclassified
Component: Server/General
git
All All
: medium normal
Assigned To: Xorg Project Team
Xorg Project Team
:
Depends on:
Blocks: 13552 13731
  Show dependency treegraph
 
Reported: 2007-03-17 08:00 UTC by Magnus Vigerlof
Modified: 2008-02-05 12:27 UTC (History)
3 users (show)

See Also:


Attachments
Reintroduction of scaling in GetPointerEvents (6.42 KB, patch)
2007-03-17 08:03 UTC, Magnus Vigerlof
no flags Details | Splinter Review
Rescale patch without any interface changes (4.97 KB, patch)
2008-01-03 10:49 UTC, Magnus Vigerlof
no flags Details | Splinter Review
dix: Always add valuator information if present in GetPointerEvents (1.87 KB, patch)
2008-01-08 13:14 UTC, Magnus Vigerlof
no flags Details | Splinter Review
dix: Do not call clipAxis for relative pointer events (3.67 KB, patch)
2008-01-08 13:16 UTC, Magnus Vigerlof
no flags Details | Splinter Review
dix: Allow arbitrary value ranges in GetPointerEvents (3.80 KB, patch)
2008-01-08 13:16 UTC, Magnus Vigerlof
no flags Details | Splinter Review
Use right screen dimension in calculation after screen crossing (1.13 KB, patch)
2008-01-09 10:47 UTC, Magnus Vigerlof
no flags Details | Splinter Review
Add scaling to absolute reporting with missing x&y (2.66 KB, patch)
2008-01-10 15:08 UTC, Magnus Vigerlof
no flags Details | Splinter Review
Move motion history update until after screen crossing and clipping (1.98 KB, patch)
2008-01-10 15:09 UTC, Magnus Vigerlof
no flags Details | Splinter Review
Store coords when crossing screen or clipping for VCP (1011 bytes, patch)
2008-01-10 15:10 UTC, Magnus Vigerlof
no flags Details | Splinter Review
dix: Call clipAxis for relative events in GetPointerEvents (1.13 KB, patch)
2008-01-12 14:14 UTC, Magnus Vigerlof
no flags Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Magnus Vigerlof 2007-03-17 08:00:23 UTC
It appears that the change in the input code where hotplugging was a part removed the call to the two conversion functions for the x&y axis defined in the device driver (reverse_conversion_proc and conversion_proc) and thus forcing the driver to scale the reported axis prior and not during event generation.

This is a semantic change in the behavior of the XInput devices, has at least two drawbacks:
* It is now impossible to report subpixel motion as an extension event as the the x&y axis must be reported after scaling.
* The InputDevice must match its x&y axis to the screen resolution and therefore also must change these when the screen resolution changes (and notify all applications about it).

One idea has been raised to generate the events, and afterwards scale the x&y axis in the genereated events. But as GetPointerEvents also moves the pointer on the screen the x&y must be in screen resolution and if the extension events should not loose precision when restoring the original x&y a backing store is needed inside the device driver.

Another idea was to introduce a pair of new conversion functions that will replace the old ones. The included patch adds these two conversion functions to the DeviceIntRec structure so the conversion functions can be called during event generation, as before but now in the dix-code.

See also the discussion on the xorg-mailinglist:
http://lists.freedesktop.org/archives/xorg/2007-March/022288.html
http://lists.freedesktop.org/archives/xorg/2007-March/022534.html
Comment 1 Magnus Vigerlof 2007-03-17 08:03:04 UTC
Created attachment 9204 [details] [review]
Reintroduction of scaling in GetPointerEvents
Comment 2 Simon Thum 2007-04-05 13:20:36 UTC
* It is now impossible to report subpixel motion as an extension event as the
the x&y axis must be reported after scaling.

Speaking of that, is there any code in existence that would emit such an event? I'm asking since my patch bug#8583 could take advantage of such a thing.
Comment 3 Magnus Vigerlof 2007-04-05 13:42:03 UTC
(In reply to comment #2)
> * It is now impossible to report subpixel motion as an extension event as the
> the x&y axis must be reported after scaling.
> 
> Speaking of that, is there any code in existence that would emit such an event?
> I'm asking since my patch bug#8583 could take advantage of such a thing.

Indeed there is. Wacom tablets have a higher resolution than the screen (from 3200*2300 (low-end) to 97000*61000 (high-end)), and the wacom driver report the full range of those axis today without scaling.
Comment 4 Daniel Stone 2007-04-10 14:09:19 UTC
hi,
the placement of reverse_conversion_proc doesn't seem quite right to me: it's only in the relative branch, and it always gets called with cp, instead of srcp.  the other concern is the call to miPointerSetPosition being moved: as you have it, the motion history and last{x,y} will always remain unclipped, whereas we rely on the call being where it is now to clip the co-ordinates before it gets its way into history.  other than that, the patch seems fine, including the cleanups.  thanks!
Comment 5 Magnus Vigerlof 2007-04-11 14:51:49 UTC
(In reply to comment #4)
Calling reverse_conversion_proc makes only any sense in relative mode. It's in that case where you need to find out where the pointer actually are in the input device coordinate system before moving it a little bit. In absolute mode we really don't care where it's been. We have a very specific point where it's supposed to be, so why bother with the old position?

The use of cp in the reverse_conversion_proc is intentional. We should only convert the core coordinates. But you may have a point.. If we shouldn't send any core-events, we should probably be using the current coordinates from the input device instead of converting them from the core. Maybe always calling miPointerSetPosition confused me here. Should we really be moving the pointer even if we're not sending any core-events?


Having updateMotionHistory where it is was based on that is should keep track of the motion history in the input device coordinate system, and in that case the x&y axis will be clipped by clipAxis a few lines up.

If that is a faulty understanding it's definitely in the wrong place...
Comment 6 Magnus Vigerlof 2008-01-03 10:49:51 UTC
Created attachment 13491 [details] [review]
Rescale patch without any interface changes
Comment 7 Peter Hutterer 2008-01-07 16:34:22 UTC
1. check is only for max > 0. I think it should be something like (min != -1 && max != -1)
2. we need a check that (max - min + 1) != 0, otherwise things can turn sour (division by 0). same thing in the VCP converstion.
3. lastx/y is only used here, nowhere else. I'd say it'd be best to store the unconverted coordinates (for the device anyway, VCP needs to store the converted obviously)


1  is a bit of a danger though, there may be devices that have -1 as min value. but then, there may be devices that only report negative coordinates? 
Comment 8 Magnus Vigerlof 2008-01-08 10:50:02 UTC
I've had some more thoughts about the whole GetPointerEvent function, and there are a few things that should be addressed before I'm satisfied with it.

* Arbitrary value ranges (scale if min < max, no limits on actual reported values)
* Clip axis only in absolute mode or if not sending core events
* Clip axis only if a value range is defined (min < max)
* Should miPointerSetPosition ever be called for events that does not send core events? Feels a bit off where it is to be honest.

(In reply to comment #7)
> 1. check is only for max > 0. I think it should be something like (min != -1 &&
> max != -1)
> 2. we need a check that (max - min + 1) != 0, otherwise things can turn sour
> (division by 0). same thing in the VCP converstion.

Arbitrary value ranges will take care of these two.

> 3. lastx/y is only used here, nowhere else. I'd say it'd be best to store the
> unconverted coordinates (for the device anyway, VCP needs to store the
> converted obviously)

You've lost me here; Is the current situation where pDev has the unconverted and cp the converted values the same as you describe above?

> 1  is a bit of a danger though, there may be devices that have -1
> as min value. but then, there may be devices that only report
> negative coordinates?

Done right there's no danger at all.. I don't know any devices that does, but we can do something that will work with that kind too easily.

I've started with changes that address these changes, I'll just clean them up, test them, and add them to the list here.
Comment 9 Magnus Vigerlof 2008-01-08 13:09:25 UTC
(In reply to comment #8)
[...]
> * Arbitrary value ranges (scale if min < max, no limits on actual reported
> values)

'no limits on actual defined min and max' was what I meant..
Comment 10 Magnus Vigerlof 2008-01-08 13:14:34 UTC
Created attachment 13591 [details] [review]
dix: Always add valuator information if present in GetPointerEvents

See discussion on xorg-ml, this is a prereq for the next two patches.
http://lists.freedesktop.org/pipermail/xorg/2007-December/031195.html
Comment 11 Magnus Vigerlof 2008-01-08 13:16:03 UTC
Created attachment 13592 [details] [review]
dix: Do not call clipAxis for relative pointer events
Comment 12 Magnus Vigerlof 2008-01-08 13:16:48 UTC
Created attachment 13593 [details] [review]
dix: Allow arbitrary value ranges in GetPointerEvents
Comment 13 Peter Hutterer 2008-01-08 15:17:49 UTC
(In reply to comment #10)
> Created an attachment (id=13591) [details]
> dix: Always add valuator information if present in GetPointerEvents

ACK
Comment 14 Peter Hutterer 2008-01-08 15:37:16 UTC
(In reply to comment #11)
> Created an attachment (id=13592) [details]
> dix: Do not call clipAxis for relative pointer events

when you're re-scaling pDev->lastx, shouldn't it be scr->width, not scr->height?
otherwise ACK.
Comment 15 Peter Hutterer 2008-01-08 15:37:48 UTC
(In reply to comment #12)
> Created an attachment (id=13593) [details]
> dix: Allow arbitrary value ranges in GetPointerEvents
> 

ACK
Comment 16 Magnus Vigerlof 2008-01-09 10:47:04 UTC
Created attachment 13624 [details] [review]
Use right screen dimension in calculation after screen crossing

(In reply to comment #14)
> (In reply to comment #11)
> > Created an attachment (id=13592) [details] [details]
> > dix: Do not call clipAxis for relative pointer events
> 
> when you're re-scaling pDev->lastx, shouldn't it be scr->width, not
> scr->height?
> otherwise ACK.

Yes, it should be width not height, thanks!
Comment 17 Magnus Vigerlof 2008-01-09 10:51:52 UTC
Two more things that I've seen in the function.

Can GetPointerEvents ever be called with the virtual core pointer as the reporting device?

Should we call miPointerSetPosition for devices that are not reporting core-events? (== moving a pointer around on the screen?)
Comment 18 Daniel Stone 2008-01-09 14:12:21 UTC
(In reply to comment #17)
> Two more things that I've seen in the function.
> 
> Can GetPointerEvents ever be called with the virtual core pointer as the
> reporting device?
> 
> Should we call miPointerSetPosition for devices that are not reporting
> core-events? (== moving a pointer around on the screen?)

Amusingly, your patch exposed a comment that is now very much incorrect.  There's now more than one mi pointer, so really, all the miPointer functions (including SetPosition) should be always called per-device, and only called for the core pointer if we're sending a core event.
Comment 19 Peter Hutterer 2008-01-09 16:26:58 UTC
(In reply to comment #17)
> Two more things that I've seen in the function.
> 
> Can GetPointerEvents ever be called with the virtual core pointer as the
> reporting device?

yes, in response to a WarpPointer request for example. Look at miPointerMove in mi/mipointer.c.

> Should we call miPointerSetPosition for devices that are not reporting
> core-events? (== moving a pointer around on the screen?)

no. (see daniel's comment).
 

Comment 20 Magnus Vigerlof 2008-01-10 15:07:07 UTC
(In reply to comment #19)
> > Should we call miPointerSetPosition for devices that are not reporting
> > core-events? (== moving a pointer around on the screen?)
> 
> no. (see daniel's comment).

But it still works as I expect it to.. Ok, that function will not do anything if the provided device should not send any core events.

I leave the comment as it is, as I don't know what should be written instead..
Comment 21 Magnus Vigerlof 2008-01-10 15:08:30 UTC
Created attachment 13651 [details] [review]
Add scaling to absolute reporting with missing x&y
Comment 22 Magnus Vigerlof 2008-01-10 15:09:22 UTC
Created attachment 13652 [details] [review]
Move motion history update until after screen crossing and clipping
Comment 23 Magnus Vigerlof 2008-01-10 15:10:12 UTC
Created attachment 13653 [details] [review]
Store coords when crossing screen or clipping for VCP
Comment 24 Magnus Vigerlof 2008-01-12 14:14:15 UTC
Created attachment 13679 [details] [review]
dix: Call clipAxis for relative events in GetPointerEvents
Comment 25 Peter Hutterer 2008-01-16 23:15:01 UTC
(In reply to comment #21)
> Created an attachment (id=13651) [details]
> Add scaling to absolute reporting with missing x&y
> 

ACK
Comment 26 Peter Hutterer 2008-01-16 23:16:59 UTC
(In reply to comment #22)
> Created an attachment (id=13652) [details]
> Move motion history update until after screen crossing and clipping
> 

I think this is wrong. you also have to update the motion history if the event is != MotionNotify. Absolute devices may just send a button press at a different location, without a motion event first. In this case the motion history should still be updated I guess.
Comment 27 Peter Hutterer 2008-01-16 23:34:08 UTC
as for the others, it's getting a bit hard for me to keep track of consecutive diffs.

daniels, any reason why magnus can't fix the remaining stuff on git master directly?
Comment 28 Daniel Stone 2008-01-17 04:51:41 UTC
i don't have the bandwidth to look at it at the moment, but if you're happy with it, peter, then i don't see any reason to keep it out of master.  after all, if it's broken, we can always fix it up later; cf. input-hotplug. ;)
Comment 29 Magnus Vigerlof 2008-01-17 13:26:35 UTC
Maybe I got a bit carried away with the motion history.. Figuring out which part of the old behaviour that was changed deliberately is not always easy.

Yes, please add them to master if you think they're good enough. I've been waiting for this moment :) I hope they will eliminate more bugs than they will introduce.

If it's possible to get the rescale patch included in the 1.4-branch I would be very grateful as that would mean that the workarounds in the linuxwacom driver that we had to add could be removed (and we can use the full potential of our tablets in X.org again).
Comment 30 Peter Hutterer 2008-01-17 18:45:52 UTC
(In reply to comment #28)
> i don't have the bandwidth to look at it at the moment, but if you're happy
> with it, peter, then i don't see any reason to keep it out of master.  after
> all, if it's broken, we can always fix it up later; cf. input-hotplug. ;)

I think it's good enough, although lacking devices I haven't had a chance to fix it. and if we wait until it's perfect we're not going anywhere. especially since git master is a moving target.

The other thing is that i-h has shown that most people who run master don't have absolute devices anyway, otherwise we'd have spotted this earlier :) so I think it's safe to get magnus an account and let him merge it.

(In reply to comment #29)
> Maybe I got a bit carried away with the motion history.. Figuring out which
> part of the old behaviour that was changed deliberately is not always easy.
> 
> Yes, please add them to master if you think they're good enough. I've been
> waiting for this moment :) I hope they will eliminate more bugs than they will
> introduce.
> 
> If it's possible to get the rescale patch included in the 1.4-branch I would be
> very grateful as that would mean that the workarounds in the linuxwacom driver
> that we had to add could be removed (and we can use the full potential of our
> tablets in X.org again).

Magnus, please request a fdo account. See http://www.freedesktop.org/wiki/AccountRequests
Comment 31 Magnus Vigerlof 2008-02-05 12:27:02 UTC
A slightly modified patchset (so they can more easily be cherry-picked) are now committed to master. The change the patches does are the same though.

Thanks for the review of my patches!