Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Olivier Fourdan]

Hi all,

Sorry to bring this back again...

> Just an addition to my previous explanation.
> 
> The systematic server side key repeating only is also a good solution,
> if we made the hypothesis that long key press are a lot less frequent
> than short key pressing.
> 
> And this solution is less complex.

Any further comments on Benoit's proposal?

Meanwhile, what about the patch series I posted as a workaround for Xwayland:

https://patchwork.freedesktop.org/patch/76323/
https://patchwork.freedesktop.org/patch/76324/
https://patchwork.freedesktop.org/patch/76326/
https://patchwork.freedesktop.org/patch/76334/

Should we continue in this way or should I just leave that alone until an 
appropriate fix is pushed in gnome-shell/mutter?

Cheers,
Olivier
 
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Benoit Gschwind]

Hi,

Just an addition to my previous explanation.

The systematic server side key repeating only is also a good solution,
if we made the hypothesis that long key press are a lot less frequent
than short key pressing.

And this solution is less complex.

Best regards.

Le 11/03/2016 11:25, Benoit Gschwind a écrit :
> Hi Olivier,
> 
> Le 11/03/2016 10:22, Olivier Fourdan a écrit :
>> Hi Benoit,
>>
>> - Original Message -
>>> Hello,
>>>
>>> Here is my little contribution to the discussion, following a discussion
>>> on #wayland irc channel.
>>>
>>> The issue discussed here, as far I understood, is due to a client that
>>> miss interpret a high latency of the compositor as a repeating key. This
>>> happen when a client receive a key press event but do not receive the
>>> corresponding release event for a large enough time even if this event
>>> have physically happened.
>>
>> If I understand correctly your description, then yes, that sounds accurate.
>>
>> In short, we want to avoid spurious client-side repeat events.
>>  
>>> The current xwayland is an instance of unexpected consequence of this
>>> issue but this issue is not limited to xwayland. Thus this issue must be
>>> fixed at the protocol level and not at software level.
>>
>> I do agree a protocol would be useful to solve this problem in a more 
>> satisfactory manner.
>>  
>>> Trying to take in account all gathered comments, I built the following
>>> proposal.
>>>
>>> First I propose to keep current behavior, i.e. client receive press and
>>> release events. But when a client is about to guess a repeating event,
>>> instead of guessing it, it have to request the server to provide
>>> repeating events.
>>
>> So that would be both client-side and server-side key repeat?
> 
> Yes, my current proposal is hybrid, instead of always reporting
> repeating key for any key combination, the server wait for a request
> from the client. Once the client made the request, the client get
> repeating key events until the client cancel the request or key release
> happen. In other words the server generate only valid repeating key events.
> 
>>
>>> To do so, the client send a request with a parameter
>>> that represent a frequency or a special value (like 0) and something to
>>> identify the related key press event. if a key release have happened
>>> before the repeat request the server send the key release immediately
>>> and ignore the repeat request, else he send a repeat event immediately
>>> and try to send repeat event at the requested frequency until key
>>> release happen or another repeating request arrive.
>>
>> Why would it need to send repeat key at a given frequency? The client 
>> requests the key repeat only once at first?
> 
> This to avoid the client to send a request on every guessed repeating
> keys. the current proposal look like the following sequence:
> 
> 1. key press event
> 2. after some time client request the repeat event, because client guess one
> 3. the sever reply by repeat event (if necessary, else he send key release)
> 4. server send a repeat event again
> 5. server send a repeat event again
> [...]
> 10. server send a repeat event again
> 11. server send a release event.
> 
> and this avoid a lot of useless client requests, most of the time, as
> the following sequence show:
> 
> 1. key press
> 2. after some time client request the repeat event.
> 3. the sever reply by repeat event (if necessary)
> 4. after some time client request the repeat event.
> 5. the sever reply by repeat event (if necessary)
> 6. after some time client request the repeat event.
> 7. the sever reply by repeat event (if necessary)
> [...]
> 11. server send a release event.
> 
>>
>> In which case another repeating request would occur in this scenario, 
>> how/why the client would emit a new repeat request while waiting for repeat 
>> events from the server?
> 
> The client may want to cancel the recursive repeating from the server,
> telling him that he doesn't want repeating any more. For example you
> reach the scroll boundary. The client may also want to change the
> frequency. Some application have 2 frequency a slow one at first and
> them a fast one if the key press is long enough.
> 
>>
>>> if a new repeating
>>> request arrive the frequency is updated to the new one if necessary or
>>> the repeat is canceled if the frequency is set to a special value (let's
>>> say 0). if the client does not receive release event or repeating key,
>>> he have to guess that the server is busy and wait for events.
>>>
>>> This approach is very flexible, allowing client to request repeating
>>> only when needed. This solve the issue above and ensure that client get
>>> valid repeat event. This approach also avoid that the client make a
>>> request on every guessed repeat event.
>>
>> But how does that solve the case where the client is already repeating keys 
>> while the user has released the key already, but the compositor hasn't 
>> propagated the event? This 

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Benoit Gschwind]

Hi Olivier,

