Patchwork dix: only transform valuators when we need them.

login
register
mail settings
Submitter Peter Hutterer
Date April 21, 2011, 2:35 p.m.
Message ID <20110421073555.GA333@barra.redhat.com>
Download mbox | patch
Permalink /patch/5024/
State New, archived
Headers show

Comments

Peter Hutterer - April 21, 2011, 2:35 p.m.
Unconditionally drop the valuators back into the mask when they were there
in the first place. Otherwise, sending identical coordinates from the driver
on a translated device causes the valuator mask to be alternatively
overwritten with the translated value or left as-is. This leads to the
device jumping around between the translated and the original position.

The same could be achieved with a valuator_mask_unset() combination.

Testcase:
xsetwacom set "device name" MapToOutput VGA1
Then press a button on the device, cursor jumps between the two positions.

Introduced in 31737fff08ec19b394837341d5e358ec401f5cd8

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
 dix/getevents.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
Daniel Stone - April 21, 2011, 6:50 p.m.
On Thu, Apr 21, 2011 at 05:35:55PM +1000, Peter Hutterer wrote:
> Unconditionally drop the valuators back into the mask when they were there
> in the first place. Otherwise, sending identical coordinates from the driver
> on a translated device causes the valuator mask to be alternatively
> overwritten with the translated value or left as-is. This leads to the
> device jumping around between the translated and the original position.
> 
> The same could be achieved with a valuator_mask_unset() combination.
> 
> Testcase:
> xsetwacom set "device name" MapToOutput VGA1
> Then press a button on the device, cursor jumps between the two positions.
> 
> Introduced in 31737fff08ec19b394837341d5e358ec401f5cd8
> 
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

Reviewed-by: Daniel Stone <daniel@fooishbar.org>
Chase Douglas - April 25, 2011, 11:58 p.m.
On 04/21/2011 03:35 AM, Peter Hutterer wrote:
> Unconditionally drop the valuators back into the mask when they were there
> in the first place. Otherwise, sending identical coordinates from the driver
> on a translated device causes the valuator mask to be alternatively
> overwritten with the translated value or left as-is. This leads to the
> device jumping around between the translated and the original position.
> 
> The same could be achieved with a valuator_mask_unset() combination.
> 
> Testcase:
> xsetwacom set "device name" MapToOutput VGA1
> Then press a button on the device, cursor jumps between the two positions.
> 
> Introduced in 31737fff08ec19b394837341d5e358ec401f5cd8
> 
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> ---
>  dix/getevents.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/dix/getevents.c b/dix/getevents.c
> index 0fa8046..7afd330 100644
> --- a/dix/getevents.c
> +++ b/dix/getevents.c
> @@ -1065,9 +1065,10 @@ transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
>  
>      pixman_f_transform_point(&dev->transform, &p);
>  
> -    if (lround(p.v[0]) != dev->last.valuators[0])
> +    if (valuator_mask_isset(mask, 0))
>          valuator_mask_set(mask, 0, lround(p.v[0]));
> -    if (lround(p.v[1]) != dev->last.valuators[1])
> +
> +    if (valuator_mask_isset(mask, 1))
>          valuator_mask_set(mask, 1, lround(p.v[1]));
>  }
>  

Why don't we change this to:

if (lround(p.v[0]) != dev->last.valuators[0])
    valuator_mask_set(mask, 0, lround(p.v[0]));
else
    valuator_mask_unset(mask, 0);

The proposed fix will cause valuators to be sent with repeated values if
they haven't actually changed.

Thanks,

-- Chase
Daniel Stone - April 28, 2011, 6:27 p.m.
On Mon, Apr 25, 2011 at 12:58:17PM -0400, Chase Douglas wrote:
> Why don't we change this to:
> 
> if (lround(p.v[0]) != dev->last.valuators[0])
>     valuator_mask_set(mask, 0, lround(p.v[0]));
> else
>     valuator_mask_unset(mask, 0);
> 
> The proposed fix will cause valuators to be sent with repeated values if
> they haven't actually changed.

Good catch.  With or without this change:
Reviewed-by: Daniel Stone <daniel@fooishbar.org>

