Re: [PATCH libinput] evdev: fix device_transform_ functions

[Jonas Ådahl]

On Tue, Feb 18, 2014 at 02:28:53PM +1000, Peter Hutterer wrote:
 On Mon, Feb 17, 2014 at 01:42:52PM -0500, Benjamin Tissoires wrote:
  X and Y are li_fixed_t, which is 24.8 fixed point real number.
  li_fixed_t max is thus ~8388607.
  
  On a touchscreen with a range of 32767 values (like a 3M sensor), and
  mapped on monitor with a resolution of 1920x1080, we currently have:
  (x - li_fixed_from_int(device-abs.min_x)) * width == 62912640
  
  which is 7 times bigger than li_fixed_t max.
  
  To keep the precision of the sensor, first compute the uniformized
  coordinate (in range 0 .. 1.0) of the touch point, then multiply it
  by the screen dimension, and revert it to a li_fixed_t.
  
  Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com
  ---
  
  Hi,
  
  I have hit this problem by playing with a touchscreen reporting 4096 
  values, on
  xf86-input-libinput. xf86-input-libinput does not use the real screen size, 
  but
  0x instead. This allows to report a touchscreen with a range of 128 
  values
  to work properly :(
  
  I went through the multitouch device database, and took one example of a 
  more
  real use-case (xf86-input-libinput is still in an early shape).
 
 fwiw, this is exactly the type of use-case where it would be simple and
 worth it to knock up a test for a single device and make sure that the
 coordinates are correct. which gives us a nice reproducer and prevents us
 from errors like this in the future.
 
   src/evdev.c | 6 --
   1 file changed, 4 insertions(+), 2 deletions(-)
  
  diff --git a/src/evdev.c b/src/evdev.c
  index d8dff65..0d033b8 100644
  --- a/src/evdev.c
  +++ b/src/evdev.c
  @@ -91,8 +91,9 @@ evdev_device_transform_x(struct evdev_device *device,
   li_fixed_t x,
   uint32_t width)
   {
  -   return (x - li_fixed_from_int(device-abs.min_x)) * width /
  +   double x_scaled = (li_fixed_to_double(x) - device-abs.min_x) /
  (device-abs.max_x - device-abs.min_x + 1);
  +   return li_fixed_from_double(x_scaled * width);
 
 A simple 1L *  should suffice, right?

It would if 1L means 64 bit, but that is not the case on all platforms.
A cast to uint64_t is enough for this case though.

Jonas

 
 Cheers,
Peter
 
   }
   
   li_fixed_t
  @@ -100,8 +101,9 @@ evdev_device_transform_y(struct evdev_device *device,
   li_fixed_t y,
   uint32_t height)
   {
  -   return (y - li_fixed_from_int(device-abs.min_y)) * height /
  +   double y_scaled = (li_fixed_to_double(y) - device-abs.min_y) /
  (device-abs.max_y - device-abs.min_y + 1);
  +   return li_fixed_from_double(y_scaled * height);
   }
   
   static void
  -- 
  1.8.5.3
  
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH libinput] evdev: fix device_transform_ functions

[Benjamin Tissoires]

X and Y are li_fixed_t, which is 24.8 fixed point real number.
li_fixed_t max is thus ~8388607.

On a touchscreen with a range of 32767 values (like a 3M sensor), and
mapped on monitor with a resolution of 1920x1080, we currently have:
(x - li_fixed_from_int(device-abs.min_x)) * width == 62912640

which is 7 times bigger than li_fixed_t max.

To keep the precision of the sensor, first compute the uniformized
coordinate (in range 0 .. 1.0) of the touch point, then multiply it
by the screen dimension, and revert it to a li_fixed_t.

Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com
---

Hi,

I have hit this problem by playing with a touchscreen reporting 4096 values, on
xf86-input-libinput. xf86-input-libinput does not use the real screen size, but
0x instead. This allows to report a touchscreen with a range of 128 values
to work properly :(

I went through the multitouch device database, and took one example of a more
real use-case (xf86-input-libinput is still in an early shape).

Cheers,
Benjamin

 src/evdev.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/evdev.c b/src/evdev.c
index d8dff65..0d033b8 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -91,8 +91,9 @@ evdev_device_transform_x(struct evdev_device *device,
 li_fixed_t x,
 uint32_t width)
 {
-   return (x - li_fixed_from_int(device-abs.min_x)) * width /
+   double x_scaled = (li_fixed_to_double(x) - device-abs.min_x) /
(device-abs.max_x - device-abs.min_x + 1);
+   return li_fixed_from_double(x_scaled * width);
 }
 
 li_fixed_t
@@ -100,8 +101,9 @@ evdev_device_transform_y(struct evdev_device *device,
 li_fixed_t y,
 uint32_t height)
 {
-   return (y - li_fixed_from_int(device-abs.min_y)) * height /
+   double y_scaled = (li_fixed_to_double(y) - device-abs.min_y) /
(device-abs.max_y - device-abs.min_y + 1);
+   return li_fixed_from_double(y_scaled * height);
 }
 
 static void
-- 
1.8.5.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput] evdev: fix device_transform_ functions

[Peter Hutterer]

On Mon, Feb 17, 2014 at 01:42:52PM -0500, Benjamin Tissoires wrote:
 X and Y are li_fixed_t, which is 24.8 fixed point real number.
 li_fixed_t max is thus ~8388607.
 
 On a touchscreen with a range of 32767 values (like a 3M sensor), and
 mapped on monitor with a resolution of 1920x1080, we currently have:
 (x - li_fixed_from_int(device-abs.min_x)) * width == 62912640
 
 which is 7 times bigger than li_fixed_t max.
 
 To keep the precision of the sensor, first compute the uniformized
 coordinate (in range 0 .. 1.0) of the touch point, then multiply it
 by the screen dimension, and revert it to a li_fixed_t.
 
 Signed-off-by: Benjamin Tissoires benjamin.tissoi...@gmail.com
 ---
 
 Hi,
 
 I have hit this problem by playing with a touchscreen reporting 4096 values, 
 on
 xf86-input-libinput. xf86-input-libinput does not use the real screen size, 
 but
 0x instead. This allows to report a touchscreen with a range of 128 values
 to work properly :(
 
 I went through the multitouch device database, and took one example of a more
 real use-case (xf86-input-libinput is still in an early shape).

fwiw, this is exactly the type of use-case where it would be simple and
worth it to knock up a test for a single device and make sure that the
coordinates are correct. which gives us a nice reproducer and prevents us
from errors like this in the future.

  src/evdev.c | 6 --
  1 file changed, 4 insertions(+), 2 deletions(-)
 
 diff --git a/src/evdev.c b/src/evdev.c
 index d8dff65..0d033b8 100644
 --- a/src/evdev.c
 +++ b/src/evdev.c
 @@ -91,8 +91,9 @@ evdev_device_transform_x(struct evdev_device *device,
li_fixed_t x,
uint32_t width)
  {
 - return (x - li_fixed_from_int(device-abs.min_x)) * width /
 + double x_scaled = (li_fixed_to_double(x) - device-abs.min_x) /
   (device-abs.max_x - device-abs.min_x + 1);
 + return li_fixed_from_double(x_scaled * width);

A simple 1L *  should suffice, right?

Cheers,
   Peter

  }
  
  li_fixed_t
 @@ -100,8 +101,9 @@ evdev_device_transform_y(struct evdev_device *device,
li_fixed_t y,
uint32_t height)
  {
 - return (y - li_fixed_from_int(device-abs.min_y)) * height /
 + double y_scaled = (li_fixed_to_double(y) - device-abs.min_y) /
   (device-abs.max_y - device-abs.min_y + 1);
 + return li_fixed_from_double(y_scaled * height);
  }
  
  static void
 -- 
 1.8.5.3
 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel