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

Tue Mar 8 09:15:32 UTC 2016

On Mon, 7 Mar 2016 19:25:54 +0100
Hans de Goede <hdegoede at redhat.com> 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 imagine that scheme to also improve user experience if any of the
compositor or the app are stalling: a user presses F11, nothing happens
(yet). User presses it again. Then the first press gets executed,
followed by the second press. Is the user left with a changed or the
original state? Wouldn't the user prefer the changed state, since the
original state is the only thing he has seen so far, and he did press
the key to change it?

Thanks,
pq
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 811 bytes
Desc: OpenPGP digital signature
URL: <https://lists.x.org/archives/xorg-devel/attachments/20160308/1078b821/attachment.sig>