Neil, do you think you could look into how this could be done?

Created attachment 257182 [details] [review]
Introduce MetaSurfaceActor for drawing MetaWindowActor content

Instead of having MetaWindowActor only have one single MetaShapedTexture
as actor drawing its content, introduce a new abstract MetaSurfaceActor
that takes care of drawing.

This is one step in the direction to decouple MetaWaylandSurface with a
MetaWindow and MetaWindowActor (except for shell/xdg surfaces) in order
to finally support subsurfaces like features, or any feature where
window is not drawn using a single texture.

The first step, implemented in this patch, is to not have
MetaWindowActor work directly with a shaped texture. There are still
some cases where it simply gets the texture and goes on as before, but
this should be changed by either removing the need of going via
MetaWindowActor or by adding some generic interface to MetaSurfaceActor
that doesn't limit its functionality to one shaped texture.

There should be no visible difference nor after this patch, but
meta_window_actor_get_texture() and meta_surface_actor_get_texture()
should be deprecated when equivalent functionality has been introduced.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Created attachment 257183 [details] [review]
MetaWindowActor: Use allocation changes signals for size changed signals

When a Wayland compositor, simply rely on the clutter actor allocation
changed signal to sync geometry and emit window actor size changed
signals.

Attaching a wl_buffer to a MetaShapedTexture will signal allocation
changed on the corresponding MetaSurfaceActor, which the MetaWindowActor
is listening to.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Created attachment 257184 [details] [review]
MetaWaylandSurface: Attach new buffers directly to MetaShapedTexture

Instead of relying on MetaWindowActor to attach to its (currently) only
single MetaShapedTexture via MetaSurfaceActor, set up a direct
relationship between MetaShapedTexture and MetaWaylandSurface so
MetaWaylandSurface can directly attach new buffers to its corresponding
MetaShapedTexture.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Hi,

I don't know if anyone is working on this, but the three attached patches are work somewhat in the direction of supporting sub-surfaces. They are more or less RFCs, and as I have little experience with mutter I'd like to get input on if this a reasonable way forward (or if there is some branch somewhere already having done something equivalent).

The idea is to abstract away the drawing of MetaWindowActor away from a single texture to a "MetaSurfaceActor" which in the future would hold multiple textures i.e. subsurface textures. The patches tries to move away from having MetaWindowActor knowing about drawing and dimensions having the "surface" actor do that, for example outer bounds when having multiple textures or the total opaque shape etc.

As far as I can tell, basic functionality still works, but there are probably something I haven't thought of. Also, I have not tried non-Wayland compositing mode.

Jonas

Review of attachment 257182 [details] [review]:

Changing status of patch to committed.

Review of attachment 257183 [details] [review]:

This one as well.

Review of attachment 257184 [details] [review]:

But not this one.

Created attachment 266390 [details] [review]
shaped-texture: Use a double when calculating clip

For x defined below, x == -INT32_MAX assuming that the arithmetic
expression actually uses the fpu.

float f = 1.0f;
int32_t x = INT32_MAX * f;

This would result in the calculated clip width/height to be -INT_MAX
if the damage width/height is INT_MAX. To solve this, use a double
variable instead.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Created attachment 266391 [details] [review]
wayland: Report error when trying to stack subsurface incorrectly

Don't allow a client to stack a subsurface next to a subsurface with
another parent, or to a non-parent non-subsurface surface.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Created attachment 266392 [details] [review]
wayland: Make wl_subsurface.set_position properly synchronized

The position set by wl_subsurface.set_position should be applied when
the parent surface invokes wl_surface.commit.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Created attachment 266393 [details] [review]
wayland: Make wl_subsurface.place_(above|below) properly synchronized

The placement set by either wl_subsurface.place_above or
wl_subsurface.place_below should be applied when the parent surface
invokes wl_surface.commit.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Created attachment 266394 [details] [review]
wayland: Support wl_subsurface.set_sync/set_desync

Implement support for synchronous subsurfaces commits. This means that
the client can, by calling wl_subsurface.set_sync, cause its surface
state to be commited not until its parent commits.

