Created attachment 322599 [details] [review]
wayland-pointer: Avoid creating an unnecessary texture

MetaWaylandBuffer allows us to get a texture already and in fact the
texture is already created when the surface is commited so we can just
re-use that instead of creating a new one here.

Created attachment 322600 [details] [review]
wayland-surface: Release the buffer only after the surface role commit

We now release shm buffers as soon as possible but the surface's role
commit handler might want to access it too so move the buffer release
after that.

Created attachment 322601 [details] [review]
wayland-pointer: Require a surface commit after a set_cursor request

Cursor surfaces pose us a problem now that we try to release shm
buffers right after copying the data into a texture at surface commit
time.

Our set_cursor request handler assumes that it can access the
surface's current buffer to copy the data down to the cursor
renderer which doesn't work anymore.

We could copy from the texture we already have but that means we'd be
doing a GPU to CPU copy. Alternatively, we could keep a buffer copy in
memory but that'd be extra wasteful, in particular since at surface
commit time we don't know if the surface will end up being used as a
cursor.

The wayland spec isn't clear about whether a surface attach and commit
is necessary after a wl_pointer.set_cursor request to make the
surface's content visible. If we require that to be the case then the
solution is to just not update the cursor sprite and renderer from the
set_cursor handler.

*** Bug 762908 has been marked as a duplicate of this bug. ***

Review of attachment 322601 [details] [review]:

No, we can't do this. I don't think the spec is unclear about this. If it would need a wl_surface.commit it would state that, either explicitly or by referring to the state being "double buffered".

Review of attachment 322599 [details] [review]:

::: src/wayland/meta-wayland-pointer.c
@@ +974,2 @@
       meta_cursor_sprite_set_texture (cursor_sprite,
+                                      meta_wayland_buffer_ensure_texture (surface->buffer),

Do we really need to "ensure" it here? We always generate the texture in apply_pending_state().

@@ +978,3 @@
       meta_cursor_renderer_realize_cursor_from_wl_buffer (cursor_renderer,
                                                           cursor_sprite,
+                                                          surface->buffer->resource);

It does indeed seem like we broke the cursor-sprite when changing the wl_buffer managing. I think the only way to solve this is by increasing the use-count of the MetaWaylandBuffer in the cursor-role-commit handler, and decrease it either if the buffer is replaced or when the cursor surface is destroyed.

It would be good to be able to release it after having realized it, but we need to make sure that the native cursor renderer can reliably keep the old gbm_bo and not try to replace it if realized a second time. We don't support not re-realizing right now.

(In reply to Jonas Ådahl from comment #5)
> Review of attachment 322601 [details] [review] [review]:
> 
> No, we can't do this. I don't think the spec is unclear about this. If it
> would need a wl_surface.commit it would state that, either explicitly or by
> referring to the state being "double buffered".

I disagree. I think that the expectation of all other systems is that it requires a commit for the surface content to go through, unless I misunderstand...

When I introduced roles I also had the expectation that roles were double-buffered state, and I think we all agreed that even set_cursor, etc. were considered double-buffered as well. I would want to double-check this.

(In reply to Jasper St. Pierre from comment #7)
> (In reply to Jonas Ådahl from comment #5)
> > Review of attachment 322601 [details] [review] [review] [review]:
> > 
> > No, we can't do this. I don't think the spec is unclear about this. If it
> > would need a wl_surface.commit it would state that, either explicitly or by
> > referring to the state being "double buffered".
> 
> I disagree. I think that the expectation of all other systems is that it
> requires a commit for the surface content to go through, unless I
> misunderstand...

This is whether wl_surface.commit on the cursor surface is needed to make wl_pointer.set_cursor have any effect. This is not a requirement anywhere, and a quick look through various client implementations, it seems more or less half commit before, and half commit after setting the cursor.

> 
> When I introduced roles I also had the expectation that roles were
> double-buffered state, and I think we all agreed that even set_cursor, etc.
> were considered double-buffered as well. I would want to double-check this.

That is not how I interpreted it, and it would not have been possible to make all role assignment double buffered, since that would have been a backward incompatible change to core protocol.

(In reply to Jonas Ådahl from comment #6)
> Review of attachment 322599 [details] [review] [review]:
> 
> It does indeed seem like we broke the cursor-sprite when changing the
> wl_buffer managing. I think the only way to solve this is by increasing the
> use-count of the MetaWaylandBuffer in the cursor-role-commit handler, and
> decrease it either if the buffer is replaced or when the cursor surface is
> destroyed.

This is not enough actually. We need to always increase the use_count on buffers attached to an unassigned surface role as well.

More or less:

The default (unassigned) role commit handler needs to increase use count.
When assigned, after calling assign() on the role, the use count need to be decreased. This is to keep the wl_buffer alive until the role that needs it is assigned (only the cursor role).

In the cursor assigned and commit role vfuncs the use count needs to be increased. This is to make it possible to realize the hw cursor on wl_pointer.set_cursor().

In the cursor commit role vfunc, the use count need to be decreased when the buffer was replaced. This is to release the buffer that won't be used to realize the hw cursor.

Since this easily crashes mutter, I suggest disabling hw cursors and using the already (in apply_pending_state) created texture if there needs to be a release made before having dealt with the unassigned/cursor-role use-count handling.

Created attachment 322957 [details] [review]
wayland: Don't access the cursor wl_buffer when updating the sprite

We may have released the wl_buffer already when doing this, which means
we should not try to access the wl_buffer content.

Regarding the cursor texture this is not an issue since we can just use
the texture created in apply_pending_state().

The hw cursor however will only be realized if the surface is already
using the the buffer (surface->using_buffer == true). This will, at the
moment, effectively disable hardware cursors for SHM buffers.

Regarding SHM buffer, I wonder, could we not just always create a EGLImage, and create the MetaWaylandBuffer's CoglTexture from that? When realizing the pointer cursor we'd just use GBM_BO_IMPORT_EGL_IMAGE? That way we'd never need to access a SHM wl_buffer after the commit. I reckon we need this anyway in order to do hw plane optimizations.

(In reply to Jonas Ådahl from comment #12)
> Regarding SHM buffer, I wonder, could we not just always create a EGLImage,
> and create the MetaWaylandBuffer's CoglTexture from that? When realizing the
> pointer cursor we'd just use GBM_BO_IMPORT_EGL_IMAGE? That way we'd never
> need to access a SHM wl_buffer after the commit. I reckon we need this
> anyway in order to do hw plane optimizations.

I realized this is more or less what cogl does internally already, without exposing anything that we can use. I guess we could just not destroy the EGLImage after loading, and keep the reference as long as the CoglTexture2D object is alive, while exposing the EGLImageKHR via an API only intended for mutter.

Review of attachment 322957 [details] [review]:

looks good for now. unfortunately I think this patch missed 3.19.91 :-(

Comment on attachment 322957 [details] [review]
wayland: Don't access the cursor wl_buffer when updating the sprite

Attachment 322957 [details] pushed as 96927b3 - wayland: Don't access the cursor wl_buffer when updating the sprite

(In reply to Rui Matos from comment #14)
> Review of attachment 322957 [details] [review] [review]:
> 
> looks good for now. unfortunately I think this patch missed 3.19.91 :-(

That's unfortunate. It'll be a very unstable release then :( Without a fix, my session didn't manage to survive more than a minute.

Created attachment 323942 [details] [review]
wayland: Replace buffer destroy wl_signal with a GObject signal

Don't use the libwayland-* utilities when we have our own that do the
same thing.

Created attachment 323943 [details] [review]
MetaWaylandSurface: Open code buffer use counting

Don't alter the buffer use count in helper functions, as it hides the
counting from where it is relevant. Also always update the using_buffer
field when a buffer is attached, given a rule whether it will be
accessed in the future or not.

This commit also does some minor refactoring of the buffer handling in
apply_pending_state() to make things a bit more obvious.

Created attachment 323944 [details] [review]
MetaWaylandSurface: Keep an extra buffer use count for role-less surfaces

Whether a surface needs to keep the committed wl_buffer un-released
depends on what role the surface gets assigned to. For example a cursor
role may need an unreleased shm buffer in order to create a hw cursor from
it.

In order to support this, keep a separate reference and use count to
the buffer on behalf of the in the future assigned role, and release
those references after the surface was assigned a role. A role that
needs its own references and use counts, must in its assign function
make sure to add those.

Created attachment 323945 [details] [review]
wayland/cursor: Keep a private use count and reference to active buffer

In order for the native cursor renderer to be able to create a hw
cursor in response to wl_pointer.set_cursor(), keep a private use-count
and reference to the active buffer, stopping it from being released
until it is replaced or the surface is destroyed.

Review of attachment 323943 [details] [review]:

::: src/wayland/meta-wayland-surface.c
@@ +656,3 @@
+       * wl_surface is destroyed.
+       */
+      surface->using_buffer = (surface->buffer &&

if surface->buffer is NULL here...

@@ +670,3 @@
+   */
+  if (!surface->using_buffer)
+    meta_wayland_buffer_unref_use_count (surface->buffer);

... we'll end up calling this on NULL

Review of attachment 323945 [details] [review]:

::: src/wayland/meta-wayland-pointer.c
@@ +984,3 @@
+      meta_cursor_renderer_realize_cursor_from_wl_buffer (cursor_renderer,
+                                                          cursor_sprite,
+                                                          buffer);

if it is a shm buffer we could release it here

Looking at this I couldn't stop thinking that we don't need to keep a separate use_count on MetaWaylandBuffer and instead we could use regular GObject references simplifying all this.

Let me know what you thing of the following patches:

Created attachment 324052 [details] [review]
simplify MetaWaylandBuffer usage

Created attachment 324053 [details] [review]
keep role-less surface buffers alive

Created attachment 324054 [details] [review]
keep cursor buffers alive

Review of attachment 324052 [details] [review]:

this doesn't work if two surfaces share the same buffer, iirc. release needs to be done after the copy is done but we won't do it here.

this was my initial approach

(In reply to Jasper St. Pierre from comment #27)
> Review of attachment 324052 [details] [review] [review]:
> 
> this doesn't work if two surfaces share the same buffer, iirc. release needs
> to be done after the copy is done but we won't do it here.
> 
> this was my initial approach

Yea, we get a bit odd with multiple surfaces sharing a buffer and these patches. It pretty much depends on how buffers are attached and committed. If one surface attaches and commits a SHM buffer, then another surface attaches and commits a SHM buffer, we'd get two wl_buffer.release events, but the compositor will also upload the same SHM buffer pixels twice to two separate textures. If both surfaces first attach, then both commits, we'd upload only once, but we'd also only ever send one wl_buffer.release().

Coming to think of this, we are doing things fairly wrong in mutter right now (and with the patches I just attached) regarding multiple surfaces using the same buffer. Right now, if the client does this:

buffer = create_shm_buffer();
surfaceA.attach(buffer);
surfaceB.attach(buffer);
surfaceA.commit();
surfaceB.commit();

mutter will reply with

buffer.release()

but the correct response is

buffer.release()
buffer.release()

This more or less means we need to move the use count we now have in MetaWaylandBuffer into MetaWaylandSurface (or something per-surface at least).

I'll write some patches that moves the use count and wl_buffer.release queuing into MetaWaylandSurface.

Created attachment 324073 [details] [review]
wayland: Move buffer use count to MetaWaylandSurface

Each wl_surface.commit with a newly attached buffer should result in
one wl_buffer.release for the attached buffer. For example attaching
the same buffer to two different surfaces must always result in two
wl_buffer.release events being emitted by the server. The client is
responsible for counting the wl_buffer.release events and be sure to
have received as many release events as it has attached and committed
the buffer, before reusing it.

Created attachment 324074 [details] [review]
MetaWaylandSurface: Keep an extra buffer use count for role-less surfaces

Whether a surface needs to keep the committed wl_buffer un-released
depends on what role the surface gets assigned to. For example a cursor
role may need an unreleased shm buffer in order to create a hw cursor from
it.

In order to support this, keep a separate reference and use count to
the buffer on behalf of the in the future assigned role, and release
those references after the surface was assigned a role. A role that
needs its own references and use counts, must in its assign function
make sure to add those.

Created attachment 324075 [details] [review]
wayland/cursor: Keep a private use count and reference to active buffer

In order for the native cursor renderer to be able to create a hw
cursor in response to wl_pointer.set_cursor(), keep a private use-count
and reference to the active buffer, stopping it from being released
until it is consumed, replaced, or the surface is destroyed.

Created attachment 324227 [details] [review]
wayland: Move buffer use count to MetaWaylandSurface

Each wl_surface.commit with a newly attached buffer should result in
one wl_buffer.release for the attached buffer. For example attaching
the same buffer to two different surfaces must always result in two
wl_buffer.release events being emitted by the server. The client is
responsible for counting the wl_buffer.release events and be sure to
have received as many release events as it has attached and committed
the buffer, before reusing it.

Created attachment 324228 [details] [review]
MetaWaylandSurface: Keep an extra buffer use count for role-less surfaces

Whether a surface needs to keep the committed wl_buffer un-released
depends on what role the surface gets assigned to. For example a cursor
role may need an unreleased shm buffer in order to create a hw cursor from
it.

In order to support this, keep a separate reference and use count to
the buffer on behalf of the in the future assigned role, and release
those references after the surface was assigned a role. A role that
needs its own references and use counts, must in its assign function
make sure to add those.

Replaced the two of the new patches, since I found a couple of issues with them.

Review of attachment 324227 [details] [review]:

::: src/wayland/meta-wayland-surface.c
@@ +713,3 @@
+   */
+  if (!surface->buffer_held && surface->buffer_ref.buffer)
+    meta_wayland_surface_unref_buffer_use_count (surface);

I'm hitting the surface->buffer_ref.use_count != 0 assertion here. It happens with gtk+ tooltips so it seems related to subsurfaces.

@@ +1148,3 @@
 
+  if (surface->buffer_held)
+    meta_wayland_surface_unref_buffer_use_count (surface);

This is the only place where we use ->buffer_held outside of apply_pending_state().

I think it would be clearer if this variable was kept only inside apply_pending_state() and meta_wayland_buffer_finalize() was changed to send a WL_BUFFER_RELEASE if buffer->resource != NULL since there's no case where it makes sense to leave an unreleased wl_buffer when its corresponding MetaWaylandBuffer is finalized.

(In reply to Rui Matos from comment #35)
> Review of attachment 324227 [details] [review] [review]:
> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +713,3 @@
> +   */
> +  if (!surface->buffer_held && surface->buffer_ref.buffer)
> +    meta_wayland_surface_unref_buffer_use_count (surface);
> 
> I'm hitting the surface->buffer_ref.use_count != 0 assertion here. It
> happens with gtk+ tooltips so it seems related to subsurfaces.

Good catch. Fixed by

-  if (!surface->buffer_held && surface->buffer_ref.buffer)
+  if (pending->newly_attached &&
+      !surface->buffer_held && surface->buffer_ref.buffer)
     meta_wayland_surface_unref_buffer_use_count (surface);

> 
> @@ +1148,3 @@
>  
> +  if (surface->buffer_held)
> +    meta_wayland_surface_unref_buffer_use_count (surface);
> 
> This is the only place where we use ->buffer_held outside of
> apply_pending_state().
> 
> I think it would be clearer if this variable was kept only inside
> apply_pending_state() and meta_wayland_buffer_finalize() was changed to send
> a WL_BUFFER_RELEASE if buffer->resource != NULL since there's no case where
> it makes sense to leave an unreleased wl_buffer when its corresponding
> MetaWaylandBuffer is finalized.

The wl_buffer resource has its own reference to the MetaWaylandBuffer. That is how we keep avoid re-uploaing the same pixel twice, would the buffer be attached again. Not until both the wl_buffer is destroyed (by the client) and all references held by MetaWaylandSurface's and its roles will the MetaWaylandBuffer ever finalize.

The buffer_held state really is a per-surface state that in the end doesn't have much to do with the actual buffer, so I don't think we can do without it.

Created attachment 324402 [details] [review]
wayland: Move buffer use count to MetaWaylandSurface

Each wl_surface.commit with a newly attached buffer should result in
one wl_buffer.release for the attached buffer. For example attaching
the same buffer to two different surfaces must always result in two
wl_buffer.release events being emitted by the server. The client is
responsible for counting the wl_buffer.release events and be sure to
have received as many release events as it has attached and committed
the buffer, before reusing it.

Created attachment 324403 [details] [review]
MetaWaylandSurface: Keep an extra buffer use count for role-less surfaces

Whether a surface needs to keep the committed wl_buffer un-released
depends on what role the surface gets assigned to. For example a cursor
role may need an unreleased shm buffer in order to create a hw cursor from
it.

In order to support this, keep a separate reference and use count to
the buffer on behalf of the in the future assigned role, and release
those references after the surface was assigned a role. A role that
needs its own references and use counts, must in its assign function
make sure to add those.

(In reply to Jonas Ådahl from comment #36)
> The wl_buffer resource has its own reference to the MetaWaylandBuffer. That
> is how we keep avoid re-uploaing the same pixel twice, would the buffer be
> attached again. Not until both the wl_buffer is destroyed (by the client)
> and all references held by MetaWaylandSurface's and its roles will the
> MetaWaylandBuffer ever finalize.
> 
> The buffer_held state really is a per-surface state that in the end doesn't
> have much to do with the actual buffer, so I don't think we can do without
> it.

Yeah, I later realized what I was proposing would again break with the same buffer attached to multiple surfaces.

Review of attachment 324402 [details] [review]:

Looks fine now

::: src/wayland/meta-wayland-surface.c
@@ +676,3 @@
+       * is symmetric.
+       */
+      if (surface->buffer_ref.buffer && surface->buffer_held)

I don't think (s->buffer_ref.buffer == NULL && s->buffer_held == TRUE) could ever happen here, could it?

Asking because in wl_surface_destructor() we don't check for that which I think is correct.

@@ +1148,3 @@
     destroy_window (surface);
 
+  if (surface->buffer_held)

Right, s->buffer_ref.buffer can't ever be NULL here

Review of attachment 324403 [details] [review]:

++

Review of attachment 324075 [details] [review]:

right

Review of attachment 323942 [details] [review]:

sure

(In reply to Rui Matos from comment #40)
> Review of attachment 324402 [details] [review] [review]:
> 
> Looks fine now
> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +676,3 @@
> +       * is symmetric.
> +       */
> +      if (surface->buffer_ref.buffer && surface->buffer_held)
> 
> I don't think (s->buffer_ref.buffer == NULL && s->buffer_held == TRUE) could
> ever happen here, could it?
> 
> Asking because in wl_surface_destructor() we don't check for that which I
> think is correct.

You are right. Removed the redundant NULL check. Thanks for the reviews!

Attachment 323942 [details] pushed as aa7bc50 - wayland: Replace buffer destroy wl_signal with a GObject signal
Attachment 324075 [details] pushed as 7173937 - wayland/cursor: Keep a private use count and reference to active buffer
Attachment 324402 [details] pushed as 10a0114 - wayland: Move buffer use count to MetaWaylandSurface
Attachment 324403 [details] pushed as f44238a - MetaWaylandSurface: Keep an extra buffer use count for role-less surfaces