Bug 49347

Summary: Jumping tablet cursor with transformation matrix
Product: xorg Reporter: Nikolai Kondrashov <spbnick>
Component: Server/Input/CoreAssignee: Peter Hutterer <peter.hutterer>
Status: RESOLVED FIXED QA Contact: Xorg Project Team <xorg-team>
Severity: normal    
Priority: medium CC: peter.hutterer, timothyhobbs
Version: 7.6 (2010.12)   
Hardware: x86 (IA32)   
OS: Linux (All)   
Whiteboard:
i915 platform: i915 features:
Attachments:
Description Flags
evemu capture of KYE EasyPen i405X movement
none
0001-dix-undo-transformation-for-missing-valuators-49347.patch
none
Fix backport to 1.11.4
none
fix-touchpoint-last-valuators-transform.patch none

Description Nikolai Kondrashov 2012-05-01 09:56:08 UTC
Created attachment 60860 [details]
evemu capture of KYE EasyPen i405X movement

A tablet handled by xf86-input-evdev has cursor jumping off the actual position when an axis is missing, if non-identity coordinate transformation matrix is set.

This breaks usage of coordinate transformation matrix with tablets handled by xf86-input-evdev.

This, apparently, happens, because xf86-input-evdev actually clears valuator mask bits for axis missing from an evdev report (as opposed to xf86-input-wacom, for example). As a result, transformAbsolute in dix/getevents.c uses values from dev->last.valuators which have been *already* transformed previously, leading to repeated transformation and shifted coordinates. Judging from the source code, this problem could also exist in the Git version.

This could be reproduced by having non-identity matrix set for a tablet handled by xf86-input-evdev.

Alternatively, the attached evemu capture could be used. Like this:

# Note the event device path output by the tool, referred to as "DEV" further on
sudo evemu-device kye_easypen_i405x.device &
# Show the (correct) movement without transformation matrix
sudo evemu-play DEV < kye_easypen_i405x.events
# Find out the input device ID, referred to as "ID" further on
xinput list
# Set non-identity transformation matrix
xinput set-float-prop ID "Coordinate Transformation Matrix" 1 0 0 0 1 0 0 0 1
# Show the (distorted) movement with transformation matrix
sudo evemu-play DEV < kye_easypen_i405x.events
Comment 1 Nikolai Kondrashov 2012-05-03 09:12:22 UTC
Of, course the "Set non-identity transformation matrix" line should instead be:

xinput set-float-prop 19 "Coordinate Transformation Matrix" 0.8 0 0.1 0 0.8 0.1 0 0 1
Comment 2 Peter Hutterer 2012-05-08 18:36:18 UTC
Created attachment 61257 [details] [review]
0001-dix-undo-transformation-for-missing-valuators-49347.patch
Comment 3 Nikolai Kondrashov 2012-05-09 03:48:25 UTC
Thanks, Peter!

Unfortunately I didn't find a good way to build and run the git version of the server and can't test this exact patch, yet.

I've looked at the possibility of backporting it to 1.11.4, which I have, using Debian testing, and found a problem. That version stores coordinates as integers and I presume applying reverse transformation to rounded-up coordinates would introduce error which could show up as occasional 1 pixel jitter.

This is my guess, though, as I haven't tested it yet.