This will mean there will be will potentially be one more surface state
(regions, buffer) at the same time: the active surface state, the mutable
pending surface state, and the immutable surface state that was pending
on last surface commit.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Review of attachment 266390 [details] [review]:

This looks good.

Review of attachment 266391 [details] [review]:

It seems like there's two fixes in here -- one for reporting errors, and another for what happens when a parent is destroyed.

Can you split the two out? The errors are OK, the other I don't think is necessary (Clutter should take care of it for us, from how I read the code)

::: src/wayland/meta-wayland-surface.c
@@ +1053,3 @@
+    {
+      wl_resource_post_error (resource, WL_SUBSURFACE_ERROR_BAD_SURFACE,
+                              "wl_subsusrface::place_above: wl_surface@%d is "

"subsurface"

@@ +1054,3 @@
+      wl_resource_post_error (resource, WL_SUBSURFACE_ERROR_BAD_SURFACE,
+                              "wl_subsusrface::place_above: wl_surface@%d is "
+                              "not a valid parnet or sibling",

"parent"

@@ +1080,3 @@
+      wl_resource_post_error (resource, WL_SUBSURFACE_ERROR_BAD_SURFACE,
+                              "wl_subsusrface::place_below: wl_surface@%d is "
+                              "not a valid parnet or sibling",

Same typos.

Review of attachment 266393 [details] [review]:

So, I'm curious: what happens if we do:

  place_above (A, B);
  place_below (A, C);

Is the protocol to apply both these operations, or will one "replace" the other? If I do:

  place_above (A, B);
  place_below (C, A);

Is there any guaranteed order? It seems that right now, we'll just act on things in the order of whichever subsurfaces were first. My attempt at this took a different approach:

  https://git.gnome.org/browse/mutter/commit/?h=wip/subsurfaces&id=2afbf369afe0e7ae96d1a90b8ce8130aa6a30518

If this is how Wayland does it, that's fine, but I'd like to see a clarification of these edge cases in the docs.

::: src/wayland/meta-wayland-surface.c
@@ +1024,3 @@
+        CLUTTER_ACTOR (surface->sub.pending_sibling->surface_actor);
+      parent_actor =
+        clutter_actor_get_parent (CLUTTER_ACTOR (surface->surface_actor));

Can you not wrap these lines, and merge them with the declarations above?

@@ +1031,3 @@
+          clutter_actor_set_child_above_sibling (parent_actor,
+                                                 surface_actor,
+                                                 sibling_actor);

Same here -- not wrapping would make things a lot easier.

@@ +1098,2 @@
 static void
+subsurface_handle_pending_sibling_destroyd (struct wl_listener *listener, void *data)

"destroyed"

@@ +1104,3 @@
+                     sub.pending_sibling_destroy_listener);
+
+  surface->sub.pending_sibling = NULL;

Doesn't this need to also set pending_placement to NONE, else we'll get a segfault?

Review of attachment 266392 [details] [review]:

Looks OK, but if we choose to go with my approach, we should handle set_position in the same way.

Review of attachment 266394 [details] [review]:

This is ridiculously clever: it took me some time to understand it. And still, I like this strategy a lot -- good job!

I think a bit more general documentation about this works would help a lot. It took me a good amount of intense staring and thinking to understand it.

::: src/wayland/meta-wayland-surface.c
@@ +281,3 @@
+
+  g_clear_pointer (&pending->input_region, cairo_region_destroy);
+  g_clear_pointer (&pending->opaque_region, cairo_region_destroy);

Could we simply do this cleanup with:

  double_buffered_state_destroy (pending);
  double_buffered_state_init (pending);

It would be a lot better to see it all in one place.

@@ +348,3 @@
+static void
+move_double_buffered_state (MetaWaylandDoubleBufferedState *from,
+                            MetaWaylandDoubleBufferedState *to)

So, is there anything that would prevent:

  /* Clean up any stale state left behind if a client
   * commits without committing to the parent. */
  double_buffered_state_destroy (to);
  *to = *from;
  double_buffered_state_destroy (from);
  double_buffered_state_init (from);

from working correctly?

@@ +365,3 @@
+  from->dx = from->dy = 0;
+
+  empty_region (to->damage);

Is there ever a realistic case where we would have stale, undestroyed surface state on the subsurface pending that would actually leak?

Would this happen if a client committed twice in a row without committing to the parent?

@@ +383,3 @@
 subsurface_surface_commit (MetaWaylandSurface *surface)
 {
+  if (surface->sub.synchronous)

Comments around this would be extremely nice, explaining the general strategy for how synchronous state works.

::: src/wayland/meta-wayland-surface.h
@@ +86,3 @@
   MetaWaylandCompositor *compositor;
   MetaWaylandBufferReference buffer_ref;
+  struct wl_list frame_callback_list;

This is the one thing I don't understand -- what's the whole point of keeping track of the frame_callback_lists in the surface, rather than just keeping them on the compositor?

Comment on attachment 266390 [details] [review]
shaped-texture: Use a double when calculating clip

Attachment 266390 [details] pushed as be698b5 - shaped-texture: Use a double when calculating clip

(In reply to comment #14)
> Review of attachment 266390 [details] [review]:
> 
> This looks good.

Something I noticed that was related to this (get_clip fix) is that calling clutter_actor_queue_redraw_with_clip() where the clip is has the width/height INT_MAX is broken. Do you know if clutter is meant to be able to deal with such clips? I guess it doesn't really make sense to queue a redraw with a clip with such a size in the first place though, but wasn't sure if a fix should be in clutter or mutter regarding that.

(In reply to comment #15)
> Review of attachment 266391 [details] [review]:
> 
> It seems like there's two fixes in here -- one for reporting errors, and
> another for what happens when a parent is destroyed.
> 
> Can you split the two out? The errors are OK, the other I don't think is
> necessary (Clutter should take care of it for us, from how I read the code)

The parent -> subsurface connection was made in order to have a surface->sub.parent pointer to the MetaWaylandSurface, and it's one commit because "is_valid_sibling()" starts using this new pointer. I guess clutter_actor_get_parent would work as well, it just felt safer and easier to understand to have an explicitly subsurface parent surface pointer.

> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +1053,3 @@
> +    {
> +      wl_resource_post_error (resource, WL_SUBSURFACE_ERROR_BAD_SURFACE,
> +                              "wl_subsusrface::place_above: wl_surface@%d is "
> 
> "subsurface"
> 
> @@ +1054,3 @@
> +      wl_resource_post_error (resource, WL_SUBSURFACE_ERROR_BAD_SURFACE,
> +                              "wl_subsusrface::place_above: wl_surface@%d is "
> +                              "not a valid parnet or sibling",
> 
> "parent"
> 
> @@ +1080,3 @@
> +      wl_resource_post_error (resource, WL_SUBSURFACE_ERROR_BAD_SURFACE,
> +                              "wl_subsusrface::place_below: wl_surface@%d is "
> +                              "not a valid parnet or sibling",
> 
> Same typos.

(In reply to comment #20)
> Something I noticed that was related to this (get_clip fix) is that calling
> clutter_actor_queue_redraw_with_clip() where the clip is has the width/height
> INT_MAX is broken. Do you know if clutter is meant to be able to deal with such
> clips? I guess it doesn't really make sense to queue a redraw with a clip with
> such a size in the first place though, but wasn't sure if a fix should be in
> clutter or mutter regarding that.

That seems like it would be a Clutter bug.

(In reply to comment #16)
> Review of attachment 266393 [details] [review]:
> 
> So, I'm curious: what happens if we do:
> 
>   place_above (A, B);
>   place_below (A, C);
> 
> Is the protocol to apply both these operations, or will one "replace" the
> other? If I do:
> 
>   place_above (A, B);
>   place_below (C, A);
> 
> Is there any guaranteed order? It seems that right now, we'll just act on
> things in the order of whichever subsurfaces were first. My attempt at this
> took a different approach:
> 
>  
> https://git.gnome.org/browse/mutter/commit/?h=wip/subsurfaces&id=2afbf369afe0e7ae96d1a90b8ce8130aa6a30518
> 
> If this is how Wayland does it, that's fine, but I'd like to see a
> clarification of these edge cases in the docs.

I took a look at how it works in weston and it seems to make changes to a pending list, which it then commits. I sent a PATCH updating the protocol to include this semantics, so I guess we'll see how that ends up.

> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +1024,3 @@
> +        CLUTTER_ACTOR (surface->sub.pending_sibling->surface_actor);
> +      parent_actor =
> +        clutter_actor_get_parent (CLUTTER_ACTOR (surface->surface_actor));
> 
> Can you not wrap these lines, and merge them with the declarations above?
> 
> @@ +1031,3 @@
> +          clutter_actor_set_child_above_sibling (parent_actor,
> +                                                 surface_actor,
> +                                                 sibling_actor);
> 
> Same here -- not wrapping would make things a lot easier.
> 
> @@ +1098,2 @@
>  static void
> +subsurface_handle_pending_sibling_destroyd (struct wl_listener *listener, void
> *data)
> 
> "destroyed"
> 
> @@ +1104,3 @@
> +                     sub.pending_sibling_destroy_listener);
> +
> +  surface->sub.pending_sibling = NULL;
> 
> Doesn't this need to also set pending_placement to NONE, else we'll get a
> segfault?

Oops. Makes sense.

(In reply to comment #18)
> Review of attachment 266394 [details] [review]:
> 
> This is ridiculously clever: it took me some time to understand it. And still,
> I like this strategy a lot -- good job!
> 
> I think a bit more general documentation about this works would help a lot. It
> took me a good amount of intense staring and thinking to understand it.

Sure, I'll add some documentation.

> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +281,3 @@
> +
> +  g_clear_pointer (&pending->input_region, cairo_region_destroy);
> +  g_clear_pointer (&pending->opaque_region, cairo_region_destroy);
> 
> Could we simply do this cleanup with:
> 
>   double_buffered_state_destroy (pending);
>   double_buffered_state_init (pending);
> 
> It would be a lot better to see it all in one place.

True. I'll look in to it.

> 
> @@ +348,3 @@
> +static void
> +move_double_buffered_state (MetaWaylandDoubleBufferedState *from,
> +                            MetaWaylandDoubleBufferedState *to)
> 
> So, is there anything that would prevent:
> 
>   /* Clean up any stale state left behind if a client
>    * commits without committing to the parent. */
>   double_buffered_state_destroy (to);
>   *to = *from;
>   double_buffered_state_destroy (from);
>   double_buffered_state_init (from);
> 
> from working correctly?

We need to move wl_list links and lists, and attach a signal listener. Those are a bit more than a "=". Could be possible by changing those from wl_ types to g_list etc maybe, but that could come later.

> 
> @@ +365,3 @@
> +  from->dx = from->dy = 0;
> +
> +  empty_region (to->damage);
> 
> Is there ever a realistic case where we would have stale, undestroyed surface
> state on the subsurface pending that would actually leak?
> 
> Would this happen if a client committed twice in a row without committing to
> the parent?

The damage region seemed to be implemented by never reallocating, always recalculating it. Now the only difference is we have two, one per d-b-state.

> 
> @@ +383,3 @@
>  subsurface_surface_commit (MetaWaylandSurface *surface)
>  {
> +  if (surface->sub.synchronous)
> 
> Comments around this would be extremely nice, explaining the general strategy
> for how synchronous state works.

Sure.

> 
> ::: src/wayland/meta-wayland-surface.h
> @@ +86,3 @@
>    MetaWaylandCompositor *compositor;
>    MetaWaylandBufferReference buffer_ref;
> +  struct wl_list frame_callback_list;
> 
> This is the one thing I don't understand -- what's the whole point of keeping
> track of the frame_callback_lists in the surface, rather than just keeping them
> on the compositor?

Eh, looks like it's something left-over from some experiment. I'll remove it.

Created attachment 267075 [details] [review]
wayland: Report error when trying to stack subsurface incorrectly

Don't allow a client to stack a subsurface next to a subsurface with
another parent, or to a non-parent non-subsurface surface.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Created attachment 267076 [details] [review]
wayland: Make wl_subsurface.set_position properly synchronized

The position set by wl_subsurface.set_position should be applied when
the parent surface invokes wl_surface.commit.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Created attachment 267077 [details] [review]
wayland: Make wl_subsurface.place_(above|below) properly synchronized

The placement set by either wl_subsurface.place_above or
wl_subsurface.place_below should be applied when the parent surface
invokes wl_surface.commit.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Created attachment 267078 [details] [review]
wayland: Support wl_subsurface.set_sync/set_desync

Implement support for synchronous subsurfaces commits. This means that
the client can, by calling wl_subsurface.set_sync, cause its surface
state to be commited not until its parent commits.

This will mean there will be will potentially be one more surface state
(regions, buffer) at the same time: the active surface state, the mutable
pending surface state, and the immutable surface state that was pending
on last surface commit.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Review of attachment 267075 [details] [review]:

OK.

Review of attachment 267076 [details] [review]:

Yep.

Review of attachment 267077 [details] [review]:

Doesn't implement the correct semantics.

Review of attachment 267078 [details] [review]:

Looks good. I'll wait for you to finish up the stacking before I push.

Created attachment 267590 [details] [review]
wayland: Make wl_subsurface.place_(above|below) properly synchronized

The placement set by either wl_subsurface.place_above or
wl_subsurface.place_below should be applied when the parent surface
invokes wl_surface.commit.

Signed-off-by: Jonas Ådahl <jadahl@gmail.com>

Review of attachment 267590 [details] [review]:

::: src/wayland/meta-wayland-surface.c
@@ +1051,3 @@
+            case META_WAYLAND_SUBSURFACE_PLACEMENT_ABOVE:
+              clutter_actor_set_child_above_sibling (
+                parent_actor, surface_actor, sibling_actor);

You can put this on one line.

::: src/wayland/meta-wayland-surface.h
@@ +95,3 @@
     int32_t pending_y;
     gboolean pending_pos;
+    GSList *pending_placement_ops;

You could also use a GArray here so you wouldn't have to allocate each op separately, but it's probably not worth it. We won't have too many ops in the general case.

(In reply to comment #34)
> Review of attachment 267590 [details] [review]:
> 
> ::: src/wayland/meta-wayland-surface.c
> @@ +1051,3 @@
> +            case META_WAYLAND_SUBSURFACE_PLACEMENT_ABOVE:
> +              clutter_actor_set_child_above_sibling (
> +                parent_actor, surface_actor, sibling_actor);
> 
> You can put this on one line.

That would make the line 97 chars long, and on my dev setup that would mean line wrap. I try to keep lines max 80 chars long.

> 
> ::: src/wayland/meta-wayland-surface.h
> @@ +95,3 @@
>      int32_t pending_y;
>      gboolean pending_pos;
> +    GSList *pending_placement_ops;
> 
> You could also use a GArray here so you wouldn't have to allocate each op
> separately, but it's probably not worth it. We won't have too many ops in the
> general case.

Yea, "normally" it should be only one really. If you insist, I can change it; either now or later.

Nah, it was just a comment on the top of my head. If you want to push now, this would be great. Or do you not have git commit access? I forget...

No commit access.

Attachment 267075 [details] pushed as 799c274 - wayland: Report error when trying to stack subsurface incorrectly
Attachment 267076 [details] pushed as 16de7f6 - wayland: Make wl_subsurface.set_position properly synchronized
Attachment 267078 [details] pushed as 4f4b1bf - wayland: Support wl_subsurface.set_sync/set_desync
Attachment 267590 [details] pushed as 9348c9b - wayland: Make wl_subsurface.place_(above|below) properly synchronized