Created attachment 253454 [details] [review]
window-actor: Mark all Wayland clients as argb32

Created attachment 253455 [details] [review]
wayland: Add support for set_opaque_region / set_input_region

Created attachment 253456 [details] [review]
shaped-texture: Simplify pipeline creation

Split out pipeline creation to a separate function so that we don't
have so much dense code in the paint function itself, and remove some
indentation levels.

Also, don't use our own template for the unmasked pipeline, since it
has nothing different from the default pipeline template.

We also don't store the pipelines anymore since their creation isn't
really helping us; we set the mask texture and paint texture on every
paint anyway.

Created attachment 253457 [details] [review]
shaped-texture: Use non-deprecated cogl APIs

Created attachment 253458 [details] [review]
shaped-texture: Turn blending off when drawing entirely opaque regions

When drawing entirely opaque regions, we traditionally kept blending on
simply because it made the code more convenient and obvious to handle.
However, this can cause lots of performance issues on GPUs that aren't
too powerful, as they have to readback the buffer underneath.

Keep track of the opaque region set by windows (through _NET_WM_OPAQUE_REGION,
Wayland opaque_region hints, standard RGB32 frame masks or similar), and draw
those rectangles separately through a different path with blending turned off.

Review of attachment 253454 [details] [review]:

::: src/compositor/meta-window-actor.c
@@ +363,3 @@
                                   XDamageReportBoundingBox);
 
+  if (window->client_type == META_WINDOW_CLIENT_TYPE_X11)

This check might be wrong, because according to the quirks or bugs in the xwayland driver, we might end up with a buffer that has alpha, even if the client visual doesn't.

@@ +375,3 @@
+    {
+      /* XXX: parse shm formats to determine argb32 */
+      priv->argb32 = TRUE;

I don't see this used in any path interesting for wayland clients.

Review of attachment 253455 [details] [review]:

::: src/compositor/meta-window-actor.c
@@ +2205,3 @@
 
+  meta_shaped_texture_set_mask_texture (META_SHAPED_TEXTURE (priv->actor), NULL);
+  if ((priv->window->shape_region != NULL) || (priv->window->frame != NULL))

I know you're not changing the existing code with this check, but why do we create a mask for all decorated windows?

@@ +2230,1 @@
+  if (priv->window->frame != NULL && priv->window->shape_region != NULL)

input_region?

@@ +2231,2 @@
     {
+      cairo_region_t *client_region = cairo_region_copy (priv->window->input_region);

Copy, not reference?

@@ +2240,2 @@
     }
+  else if (priv->window->shape_region != NULL)

input_region?

@@ +2375,3 @@
     maybe_emit_size_changed (self, surface->buffer_ref.buffer);
+
+  meta_window_actor_invalidate_shadow (self);

Why is this added here?

::: src/wayland/meta-wayland.c
@@ +329,3 @@
 {
+  MetaWaylandSurface *surface = wl_resource_get_user_data (surface_resource);
+  MetaWaylandRegion *region = wl_resource_get_user_data (region_resource);

Region is a bit overloaded here (the resource, the MetaWayland wrapper and the cairo one), maybe meta_region?

@@ +336,3 @@
+
+  if (surface->window)
+    meta_window_set_opaque_region (surface->window, cairo_region_copy (region->region));

Do we assume that Xwayland will never call this? Or should we check for the client type?

Review of attachment 253456 [details] [review]:

Uhm I'm not really sure of the first part, because the default pipeline has no samplers, but we want at least one...

::: src/compositor/meta-shaped-texture.c
@@ +167,3 @@
+      cogl_pipeline_set_layer_combine (template, 1,
+                                       "RGBA = MODULATE (PREVIOUS, TEXTURE[A])",
+                                       NULL);

Should we add layer null textures, so that the template pipeline can cache the sampler objects?

Review of attachment 253457 [details] [review]:

Ok

Review of attachment 253458 [details] [review]:

::: src/compositor/meta-shaped-texture.c
@@ +196,3 @@
+                         CoglPipeline          *pipeline,
+                         cairo_rectangle_int_t *rect,
+                         ClutterActorBox       *alloc)

This function name looks wrong, and doesn't help understanding what is going on.
From what I see, this is drawing a rect, as a portion of a texture sized as alloc.
paint_textured_rect maybe? (and maybe taking the texture size instead of the actor allocation?)

@@ +329,3 @@
+      int i;
+
+      region = cairo_region_copy (priv->clip_region);

What if clip_region is null? You check this case later...

@@ +333,3 @@
+
+      if (cairo_region_is_empty (region))
+        goto paint_blended;

This goto is quite bad, it implies "go to to next step", but really means "go to cleanup for this step". Either move the label out of the if, or rename it.

@@ +358,3 @@
+        }
+
+      cairo_region_subtract (blended_region, priv->opaque_region);

