Re: [PATCH] xwayland: Clear pending cursor frame callbacks on pointer enter

[Adam Jackson]

On Fri, 2016-02-05 at 07:54 +0800, Jonas Ådahl wrote:
> 
> On Feb 5, 2016 12:53 AM, "Rui Matos"  wrote:
> >
> > The last cursor frame we commited before the pointer left one of our
> > surfaces might not have been shown. In that case we'll have a cursor
> > surface frame callback pending which we need to clear so that we can
> > continue submitting new cursor frames.
> >
> > Signed-off-by: Rui Matos 
> > Reviewed-by: Daniel Stone 
> Reviewed-by: Jonas Ådahl 

To ssh://git.freedesktop.org/git/xorg/xserver
   b7d3929..87d5534  master -> master

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xwayland: Clear pending cursor frame callbacks on pointer enter

[Daniel Stone]

On 3 February 2016 at 15:14, Rui Matos  wrote:
> The last cursor frame we commited before the pointer left one of our
> surfaces might not have been shown. In that case we'll have a cursor
> surface frame callback pending which we need to clear so that we can
> continue submitting new cursor frames.
>
> Signed-off-by: Rui Matos 

Ah, that explains a lot ... thanks Rui!

Reviewed-by: Daniel Stone 

Cheers,
Daniel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xwayland: Clear pending cursor frame callbacks on pointer enter

[Jonas Ådahl]

On Feb 5, 2016 12:53 AM, "Rui Matos"  wrote:
>
> The last cursor frame we commited before the pointer left one of our
> surfaces might not have been shown. In that case we'll have a cursor
> surface frame callback pending which we need to clear so that we can
> continue submitting new cursor frames.
>
> Signed-off-by: Rui Matos 
> Reviewed-by: Daniel Stone 

Reviewed-by: Jonas Ådahl 

> ---
>
> v2: as suggested by Jonas, moved the hunk further up to stay close to
> another related hack we already have and also removed the
> xwl_seat_set_cursor() call since CheckMotion() will do that too.
>
> I think Daniel's r-b still stands anyway.
>
>  hw/xwayland/xwayland-input.c | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 61ca70b..d6cbf32 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -251,6 +251,15 @@ pointer_handle_enter(void *data, struct wl_pointer
*pointer,
>  mipointer = MIPOINTER(master);
>  mipointer->pSpriteCursor = (CursorPtr) 1;
>
> +/* The last cursor frame we commited before the pointer left one
> + * of our surfaces might not have been shown. In that case we'll
> + * have a cursor surface frame callback pending which we need to
> + * clear so that we can continue submitting new cursor frames. */
> +if (xwl_seat->cursor_frame_cb) {
> +wl_callback_destroy(xwl_seat->cursor_frame_cb);
> +xwl_seat->cursor_frame_cb = NULL;
> +}
> +
>  CheckMotion(NULL, master);
>
>  /* Ideally, X clients shouldn't see these button releases.  When
> --
> 2.5.0
>
> ___
> xorg-devel@lists.x.org: X.Org development
> Archives: http://lists.x.org/archives/xorg-devel
> Info: http://lists.x.org/mailman/listinfo/xorg-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xwayland: Clear pending cursor frame callbacks on pointer enter

[Pekka Paalanen]

On Wed,  3 Feb 2016 16:14:09 +0100
Rui Matos  wrote:

> The last cursor frame we commited before the pointer left one of our
> surfaces might not have been shown. In that case we'll have a cursor
> surface frame callback pending which we need to clear so that we can
> continue submitting new cursor frames.
> 
> Signed-off-by: Rui Matos 
> ---
> 
> On Wed, Feb 3, 2016 at 9:30 AM, Pekka Paalanen  wrote:
> > 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?  
> 
> Yes, this seems like the proper fix indeed. Thanks,
> 
> Rui
> 
>  hw/xwayland/xwayland-input.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 61ca70b..f9e3255 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -267,6 +267,16 @@ pointer_handle_enter(void *data, struct wl_pointer 
> *pointer,
>  for (i = 0; i < dev->button->numButtons; i++)
>  if (BitIsOn(dev->button->down, i))
>  QueuePointerEvents(dev, ButtonRelease, i, 0, );
> +
> +/* The last cursor frame we commited before the pointer left one
> + * of our surfaces might not have been shown. In that case we'll
> + * have a cursor surface frame callback pending which we need to
> + * clear so that we can continue submitting new cursor frames. */
> +if (xwl_seat->cursor_frame_cb) {
> +wl_callback_destroy(xwl_seat->cursor_frame_cb);
> +xwl_seat->cursor_frame_cb = NULL;
> +xwl_seat_set_cursor(xwl_seat);
> +}
>  }

