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

Tue Mar 8 12:07:17 UTC 2016

On Tue, 8 Mar 2016 11:08:45 +0100
Hans de Goede <hdegoede at redhat.com> wrote:

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

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

Would you be sending a repeat event with all pressed keys? Hmm,
actually the repeat event does not need to send *any* keys, because the
client already knows them. Which means that essentially the compositor
is sending simple timer events any time any key is down.

It does still feel like it would be an event for "the compositor is
running just fine". Doesn't that sound a bit silly, put like that?

FWIW, we cannot remove the client-side repeating support and the rate is
already advertised, but adding a new repeat event is possible.

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

IMHO that falls to the category "your compositor is too slow anyway,
your experience does suffer whatever you do".

The key release event will contain an accurate timestamp in any case,
and if you know your scroll code, you can rewind it to that time and
position, with or without compositor-sent repeat events.

OTOH, how would you implement such scrolling based on server-generated
repeat events, and at the same time avoid jerkyness and velocity
dependence on the compositor responsiveness? I'd assume you would want
these qualities for the scrolling, right?

If you really wanted the scrolling to stop at what the user actually
sees rather than how long the key was down, would you not use frame
callbacks and wp_presentation_feedback to know what the user was seeing
when when he released the key and rewind to that place?

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

Egads, no. We're trying to save power with Wayland, not waste it.

Olivier's patch seems to be requesting a sync on the first repeat, and
then checking the previous sync has completed on the following repeats.
This seems fine for X from Wayland PoV since it is limited to the one
client that actually needs it and when it needs it. Heartbeat very much
not.

If the X server has to sync to ensure that it won't generate too many
repeat events because X11 clients have no reliable way to be throttled
otherwise, then so be it, but extending that chatter to everything
Wayland plus going far over the top is... not good, IMO.

The original designs especially in Weston went very far to try and
avoid waking up processes unless absolutely necessary.

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

I'm getting the feeling that for the two use cases you presented, F11
toggle and scrolling, we'd be better off solving them case by case to
reach the full solutions rather than generic duct tape in the form of
server-side repeat.

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.

In the end, the application has to decide which matters:
- the exact length of time a key was down, or
- synchronizing to what the user is seeing vs. if keys are down

Server-side repeat does not look like an exact solution to either: you
already get the length of time, and there are much better ways to
synchronize to the display.

Unfortunately I don't believe Xwayland can use either, so it has to
just make sure the Wayland compositor is somewhat on track.

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/6f1fac72/attachment.sig>