[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-...@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[Olivier Fourdan]

The xserver generates the key repeat by itself.

But when used with another server processing inputs first (e.g. a
Wayland compositor), the other server may be busy dealing with some
other things and not queue up key release events in time.

Add a vfunc in XkbSrvInfo to possibly add a check before re-emitting a
keypress event in the AccessX timer handler, so that the key repeat has
a chance to be denied if the server processing the input is not ready.

Signed-off-by: Olivier Fourdan <ofou...@redhat.com>
---

include/xkbsrv.h | 6 ++++++
xkb/xkbAccessX.c | 4 +++-

2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/xkbsrv.h b/include/xkbsrv.h
index cc6307a..05d5a03 100644
--- a/include/xkbsrv.h
+++ b/include/xkbsrv.h
@@ -142,6 +142,10 @@ typedef struct _XkbFilter {
struct _XkbFilter *next;
} XkbFilterRec, *XkbFilterPtr;

+typedef Bool (*XkbSrvCheckRepeatPtr) (DeviceIntPtr dev,
+ struct _XkbSrvInfo * /* xkbi */ ,
+ unsigned /* keycode */);
+
typedef struct _XkbSrvInfo {
XkbStateRec prev_state;
XkbStateRec state;
@@ -189,6 +193,8 @@ typedef struct _XkbSrvInfo {

int szFilters;
XkbFilterPtr filters;
+
+ XkbSrvCheckRepeatPtr checkRepeat;
} XkbSrvInfoRec, *XkbSrvInfoPtr;

#define XkbSLI_IsDefault (1L<<0)
diff --git a/xkb/xkbAccessX.c b/xkb/xkbAccessX.c
index 02e820b..f59a187 100644
--- a/xkb/xkbAccessX.c
+++ b/xkb/xkbAccessX.c
@@ -89,6 +89,7 @@ AccessXInit(DeviceIntPtr keybd)
xkbi->repeatKeyTimer = NULL;
xkbi->krgTimer = NULL;
xkbi->beepTimer = NULL;
+ xkbi->checkRepeat = NULL;
ctrls->repeat_delay = XkbDfltRepeatDelay;
ctrls->repeat_interval = XkbDfltRepeatInterval;
ctrls->debounce_delay = 300;
@@ -317,7 +318,8 @@ AccessXRepeatKeyExpire(OsTimerPtr timer, CARD32 now, void *arg)
if (xkbi->repeatKey == 0)
return 0;

- AccessXKeyboardEvent(dev, ET_KeyPress, xkbi->repeatKey, TRUE);
+ if (xkbi->checkRepeat == NULL || xkbi->checkRepeat (dev, xkbi, xkbi->repeatKey))
+ AccessXKeyboardEvent(dev, ET_KeyPress, xkbi->repeatKey, TRUE);

return xkbi->desc->ctrls->repeat_interval;

[Olivier Fourdan]

Key repeat is handled by the X server, but input events need to be

processed and forwarded by the Wayland compositor first.

Make sure the Wayland compositor is actually processing events, to
avoid repeating keys in xwayland while the Wayland compositor cannot
deal with input events for whatever reason, thus not dispatching key
release events, leading to fictious repeat keys while the use has
already released the key.

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

Signed-off-by: Olivier Fourdan <ofou...@redhat.com>
---

hw/xwayland/xwayland-input.c | 36 ++++++++++++++++++++++++++++++++++++
1 file changed, 36 insertions(+)

diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index f9e3255..ca60505 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -527,6 +527,41 @@ keyboard_handle_modifiers(void *data, struct wl_keyboard *keyboard,
}

static void
+sync_callback(void *data, struct wl_callback *callback, uint32_t serial)
+{
+ Bool *sync_pending = (Bool *) data;
+
+ *sync_pending = FALSE;
+ wl_callback_destroy(callback);
+}
+
+static const struct wl_callback_listener sync_listener = {
+ sync_callback
+};
+
+static Bool
+keyboard_check_repeat (DeviceIntPtr dev, XkbSrvInfoPtr xkbi, unsigned key)
+{
+ static Bool sync_pending;
+ struct xwl_seat *xwl_seat = dev->public.devicePrivate;
+ struct xwl_screen *xwl_screen = xwl_seat->xwl_screen;
+ struct wl_callback *callback;
+
+ if (!sync_pending) {
+ callback = wl_display_sync (xwl_screen->display);
+ wl_callback_add_listener(callback, &sync_listener, &sync_pending);
+ sync_pending = TRUE;
+
+ return TRUE;
+ }
+
+ ErrorF("Key repeat discarded, Wayland compositor doesn't seem to "
+ "be processing events fast enough!\n");
+
+ return FALSE;
+}
+
+static void
keyboard_handle_repeat_info (void *data, struct wl_keyboard *keyboard,
int32_t rate, int32_t delay)
{
@@ -551,6 +586,7 @@ keyboard_handle_repeat_info (void *data, struct wl_keyboard *keyboard,
ctrl->repeat_interval = 1000 / rate;

XkbSetRepeatKeys(dev, -1, AutoRepeatModeOn);
+ dev->key->xkbInfo->checkRepeat = keyboard_check_repeat;
} else
XkbSetRepeatKeys(dev, -1, AutoRepeatModeOff);

[Hans de Goede]

Hi,

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

[Hans de Goede]

Hi,

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.

[Pekka Paalanen]

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

[Hans de Goede]

Hi,

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,

[Daniel Stone]

Hi,

On 7 March 2016 at 17:45, Olivier Fourdan <ofou...@redhat.com> wrote:
> Key repeat is handled by the X server, but input events need to be
> processed and forwarded by the Wayland compositor first.
>
> Make sure the Wayland compositor is actually processing events, to
> avoid repeating keys in xwayland while the Wayland compositor cannot
> deal with input events for whatever reason, thus not dispatching key
> release events, leading to fictious repeat keys while the use has
> already released the key.

I worry about the potential for deadlock here: what happens if the
compositor is blocked on a roundtrip to the X server (which can easily
happen with Mutter I believe), but the server processes the key repeat
timer before the client request? At that point, no-one can make
progress ...

Cheers,
Daniel

[Daniel Stone]

Hi,

On 8 March 2016 at 09:15, Pekka Paalanen <ppaa...@gmail.com> wrote:
> On Mon, 7 Mar 2016 19:25:54 +0100
> Hans de Goede <hdeg...@redhat.com> 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

[Pekka Paalanen]

On Tue, 8 Mar 2016 11:08:45 +0100

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

[Daniel Stone]

Hi,

On 8 March 2016 at 12:07, Pekka Paalanen <ppaa...@gmail.com> 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

[Olivier Fourdan]

[oops, forgot to cc the list as well in my reply]

On 8 March 2016 at 13:06, Olivier Fourdan <fou...@gmail.com> wrote:

Hi

On 8 March 2016 at 12:59, Daniel Stone <dan...@fooishbar.org> wrote:

Hi,

On 7 March 2016 at 17:45, Olivier Fourdan <ofou...@redhat.com> wrote:
> Key repeat is handled by the X server, but input events need to be
> processed and forwarded by the Wayland compositor first.
>
> Make sure the Wayland compositor is actually processing events, to
> avoid repeating keys in xwayland while the Wayland compositor cannot
> deal with input events for whatever reason, thus not dispatching key
> release events, leading to fictious repeat keys while the use has
> already released the key.

I worry about the potential for deadlock here: what happens if the
compositor is blocked on a roundtrip to the X server (which can easily
happen with Mutter I believe), but the server processes the key repeat
timer before the client request? At that point, no-one can make
progress ...

​The key repeat is simply discarded, ie no (fake, repeated) key press event will be emitted but the X server is not blocked and still process other events, so I am not sure how we could end up in a deadlock. Unless I don't understand what you mean.

BTW, this patch is simply a proof of concept, I'll post a slightly less "hack-y-ish" patch is a short while.

Cheers,

Olivier

[Daniel Stone]

Hi,

Ah yes, right you are. More coffee required.

We don't really make any effort to dispatch the queue though, so this
could also trigger just because the X server itself has been blocked
for too long and hasn't dispatched the Wayland event queue, or because
the server has fired the repeat timer before it's processed the
Wayland event queue. But this is easily fixed by just adding a
dispatch before the check.

[Olivier Fourdan]

Key repeat is handled by the X server, but input events need to be
processed and forwarded by the Wayland compositor first.

Make sure the Wayland compositor is actually processing events, to
avoid repeating keys in xwayland while the Wayland compositor cannot
deal with input events for whatever reason, thus not dispatching key
release events, leading to fictious repeat keys while the use has
already released the key.

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

Signed-off-by: Olivier Fourdan <ofou...@redhat.com>
---

v2: A slightly less hack-y-ish version of the patch
Use xwl_seat to store the pending sync, set up callback fpr keyboard
in seat_handle_capabilities() and add a call to _dispatch_pending()
in the callback function to make sure we didn't miss a reply

hw/xwayland/xwayland-input.c | 46 ++++++++++++++++++++++++++++++++++++++++++--
hw/xwayland/xwayland.h | 2 ++
2 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index f9e3255..afe2e27 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -41,7 +41,7 @@
(miPointerPtr)dixLookupPrivate(&(dev)->devPrivates, miPointerPrivKey): \
(miPointerPtr)dixLookupPrivate(&(GetMaster(dev, MASTER_POINTER))->devPrivates, miPointerPrivKey))

-static void
+ static void
xwl_pointer_control(DeviceIntPtr device, PtrCtrl *ctrl)
{
/* Nothing to do, dix handles all settings */
@@ -527,6 +527,43 @@ keyboard_handle_modifiers(void *data, struct wl_keyboard *keyboard,

}

static void
+sync_callback(void *data, struct wl_callback *callback, uint32_t serial)
+{

+ struct xwl_seat *xwl_seat = data;
+
+ xwl_seat->sync_pending = FALSE;

+ wl_callback_destroy(callback);
+}
+
+static const struct wl_callback_listener sync_listener = {
+ sync_callback
+};
+
+static Bool
+keyboard_check_repeat (DeviceIntPtr dev, XkbSrvInfoPtr xkbi, unsigned key)
+{

+ struct xwl_seat *xwl_seat = dev->public.devicePrivate;
+ struct xwl_screen *xwl_screen = xwl_seat->xwl_screen;
+ struct wl_callback *callback;
+

+ /* Make sure we didn't miss a possible reply from the compositor */
+ wl_display_dispatch_pending (xwl_screen->display);
+
+ if (!xwl_seat->sync_pending) {

+ callback = wl_display_sync (xwl_screen->display);

+ wl_callback_add_listener(callback, &sync_listener, xwl_seat);
+ xwl_seat->sync_pending = TRUE;

+
+ return TRUE;
+ }
+
+ ErrorF("Key repeat discarded, Wayland compositor doesn't seem to "
+ "be processing events fast enough!\n");
+
+ return FALSE;
+}
+
+static void
keyboard_handle_repeat_info (void *data, struct wl_keyboard *keyboard,
int32_t rate, int32_t delay)
{

@@ -716,6 +753,7 @@ seat_handle_capabilities(void *data, struct wl_seat *seat,
enum wl_seat_capability caps)
{
struct xwl_seat *xwl_seat = data;
+ DeviceIntPtr master;

if (caps & WL_SEAT_CAPABILITY_POINTER && xwl_seat->wl_pointer == NULL) {
xwl_seat->wl_pointer = wl_seat_get_pointer(seat);
@@ -748,12 +786,16 @@ seat_handle_capabilities(void *data, struct wl_seat *seat,
ActivateDevice(xwl_seat->keyboard, TRUE);
}
EnableDevice(xwl_seat->keyboard, TRUE);
+ master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);
+ master->key->xkbInfo->checkRepeat = keyboard_check_repeat;
} else if (!(caps & WL_SEAT_CAPABILITY_KEYBOARD) && xwl_seat->wl_keyboard) {
wl_keyboard_release(xwl_seat->wl_keyboard);
xwl_seat->wl_keyboard = NULL;

- if (xwl_seat->keyboard)
+ if (xwl_seat->keyboard) {
+ xwl_seat->keyboard->key->xkbInfo->checkRepeat = NULL;
DisableDevice(xwl_seat->keyboard, TRUE);
+ }
}

if (caps & WL_SEAT_CAPABILITY_TOUCH && xwl_seat->wl_touch == NULL) {
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 2a54183..aaf3b40 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -138,6 +138,8 @@ struct xwl_seat {
size_t keymap_size;
char *keymap;
struct wl_surface *keyboard_focus;
+
+ Bool sync_pending;
};

struct xwl_output {
--
2.5.0

[Ray Strode]

Hi,

On Tue, Mar 8, 2016 at 7:09 AM, Daniel Stone <dan...@fooishbar.org> 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

[David Herrmann]

Hi

On Tue, Mar 8, 2016 at 4:41 PM, Ray Strode <half...@gmail.com> wrote:
> On Tue, Mar 8, 2016 at 7:09 AM, Daniel Stone <dan...@fooishbar.org> 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

[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

[Olivier Fourdan]

The xserver generates the key repeat by itself.

But when used with another server processing inputs first (e.g. a
Wayland compositor), the other server may be busy dealing with some
other things and not queue up key release events in time.

Add a vfunc in XkbSrvInfo to possibly add a check before re-emitting a
keypress event in the AccessX timer handler, so that the key repeat has
a chance to be denied if the server processing the input is not ready.

Signed-off-by: Olivier Fourdan <ofou...@redhat.com>
---

v2: No change, just add 2 more patches to the series

[Olivier Fourdan]

Key repeat is handled by the X server, but input events need to be
processed and forwarded by the Wayland compositor first.

Make sure the Wayland compositor is actually processing events, to

avoid repeating keys in Xwayland while the Wayland compositor cannot

deal with input events for whatever reason, thus not dispatching key

release events, leading to repeated keys while the user has already
released the key.

Bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=762618
Signed-off-by: Olivier Fourdan <ofou...@redhat.com>
---
v2: A slightly less hack-y-ish version of the patch
Use xwl_seat to store the pending sync, set up callback fpr keyboard
in seat_handle_capabilities() and add a call to _dispatch_pending()
in the callback function to make sure we didn't miss a reply

v3: Small clean-up,
Remove the call to wl_display_dispatch_pending() that we added
in v2 as we shall do it in two other separate patches to follow.

hw/xwayland/xwayland-input.c | 38 ++++++++++++++++++++++++++++++++++++++
hw/xwayland/xwayland.h | 2 ++
2 files changed, 40 insertions(+)

diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index f9e3255..28d8b54 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -527,6 +527,40 @@ keyboard_handle_modifiers(void *data, struct wl_keyboard *keyboard,

}

static void
+sync_callback(void *data, struct wl_callback *callback, uint32_t serial)
+{
+ struct xwl_seat *xwl_seat = data;
+
+ xwl_seat->sync_pending = FALSE;
+ wl_callback_destroy(callback);
+}
+
+static const struct wl_callback_listener sync_listener = {
+ sync_callback
+};
+
+static Bool
+keyboard_check_repeat (DeviceIntPtr dev, XkbSrvInfoPtr xkbi, unsigned key)
+{
+ struct xwl_seat *xwl_seat = dev->public.devicePrivate;
+ struct xwl_screen *xwl_screen = xwl_seat->xwl_screen;
+ struct wl_callback *callback;
+

+ if (!xwl_seat->sync_pending) {
+ callback = wl_display_sync (xwl_screen->display);
+ wl_callback_add_listener(callback, &sync_listener, xwl_seat);
+ xwl_seat->sync_pending = TRUE;
+
+ return TRUE;
+ }
+
+ ErrorF("Key repeat discarded, Wayland compositor doesn't seem to "
+ "be processing events fast enough!\n");
+
+ return FALSE;
+}
+
+static void
keyboard_handle_repeat_info (void *data, struct wl_keyboard *keyboard,
int32_t rate, int32_t delay)
{

@@ -716,6 +750,7 @@ seat_handle_capabilities(void *data, struct wl_seat *seat,

enum wl_seat_capability caps)
{
struct xwl_seat *xwl_seat = data;
+ DeviceIntPtr master;

if (caps & WL_SEAT_CAPABILITY_POINTER && xwl_seat->wl_pointer == NULL) {
xwl_seat->wl_pointer = wl_seat_get_pointer(seat);

@@ -748,6 +783,9 @@ seat_handle_capabilities(void *data, struct wl_seat *seat,

ActivateDevice(xwl_seat->keyboard, TRUE);
}
EnableDevice(xwl_seat->keyboard, TRUE);
+ master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);

+ if (master)

+ master->key->xkbInfo->checkRepeat = keyboard_check_repeat;
} else if (!(caps & WL_SEAT_CAPABILITY_KEYBOARD) && xwl_seat->wl_keyboard) {
wl_keyboard_release(xwl_seat->wl_keyboard);
xwl_seat->wl_keyboard = NULL;

[Olivier Fourdan]

To be able to reuse some code.

Signed-off-by: Olivier Fourdan <ofou...@redhat.com>
---

hw/xwayland/xwayland.c | 44 ++++++++++++++++++++++++++++++++------------
hw/xwayland/xwayland.h | 2 ++
2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 151e044..4c0ed0d 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -460,14 +460,13 @@ static const struct wl_registry_listener registry_listener = {
};

static void
-socket_handler(int fd, int ready, void *data)
+xwl_read_events (struct xwl_screen *xwl_screen)
{
- struct xwl_screen *xwl_screen = data;
int ret;

ret = wl_display_read_events(xwl_screen->display);
if (ret == -1)
- FatalError("failed to dispatch Wayland events: %s\n", strerror(errno));
+ FatalError("failed to read Wayland events: %s\n", strerror(errno));

xwl_screen->prepare_read = 0;

@@ -477,18 +476,10 @@ socket_handler(int fd, int ready, void *data)
}

static void
-wakeup_handler(void *data, int err, void *pRead)
-{
-}
-
-static void
-block_handler(void *data, OSTimePtr pTimeout, void *pRead)
+xwl_dispatch_events (struct xwl_screen *xwl_screen)
{
- struct xwl_screen *xwl_screen = data;
int ret;

- xwl_screen_post_damage(xwl_screen);
-
while (xwl_screen->prepare_read == 0 &&
wl_display_prepare_read(xwl_screen->display) == -1) {
ret = wl_display_dispatch_pending(xwl_screen->display);
@@ -504,6 +495,35 @@ block_handler(void *data, OSTimePtr pTimeout, void *pRead)
FatalError("failed to write to XWayland fd: %s\n", strerror(errno));
}

+static void
+socket_handler(int fd, int ready, void *data)
+{
+ struct xwl_screen *xwl_screen = data;
+
+ xwl_read_events (xwl_screen);
+}
+
+static void
+wakeup_handler(void *data, int err, void *pRead)
+{
+}
+
+static void
+block_handler(void *data, OSTimePtr pTimeout, void *pRead)
+{
+ struct xwl_screen *xwl_screen = data;
+
+ xwl_screen_post_damage(xwl_screen);
+ xwl_dispatch_events (xwl_screen);
+}
+
+void
+xwl_sync_events (struct xwl_screen *xwl_screen)
+{
+ xwl_read_events (xwl_screen);
+ xwl_dispatch_events (xwl_screen);
+}
+
static CARD32
add_client_fd(OsTimerPtr timer, CARD32 time, void *arg)
{
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index aaf3b40..5fc0cfa 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -155,6 +155,8 @@ struct xwl_output {

struct xwl_pixmap;

+void xwl_sync_events (struct xwl_screen *xwl_screen);
+
Bool xwl_screen_init_cursor(struct xwl_screen *xwl_screen);

struct xwl_screen *xwl_screen_get(ScreenPtr screen);

[Olivier Fourdan]

Read and dispatch pending Wayland events to make sure we do nto miss a
possible reply from the compositor prior to discard a key repeat.

Signed-off-by: Olivier Fourdan <ofou...@redhat.com>
---

hw/xwayland/xwayland-input.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 28d8b54..89f6faf 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -546,6 +546,9 @@ keyboard_check_repeat (DeviceIntPtr dev, XkbSrvInfoPtr xkbi, unsigned key)

struct xwl_screen *xwl_screen = xwl_seat->xwl_screen;

struct wl_callback *callback;

+ /* Make sure we didn't miss a possible reply from the compositor */

+ xwl_sync_events (xwl_screen);
+
if (!xwl_seat->sync_pending) {

callback = wl_display_sync (xwl_screen->display);

wl_callback_add_listener(callback, &sync_listener, xwl_seat);

[Olivier Fourdan]

----- Original Message -----
> Read and dispatch pending Wayland events to make sure we do nto miss a
> possible reply from the compositor prior to discard a key repeat.
>
> Signed-off-by: Olivier Fourdan <ofou...@redhat.com>
> ---
> hw/xwayland/xwayland-input.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 28d8b54..89f6faf 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -546,6 +546,9 @@ keyboard_check_repeat (DeviceIntPtr dev, XkbSrvInfoPtr
> xkbi, unsigned key)
> struct xwl_screen *xwl_screen = xwl_seat->xwl_screen;
> struct wl_callback *callback;
>
> + /* Make sure we didn't miss a possible reply from the compositor */
> + xwl_sync_events (xwl_screen);
> +

Unfortunately it still cause a deadlock apprently:

#0 0x00007fab4b6f93a0 in pthread_cond_wait@@GLIBC_2.3.2 () from /lib64/libpthread.so.0
#1 0x00007fab4df3249b in read_events (display=0x17f7b80) at src/wayland-client.c:1326
#2 wl_display_read_events (display=0x17f7b80) at src/wayland-client.c:1396
#3 0x0000000000423e34 in xwl_read_events (xwl_screen=<optimized out>, xwl_screen=<optimized out>) at xwayland.c:465
#4 0x0000000000424adf in xwl_sync_events (xwl_screen=xwl_screen@entry=0x17f7910) at xwayland.c:524
#5 0x0000000000424e15 in keyboard_check_repeat (dev=<optimized out>, xkbi=<optimized out>, key=<optimized out>) at xwayland-input.c:536
#6 0x0000000000525417 in AccessXRepeatKeyExpire (timer=<optimized out>, now=<optimized out>, arg=0x22c61f0) at xkbAccessX.c:321
#7 0x000000000058b6c3 in DoTimer (timer=0x2923510, now=now@entry=3317763, prev=prev@entry=0x81f6f8 <timers>) at WaitFor.c:420
#8 0x000000000058c36a in WaitForSomething (pClientsReady=pClientsReady@entry=0x17efab0) at WaitFor.c:291
#9 0x00000000005572ee in Dispatch () at dispatch.c:359
#10 0x000000000055b503 in dix_main (argc=10, argv=0x7ffca1cbd528, envp=<optimized out>) at main.c:300
#11 0x00007fab4bfaa0c1 in __libc_start_main () from /lib64/libc.so.6
#12 0x0000000000423599 in _start ()

And that will freeze both the Xwayland server and the compositor, so not good.

[Olivier Fourdan]

To be able to reuse some code.

Signed-off-by: Olivier Fourdan <ofou...@redhat.com>
---

v2: call prepre_read() before read() so we don't end up in a deadlock

hw/xwayland/xwayland.c | 44 ++++++++++++++++++++++++++++++++------------
hw/xwayland/xwayland.h | 2 ++
2 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 151e044..748abdf 100644

+ xwl_dispatch_events (xwl_screen);

+ xwl_read_events (xwl_screen);
+}
+

[Olivier Fourdan]

That is actually fixed with v2 of the patch #3 (ie call prepare_read() before read() to avoid the deadlock as Pekka pointed out on irc).

Cheers,
Olivier

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

[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

[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
events to the X11 client because he has the certitude that this events
are valid.

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

imo, you are wrong here, a server side only key repeating will flood the
client, like games or for scrolling. a client side (i.e. the client
validate each repeat request) will flood the server. My proposal
mitigate both.

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

In my proposal, the client should not expect that repeat are provided
with accurate timing, but the compositor ensure that repeating are
valid. The compositor try his best to follow the requested timing, but
he may be not able to provide them in time, most likely because he is
busy. In that case the client have to wait, and do not flood the server
for key repeating request (that will make the server even more busy)

>
> Cheers,
> Olivier
>

I hope this clarify my point of view :)

Best Regards.

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

[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

[Daniel Stone]

Hi,

On 9 March 2016 at 09:31, Olivier Fourdan <ofou...@redhat.com> wrote:
> @@ -748,6 +783,9 @@ seat_handle_capabilities(void *data, struct wl_seat *seat,
> ActivateDevice(xwl_seat->keyboard, TRUE);
> }
> EnableDevice(xwl_seat->keyboard, TRUE);
> + master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);
> + if (master)
> + master->key->xkbInfo->checkRepeat = keyboard_check_repeat;

This needs to be done on xwl_seat->keyboard as well: both devices
generate events. You can see this with 'xinput test'.

Cheers,
Daniel

[Olivier Fourdan]

Hi Daniel,

Thanks for the feedback!

----- Original Message -----
> Hi,
>
> On 9 March 2016 at 09:31, Olivier Fourdan <ofou...@redhat.com> wrote:
> > @@ -748,6 +783,9 @@ seat_handle_capabilities(void *data, struct wl_seat
> > *seat,
> > ActivateDevice(xwl_seat->keyboard, TRUE);
> > }
> > EnableDevice(xwl_seat->keyboard, TRUE);
> > + master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);
> > + if (master)
> > + master->key->xkbInfo->checkRepeat = keyboard_check_repeat;
>
> This needs to be done on xwl_seat->keyboard as well: both devices
> generate events. You can see this with 'xinput test'.

keyboard_check_repeat() operates on the xwl_seat itself, so I reckon it works for both devices.

If I explicitely set the callback function for both devices as you suggest, key repeat becomes completely erratic as both devices share the same xwl_seat, so doing it on the master device alone seems to work better.

Cheers,
Olivier

[Olivier Fourdan]

Hi

----- Original Message -----
> > On 9 March 2016 at 09:31, Olivier Fourdan <ofou...@redhat.com> wrote:
> > > @@ -748,6 +783,9 @@ seat_handle_capabilities(void *data, struct wl_seat
> > > *seat,
> > > ActivateDevice(xwl_seat->keyboard, TRUE);
> > > }
> > > EnableDevice(xwl_seat->keyboard, TRUE);
> > > + master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);
> > > + if (master)
> > > + master->key->xkbInfo->checkRepeat = keyboard_check_repeat;
> >
> > This needs to be done on xwl_seat->keyboard as well: both devices
> > generate events. You can see this with 'xinput test'.
>
> keyboard_check_repeat() operates on the xwl_seat itself, so I reckon it works
> for both devices.
>
> If I explicitely set the callback function for both devices as you suggest,
> key repeat becomes completely erratic as both devices share the same
> xwl_seat, so doing it on the master device alone seems to work better.

Sorry for the delay...

My problem was that each device is attached to an xwl_seat, bit xwl_seats do have multiple devices, including multiple keyboards.

With your example, both "xwayland-keyboard" and "Virtual core XTEST keyboard" belong to the same xwl_seat, so using the same bool flag for both would obviously not work with my previous implementation, so it needed a bit more surgery^W rework...

The "public.devicePrivate" is used to point to the xwl_seat so we cannot use that to store the "pending sync" flag. The idea is to add a list of devices with pending sync to the wl_seat, and check if a given device is among that list, and remove it once we receive the callback notification for that device.

I will be sending an updated series (4 patches, so it will be easier to review at once) with this implementation in a second.