Hi,

this idea looks fine to me, so consider this:
Acked-by: Pekka Paalanen 

I have to leave the actual review of xserver code for others.


Thanks,
pq


pgpunIm08jpek.pgp
Description: OpenPGP digital signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xwayland: Clear pending cursor frame callbacks on pointer enter

[Jonas Ådahl]

On Wed, Feb 03, 2016 at 05:46:35PM -0800, Bryce Harrington wrote:
> On Wed, Feb 03, 2016 at 04:14:09PM +0100, Rui Matos wrote:
> > The last cursor frame we commited before the pointer left one of our
> > surfaces might not have been shown. In that case we'll have a cursor
> > surface frame callback pending which we need to clear so that we can
> > continue submitting new cursor frames.
> > 
> > Signed-off-by: Rui Matos 
> > ---
> > 
> > On Wed, Feb 3, 2016 at 9:30 AM, Pekka Paalanen  wrote:
> > > 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?
> > 
> > Yes, this seems like the proper fix indeed. Thanks,
> > 
> > Rui
> > 
> >  hw/xwayland/xwayland-input.c | 10 ++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> > index 61ca70b..f9e3255 100644
> > --- a/hw/xwayland/xwayland-input.c
> > +++ b/hw/xwayland/xwayland-input.c
> > @@ -267,6 +267,16 @@ pointer_handle_enter(void *data, struct wl_pointer 
> > *pointer,
> >  for (i = 0; i < dev->button->numButtons; i++)
> >  if (BitIsOn(dev->button->down, i))
> >  QueuePointerEvents(dev, ButtonRelease, i, 0, );
> > +
> > +/* The last cursor frame we commited before the pointer left one
> > + * of our surfaces might not have been shown. In that case we'll
> > + * have a cursor surface frame callback pending which we need to
> > + * clear so that we can continue submitting new cursor frames. */
> > +if (xwl_seat->cursor_frame_cb) {
> > +wl_callback_destroy(xwl_seat->cursor_frame_cb);
> > +xwl_seat->cursor_frame_cb = NULL;
> > +xwl_seat_set_cursor(xwl_seat);
> > +}

If think it'd be better to move this to just below
"mipointer->pSpriteCursor = (CursorPtr) 1;" and skip the
xwl_seat_set_cursor(xwl_seat) call. The "pSpriteCursor = 1" thing is
another hack to avoid skipping to set the cursor on enter, and it'd be
good to keep them close. xwl_seat_set_cursor() will be called as well
indirectly by the "CheckMotion()" call.

> >  }
> >  
> >  static void
> 
> Reviewed-by: Bryce Harrington 
> 
> How noticeable/severe is this problem?  Can this be left to 1.11?

This patch is for the xserver, not weston, so no need to worry about
this for the 1.10 release.


Jonas

> ___
> wayland-devel mailing list
> wayland-de...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] xwayland: Clear pending cursor frame callbacks on pointer enter

[Bryce Harrington]

On Wed, Feb 03, 2016 at 04:14:09PM +0100, Rui Matos wrote:
> The last cursor frame we commited before the pointer left one of our
> surfaces might not have been shown. In that case we'll have a cursor
> surface frame callback pending which we need to clear so that we can
> continue submitting new cursor frames.
> 
> Signed-off-by: Rui Matos 
> ---
> 
> On Wed, Feb 3, 2016 at 9:30 AM, Pekka Paalanen  wrote:
> > 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?
> 
> Yes, this seems like the proper fix indeed. Thanks,
> 
> Rui
> 
>  hw/xwayland/xwayland-input.c | 10 ++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 61ca70b..f9e3255 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -267,6 +267,16 @@ pointer_handle_enter(void *data, struct wl_pointer 
> *pointer,
>  for (i = 0; i < dev->button->numButtons; i++)
>  if (BitIsOn(dev->button->down, i))
>  QueuePointerEvents(dev, ButtonRelease, i, 0, );
> +
> +/* The last cursor frame we commited before the pointer left one
> + * of our surfaces might not have been shown. In that case we'll
> + * have a cursor surface frame callback pending which we need to
> + * clear so that we can continue submitting new cursor frames. */
> +if (xwl_seat->cursor_frame_cb) {
> +wl_callback_destroy(xwl_seat->cursor_frame_cb);
> +xwl_seat->cursor_frame_cb = NULL;
> +xwl_seat_set_cursor(xwl_seat);
> +}
>  }
>  
>  static void

Reviewed-by: Bryce Harrington 

How noticeable/severe is this problem?  Can this be left to 1.11?
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel