[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 <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.

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,
> 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?

This specific issue yet, but what about scrolling down by keeping
the cursor down key pressed releasing it when you are where you want
to be and then scrolling down some more because of latency in the
release event processing and then the client doing some more repeat
events because it has no idea of this latency ?

Note another solution would be some sort of hartbeat ping from
the server with a high enough frequency for clients to use as a timer
and clients using this as timer source for keyrepeat. This is more or
less what the current patch suggested by Olivier for Xwayland is doing.

> 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?

Regards,

Hans