[RFC,weston] clients: Don't attach a buffer if mouse cursor surface is unchanged

Submitted by Derek Foreman on Feb. 22, 2018, 10:15 p.m.

Details

Message ID 20180222221546.27537-1-derekf@osg.samsung.com
State New
Headers show
Series "clients: Don't attach a buffer if mouse cursor surface is unchanged" ( rev: 1 ) in Wayland

Not browsing as part of any series.

Commit Message

Derek Foreman Feb. 22, 2018, 10:15 p.m.
Keep track of what cusor image buffer is attached to the cursor
surface and avoid re-attaching it if we don't have to.

This isn't just an obviously pointless optimization, it turns all
of toy toolkit into a test case for handling this properly.

Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
---

Continuing my streak of posting unpopular patches, this patch breaks
weston, so I'm not pushing for inclusion, but I think we need to
resolve why it breaks, and fix either weston or wayland documentation
to reflect expected behaviour.

I think this can be attributed to a weston bug, and we should be able to
expect that the compositor will be able to redisplay the surface without
needing to attach a new buffer, and that if the compositor has released
the buffer then it has a kept copy somewhere...

Any other opinions?

 clients/window.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

Patch hide | download patch | download mbox

diff --git a/clients/window.c b/clients/window.c
index 15a86e15..81417bd2 100644
--- a/clients/window.c
+++ b/clients/window.c
@@ -353,6 +353,7 @@  struct input {
 	bool cursor_timer_running;
 	struct task cursor_task;
 	struct wl_surface *pointer_surface;
+	struct wl_buffer *current_cursor_buffer;
 	uint32_t modifiers;
 	uint32_t pointer_enter_serial;
 	uint32_t cursor_serial;
@@ -3769,10 +3770,14 @@  input_set_pointer_image_index(struct input *input, int index)
 	if (!buffer)
 		return;
 
-	wl_surface_attach(input->pointer_surface, buffer, 0, 0);
-	wl_surface_damage(input->pointer_surface, 0, 0,
-			  image->width, image->height);
-	wl_surface_commit(input->pointer_surface);
+	if (buffer != input->current_cursor_buffer) {
+		wl_surface_attach(input->pointer_surface, buffer, 0, 0);
+		wl_surface_damage(input->pointer_surface, 0, 0,
+				  image->width, image->height);
+		wl_surface_commit(input->pointer_surface);
+	}
+
+	input->current_cursor_buffer = buffer;
 	wl_pointer_set_cursor(input->pointer, input->pointer_enter_serial,
 			      input->pointer_surface,
 			      image->hotspot_x, image->hotspot_y);

Comments

Hey Derek,

On Thu, 22 Feb 2018 at 22:16, Derek Foreman <derekf@osg.samsung.com> wrote:
> Keep track of what cusor image buffer is attached to the cursor
> surface and avoid re-attaching it if we don't have to.
>
> This isn't just an obviously pointless optimization, it turns all
> of toy toolkit into a test case for handling this properly.
>
> Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
> ---
>
> Continuing my streak of posting unpopular patches, this patch breaks
> weston, so I'm not pushing for inclusion, but I think we need to
> resolve why it breaks, and fix either weston or wayland documentation
> to reflect expected behaviour.
>
> I think this can be attributed to a weston bug, and we should be able to
> expect that the compositor will be able to redisplay the surface without
> needing to attach a new buffer, and that if the compositor has released
> the buffer then it has a kept copy somewhere...
>
> Any other opinions?

Hmm, I'm really not sure what to think about this.

I take it the problem is that the client sets a particular surface as
the pointer surface, loses focus, sets the same surface as the pointer
surface on re-enter after not changing the content, and then the
content is never shown?

If so, the two solutions would be:
  - the surface content should be preserved across leave/enter; Weston
is buggy for not showing the content on re-enter
  - mouse leave and your surface no longer being shown as a pointer
surface causes the surface to lose that role; Wayland protocol spec
should explicitly state this

I have a very slight lean towards the latter because it more closely
matches my recollection of what the XDG roles do. But that
recollection might be totally wrong, who knows.

Cheers,
Daniel
On 2018-07-22 05:55 AM, Daniel Stone wrote:
> Hey Derek,
> 
> On Thu, 22 Feb 2018 at 22:16, Derek Foreman <derekf@osg.samsung.com> wrote:
>> Keep track of what cusor image buffer is attached to the cursor
>> surface and avoid re-attaching it if we don't have to.
>>
>> This isn't just an obviously pointless optimization, it turns all
>> of toy toolkit into a test case for handling this properly.
>>
>> Signed-off-by: Derek Foreman <derekf@osg.samsung.com>
>> ---
>>
>> Continuing my streak of posting unpopular patches, this patch breaks
>> weston, so I'm not pushing for inclusion, but I think we need to
>> resolve why it breaks, and fix either weston or wayland documentation
>> to reflect expected behaviour.
>>
>> I think this can be attributed to a weston bug, and we should be able to
>> expect that the compositor will be able to redisplay the surface without
>> needing to attach a new buffer, and that if the compositor has released
>> the buffer then it has a kept copy somewhere...
>>
>> Any other opinions?
> 
> Hmm, I'm really not sure what to think about this.
> 
> I take it the problem is that the client sets a particular surface as
> the pointer surface, loses focus, sets the same surface as the pointer
> surface on re-enter after not changing the content, and then the
> content is never shown?

That's my understanding of what I'm seeing, yes.

Note that it only happens when the cursor can be placed in the cursor
plane (ie: it's wayland_shm).

The old cursor continues to be shown - if I move into an EFL client from
the desktop, the desktop cursor arrow is sometimes unchanged.

I do get a surface enter for my pointer surface, though.

I'm reasonably confident the first time I saw this I would get no cursor
at all on re-enter, but now I get the existing cursor.  Though sometimes
it updates to my client cursor, despite no damage or new buffer attach
on the surface.

> If so, the two solutions would be:
>   - the surface content should be preserved across leave/enter; Weston
> is buggy for not showing the content on re-enter
>   - mouse leave and your surface no longer being shown as a pointer
> surface causes the surface to lose that role; Wayland protocol spec
> should explicitly state this

Does losing the role imply the content is removed from the surface? If
so, yes, these are the only two options I can think of.

> I have a very slight lean towards the latter because it more closely
> matches my recollection of what the XDG roles do. But that
> recollection might be totally wrong, who knows.

I have no strong preference, though I won't have to fix my client code
to deal with the former. ;)

> Cheers,
> Daniel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
Hey,

On Tue, 24 Jul 2018 at 22:45, Derek Foreman
<derek.foreman.samsung@gmail.com> wrote:
> On 2018-07-22 05:55 AM, Daniel Stone wrote:
> > I take it the problem is that the client sets a particular surface as
> > the pointer surface, loses focus, sets the same surface as the pointer
> > surface on re-enter after not changing the content, and then the
> > content is never shown?
>
> That's my understanding of what I'm seeing, yes.
>
> Note that it only happens when the cursor can be placed in the cursor
> plane (ie: it's wayland_shm).
>
> The old cursor continues to be shown - if I move into an EFL client from
> the desktop, the desktop cursor arrow is sometimes unchanged.
>
> I do get a surface enter for my pointer surface, though.
>
> I'm reasonably confident the first time I saw this I would get no cursor
> at all on re-enter, but now I get the existing cursor.  Though sometimes
> it updates to my client cursor, despite no damage or new buffer attach
> on the surface.

To be fair, this has changed a fair bit with atomic, so I'm not
surprised it's different in master. But I am a bit disappointed it's
not perfect yet. ;)

Cheers,
Daniel
Hi,

From IRC conversations with krh a long time ago, this is indeed intentional
and the cursor surface should "lose its role" in modern parlance.

The original intention was to prevent glitching of the cursor surface. e.g.
If the left side of the surface has a resize left cursor, you leave, and
hover over the right, you don't want to see the resize left cursor for a
split second before the resize right cursor appears.

The original implementation of Weston respected this and would only change
the cursor on set_cursor calls and would not even remember a per-client
cursor surface. This behavior has probably been lost in numerous reactors
by now.

On Tue, Jul 24, 2018, 2:52 PM Daniel Stone <daniel@fooishbar.org> wrote:

> Hey,
>
> On Tue, 24 Jul 2018 at 22:45, Derek Foreman
> <derek.foreman.samsung@gmail.com> wrote:
> > On 2018-07-22 05:55 AM, Daniel Stone wrote:
> > > I take it the problem is that the client sets a particular surface as
> > > the pointer surface, loses focus, sets the same surface as the pointer
> > > surface on re-enter after not changing the content, and then the
> > > content is never shown?
> >
> > That's my understanding of what I'm seeing, yes.
> >
> > Note that it only happens when the cursor can be placed in the cursor
> > plane (ie: it's wayland_shm).
> >
> > The old cursor continues to be shown - if I move into an EFL client from
> > the desktop, the desktop cursor arrow is sometimes unchanged.
> >
> > I do get a surface enter for my pointer surface, though.
> >
> > I'm reasonably confident the first time I saw this I would get no cursor
> > at all on re-enter, but now I get the existing cursor.  Though sometimes
> > it updates to my client cursor, despite no damage or new buffer attach
> > on the surface.
>
> To be fair, this has changed a fair bit with atomic, so I'm not
> surprised it's different in master. But I am a bit disappointed it's
> not perfect yet. ;)
>
> Cheers,
> Daniel
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
Hi Jasper,

On Thu, 26 Jul 2018 at 03:53, Jasper St. Pierre <jstpierre@mecheye.net> wrote:
> From IRC conversations with krh a long time ago, this is indeed intentional and the cursor surface should "lose its role" in modern parlance.
>
> The original intention was to prevent glitching of the cursor surface. e.g. If the left side of the surface has a resize left cursor, you leave, and hover over the right, you don't want to see the resize left cursor for a split second before the resize right cursor appears.
>
> The original implementation of Weston respected this and would only change the cursor on set_cursor calls and would not even remember a per-client cursor surface. This behavior has probably been lost in numerous reactors by now.

Right, it's not about having the cursor surface stick to the normal
surface, but about having the buffer stick to the cursor surface. The
former definitely isn't controversial, and EFL is re-associating the
cursor surface with the normal surface. What's happening is that they
need to attach/damage/commit to that surface to get the content.

Cheers,
Daniel