After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 749913 - wayland: Send frame callbacks when native hardware cursors get set
wayland: Send frame callbacks when native hardware cursors get set
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: wayland
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2015-05-26 15:14 UTC by Rui Matos
Modified: 2016-08-31 18:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
meta-cursor-renderer: Add a cursor painted signal (2.06 KB, patch)
2015-05-26 15:14 UTC, Rui Matos
none Details | Review
wayland: Send frame callbacks when native hardware cursors get set (1.91 KB, patch)
2015-05-26 15:14 UTC, Rui Matos
none Details | Review
cursor-renderer: Add a cursor painted signal (1.85 KB, patch)
2016-01-10 17:06 UTC, Rui Matos
none Details | Review
wayland-pointer: Send frame callbacks for backend handled cursors (4.32 KB, patch)
2016-01-10 17:06 UTC, Rui Matos
none Details | Review
meta-wayland: Use g_get_monotonic_time() instead of gettimeofday() (1.41 KB, patch)
2016-01-10 17:07 UTC, Rui Matos
committed Details | Review
wayland-pointer: Don't send frame callbacks for invisible surfaces (1.82 KB, patch)
2016-03-04 20:01 UTC, Rui Matos
none Details | Review
cursor-renderer: Add a cursor painted signal (3.52 KB, patch)
2016-03-23 19:07 UTC, Rui Matos
none Details | Review
wayland-pointer: Send frame callbacks for backend handled cursors too (3.44 KB, patch)
2016-03-23 19:07 UTC, Rui Matos
none Details | Review
wayland-pointer: Don't send frame callbacks for invisible surfaces (1.70 KB, patch)
2016-03-23 19:10 UTC, Rui Matos
none Details | Review
cursor-renderer: Add a cursor painted signal (3.40 KB, patch)
2016-05-12 19:39 UTC, Rui Matos
none Details | Review
surface-role-cursor: Send frame callbacks for backend handled cursors (4.88 KB, patch)
2016-05-12 19:39 UTC, Rui Matos
none Details | Review
surface-role-cursor: Don't send frame callbacks for invisible surfaces (1.18 KB, patch)
2016-05-12 19:40 UTC, Rui Matos
none Details | Review
cursor-renderer: Add a cursor painted signal (3.82 KB, patch)
2016-06-24 14:40 UTC, Rui Matos
none Details | Review
surface-role-cursor: Send frame callbacks for backend handled cursors (5.01 KB, patch)
2016-06-24 14:41 UTC, Rui Matos
none Details | Review
cursor-renderer: Add a cursor painted signal (5.27 KB, patch)
2016-08-30 15:44 UTC, Rui Matos
committed Details | Review
surface-role-cursor: Send frame callbacks for backend handled cursors (5.14 KB, patch)
2016-08-30 15:44 UTC, Rui Matos
committed Details | Review

Description Rui Matos 2015-05-26 15:14:01 UTC
I'm not really happy with this but it make animated cursors advance
properly when the client uses frame callbacks.

Questions:

1. Should we move the cursor surface frame callbacks to a different
list? But then we still need to send callbacks from clutter painting
when the cursor isn't a hardware cursor.

2. I'm not sure where to hook this up for the X11 backend.

Comments very welcome.
Comment 1 Rui Matos 2015-05-26 15:14:07 UTC
Created attachment 304025 [details] [review]
meta-cursor-renderer: Add a cursor painted signal
Comment 2 Rui Matos 2015-05-26 15:14:13 UTC
Created attachment 304026 [details] [review]
wayland: Send frame callbacks when native hardware cursors get set

On the native backend with hardware cursor support we can't rely on
clutter painting to send frame callbacks for the cursor surface.
Comment 3 Jonas Ådahl 2015-06-30 08:39:06 UTC
(In reply to Rui Matos from comment #0)
> I'm not really happy with this but it make animated cursors advance
> properly when the client uses frame callbacks.
> 
> Questions:
> 
> 1. Should we move the cursor surface frame callbacks to a different
> list? But then we still need to send callbacks from clutter painting
> when the cursor isn't a hardware cursor.
> 
> 2. I'm not sure where to hook this up for the X11 backend.
> 
> Comments very welcome.

In https://bugzilla.gnome.org/show_bug.cgi?id=744932 I refactored the cursor code in mutter quite a bit to be able to do things like this. In short, more logic is moved into a "cursor" (in those patches called MetaCursorSprite). Currently I use that structure to enable sending wl_surface.enter/leave as well as changing the theme scale so that the cursor looks good when it should. That is in order to support HiDPI properly. I think logic frame callbacks for cursors could be added without much hassle if we'd go with something like that.
Comment 4 Rui Matos 2016-01-10 17:06:32 UTC
Created attachment 318665 [details] [review]
cursor-renderer: Add a cursor painted signal

This signal allows interested parties to be notified of a new cursor
frame being painted and whether it's being painted by the backend
directly or if it's a software rendered cursor frame handled by
clutter.
Comment 5 Rui Matos 2016-01-10 17:06:53 UTC
Created attachment 318666 [details] [review]
wayland-pointer: Send frame callbacks for backend handled cursors

For backend handled cursors, if nothing else changes on the clutter
stage, we end up not sending out frame callbacks since clutter doesn't
draw a new frame.

To fix this, we'll keep cursor surfaces' frame callbacks in a separate
list and either send out the callbacks ourselves or hand the callbacks
to MetaWaylandCompositor's main frame callback list depending on
whether the new cursor frame was handled by the backend.
Comment 6 Rui Matos 2016-01-10 17:07:00 UTC
Created attachment 318667 [details] [review]
meta-wayland: Use g_get_monotonic_time() instead of gettimeofday()
Comment 7 Matthias Clasen 2016-02-02 14:40:49 UTC
I think the lack of frame events for cursor surfaces is what causes firefox to not show any cursor under Wayland.
Comment 8 Jonas Ådahl 2016-02-02 14:51:52 UTC
(In reply to Matthias Clasen from comment #7)
> I think the lack of frame events for cursor surfaces is what causes firefox
> to not show any cursor under Wayland.

When does that happen? I don't think I've ever noticed Firefox not showing any pointer cursor. At least not since the Xwayland cursor change fix landed.
Comment 9 Matthias Clasen 2016-02-25 04:23:37 UTC
Are the patches here still needed / useful ?
Comment 10 Jonas Ådahl 2016-02-25 04:37:30 UTC
Review of attachment 318667 [details] [review]:

This one seems reasonable.
Comment 11 Jonas Ådahl 2016-02-25 04:58:01 UTC
Review of attachment 318665 [details] [review]:

::: src/backends/meta-cursor-renderer.c
@@ +173,3 @@
     queue_redraw (renderer, cursor_sprite);
+
+  g_signal_emit (renderer, signals[CURSOR_PAINTED], 0, handled_by_backend);

Having the emitting here and handled_by_backend state leak is awkward. The better way IMO would be to have the renderer listen for a stage paint signal if it was not handled by the backend and emit a cursor-painted signal if the cursor was painted.

The native backend could, for now, pretend it was painted immediately (like you do in the next patch), but later when we have a tighter coupling of the KMS interaction we should just queue an empty flip event and signal the "cursor-painted" then. Or could we perhaps queue a non-painting clutter stage "paint" that at least will throttle the frame callback? The damage region should be empty which should make it possible to not spend time in stage actors etc.
Comment 12 Matthias Clasen 2016-03-01 16:54:57 UTC
Review of attachment 318667 [details] [review]:

Tihs was already committed independently in https://git.gnome.org/browse/mutter/commit/?id=50099c4c10c9311c6d0deb978d21637c9437c027
Comment 13 Rui Matos 2016-03-04 19:00:54 UTC
(In reply to Jonas Ådahl from comment #11)
> Having the emitting here and handled_by_backend state leak is awkward. The
> better way IMO would be to have the renderer listen for a stage paint signal
> if it was not handled by the backend and emit a cursor-painted signal if the
> cursor was painted.

That sounds like boilerplate for something we can achieve easily like this. In fact:

> later when we have a tighter coupling of the
> KMS interaction we should just queue an empty flip event and signal the
> "cursor-painted" then. Or could we perhaps queue a non-painting clutter
> stage "paint" that at least will throttle the frame callback? The damage
> region should be empty which should make it possible to not spend time in
> stage actors etc.

At this point we won't need this code at all and we can just revert these patches since the frame callbacks can then go back to being sent from the stage's after-paint signal handler.
Comment 14 Rui Matos 2016-03-04 20:01:27 UTC
Created attachment 323127 [details] [review]
wayland-pointer: Don't send frame callbacks for invisible surfaces

According to the specification we should avoid sending frame callbacks
for invisible surfaces.
--

Sending this as a patch on top of the others here, but note that this
can also be done for the current code in master.
Comment 15 Jonas Ådahl 2016-03-10 03:32:05 UTC
Review of attachment 323127 [details] [review]:

::: src/wayland/meta-wayland-pointer.c
@@ +1356,3 @@
+  if (displayed_sprite == NULL ||
+      displayed_sprite != cursor_role->cursor_sprite)
+    return;

I can't see how it makes sense to do this here. Why would you receive a "painted" signal when you were not painted?

(In reply to Rui Matos from comment #13)
> (In reply to Jonas Ådahl from comment #11)
> > Having the emitting here and handled_by_backend state leak is awkward. The
> > better way IMO would be to have the renderer listen for a stage paint signal
> > if it was not handled by the backend and emit a cursor-painted signal if the
> > cursor was painted.
> 
> That sounds like boilerplate for something we can achieve easily like this.
> In fact:
> 
> > later when we have a tighter coupling of the
> > KMS interaction we should just queue an empty flip event and signal the
> > "cursor-painted" then. Or could we perhaps queue a non-painting clutter
> > stage "paint" that at least will throttle the frame callback? The damage
> > region should be empty which should make it possible to not spend time in
> > stage actors etc.
> 
> At this point we won't need this code at all and we can just revert these
> patches since the frame callbacks can then go back to being sent from the
> stage's after-paint signal handler.

Yes, we might be able to remove such boiler plate in the future, but I don't see that as a reason for not leaking the cursor renderer state into MetaWaylandPointer. I'm also wandering if it is safe to reply directly after setting the cursor as done in the patch. Imagine the client does this:

void paint_cursor(surface) {
  wl_surface.attach(surface, buffer);
  callback = wl_surface.frame(surface);
  wl_callback.set_listener(surface); /* Draw again when the attached buffer was drawn. */
  wl_surface.commit();
  wl_pointer.set_cursor(surface);
}

A client doing this is completely valid, but if I'm not misreading the code, this would result in a busy-loop when the cursor is drawn by MetaCursorRendererNative.
Comment 16 Rui Matos 2016-03-10 19:41:25 UTC
(In reply to Jonas Ådahl from comment #15)
> ::: src/wayland/meta-wayland-pointer.c
> @@ +1356,3 @@
> +  if (displayed_sprite == NULL ||
> +      displayed_sprite != cursor_role->cursor_sprite)
> +    return;
> 
> I can't see how it makes sense to do this here. Why would you receive a
> "painted" signal when you were not painted?

With these patches, the painted signal is fired by the backend and this handler runs for all cursor surface instances so each of them needs to check if the cursor that the backend painted was their own.

As the code is in master right now, we have the same issue since each cursor surface instance moves its frame callbacks to the global frame callback list on role_commit() without checking if they are the displayed cursor.

> Yes, we might be able to remove such boiler plate in the future, but I don't
> see that as a reason for not leaking the cursor renderer state into
> MetaWaylandPointer.

At some point, frontends need to speak with backends. Perhaps I didn't understand your proposal ?

> I'm also wandering if it is safe to reply directly after
> setting the cursor as done in the patch. Imagine the client does this:
> 
> void paint_cursor(surface) {
>   wl_surface.attach(surface, buffer);
>   callback = wl_surface.frame(surface);
>   wl_callback.set_listener(surface); /* Draw again when the attached buffer
> was drawn. */
>   wl_surface.commit();
>   wl_pointer.set_cursor(surface);
> }
> 
> A client doing this is completely valid, but if I'm not misreading the code,
> this would result in a busy-loop when the cursor is drawn by
> MetaCursorRendererNative.

Yes, we're just doing what the client requests. It's a busy loop but not a DoS since the compositor can still handle other clients' requests.

Implementing your proposal for a non-painting clutter stage paint would immediately rate limit this. Another (crude) option is to delay sending the frame callbacks to hit a maximum target frame rate.
Comment 17 Jonas Ådahl 2016-03-11 02:54:46 UTC
(In reply to Rui Matos from comment #16)
> (In reply to Jonas Ådahl from comment #15)
> > ::: src/wayland/meta-wayland-pointer.c
> > @@ +1356,3 @@
> > +  if (displayed_sprite == NULL ||
> > +      displayed_sprite != cursor_role->cursor_sprite)
> > +    return;
> > 
> > I can't see how it makes sense to do this here. Why would you receive a
> > "painted" signal when you were not painted?
> 
> With these patches, the painted signal is fired by the backend and this
> handler runs for all cursor surface instances so each of them needs to check
> if the cursor that the backend painted was their own.
> 
> As the code is in master right now, we have the same issue since each cursor
> surface instance moves its frame callbacks to the global frame callback list
> on role_commit() without checking if they are the displayed cursor.

Ah, I see.

> 
> > Yes, we might be able to remove such boiler plate in the future, but I don't
> > see that as a reason for not leaking the cursor renderer state into
> > MetaWaylandPointer.
> 
> At some point, frontends need to speak with backends. Perhaps I didn't
> understand your proposal ?

My point is that you are leaking backend internal state (was it drawn by drm, or will it be drawn by the fallback cursor renderer), and this is what I think shouldn't be leaked out. If the src/wayland/ layer needs to special case depending on what backends/* implementation is used, we have failed to abstract away the backend implementation.

> 
> > I'm also wandering if it is safe to reply directly after
> > setting the cursor as done in the patch. Imagine the client does this:
> > 
> > void paint_cursor(surface) {
> >   wl_surface.attach(surface, buffer);
> >   callback = wl_surface.frame(surface);
> >   wl_callback.set_listener(surface); /* Draw again when the attached buffer
> > was drawn. */
> >   wl_surface.commit();
> >   wl_pointer.set_cursor(surface);
> > }
> > 
> > A client doing this is completely valid, but if I'm not misreading the code,
> > this would result in a busy-loop when the cursor is drawn by
> > MetaCursorRendererNative.
> 
> Yes, we're just doing what the client requests. It's a busy loop but not a
> DoS since the compositor can still handle other clients' requests.

No, the point of wl_surface.frame() is to throttle, and if we reply immediately we don't do what we are required to do, and the client and compositor will both use as much CPU time they are given to update the cursor without anything being throttled to the monitor refresh rate.

> 
> Implementing your proposal for a non-painting clutter stage paint would
> immediately rate limit this. Another (crude) option is to delay sending the
> frame callbacks to hit a maximum target frame rate.

The point of frame callbacks is to throttle, so I'm not sure I understand the problem.
Comment 18 Rui Matos 2016-03-23 19:07:13 UTC
Created attachment 324618 [details] [review]
cursor-renderer: Add a cursor painted signal

This signal allows interested parties to be notified of a new cursor
frame being painted regardless of whether it's being painted by the
backend directly or if it's a software rendered cursor frame handled
by clutter.
Comment 19 Rui Matos 2016-03-23 19:07:42 UTC
Created attachment 324619 [details] [review]
wayland-pointer: Send frame callbacks for backend handled cursors too

For backend handled cursors, if nothing else changes on the clutter
stage, we end up not sending out frame callbacks since clutter doesn't
draw a new frame.

To fix this, we'll keep cursor surfaces' frame callbacks separate from
other surfaces' and trigger them from the new
MetaCursorRenderer::cursor-painted signal which handles both software
and hardware cursors.
Comment 20 Rui Matos 2016-03-23 19:10:12 UTC
Created attachment 324620 [details] [review]
wayland-pointer: Don't send frame callbacks for invisible surfaces

According to the specification we should avoid sending frame callbacks
for invisible surfaces.
--

I think this patch could go in separately as the code in master
contains the same bug so this makes the fix explicit. OTOH the
previous patch isn't fully correct without it so I can squash them if
you prefer.
Comment 21 Rui Matos 2016-03-23 19:13:16 UTC
Addressed the cursor renderer state leak but didn't do anything regarding backend handled frame throttling yet. I think we can address that when we get clutter merged in and we don't need this cursor-painted signal anymore.
Comment 22 Rui Matos 2016-05-12 19:39:36 UTC
Created attachment 327737 [details] [review]
cursor-renderer: Add a cursor painted signal

--

rebased
Comment 23 Rui Matos 2016-05-12 19:39:52 UTC
Created attachment 327738 [details] [review]
surface-role-cursor: Send frame callbacks for backend handled cursors

--

rebased
Comment 24 Rui Matos 2016-05-12 19:40:17 UTC
Created attachment 327739 [details] [review]
surface-role-cursor: Don't send frame callbacks for invisible surfaces

--

rebased
Comment 25 Jonas Ådahl 2016-06-23 04:57:59 UTC
Review of attachment 327737 [details] [review]:

This looks fine to me. An alternative approach would be to have the MetaCursorRenderer handle the signal emitting, and only emit on the displayed cursor. This way you'd have a single repaint func callback per renderer instead of one per cursor.
Comment 26 Jonas Ådahl 2016-06-23 05:02:11 UTC
Review of attachment 327737 [details] [review]:

Disregard that comment, I confused myself. Still looks good.
Comment 27 Jonas Ådahl 2016-06-23 05:06:31 UTC
Review of attachment 327738 [details] [review]:

Looks good to me.
Comment 28 Jonas Ådahl 2016-06-23 05:07:04 UTC
Review of attachment 327739 [details] [review]:

::: src/wayland/meta-wayland-surface-role-cursor.c
@@ +279,3 @@
+  if (displayed_sprite == NULL ||
+      displayed_sprite != cursor_role->cursor_sprite)
+    return;

I think this check should be moved to where the signal is emitted.
Comment 29 Jonas Ådahl 2016-06-23 05:08:49 UTC
Review of attachment 327739 [details] [review]:

::: src/wayland/meta-wayland-surface-role-cursor.c
@@ +279,3 @@
+  if (displayed_sprite == NULL ||
+      displayed_sprite != cursor_role->cursor_sprite)
+    return;

And for that to work I suppose we need to pass the cursor painted in the signal, and check painted_cursor == cursor_role->cursor_sprite instead.
Comment 30 Rui Matos 2016-06-24 14:40:12 UTC
Created attachment 330322 [details] [review]
cursor-renderer: Add a cursor painted signal

--

Your review actually made me realize that for software cursors we'd
possibly miss sending out frame callbacks if more than one cursor got
queued for drawing before a new clutter frame goes out. I fixed that
by adding a queue and emiting the signal for each cursor added to the
queue since the previous clutter frame.
Comment 31 Rui Matos 2016-06-24 14:41:37 UTC
Created attachment 330323 [details] [review]
surface-role-cursor: Send frame callbacks for backend handled cursors

--

I squashed the last patch here since I don't think it makes sense to
introduce a patch with a bug that we already know about just to fix it
in the next one.
Comment 32 Rui Matos 2016-08-29 15:38:30 UTC
Can you take another look at this, Jonas?
Comment 33 Jonas Ådahl 2016-08-30 08:32:25 UTC
Review of attachment 330323 [details] [review]:

Looks good. Just have a minor nit.

::: src/wayland/meta-wayland-surface-role-cursor.c
@@ +40,3 @@
   MetaWaylandBuffer *buffer;
+  struct wl_list frame_callbacks;
+  gulong cursor_painted_id;

nit: cursor_painted_*handler*_id; (for consistency)
Comment 34 Jonas Ådahl 2016-08-30 08:39:10 UTC
Review of attachment 330322 [details] [review]:

::: src/backends/meta-cursor-renderer.c
@@ +44,3 @@
   gboolean handled_by_backend;
+  guint post_paint_func_id;
+  GQueue *software_painted_cursors;

nit: overlay_painted_cursors. since it's still drawn via hardware, just not hw planes.

However, do we really need a list, and won't it potentially be incorrect?

For example if a client sets three different cursors, we'd end up in update_cursor() thrice with three different cursors, all getting their frame callbacks invoked on the next paint even though at most one got painted.

For the hw plane case its different of course, and I think this is good enough for now even though it has a similar issue.

Shouldn't we be able to, in the post_paint func you added, look at displayed_cursor, and emit "cursor-painted" with that, if priv->handled_by_backend == FALSE? We would know that for that frame, it was only that cursor that was painted, and that we painted it ourself (via the overlay).

There will always only ever be at most one cursor displayed per renderer per frame, so if you set multiple cursor on the same renderer, but only one actually ended up on screen, only the one that was shown should get frame callbacks invoked.

Since the tablet code has its own renderer for its cursor, it'd still work if multiple cursors are shown.

@@ +216,3 @@
+    {
+      if (priv->handled_by_backend)
+        g_signal_emit (renderer, signals[CURSOR_PAINTED], 0, cursor_sprite);

nit: its the backend that knows whether this is true now or not, thus it feels more natural to let MetaCursorRenderrNative handle the emitting when the hw plane is used.
Comment 35 Rui Matos 2016-08-30 15:44:13 UTC
Created attachment 334453 [details] [review]
cursor-renderer: Add a cursor painted signal

--

(In reply to Jonas Ådahl from comment #34)
> There will always only ever be at most one cursor displayed per renderer per
> frame, so if you set multiple cursor on the same renderer, but only one
> actually ended up on screen, only the one that was shown should get frame
> callbacks invoked.

Yes, not sure anymore what I was thinking when I added that queue. The
only way for multiple frame callbacks to be pending for a cursor
surface is if a client isn't actually waiting for the callbacks.

> nit: its the backend that knows whether this is true now or not, thus it
> feels more natural to let MetaCursorRenderrNative handle the emitting when
> the hw plane is used.

Sure, in fact doing it there means that if the cursor is outside any
crtc we can skip sending callbacks.
Comment 36 Rui Matos 2016-08-30 15:44:35 UTC
Created attachment 334454 [details] [review]
surface-role-cursor: Send frame callbacks for backend handled cursors

--

Changed the variable name as suggested.
Comment 37 Jonas Ådahl 2016-08-31 02:54:14 UTC
Review of attachment 334454 [details] [review]:

Looks good to me.
Comment 38 Jonas Ådahl 2016-08-31 02:54:38 UTC
Review of attachment 334453 [details] [review]:

Looks good to me.
Comment 39 Rui Matos 2016-08-31 18:15:11 UTC
Attachment 334453 [details] pushed as 2c1d3e5 - cursor-renderer: Add a cursor painted signal
Attachment 334454 [details] pushed as f692c52 - surface-role-cursor: Send frame callbacks for backend handled cursors