Could there be another fix more easily back-portable to the older versions? Like storing previous untransformed coordinates?
Comment 4 Peter Hutterer 2012-05-09 16:48:30 UTC
(In reply to comment #3)
> Unfortunately I didn't find a good way to build and run the git version of the
> server and can't test this exact patch, yet.

http://who-t.blogspot.com.au/2012/05/testing-x-servers-from-git.html explains that

> I've looked at the possibility of backporting it to 1.11.4, which I have, using
> Debian testing, and found a problem. That version stores coordinates as
> integers and I presume applying reverse transformation to rounded-up
> coordinates would introduce error which could show up as occasional 1 pixel
> jitter.
> 
> This is my guess, though, as I haven't tested it yet.
> 
> Could there be another fix more easily back-portable to the older versions?
> Like storing previous untransformed coordinates?

I quickly looked at that but it would mean breaking the ABI which, for backports, is even more work. I'd probably go with the odd 1px offset if I were you.
Comment 5 Nikolai Kondrashov 2012-05-15 13:04:59 UTC
Created attachment 61690 [details] [review]
Fix backport to 1.11.4

I didn't manage to run the Git version yet, although it built fine.

However, I have backported the 0001-dix-undo-transformation-for-missing-valuators-49347.patch to the server version Debian testing uses (1.11.4) and it seems to work fine.
Comment 6 Peter Hutterer 2012-05-16 19:00:33 UTC
Comment on attachment 61690 [details] [review]
Fix backport to 1.11.4

Review of attachment 61690 [details] [review]:
-----------------------------------------------------------------

Looks ok on a glance. 1.11 is EOL though, I don't think we'll merge this patch upstream.
Comment 7 Peter Hutterer 2012-05-17 16:59:59 UTC
commit 749a593e49adccdf1225be28a521412ec85333f4
Author: Peter Hutterer <peter.hutterer@who-t.net>
Date:   Wed May 9 11:30:46 2012 +1000

    dix: undo transformation for missing valuators (#49347)
Comment 8 Jens Harms 2012-11-07 09:56:48 UTC
A "eGalax Inc. USB TouchController" attached to a 17 inch touchscreen creates the described cursor movements too, if a CTM is applied.


From Xorg.0.log:

X.Org X Server 1.13.0
Release Date: 2012-09-05

eGalax Inc. USB TouchController 

From lsusb:

Bus 003 Device 003: ID 0eef:0001 D-WAV Scientific Co., Ltd eGalax TouchScreen
Comment 9 Peter Hutterer 2012-11-07 11:27:09 UTC
Jens: could this be https://bugzilla.redhat.com/show_bug.cgi?id=852841?
If so, that's fixed by 3d1051aecbb1955084804133cacd12c7f696833a
Comment 10 Yuly Novikov 2012-11-07 22:28:47 UTC
Created attachment 69673 [details] [review]
fix-touchpoint-last-valuators-transform.patch

Temporary patch version. Should be good for version 1.12.4, needs verification against master.
Comment 11 Yuly Novikov 2012-11-07 22:29:56 UTC
I have also observed a similar behaviour with a touchscreen.
The original fix didn't affect touchscreen devices, as for them last coordinates are stored in DDXTouchPointInfoRec.valuators and not DeviceIntRec.last.valuators.
I have a patch which works for our local branch.
I'd like to send it for a review once I've checked it also applies to master branch.
Comment 12 Wesley DvH 2012-11-15 12:22:38 UTC
I am experiencing the same problems as mentioned above, tested with Xorg 12.4
Comment 13 Wesley DvH 2012-11-15 12:30:27 UTC
(Sorry, accidentally send comment and can't seem to edit)

I am experiencing the same problems as mentioned above, tested with Xorg 12.4 and Xorg 13.0. I managed to patch 12.4 with Novikov's patch which seemed to fix the problem. 

Sadly I wasn't able to manually apply the patch to 13.0 since there are some references to _DDXTouchPointInfo's valuators (At least I think) Tested it with both a Quanta and PixArt touchscreen.

Will the patch be added to the main soon?
Comment 14 Yuly Novikov 2012-11-15 17:26:02 UTC
See http://www.mail-archive.com/xorg-devel@lists.x.org/msg33660.html for the patch I've sent for review, which should work for 13.0.
Comment 15 Wesley DvH 2012-11-20 13:54:45 UTC
Awesome, the patches you provided fixed the problem for me in 13.0. 

Thank you, Yuly Novikov!
Comment 16 Peter Hutterer 2012-12-02 23:29:33 UTC
commit 3b9f1c701787965246638c1a6fd99fb2b6078114
Author: Yuly Novikov <ynovikov@chromium.org>
Date:   Mon Nov 19 21:04:57 2012 -0500

    dix: Save touchpoint last coordinates before transform. #49347
Comment 17 Peter Hutterer 2013-01-21 03:38:03 UTC
*** Bug 56008 has been marked as a duplicate of this bug. ***

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.