(Peter, I haven't picked this one up -- up to you to decide what to do.)

Cheers,
Daniel
Peter Hutterer - May 3, 2011, 8:28 a.m.
On Mon, Apr 25, 2011 at 12:58:17PM -0400, Chase Douglas wrote:
> On 04/21/2011 03:35 AM, Peter Hutterer wrote:
> > Unconditionally drop the valuators back into the mask when they were there
> > in the first place. Otherwise, sending identical coordinates from the driver
> > on a translated device causes the valuator mask to be alternatively
> > overwritten with the translated value or left as-is. This leads to the
> > device jumping around between the translated and the original position.
> > 
> > The same could be achieved with a valuator_mask_unset() combination.
> > 
> > Testcase:
> > xsetwacom set "device name" MapToOutput VGA1
> > Then press a button on the device, cursor jumps between the two positions.
> > 
> > Introduced in 31737fff08ec19b394837341d5e358ec401f5cd8
> > 
> > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> > ---
> >  dix/getevents.c |    5 +++--
> >  1 files changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/dix/getevents.c b/dix/getevents.c
> > index 0fa8046..7afd330 100644
> > --- a/dix/getevents.c
> > +++ b/dix/getevents.c
> > @@ -1065,9 +1065,10 @@ transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
> >  
> >      pixman_f_transform_point(&dev->transform, &p);
> >  
> > -    if (lround(p.v[0]) != dev->last.valuators[0])
> > +    if (valuator_mask_isset(mask, 0))
> >          valuator_mask_set(mask, 0, lround(p.v[0]));
> > -    if (lround(p.v[1]) != dev->last.valuators[1])
> > +
> > +    if (valuator_mask_isset(mask, 1))
> >          valuator_mask_set(mask, 1, lround(p.v[1]));
> >  }
> >  
> 
> Why don't we change this to:
> 
> if (lround(p.v[0]) != dev->last.valuators[0])
>     valuator_mask_set(mask, 0, lround(p.v[0]));
> else
>     valuator_mask_unset(mask, 0);
> 
> The proposed fix will cause valuators to be sent with repeated values if
> they haven't actually changed.

I think this would generally need a bigger patch set than just the above
(which was my first attempt at the patch, see also the commit message) to be
addressed properly.

We don't have any guidelines for drivers regarding sending identical
coordinates and the server does a pretty poor job in filtering them,
especially for core events.
https://bugs.freedesktop.org/show_bug.cgi?id=23985

Plus the need for continuous valuator ranges in XI1 also means that even
when unsetting valuators in the generation path, they may (should!) get
added later anyway.

At least in the Wacom driver we need the coordinates to not be filtered when
identical, for the clients' sake. the specific use-case here is sending a
motion event followed by a button event. They have identical coordinates,
but if a client only listens to button events and the coordinates are
filtered, they cannot get the valuator values from the button event only.

Cheers,
  Peter
Chase Douglas - May 4, 2011, 3:15 p.m.
On 05/03/2011 03:28 AM, Peter Hutterer wrote:
> On Mon, Apr 25, 2011 at 12:58:17PM -0400, Chase Douglas wrote:
>> On 04/21/2011 03:35 AM, Peter Hutterer wrote:
>>> Unconditionally drop the valuators back into the mask when they were there
>>> in the first place. Otherwise, sending identical coordinates from the driver
>>> on a translated device causes the valuator mask to be alternatively
>>> overwritten with the translated value or left as-is. This leads to the
>>> device jumping around between the translated and the original position.
>>>
>>> The same could be achieved with a valuator_mask_unset() combination.
>>>
>>> Testcase:
>>> xsetwacom set "device name" MapToOutput VGA1
>>> Then press a button on the device, cursor jumps between the two positions.
>>>
>>> Introduced in 31737fff08ec19b394837341d5e358ec401f5cd8
>>>
>>> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
>>> ---
>>>  dix/getevents.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/dix/getevents.c b/dix/getevents.c
>>> index 0fa8046..7afd330 100644
>>> --- a/dix/getevents.c
>>> +++ b/dix/getevents.c
>>> @@ -1065,9 +1065,10 @@ transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
>>>  
>>>      pixman_f_transform_point(&dev->transform, &p);
>>>  
>>> -    if (lround(p.v[0]) != dev->last.valuators[0])
>>> +    if (valuator_mask_isset(mask, 0))
>>>          valuator_mask_set(mask, 0, lround(p.v[0]));
>>> -    if (lround(p.v[1]) != dev->last.valuators[1])
>>> +
>>> +    if (valuator_mask_isset(mask, 1))
>>>          valuator_mask_set(mask, 1, lround(p.v[1]));
>>>  }
>>>  
>>
>> Why don't we change this to:
>>
>> if (lround(p.v[0]) != dev->last.valuators[0])
>>     valuator_mask_set(mask, 0, lround(p.v[0]));
>> else
>>     valuator_mask_unset(mask, 0);
>>
>> The proposed fix will cause valuators to be sent with repeated values if
>> they haven't actually changed.
> 
> I think this would generally need a bigger patch set than just the above
> (which was my first attempt at the patch, see also the commit message) to be
> addressed properly.

Yeah... We received a bug report for this and I started diving into it
and realized it will require a bigger patch.

> We don't have any guidelines for drivers regarding sending identical
> coordinates and the server does a pretty poor job in filtering them,
> especially for core events.
> https://bugs.freedesktop.org/show_bug.cgi?id=23985
> 
> Plus the need for continuous valuator ranges in XI1 also means that even
> when unsetting valuators in the generation path, they may (should!) get
> added later anyway.

Yeah.