Le 11/03/2016 10:22, Olivier Fourdan a écrit :
> Hi Benoit,
> 
> - Original Message -
>> Hello,
>>
>> Here is my little contribution to the discussion, following a discussion
>> on #wayland irc channel.
>>
>> The issue discussed here, as far I understood, is due to a client that
>> miss interpret a high latency of the compositor as a repeating key. This
>> happen when a client receive a key press event but do not receive the
>> corresponding release event for a large enough time even if this event
>> have physically happened.
> 
> If I understand correctly your description, then yes, that sounds accurate.
> 
> In short, we want to avoid spurious client-side repeat events.
>  
>> The current xwayland is an instance of unexpected consequence of this
>> issue but this issue is not limited to xwayland. Thus this issue must be
>> fixed at the protocol level and not at software level.
> 
> I do agree a protocol would be useful to solve this problem in a more 
> satisfactory manner.
>  
>> Trying to take in account all gathered comments, I built the following
>> proposal.
>>
>> First I propose to keep current behavior, i.e. client receive press and
>> release events. But when a client is about to guess a repeating event,
>> instead of guessing it, it have to request the server to provide
>> repeating events.
> 
> So that would be both client-side and server-side key repeat?

Yes, my current proposal is hybrid, instead of always reporting
repeating key for any key combination, the server wait for a request
from the client. Once the client made the request, the client get
repeating key events until the client cancel the request or key release
happen. In other words the server generate only valid repeating key events.

> 
>> To do so, the client send a request with a parameter
>> that represent a frequency or a special value (like 0) and something to
>> identify the related key press event. if a key release have happened
>> before the repeat request the server send the key release immediately
>> and ignore the repeat request, else he send a repeat event immediately
>> and try to send repeat event at the requested frequency until key
>> release happen or another repeating request arrive.
> 
> Why would it need to send repeat key at a given frequency? The client 
> requests the key repeat only once at first?

This to avoid the client to send a request on every guessed repeating
keys. the current proposal look like the following sequence:

1. key press event
2. after some time client request the repeat event, because client guess one
3. the sever reply by repeat event (if necessary, else he send key release)
4. server send a repeat event again
5. server send a repeat event again
[...]
10. server send a repeat event again
11. server send a release event.

and this avoid a lot of useless client requests, most of the time, as
the following sequence show:

1. key press
2. after some time client request the repeat event.
3. the sever reply by repeat event (if necessary)
4. after some time client request the repeat event.
5. the sever reply by repeat event (if necessary)
6. after some time client request the repeat event.
7. the sever reply by repeat event (if necessary)
[...]
11. server send a release event.

> 
> In which case another repeating request would occur in this scenario, how/why 
> the client would emit a new repeat request while waiting for repeat events 
> from the server?

The client may want to cancel the recursive repeating from the server,
telling him that he doesn't want repeating any more. For example you
reach the scroll boundary. The client may also want to change the
frequency. Some application have 2 frequency a slow one at first and
them a fast one if the key press is long enough.

> 
>> if a new repeating
>> request arrive the frequency is updated to the new one if necessary or
>> the repeat is canceled if the frequency is set to a special value (let's
>> say 0). if the client does not receive release event or repeating key,
>> he have to guess that the server is busy and wait for events.
>>
>> This approach is very flexible, allowing client to request repeating
>> only when needed. This solve the issue above and ensure that client get
>> valid repeat event. This approach also avoid that the client make a
>> request on every guessed repeat event.
> 
> But how does that solve the case where the client is already repeating keys 
> while the user has released the key already, but the compositor hasn't 
> propagated the event? This actually more likely to happen because the rate is 
> usually higher than the initial delay to repeat.

The client never guess the key repeat, he ask the server to send them if
they actually had happen, i.e. the key wasn't released. In the case
above (Xwayland), Xwayland ask for repeating keys, if the key still
pressed the repeat event are sent else the release event is sent, fixing
the issue. Afterward, If new repeat event arrive he can sent those

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Olivier Fourdan]

Hi Benoit,

- Original Message -
> Hello,
> 
> Here is my little contribution to the discussion, following a discussion
> on #wayland irc channel.
> 
> The issue discussed here, as far I understood, is due to a client that
> miss interpret a high latency of the compositor as a repeating key. This
> happen when a client receive a key press event but do not receive the
> corresponding release event for a large enough time even if this event
> have physically happened.

If I understand correctly your description, then yes, that sounds accurate.

In short, we want to avoid spurious client-side repeat events.
 
> The current xwayland is an instance of unexpected consequence of this
> issue but this issue is not limited to xwayland. Thus this issue must be
> fixed at the protocol level and not at software level.

I do agree a protocol would be useful to solve this problem in a more 
satisfactory manner.
 
> Trying to take in account all gathered comments, I built the following
> proposal.
> 
> First I propose to keep current behavior, i.e. client receive press and
> release events. But when a client is about to guess a repeating event,
> instead of guessing it, it have to request the server to provide
> repeating events.

So that would be both client-side and server-side key repeat?

> To do so, the client send a request with a parameter
> that represent a frequency or a special value (like 0) and something to
> identify the related key press event. if a key release have happened
> before the repeat request the server send the key release immediately
> and ignore the repeat request, else he send a repeat event immediately
> and try to send repeat event at the requested frequency until key
> release happen or another repeating request arrive.

Why would it need to send repeat key at a given frequency? The client requests 
the key repeat only once at first?

In which case another repeating request would occur in this scenario, how/why 
the client would emit a new repeat request while waiting for repeat events from 
the server?

> if a new repeating
> request arrive the frequency is updated to the new one if necessary or
> the repeat is canceled if the frequency is set to a special value (let's
> say 0). if the client does not receive release event or repeating key,
> he have to guess that the server is busy and wait for events.
> 
> This approach is very flexible, allowing client to request repeating
> only when needed. This solve the issue above and ensure that client get
> valid repeat event. This approach also avoid that the client make a
> request on every guessed repeat event.

But how does that solve the case where the client is already repeating keys 
while the user has released the key already, but the compositor hasn't 
propagated the event? This actually more likely to happen because the rate is 
usually higher than the initial delay to repeat.

I think such a proposal would benefit from a more formal description, because I 
am not sure I completely follow your explanation.

FWIW, I fancy simple solutions (where applicable), so I'd rather have a 
client-side repeat or a server side repeat, but not a mix of the two (but maybe 
I did not fully understand your description).

Assuming we want to keep client-side repeat, the client needs to make sure the 
compositor is still providing the events accurately, in time, to avoid spurious 
key repeats, any time it's about to emit a repeated event. But maybe I am over 
simplifying the problem.

Cheers,
Olivier

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Benoit Gschwind]

Hello,

Here is my little contribution to the discussion, following a discussion
on #wayland irc channel.

The issue discussed here, as far I understood, is due to a client that
miss interpret a high latency of the compositor as a repeating key. This
happen when a client receive a key press event but do not receive the
corresponding release event for a large enough time even if this event
have physically happened.

The current xwayland is an instance of unexpected consequence of this
issue but this issue is not limited to xwayland. Thus this issue must be
fixed at the protocol level and not at software level.

Trying to take in account all gathered comments, I built the following
proposal.

First I propose to keep current behavior, i.e. client receive press and
release events. But when a client is about to guess a repeating event,
instead of guessing it, it have to request the server to provide
repeating events. To do so, the client send a request with a parameter
that represent a frequency or a special value (like 0) and something to
identify the related key press event. if a key release have happened
before the repeat request the server send the key release immediately
and ignore the repeat request, else he send a repeat event immediately
and try to send repeat event at the requested frequency until key
release happen or another repeating request arrive. if a new repeating
request arrive the frequency is updated to the new one if necessary or
the repeat is canceled if the frequency is set to a special value (let's
say 0). if the client does not receive release event or repeating key,
he have to guess that the server is busy and wait for events.

This approach is very flexible, allowing client to request repeating
only when needed. This solve the issue above and ensure that client get
valid repeat event. This approach also avoid that the client make a
request on every guessed repeat event.

Best regards.









Le 07/03/2016 18:44, Olivier Fourdan a écrit :
> Key repeat is handled by the X server, but for Wayland, the key
> press/release events need to be processed and forwarded by the Wayland
> compositor first.
> 
> If the Wayland compositor get stuck for whatever reason and does not
> process the key release event fast enough, this may cause the Xwayland
> server to generate spurious key repeat events.
> 
> That actually can lead to a complete hang of both the Wayland compositor
> and Xwayland as seen in the following GNOME bug:
> 
>   https://bugzilla.gnome.org/show_bug.cgi?id=762618
> 
> Basically, what happens here is that the Wayland compositor takes longer
> to actually process the fullscreen/unfullscreen transition than the
> repeat delay.
> 
> As a result, while the user has released the F11 key that triggers the 
> fullscreen/unfullscreen transition, the Wayland compositor is stuck
> is a graphical operation and cannot process the key release events, which
> triggers the auto-repeat in Xwayland, and therefore cause even more 
> fullscreen transitions. Only way out here is to kill the Xwayland
> application and wait for the queued up event to clear out...
> 
> While this is arguably a bug in the Wayland compositor, there is no
> way that I can think of to guarantee that this cannot happen.
> 
> One possiblity to mitigate the problem is to block the key repeat until
> we are sure the Wayland compositor is actually processing events.
> 
> The idea comes from a similar patch for gtk+ by Ray Strode here:
> 
>   https://bug757942.bugzilla-attachments.gnome.org/attachment.cgi?id=322876
> 
> from bug https://bugzilla.gnome.org/show_bug.cgi?id=757942
> 
> While the following patches do fix the issue in my case, they are not
> perfect and only a mitigation of the problem (e.g. they could easily be
> defeated by a Wayland compositor that would have different priority for
> Wayland protocol and input processing), that's why I send these couple
> of patches as an RFC for now.
> 
> Cheers,
> Olivier
> 
> 
> Olivier Fourdan (2):
>   xkb: add hook to allow/deny AccessX key repeat
>   xwayland: add a server sync before repeating keys
> 
>  hw/xwayland/xwayland-input.c | 36 
>  include/xkbsrv.h |  6 ++
>  xkb/xkbAccessX.c |  4 +++-
>  3 files changed, 45 insertions(+), 1 deletion(-)
> 
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Ray Strode]

Hi,

> This still suffers from your priority-starvation-bug. If a simple
> ping/pong protocol barrier doesn't solve the issue, why would the
> timer? You'd have to put the timer on lowest priority to make that
> work, but then again you can do the same for wl_display_sync.
Yup, indeed, you're right. I guess we just need to fix the compositor
here.

> Btw., if your compositor stalls for a bit more, you end up with an
> evdev-queue overrun and have more trouble than you can solve. Is it
> really worth solving that single race, while there're still several
> known other issues that happen on stalling compositors?
My concern is that we don't end up with spurious repeat events.
I don't want a relatively benign compositor stall (which is a bug
and should be fixed!) leading to 15 mails getting deleted instead
of 1 mail getting deleted when the user hits delete once.

> Why not rather put keyboard handling on high-priority?
> It is low-throughput, anyway.
fair.

--Ray
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[David Herrmann]