Should we cache the blended region for the common case of painting unclipped?

@@ +400,3 @@
 	{
+          int i;
+          cairo_rectangle_int_t tex_rect = { 0, 0, tex_width, tex_height };

Why did you move this?

@@ +950,3 @@
+
+  if (priv->opaque_region)
+    cairo_region_destroy (priv->opaque_region);

g_clear_pointer(), like you do everywhere else?

(In reply to comment #6)
> This check might be wrong, because according to the quirks or bugs in the
> xwayland driver, we might end up with a buffer that has alpha, even if the
> client visual doesn't.

It's what the existing code was. If it's wrong, that's a separate commit.

> I don't see this used in any path interesting for wayland clients.

argb32 is used to determine whether the opaque region should be taken from the client or not.

(In reply to comment #7)
> +  meta_shaped_texture_set_mask_texture (META_SHAPED_TEXTURE (priv->actor),
> NULL);
> +  if ((priv->window->shape_region != NULL) || (priv->window->frame != NULL))
> 
> I know you're not changing the existing code with this check, but why do we
> create a mask for all decorated windows?

Antialiased window corners. It's on my TODO list to change that to use MetaWindowShape to simply create a corner shape we can reuse.

> @@ +2375,3 @@
>      maybe_emit_size_changed (self, surface->buffer_ref.buffer);
> +
> +  meta_window_actor_invalidate_shadow (self);
> 
> Why is this added here?

Looks like a bad cherry-pick from master.

> Region is a bit overloaded here (the resource, the MetaWayland wrapper and the
> cairo one), maybe meta_region?

OK.

> @@ +336,3 @@
> +
> +  if (surface->window)
> +    meta_window_set_opaque_region (surface->window, cairo_region_copy
> (region->region));
> 
> Do we assume that Xwayland will never call this? Or should we check for the
> client type?

We should certainly double-check, but I don't see how XWayland would call this; it doesn't have that information for ARGB32 windows. If it calls it with the correct shape for RGB32 windows, that doesn't matter; we won't use it.

(In reply to comment #8)
> Should we add layer null textures, so that the template pipeline can cache the
> sampler objects?

I can't find any evidence that the sampler cache obeys the template for anything -- it seems it's always looked up in the sampler cache from the current layer state, and not copied from the parent pipeline.

(In reply to comment #10)
> Review of attachment 253458 [details] [review]:
> This function name looks wrong, and doesn't help understanding what is going
> on.

Then we need to fix it on master, or leave it alone here :)

But yes, given a texture and quad, it crops it and paints it as if it was scissored.

> From what I see, this is drawing a rect, as a portion of a texture sized as
> alloc.
> paint_textured_rect maybe? (and maybe taking the texture size instead of the
> actor allocation?)

We need it to be the allocation; if the actor is scaled down like in the overview, tex width/height will give incorrect results.

> What if clip_region is null? You check this case later...

Whoops. I'll need to fix this on master.

> This goto is quite bad, it implies "go to to next step", but really means "go
> to cleanup for this step". Either move the label out of the if, or rename it.

Any suggestions for names?

> Should we cache the blended region for the common case of painting unclipped?

Painting clipped and unclipped are equally common cases; if I have several unmaximized windows, one obscuring another, then the obscured window will be painted clipped. If I have N stacked windows, only the top window will be painted unclipped... assuming there's a full-stage redraw. If we decide to do a clipped redraw (e.g. we get a damage event) then the clip region won't be NULL and will contain the stage clip.

> Why did you move this?

Because it's not used outside the loop, and it improved readability enough.

> g_clear_pointer(), like you do everywhere else?

Will fix here and on master.

Attachment 253454 [details] pushed as 95e2d26 - window-actor: Mark all Wayland clients as argb32
Attachment 253455 [details] pushed as 2d35e07 - wayland: Add support for set_opaque_region / set_input_region
Attachment 253456 [details] pushed as 69f038f - shaped-texture: Simplify pipeline creation
Attachment 253457 [details] pushed as 452be05 - shaped-texture: Use non-deprecated cogl APIs
Attachment 253458 [details] pushed as 0089b57 - shaped-texture: Turn blending off when drawing entirely opaque regions


approved on irc