[PATCH] xwayland: Only request cursor frame events if the surface is visible

[Pekka Paalanen]

On Tue,  2 Feb 2016 21:06:33 +0100
Rui Matos <tiagomatos at gmail.com> wrote:

> If the wayland compositor hides our cursor surface (e.g. because the
> pointer moved over a different wayland client) before our last
> submitted buffer gets a chance to be displayed, no frame event will be
> sent and thus we end up in a state where we'll never do any more
> cursor updates since we never clear cursor_frame_cb.
> 
> Signed-off-by: Rui Matos <tiagomatos at gmail.com>
> ---
> 
> If you have seen stuck cursors with xwayland windows lately this is
> probably why.
> 
>  hw/xwayland/xwayland-cursor.c |  6 ++++--
>  hw/xwayland/xwayland-input.c  | 30 ++++++++++++++++++++++++++++++
>  hw/xwayland/xwayland.h        |  1 +
>  3 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/xwayland/xwayland-cursor.c b/hw/xwayland/xwayland-cursor.c
> index 76729db..a6ee78f 100644
> --- a/hw/xwayland/xwayland-cursor.c
> +++ b/hw/xwayland/xwayland-cursor.c
> @@ -140,8 +140,10 @@ xwl_seat_set_cursor(struct xwl_seat *xwl_seat)
>                        xwl_seat->x_cursor->bits->width,
>                        xwl_seat->x_cursor->bits->height);
>  
> -    xwl_seat->cursor_frame_cb = wl_surface_frame(xwl_seat->cursor);
> -    wl_callback_add_listener(xwl_seat->cursor_frame_cb, &frame_listener, xwl_seat);
> +    if (xwl_seat->cursor_visible) {
> +        xwl_seat->cursor_frame_cb = wl_surface_frame(xwl_seat->cursor);
> +        wl_callback_add_listener(xwl_seat->cursor_frame_cb, &frame_listener, xwl_seat);
> +    }
>  
>      wl_surface_commit(xwl_seat->cursor);
>  }
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 61ca70b..bdf1fbc 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -781,6 +781,34 @@ static const struct wl_seat_listener seat_listener = {
>  };
>  
>  static void
> +cursor_surface_handle_enter(void *data, struct wl_surface *surface,
> +                            struct wl_output *output)
> +{
> +    struct xwl_seat *xwl_seat = data;
> +    xwl_seat->cursor_visible = TRUE;
> +    if (xwl_seat->cursor_needs_update) {
> +        xwl_seat->cursor_needs_update = FALSE;
> +        xwl_seat_set_cursor(xwl_seat);
> +    }
> +}
> +
> +static void
> +cursor_surface_handle_leave(void *data, struct wl_surface *surface,
> +                            struct wl_output *output)
> +{
> +    struct xwl_seat *xwl_seat = data;
> +    xwl_seat->cursor_visible = FALSE;
> +    if (xwl_seat->cursor_frame_cb)
> +        wl_callback_destroy(xwl_seat->cursor_frame_cb);
> +    xwl_seat->cursor_frame_cb = NULL;
> +}
> +
> +static const struct wl_surface_listener cursor_surface_listener = {
> +    cursor_surface_handle_enter,
> +    cursor_surface_handle_leave
> +};
> +
> +static void
>  create_input_device(struct xwl_screen *xwl_screen, uint32_t id, uint32_t version)
>  {
>      struct xwl_seat *xwl_seat;
> @@ -800,6 +828,8 @@ create_input_device(struct xwl_screen *xwl_screen, uint32_t id, uint32_t version
>      xwl_seat->id = id;
>  
>      xwl_seat->cursor = wl_compositor_create_surface(xwl_screen->compositor);
> +    wl_surface_add_listener(xwl_seat->cursor, &cursor_surface_listener, xwl_seat);
> +
>      wl_seat_add_listener(xwl_seat->seat, &seat_listener, xwl_seat);
>      wl_array_init(&xwl_seat->keys);
>  
> diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
> index a7d7119..a0af07a 100644
> --- a/hw/xwayland/xwayland.h
> +++ b/hw/xwayland/xwayland.h
> @@ -131,6 +131,7 @@ struct xwl_seat {
>      struct wl_surface *cursor;
>      struct wl_callback *cursor_frame_cb;
>      Bool cursor_needs_update;
> +    Bool cursor_visible;
>  
>      struct xorg_list touches;
>  

Hi,

I'm not completely convinced that using the
wl_surface.enter/leave(output) events is totally correct for
determining whether you will get a frame callback or not. I do not
recall anything in the protocol implying or guaranteering this.

Compositors are encouraged to stop issuing frame callbacks or to reduce
their rate if the wl_surface is not visible in a normal way. This can
also happen when the surface is mapped on an output but completely
obscured by other surfaces, though I'm not sure if compositors today
implement that. Stopping frame callbacks does not imply a
wl_surface.leave.

Of course, getting a cursor obscured like that can usually happen only
by another cursor, so it's very hard to trigger, but not impossible.

However, when the cursor surface becomes visible again, I'd expect all
pending frame callbacks to be emitted. So why does the surface not
become visible? I suppose the compositor is expecting the cursor
role to be re-set on the surface, otherwise it won't get mapped again.

How does the original problem of a stuck cursor happen?

Xwayland commits a wl_buffer to a cursor wl_surface with a frame
callback, and the frame callback may never be emitted by the
compositor, right?

Is Xwayland waiting for any previous frame callback to be signalled
before it commits a buffer or re-sets the cursor role on the
wl_surface? Even if the commit and re-set is caused by wl_pointer.enter?

Would it be a better fix to destroy any pending frame callback and
commit and re-set the role unconditionally on wl_pointer.enter?


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: <http://lists.x.org/archives/xorg-devel/attachments/20160203/ec8aad0e/attachment.sig>