Created attachment 304025 [details] [review]
meta-cursor-renderer: Add a cursor painted signal

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.

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

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.

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.

Created attachment 318667 [details] [review]
meta-wayland: Use g_get_monotonic_time() instead of gettimeofday()

I think the lack of frame events for cursor surfaces is what causes firefox to not show any cursor under Wayland.

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

Are the patches here still needed / useful ?

Review of attachment 318667 [details] [review]:

This one seems reasonable.

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.

Review of attachment 318667 [details] [review]:

Tihs was already committed independently in https://git.gnome.org/browse/mutter/commit/?id=50099c4c10c9311c6d0deb978d21637c9437c027

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

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.

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.

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

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

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.

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.

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.

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.

Created attachment 327737 [details] [review]
cursor-renderer: Add a cursor painted signal

--

rebased

Created attachment 327738 [details] [review]
surface-role-cursor: Send frame callbacks for backend handled cursors

--

rebased

Created attachment 327739 [details] [review]
surface-role-cursor: Don't send frame callbacks for invisible surfaces

--

rebased

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.

Review of attachment 327737 [details] [review]:

Disregard that comment, I confused myself. Still looks good.

Review of attachment 327738 [details] [review]:

Looks good to me.

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.

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.

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.

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.

Can you take another look at this, Jonas?

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)

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.

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.

Created attachment 334454 [details] [review]
surface-role-cursor: Send frame callbacks for backend handled cursors

--

Changed the variable name as suggested.

Review of attachment 334454 [details] [review]:

Looks good to me.

Review of attachment 334453 [details] [review]:

Looks good to me.

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