> At least in the Wacom driver we need the coordinates to not be filtered when
> identical, for the clients' sake. the specific use-case here is sending a
> motion event followed by a button event. They have identical coordinates,
> but if a client only listens to button events and the coordinates are
> filtered, they cannot get the valuator values from the button event only.

Good point.

Reviewed-by: Chase Douglas <chase.douglas@canonical.com>

Thanks!
Peter Hutterer - May 5, 2011, 7:29 a.m.
On Wed, May 04, 2011 at 10:15:22AM +0200, Chase Douglas wrote:
> On 05/03/2011 03:28 AM, Peter Hutterer wrote:
> > On Mon, Apr 25, 2011 at 12:58:17PM -0400, Chase Douglas wrote:
> >> On 04/21/2011 03:35 AM, Peter Hutterer wrote:
> >>> Unconditionally drop the valuators back into the mask when they were there
> >>> in the first place. Otherwise, sending identical coordinates from the driver
> >>> on a translated device causes the valuator mask to be alternatively
> >>> overwritten with the translated value or left as-is. This leads to the
> >>> device jumping around between the translated and the original position.
> >>>
> >>> The same could be achieved with a valuator_mask_unset() combination.
> >>>
> >>> Testcase:
> >>> xsetwacom set "device name" MapToOutput VGA1
> >>> Then press a button on the device, cursor jumps between the two positions.
> >>>
> >>> Introduced in 31737fff08ec19b394837341d5e358ec401f5cd8
> >>>
> >>> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> >>> ---
> >>>  dix/getevents.c |    5 +++--
> >>>  1 files changed, 3 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/dix/getevents.c b/dix/getevents.c
> >>> index 0fa8046..7afd330 100644
> >>> --- a/dix/getevents.c
> >>> +++ b/dix/getevents.c
> >>> @@ -1065,9 +1065,10 @@ transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
> >>>  
> >>>      pixman_f_transform_point(&dev->transform, &p);
> >>>  
> >>> -    if (lround(p.v[0]) != dev->last.valuators[0])
> >>> +    if (valuator_mask_isset(mask, 0))
> >>>          valuator_mask_set(mask, 0, lround(p.v[0]));
> >>> -    if (lround(p.v[1]) != dev->last.valuators[1])
> >>> +
> >>> +    if (valuator_mask_isset(mask, 1))
> >>>          valuator_mask_set(mask, 1, lround(p.v[1]));
> >>>  }
> >>>  
> >>
> >> Why don't we change this to:
> >>
> >> if (lround(p.v[0]) != dev->last.valuators[0])
> >>     valuator_mask_set(mask, 0, lround(p.v[0]));
> >> else
> >>     valuator_mask_unset(mask, 0);
> >>
> >> The proposed fix will cause valuators to be sent with repeated values if
> >> they haven't actually changed.
> > 
> > I think this would generally need a bigger patch set than just the above
> > (which was my first attempt at the patch, see also the commit message) to be
> > addressed properly.
> 
> Yeah... We received a bug report for this and I started diving into it
> and realized it will require a bigger patch.
> 
> > We don't have any guidelines for drivers regarding sending identical
> > coordinates and the server does a pretty poor job in filtering them,
> > especially for core events.
> > https://bugs.freedesktop.org/show_bug.cgi?id=23985
> > 
> > Plus the need for continuous valuator ranges in XI1 also means that even
> > when unsetting valuators in the generation path, they may (should!) get
> > added later anyway.
> 
> Yeah.
> 
> > At least in the Wacom driver we need the coordinates to not be filtered when
> > identical, for the clients' sake. the specific use-case here is sending a
> > motion event followed by a button event. They have identical coordinates,
> > but if a client only listens to button events and the coordinates are
> > filtered, they cannot get the valuator values from the button event only.
> 
> Good point.
> 
> Reviewed-by: Chase Douglas <chase.douglas@canonical.com>

heh, I just noticed that you had also fixed this in the commit below that
was in daniels' for-keith tree. I'll skip the one above and just merge yours
in then.

commit 65b54548dce80c8e8ff5ff91fc4f0659e9b2d921
Author: Chase Douglas <chase.douglas@canonical.com>
Date:   Tue Jan 18 20:08:09 2011 +0000

    Input: Pass co-ordinates by reference to transformAbsolute


Cheers,
  Peter

Patch

diff --git a/dix/getevents.c b/dix/getevents.c
index 0fa8046..7afd330 100644
--- a/dix/getevents.c
+++ b/dix/getevents.c
@@ -1065,9 +1065,10 @@  transformAbsolute(DeviceIntPtr dev, ValuatorMask *mask)
 
     pixman_f_transform_point(&dev->transform, &p);
 
-    if (lround(p.v[0]) != dev->last.valuators[0])
+    if (valuator_mask_isset(mask, 0))
         valuator_mask_set(mask, 0, lround(p.v[0]));
-    if (lround(p.v[1]) != dev->last.valuators[1])
+
+    if (valuator_mask_isset(mask, 1))
         valuator_mask_set(mask, 1, lround(p.v[1]));
 }