Hi

On Tue, Mar 8, 2016 at 4:41 PM, Ray Strode  wrote:
> On Tue, Mar 8, 2016 at 7:09 AM, Daniel Stone  wrote:
>>> If one justification for server-side repeat is that if the compositor
>>> is hosed and the user cannot see how many characters have been
>>> repeated, then you could as well solve that in the client too, by
>>> throttling your repeats to frame callbacks, but only when it makes sense
>>> and undoing repeats is not feasible.
>>
>> This is what has just been implemented for GTK+.
[...]
> The current approach uses wl_display_sync to throttle, but that has a
> problem too: clutter processes hardware input events at a lower priority
> than mutter processes wayland events
> ( G_PRIORITY_HIGH_IDLE + 50 (150) vs G_PRIORITY_DEFAULT + 1 (1),
>  lower is more urgent), so it's possible the sync could reply before a user
> release event is processed.  It might be possible to fix that in
> mutter/clutter, but might be tricky.
[...]
> I guess if we don't do server repeat events, an alternative would be to
> add a server timer protocol.  a client could request to be notified when
> n milliseconds has passed.  Might be useful for other use cases too.

This still suffers from your priority-starvation-bug. If a simple
ping/pong protocol barrier doesn't solve the issue, why would the
timer? You'd have to put the timer on lowest priority to make that
work, but then again you can do the same for wl_display_sync.

Btw., if your compositor stalls for a bit more, you end up with an
evdev-queue overrun and have more trouble than you can solve. Is it
really worth solving that single race, while there're still several
known other issues that happen on stalling compositors? Why not rather
put keyboard handling on high-priority? It is low-throughput, anyway.

David
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Ray Strode]

Hi,

On Tue, Mar 8, 2016 at 7:09 AM, Daniel Stone  wrote:
>> If one justification for server-side repeat is that if the compositor
>> is hosed and the user cannot see how many characters have been
>> repeated, then you could as well solve that in the client too, by
>> throttling your repeats to frame callbacks, but only when it makes sense
>> and undoing repeats is not feasible.
>
> This is what has just been implemented for GTK+.
No, the first draft of the patch was implemented that way for GTK+, but
Jonas pointed out a problem with the approach.  Not all key events lead
to screen updates.  For instance, some users hold down backspace for
a few seconds if they bungle their password at a terminal.  The expectation
is that holding down backspace in that case, should do the same thing
as hitting ctrl-u I guess (clear everything).

The current approach uses wl_display_sync to throttle, but that has a
problem too: clutter processes hardware input events at a lower priority
than mutter processes wayland events
( G_PRIORITY_HIGH_IDLE + 50 (150) vs G_PRIORITY_DEFAULT + 1 (1),
 lower is more urgent), so it's possible the sync could reply before a user
release event is processed.  It might be possible to fix that in
mutter/clutter, but might be tricky.

Anyway, clearly, the compositor stalling is bad, but if it happens we
shouldn't send a stream of the last character the user typed into the
application. That turns a momentary lock up into unpredictable
interaction with the application.  that's awful.

I guess if we don't do server repeat events, an alternative would be to
add a server timer protocol.  a client could request to be notified when
n milliseconds has passed.  Might be useful for other use cases too.

--Ray
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Daniel Stone]

Hi,

On 8 March 2016 at 12:07, Pekka Paalanen  wrote:
> If one justification for server-side repeat is that if the compositor
> is hosed and the user cannot see how many characters have been
> repeated, then you could as well solve that in the client too, by
> throttling your repeats to frame callbacks, but only when it makes sense
> and undoing repeats is not feasible.

This is what has just been implemented for GTK+.

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Pekka Paalanen]

On Tue, 8 Mar 2016 11:08:45 +0100
Hans de Goede  wrote:

> Hi,
> 
> On 08-03-16 10:15, Pekka Paalanen wrote:
> > On Mon, 7 Mar 2016 19:25:54 +0100
> > Hans de Goede  wrote:
> >  
> >> Hi,
> >>
> >> On 07-03-16 19:23, Hans de Goede wrote:  
> >>> Hi,
> >>>
> >>> On 07-03-16 18:44, Olivier Fourdan wrote:  
>  Key repeat is handled by the X server, but for Wayland, the key
>  press/release events need to be processed and forwarded by the Wayland
>  compositor first.
> 
>  If the Wayland compositor get stuck for whatever reason and does not
>  process the key release event fast enough, this may cause the Xwayland
>  server to generate spurious key repeat events.
> 
>  That actually can lead to a complete hang of both the Wayland compositor
>  and Xwayland as seen in the following GNOME bug:
> 
>  https://bugzilla.gnome.org/show_bug.cgi?id=762618
> 
>  Basically, what happens here is that the Wayland compositor takes longer
>  to actually process the fullscreen/unfullscreen transition than the
>  repeat delay.
> 
>  As a result, while the user has released the F11 key that triggers the
>  fullscreen/unfullscreen transition, the Wayland compositor is stuck
>  is a graphical operation and cannot process the key release events, which
>  triggers the auto-repeat in Xwayland, and therefore cause even more
>  fullscreen transitions. Only way out here is to kill the Xwayland
>  application and wait for the queued up event to clear out...
> 
>  While this is arguably a bug in the Wayland compositor, there is no
>  way that I can think of to guarantee that this cannot happen.
> 
>  One possiblity to mitigate the problem is to block the key repeat until
>  we are sure the Wayland compositor is actually processing events.
> 
>  The idea comes from a similar patch for gtk+ by Ray Strode here:
> 
>  
>  https://bug757942.bugzilla-attachments.gnome.org/attachment.cgi?id=322876
> 
>  from bug https://bugzilla.gnome.org/show_bug.cgi?id=757942
> 
>  While the following patches do fix the issue in my case, they are not
>  perfect and only a mitigation of the problem (e.g. they could easily be
>  defeated by a Wayland compositor that would have different priority for
>  Wayland protocol and input processing), that's why I send these couple
>  of patches as an RFC for now.  
> >>>
> >>> Why not simply rely on the keyrepeat of the compositor and forward repeat
> >>> events from it ? That way xwayland will also end up using the compositors
> >>> keyrepeat settings which would mean the user does not need to worry
> >>> about configuring this in multiple places ?
> >>>
> >>> Or is keyrepeat left to the toolkit under wayland ? In that case please
> >>> ignore my comment :)  
> >>
> >> OK, I've just seen that key-repeat is indeed left to the client / toolkit
> >> under wayland. I'm wondering if that is a deliberate decision or more
> >> of an accident, and if maybe we need a protocol ext for optional compositor
> >> side key-repeat, that seems better then all clients needing to implement
> >> this workaround.  
> >
> > Hi,
> >
> > it might be an oversight or a desire for simplicity: wl_keyboard key
> > events are promised to be physically generated by input devices and
> > never faked by a compositor. Obviously compositor doing the key repeat
> > does not fit in that scheme. Key events also carry a timestamp of the
> > physical action gnerating it, and for compositors to fake events, it
> > would need to be able to fake the timestamp too. I don't know if
> > anything actually specifies what the clock for input events is, even
> > for evdev specifically, though I suppose it'd be trivial to just
> > maintain a timestamp and increment it by the repeat period.  
> 
> Ack.
> 
> > However,
> > that has the race that you might send a repeated event with a timestamp
> > greater than a following key release event which is sampled by the
> > kernel or hardware.  
> 
> No, the whole idea of doing repeat in the server is that the server
> can ensure that it has seen the release (e.g. by draining the evdev
> fd for the device) before sending a repeat, so the whole idea is to avoid
> the race.

Hi,

ok, that could work.

> > You'd have to ask Kristian to be sure. Another question I have no idea
> > about is how you determine what keys repeat - would the compositor have
> > to maintain xkbcommon state for each client?  
> 
> You could mark repeat events as such, actually they should probably
> be named repeat-hint-events or something, and then the client can
> decide whether it is a good idea to actually listen to the hint
> and do key-repeat or not. So basically the idea would be to bring
> the timing to the server side which has 2 advantages:
> 
> 1) Server side this can be done race free
> 2) 1 common repeat-delay / speed setting for 

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Daniel Stone]

Hi,

On 8 March 2016 at 09:15, Pekka Paalanen  wrote:
> On Mon, 7 Mar 2016 19:25:54 +0100
> Hans de Goede  wrote:
>> On 07-03-16 19:23, Hans de Goede wrote:
>> > On 07-03-16 18:44, Olivier Fourdan wrote:
>> > Why not simply rely on the keyrepeat of the compositor and forward repeat
>> > events from it ? That way xwayland will also end up using the compositors
>> > keyrepeat settings which would mean the user does not need to worry
>> > about configuring this in multiple places ?
>> >
>> > Or is keyrepeat left to the toolkit under wayland ? In that case please
>> > ignore my comment :)
>>
>> OK, I've just seen that key-repeat is indeed left to the client / toolkit
>> under wayland. I'm wondering if that is a deliberate decision or more
>> of an accident, and if maybe we need a protocol ext for optional compositor
>> side key-repeat, that seems better then all clients needing to implement
>> this workaround.
>
> it might be an oversight or a desire for simplicity: wl_keyboard key
> events are promised to be physically generated by input devices and
> never faked by a compositor.

It was a deliberate choice. Compositors should always be responsive
and not being so is a pretty bad position to be in from the start;
clients have _even more_ information than the server about when repeat
is appropriate; sending them from the server to the client just causes
two sets of wakeups rather than one in order to perform repeat.
(Though, given they're likely to provoke rendering, that battle is
basically lost anyway.)

> Obviously compositor doing the key repeat
> does not fit in that scheme. Key events also carry a timestamp of the
> physical action gnerating it, and for compositors to fake events, it
> would need to be able to fake the timestamp too. I don't know if
> anything actually specifies what the clock for input events is, even
> for evdev specifically,

Unspecified base, millisecond granularity, monotonic. Same as most X11
and Wayland timestamps really.

> though I suppose it'd be trivial to just
> maintain a timestamp and increment it by the repeat period. However,
> that has the race that you might send a repeated event with a timestamp
> greater than a following key release event which is sampled by the
> kernel or hardware.
>
> You'd have to ask Kristian to be sure. Another question I have no idea
> about is how you determine what keys repeat - would the compositor have
> to maintain xkbcommon state for each client?

We effectively already do this, so not a real problem tbh.

> I understand having safeguards against the very busy-loop-flooding
> issue that raised these patches is a huge improvement, but at the same
> time IMHO there is a "bug" in the compositor that makes it unresponsive
> or not fluent. This means the compositor is already having issues
> providing a good user experience. That can of course be due to simply
> too slow hardware, so something has to stop the flood.

This is my position as well.

> Protecting XWayland against this flooding is probably necessary,
> because XWayland is producing the repeat events which causes X11
> clients to do things unthrottled. However, I'm not sure the *same* fix
> should be applied to native Wayland clients.
>
> Wouldn't native Wayland clients have means to throttle their state
> changes to compositor response already? E.g. switching to/from
> fullscreen you have to wait for a configure event, then you draw in new
> state, and then you wait for a frame callback to have the new state
> show on screen. Only after all that, it makes sense to change state
> again. Does this not solve the issue for native Wayland apps?

I'd imagine so, but if the situation is this bad anyway, clients just
queuing non-state-mutating changes (e.g. changed content buffer) may
swamp the compositor anyway. Unfortunately this seems to be basically
intractable until at least the point at which the Clutter (+ Cogl?)
and Mutter source trees are merged, so there's some urgency from that
corner to push an interim solution. Having looked at patch 2/2 though,
I don't think the same GTK+ solution is applicable to Xwayland, so
short of introducing server-side repeat into Wayland (a possibility
with a wl_keyboard version bump), I'm not sure what we can do here.

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Hans de Goede]

Hi,

On 08-03-16 10:15, Pekka Paalanen wrote:

On Mon, 7 Mar 2016 19:25:54 +0100
Hans de Goede  wrote:


Hi,

On 07-03-16 19:23, Hans de Goede wrote:

Hi,

On 07-03-16 18:44, Olivier Fourdan wrote:

Key repeat is handled by the X server, but for Wayland, the key
press/release events need to be processed and forwarded by the Wayland
compositor first.

If the Wayland compositor get stuck for whatever reason and does not
process the key release event fast enough, this may cause the Xwayland
server to generate spurious key repeat events.

That actually can lead to a complete hang of both the Wayland compositor
and Xwayland as seen in the following GNOME bug:

https://bugzilla.gnome.org/show_bug.cgi?id=762618

Basically, what happens here is that the Wayland compositor takes longer
to actually process the fullscreen/unfullscreen transition than the
repeat delay.

As a result, while the user has released the F11 key that triggers the
fullscreen/unfullscreen transition, the Wayland compositor is stuck
is a graphical operation and cannot process the key release events, which
triggers the auto-repeat in Xwayland, and therefore cause even more
fullscreen transitions. Only way out here is to kill the Xwayland
application and wait for the queued up event to clear out...

While this is arguably a bug in the Wayland compositor, there is no
way that I can think of to guarantee that this cannot happen.

One possiblity to mitigate the problem is to block the key repeat until
we are sure the Wayland compositor is actually processing events.

The idea comes from a similar patch for gtk+ by Ray Strode here:

https://bug757942.bugzilla-attachments.gnome.org/attachment.cgi?id=322876

from bug https://bugzilla.gnome.org/show_bug.cgi?id=757942

While the following patches do fix the issue in my case, they are not
perfect and only a mitigation of the problem (e.g. they could easily be
defeated by a Wayland compositor that would have different priority for
Wayland protocol and input processing), that's why I send these couple
of patches as an RFC for now.


Why not simply rely on the keyrepeat of the compositor and forward repeat
events from it ? That way xwayland will also end up using the compositors
keyrepeat settings which would mean the user does not need to worry
about configuring this in multiple places ?

Or is keyrepeat left to the toolkit under wayland ? In that case please
ignore my comment :)


OK, I've just seen that key-repeat is indeed left to the client / toolkit
under wayland. I'm wondering if that is a deliberate decision or more
of an accident, and if maybe we need a protocol ext for optional compositor
side key-repeat, that seems better then all clients needing to implement
this workaround.


Hi,

it might be an oversight or a desire for simplicity: wl_keyboard key
events are promised to be physically generated by input devices and
never faked by a compositor. Obviously compositor doing the key repeat
does not fit in that scheme. Key events also carry a timestamp of the
physical action gnerating it, and for compositors to fake events, it
would need to be able to fake the timestamp too. I don't know if
anything actually specifies what the clock for input events is, even
for evdev specifically, though I suppose it'd be trivial to just
maintain a timestamp and increment it by the repeat period.


Ack.


However,
that has the race that you might send a repeated event with a timestamp
greater than a following key release event which is sampled by the
kernel or hardware.


No, the whole idea of doing repeat in the server is that the server
can ensure that it has seen the release (e.g. by draining the evdev
fd for the device) before sending a repeat, so the whole idea is to avoid
the race.


You'd have to ask Kristian to be sure. Another question I have no idea
about is how you determine what keys repeat - would the compositor have
to maintain xkbcommon state for each client?


You could mark repeat events as such, actually they should probably
be named repeat-hint-events or something, and then the client can
decide whether it is a good idea to actually listen to the hint
and do key-repeat or not. So basically the idea would be to bring
the timing to the server side which has 2 advantages:

1) Server side this can be done race free
2) 1 common repeat-delay / speed setting for all apps / toolkits

And then let the client decide if it actually wants keyrepeat events
for the key in question or not, and if not simply drop them.


I understand having safeguards against the very busy-loop-flooding
issue that raised these patches is a huge improvement, but at the same
time IMHO there is a "bug" in the compositor that makes it unresponsive
or not fluent. This means the compositor is already having issues
providing a good user experience. That can of course be due to simply
too slow hardware, so something has to stop the flood.

Protecting XWayland against this flooding is probably necessary,

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Pekka Paalanen]

On Mon, 7 Mar 2016 19:25:54 +0100
Hans de Goede  wrote:

> Hi,
> 
> On 07-03-16 19:23, Hans de Goede wrote:
> > Hi,
> >
> > On 07-03-16 18:44, Olivier Fourdan wrote:  
> >> Key repeat is handled by the X server, but for Wayland, the key
> >> press/release events need to be processed and forwarded by the Wayland
> >> compositor first.
> >>
> >> If the Wayland compositor get stuck for whatever reason and does not
> >> process the key release event fast enough, this may cause the Xwayland
> >> server to generate spurious key repeat events.
> >>
> >> That actually can lead to a complete hang of both the Wayland compositor
> >> and Xwayland as seen in the following GNOME bug:
> >>
> >>https://bugzilla.gnome.org/show_bug.cgi?id=762618
> >>
> >> Basically, what happens here is that the Wayland compositor takes longer
> >> to actually process the fullscreen/unfullscreen transition than the
> >> repeat delay.
> >>
> >> As a result, while the user has released the F11 key that triggers the
> >> fullscreen/unfullscreen transition, the Wayland compositor is stuck
> >> is a graphical operation and cannot process the key release events, which
> >> triggers the auto-repeat in Xwayland, and therefore cause even more
> >> fullscreen transitions. Only way out here is to kill the Xwayland
> >> application and wait for the queued up event to clear out...
> >>
> >> While this is arguably a bug in the Wayland compositor, there is no
> >> way that I can think of to guarantee that this cannot happen.
> >>
> >> One possiblity to mitigate the problem is to block the key repeat until
> >> we are sure the Wayland compositor is actually processing events.
> >>
> >> The idea comes from a similar patch for gtk+ by Ray Strode here:
> >>
> >>
> >> https://bug757942.bugzilla-attachments.gnome.org/attachment.cgi?id=322876
> >>
> >> from bug https://bugzilla.gnome.org/show_bug.cgi?id=757942
> >>
> >> While the following patches do fix the issue in my case, they are not
> >> perfect and only a mitigation of the problem (e.g. they could easily be
> >> defeated by a Wayland compositor that would have different priority for
> >> Wayland protocol and input processing), that's why I send these couple
> >> of patches as an RFC for now.  
> >
> > Why not simply rely on the keyrepeat of the compositor and forward repeat
> > events from it ? That way xwayland will also end up using the compositors
> > keyrepeat settings which would mean the user does not need to worry
> > about configuring this in multiple places ?
> >
> > Or is keyrepeat left to the toolkit under wayland ? In that case please
> > ignore my comment :)  
> 
> OK, I've just seen that key-repeat is indeed left to the client / toolkit
> under wayland. I'm wondering if that is a deliberate decision or more
> of an accident, and if maybe we need a protocol ext for optional compositor
> side key-repeat, that seems better then all clients needing to implement
> this workaround.

Hi,

it might be an oversight or a desire for simplicity: wl_keyboard key
events are promised to be physically generated by input devices and
never faked by a compositor. Obviously compositor doing the key repeat
does not fit in that scheme. Key events also carry a timestamp of the
physical action gnerating it, and for compositors to fake events, it
would need to be able to fake the timestamp too. I don't know if
anything actually specifies what the clock for input events is, even
for evdev specifically, though I suppose it'd be trivial to just
maintain a timestamp and increment it by the repeat period. However,
that has the race that you might send a repeated event with a timestamp
greater than a following key release event which is sampled by the
kernel or hardware.

You'd have to ask Kristian to be sure. Another question I have no idea
about is how you determine what keys repeat - would the compositor have
to maintain xkbcommon state for each client?

I understand having safeguards against the very busy-loop-flooding
issue that raised these patches is a huge improvement, but at the same
time IMHO there is a "bug" in the compositor that makes it unresponsive
or not fluent. This means the compositor is already having issues
providing a good user experience. That can of course be due to simply
too slow hardware, so something has to stop the flood.

Protecting XWayland against this flooding is probably necessary,
because XWayland is producing the repeat events which causes X11
clients to do things unthrottled. However, I'm not sure the *same* fix
should be applied to native Wayland clients.

Wouldn't native Wayland clients have means to throttle their state
changes to compositor response already? E.g. switching to/from
fullscreen you have to wait for a configure event, then you draw in new
state, and then you wait for a frame callback to have the new state
show on screen. Only after all that, it makes sense to change state
again. Does this not solve the issue for native Wayland apps?

I'd 

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Hans de Goede]

Hi,

On 07-03-16 19:23, Hans de Goede wrote:

Hi,

On 07-03-16 18:44, Olivier Fourdan wrote:

Key repeat is handled by the X server, but for Wayland, the key
press/release events need to be processed and forwarded by the Wayland
compositor first.

If the Wayland compositor get stuck for whatever reason and does not
process the key release event fast enough, this may cause the Xwayland
server to generate spurious key repeat events.

That actually can lead to a complete hang of both the Wayland compositor
and Xwayland as seen in the following GNOME bug:

   https://bugzilla.gnome.org/show_bug.cgi?id=762618

Basically, what happens here is that the Wayland compositor takes longer
to actually process the fullscreen/unfullscreen transition than the
repeat delay.

As a result, while the user has released the F11 key that triggers the
fullscreen/unfullscreen transition, the Wayland compositor is stuck
is a graphical operation and cannot process the key release events, which
triggers the auto-repeat in Xwayland, and therefore cause even more
fullscreen transitions. Only way out here is to kill the Xwayland
application and wait for the queued up event to clear out...

While this is arguably a bug in the Wayland compositor, there is no
way that I can think of to guarantee that this cannot happen.

One possiblity to mitigate the problem is to block the key repeat until
we are sure the Wayland compositor is actually processing events.

The idea comes from a similar patch for gtk+ by Ray Strode here:

   https://bug757942.bugzilla-attachments.gnome.org/attachment.cgi?id=322876

from bug https://bugzilla.gnome.org/show_bug.cgi?id=757942

While the following patches do fix the issue in my case, they are not
perfect and only a mitigation of the problem (e.g. they could easily be
defeated by a Wayland compositor that would have different priority for
Wayland protocol and input processing), that's why I send these couple
of patches as an RFC for now.


Why not simply rely on the keyrepeat of the compositor and forward repeat
events from it ? That way xwayland will also end up using the compositors
keyrepeat settings which would mean the user does not need to worry
about configuring this in multiple places ?

Or is keyrepeat left to the toolkit under wayland ? In that case please
ignore my comment :)


OK, I've just seen that key-repeat is indeed left to the client / toolkit
under wayland. I'm wondering if that is a deliberate decision or more
of an accident, and if maybe we need a protocol ext for optional compositor
side key-repeat, that seems better then all clients needing to implement
this workaround.

Regards,

Hans
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Hans de Goede]

Hi,

On 07-03-16 18:44, Olivier Fourdan wrote:

Key repeat is handled by the X server, but for Wayland, the key
press/release events need to be processed and forwarded by the Wayland
compositor first.

If the Wayland compositor get stuck for whatever reason and does not
process the key release event fast enough, this may cause the Xwayland
server to generate spurious key repeat events.

That actually can lead to a complete hang of both the Wayland compositor
and Xwayland as seen in the following GNOME bug:

   https://bugzilla.gnome.org/show_bug.cgi?id=762618

Basically, what happens here is that the Wayland compositor takes longer
to actually process the fullscreen/unfullscreen transition than the
repeat delay.

As a result, while the user has released the F11 key that triggers the
fullscreen/unfullscreen transition, the Wayland compositor is stuck
is a graphical operation and cannot process the key release events, which
triggers the auto-repeat in Xwayland, and therefore cause even more
fullscreen transitions. Only way out here is to kill the Xwayland
application and wait for the queued up event to clear out...

While this is arguably a bug in the Wayland compositor, there is no
way that I can think of to guarantee that this cannot happen.

One possiblity to mitigate the problem is to block the key repeat until
we are sure the Wayland compositor is actually processing events.

The idea comes from a similar patch for gtk+ by Ray Strode here:

   https://bug757942.bugzilla-attachments.gnome.org/attachment.cgi?id=322876

from bug https://bugzilla.gnome.org/show_bug.cgi?id=757942

While the following patches do fix the issue in my case, they are not
perfect and only a mitigation of the problem (e.g. they could easily be
defeated by a Wayland compositor that would have different priority for
Wayland protocol and input processing), that's why I send these couple
of patches as an RFC for now.


Why not simply rely on the keyrepeat of the compositor and forward repeat
events from it ? That way xwayland will also end up using the compositors
keyrepeat settings which would mean the user does not need to worry
about configuring this in multiple places ?

Or is keyrepeat left to the toolkit under wayland ? In that case please
ignore my comment :)

Regards,

Hans
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver 0/2] RFC: Sync key repeat with Wayland compositor

[Olivier Fourdan]

Key repeat is handled by the X server, but for Wayland, the key
press/release events need to be processed and forwarded by the Wayland
compositor first.

If the Wayland compositor get stuck for whatever reason and does not
process the key release event fast enough, this may cause the Xwayland
server to generate spurious key repeat events.

That actually can lead to a complete hang of both the Wayland compositor
and Xwayland as seen in the following GNOME bug:

  https://bugzilla.gnome.org/show_bug.cgi?id=762618

Basically, what happens here is that the Wayland compositor takes longer
to actually process the fullscreen/unfullscreen transition than the
repeat delay.

As a result, while the user has released the F11 key that triggers the 
fullscreen/unfullscreen transition, the Wayland compositor is stuck
is a graphical operation and cannot process the key release events, which
triggers the auto-repeat in Xwayland, and therefore cause even more 
fullscreen transitions. Only way out here is to kill the Xwayland
application and wait for the queued up event to clear out...

While this is arguably a bug in the Wayland compositor, there is no
way that I can think of to guarantee that this cannot happen.

One possiblity to mitigate the problem is to block the key repeat until
we are sure the Wayland compositor is actually processing events.

The idea comes from a similar patch for gtk+ by Ray Strode here:

  https://bug757942.bugzilla-attachments.gnome.org/attachment.cgi?id=322876

from bug https://bugzilla.gnome.org/show_bug.cgi?id=757942

While the following patches do fix the issue in my case, they are not
perfect and only a mitigation of the problem (e.g. they could easily be
defeated by a Wayland compositor that would have different priority for
Wayland protocol and input processing), that's why I send these couple
of patches as an RFC for now.

Cheers,
Olivier


Olivier Fourdan (2):
  xkb: add hook to allow/deny AccessX key repeat
  xwayland: add a server sync before repeating keys

 hw/xwayland/xwayland-input.c | 36 
 include/xkbsrv.h |  6 ++
 xkb/xkbAccessX.c |  4 +++-
 3 files changed, 45 insertions(+), 1 deletion(-)

-- 
2.5.0

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel