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 706930 - shaped-texture: Turn blending off when drawing entirely opaque regions
shaped-texture: Turn blending off when drawing entirely opaque regions
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
Depends on:
Blocks:
 
 
Reported: 2013-08-27 20:53 UTC by Jasper St. Pierre (not reading bugmail)
Modified: 2013-08-28 15:17 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Refactor how shapes are done (17.85 KB, patch)
2013-08-27 20:53 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
shaped-texture: Simplify pipeline creation (4.29 KB, patch)
2013-08-27 20:53 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
shaped-texture: Use non-deprecated cogl APIs (4.14 KB, patch)
2013-08-27 20:53 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
shaped-texture: Turn blending off when drawing entirely opaque regions (9.18 KB, patch)
2013-08-27 20:53 UTC, Jasper St. Pierre (not reading bugmail)
reviewed Details | Review
Refactor how shapes are done (18.24 KB, patch)
2013-08-27 21:20 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
shaped-texture: Simplify pipeline creation (5.05 KB, patch)
2013-08-27 21:20 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
shaped-texture: Use non-deprecated cogl APIs (4.17 KB, patch)
2013-08-27 21:20 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
shaped-texture: Turn blending off when drawing entirely opaque regions (9.49 KB, patch)
2013-08-27 21:20 UTC, Jasper St. Pierre (not reading bugmail)
needs-work Details | Review
shaped-texture: Turn blending off when drawing entirely opaque regions (9.49 KB, patch)
2013-08-27 21:56 UTC, Jasper St. Pierre (not reading bugmail)
none Details | Review
shaped-texture: Use non-deprecated cogl APIs (4.74 KB, patch)
2013-08-28 14:17 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review
shaped-texture: Turn blending off when drawing entirely opaque regions (10.23 KB, patch)
2013-08-28 14:17 UTC, Jasper St. Pierre (not reading bugmail)
committed Details | Review

Description Jasper St. Pierre (not reading bugmail) 2013-08-27 20:53:40 UTC
This is something that robclark suggested when trying to bring up
performance under ARM.

This should work in all cases (CSD with _NET_WM_OPAQUE_REGION and
frames with corners), but it should probably be inspected closely.

I haven't verified that we are turning off blending with apitrace
or similar; I should probably do that before this lands.
Comment 1 Jasper St. Pierre (not reading bugmail) 2013-08-27 20:53:42 UTC
Created attachment 253310 [details] [review]
Refactor how shapes are done
Comment 2 Jasper St. Pierre (not reading bugmail) 2013-08-27 20:53:45 UTC
Created attachment 253312 [details] [review]
shaped-texture: Simplify pipeline creation
Comment 3 Jasper St. Pierre (not reading bugmail) 2013-08-27 20:53:47 UTC
Created attachment 253313 [details] [review]
shaped-texture: Use non-deprecated cogl APIs
Comment 4 Jasper St. Pierre (not reading bugmail) 2013-08-27 20:53:50 UTC
Created attachment 253314 [details] [review]
shaped-texture: Turn blending off when drawing entirely opaque regions
Comment 5 drago01 2013-08-27 21:03:58 UTC
(In reply to comment #0)
> This is something that robclark suggested when trying to bring up
> performance under ARM.
> 
> This should work in all cases (CSD with _NET_WM_OPAQUE_REGION and
> frames with corners), but it should probably be inspected closely.
> 
> I haven't verified that we are turning off blending with apitrace
> or similar; I should probably do that before this lands.

I like the idea but I rather have it tested before it lands at this stage of the cycle. 

I will take a closer look tomorrow in the mean time testing (and better commit messages) would be appreciated.
Comment 6 drago01 2013-08-27 21:04:07 UTC
Review of attachment 253310 [details] [review]:

This commit message is useless.
Comment 7 drago01 2013-08-27 21:06:33 UTC
Review of attachment 253312 [details] [review]:

::: src/compositor/meta-shaped-texture.c
@@ +126,3 @@
 
+static CoglPipeline *
+get_unmasked_pipeline (CoglContext *ctx)

Why introduce a wrapper around cogl_pipeline_new ?

@@ +196,3 @@
   if (priv->mask_texture == NULL)
     {
+      pipeline = get_unmasked_pipeline (ctx);

Doesn't this kill the sharing that we had before? Or does cogl does that for us know?
Comment 8 drago01 2013-08-27 21:08:28 UTC
Review of attachment 253313 [details] [review]:

::: src/compositor/meta-shaped-texture.c
@@ +307,3 @@
+      cogl_color_init_from_4ub (&cogl_color, color->red, color->green, color->blue, color->alpha);
+
+      pipeline = cogl_pipeline_new (ctx);

Shouldn't we use the cogl_pipeline_copy pattern here? (To avoid shader recompiles) ?
Comment 9 drago01 2013-08-27 21:13:59 UTC
Review of attachment 253314 [details] [review]:

Just some comments will have a closer look (and do some testing) tomorrow oh and a more verbose commit message wont hurt here as well.

::: src/compositor/meta-shaped-texture.c
@@ +264,3 @@
+
+      if (cairo_region_is_empty (region))
+        goto paint_blended;

You leak region here.

@@ +718,3 @@
+    {
+      cairo_region_destroy (priv->opaque_region);
+      priv->opaque_region = NULL;

No need to set it to NULL here.
Comment 10 drago01 2013-08-27 21:14:35 UTC
(In reply to comment #8)
> Review of attachment 253313 [details] [review]:
> 
> ::: src/compositor/meta-shaped-texture.c
> @@ +307,3 @@
> +      cogl_color_init_from_4ub (&cogl_color, color->red, color->green,
> color->blue, color->alpha);
> +
> +      pipeline = cogl_pipeline_new (ctx);
> 
> Shouldn't we use the cogl_pipeline_copy pattern here? (To avoid shader
> recompiles) ?

Ok apparently cogl does that now.
Comment 11 Jasper St. Pierre (not reading bugmail) 2013-08-27 21:20:45 UTC
Created attachment 253320 [details] [review]
Refactor how shapes are done

As part of Wayland support, we should hold the shape and opaque regions
on the MetaWindow rather than fetching them in the MetaWindowActor, as
this gives us better flexibility as to where the regions are set, and
allows for easier Wayland support.

To make merging easier with the Wayland branch, we also append the _x11
suffix to functions that use the X SHAPE extension to fetch the shaped
regions.
Comment 12 Jasper St. Pierre (not reading bugmail) 2013-08-27 21:20:48 UTC
Created attachment 253321 [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.
Comment 13 Jasper St. Pierre (not reading bugmail) 2013-08-27 21:20:51 UTC
Created attachment 253322 [details] [review]
shaped-texture: Use non-deprecated cogl APIs
Comment 14 Jasper St. Pierre (not reading bugmail) 2013-08-27 21:20:55 UTC
Created attachment 253323 [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,
standard RGB32 frame masks or similar), and draw those rectangles
separately through a different path with blending turned off.
Comment 15 Jasper St. Pierre (not reading bugmail) 2013-08-27 21:25:27 UTC
(In reply to comment #6)
> This commit message is useless.

Fixed.

(In reply to comment #7)
> Why introduce a wrapper around cogl_pipeline_new ?

Just in case we want to add more later. Also, so it matches the other two get_pipeline functions. The compiler will inline it, it's just so the intention of what we're doing is clearer.

> @@ +196,3 @@
>    if (priv->mask_texture == NULL)
>      {
> +      pipeline = get_unmasked_pipeline (ctx);
> 
> Doesn't this kill the sharing that we had before? Or does cogl does that for us
> know?

What sharing? We still use a static template variable that's initialized once.

(In reply to comment #8)
> Shouldn't we use the cogl_pipeline_copy pattern here? (To avoid shader
> recompiles) ?

cogl_pipeline_new is equivalent to copying the default template. The only thing that we could change is if the pick color is constant, which we might be able to hack together, but I think it's a uniform anyway, so there wouldn't be a shader recompile (I think this would use the fixed function pipeline or arbfb anyway)

(In reply to comment #9)
> Just some comments will have a closer look (and do some testing) tomorrow oh
> and a more verbose commit message wont hurt here as well.

Done.

> You leak region here.

Embarrassing leak fixed :)

> No need to set it to NULL here.

This was copied from set_clip_region. I fixed that one too.
Comment 16 drago01 2013-08-27 21:50:44 UTC
Review of attachment 253323 [details] [review]:

::: src/compositor/meta-shaped-texture.c
@@ +264,3 @@
+
+      if (cairo_region_is_empty (region))
+        goto paint_blended;

Still leaks.

@@ +278,3 @@
+      if (priv->clip_region != NULL)
+        {
+          blended_region = cairo_region_copy (priv->clip_region);

Here you use a copy.

@@ +293,3 @@
+
+  if (blended_region == NULL && priv->clip_region != NULL)
+    blended_region = cairo_region_reference (priv->clip_region);

Here just a reference. Any reason for that?
Comment 17 Jasper St. Pierre (not reading bugmail) 2013-08-27 21:54:52 UTC
(In reply to comment #16)
> @@ +278,3 @@
> +      if (priv->clip_region != NULL)
> +        {
> +          blended_region = cairo_region_copy (priv->clip_region);
> 
> Here you use a copy.
> 
> @@ +293,3 @@
> +
> +  if (blended_region == NULL && priv->clip_region != NULL)
> +    blended_region = cairo_region_reference (priv->clip_region);
> 
> Here just a reference. Any reason for that?

Because in one case, we modify the blended region by subtracting out the opaque region we've already painted. In the other, we can simply use the clip region unmodified, so there's no need to copy it.

(I'll reattach the patch, but I just encountered something: (lt-gnome-shell-real:15817): Cogl-WARNING **: (./cogl-pipeline.c:594):_cogl_pipeline_update_layers_cache: code should not be reached)
Comment 18 Jasper St. Pierre (not reading bugmail) 2013-08-27 21:56:09 UTC
Created attachment 253325 [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,
standard RGB32 frame masks or similar), and draw those rectangles
separately through a different path with blending turned off.
Comment 19 Jasper St. Pierre (not reading bugmail) 2013-08-28 14:17:09 UTC
Created attachment 253382 [details] [review]
shaped-texture: Use non-deprecated cogl APIs
Comment 20 Jasper St. Pierre (not reading bugmail) 2013-08-28 14:17:14 UTC
Created attachment 253383 [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,
standard RGB32 frame masks or similar), and draw those rectangles
separately through a different path with blending turned off.
Comment 21 Jasper St. Pierre (not reading bugmail) 2013-08-28 14:18:13 UTC
Now with segfaults and assertion fails fixed!
Comment 22 drago01 2013-08-28 15:06:04 UTC
Review of attachment 253320 [details] [review]:

Looks good, just one small style thing.

::: src/compositor/meta-window-actor.c
@@ +2253,3 @@
     {
+      meta_window_shape_unref (priv->shadow_shape);
+      priv->shadow_shape = NULL;

Use g_clear_pointer()

::: src/core/window.c
@@ +7625,3 @@
+    window->opaque_region = cairo_region_reference (region);
+
+  if (window->display->compositor)

Can this ever be not the case? I mean do we still support running uncomposited?
Comment 23 drago01 2013-08-28 15:07:23 UTC
Review of attachment 253321 [details] [review]:

OK.
Comment 24 drago01 2013-08-28 15:09:33 UTC
Review of attachment 253382 [details] [review]:

OK.
Comment 25 drago01 2013-08-28 15:15:04 UTC
Review of attachment 253383 [details] [review]:

Looks good. Did some test could not spot any noticeable difference on this hardware but no regressions either.
Comment 26 Jasper St. Pierre (not reading bugmail) 2013-08-28 15:17:05 UTC
Attachment 253320 [details] pushed as f1df49a - Refactor how shapes are done
Attachment 253321 [details] pushed as 57258dc - shaped-texture: Simplify pipeline creation
Attachment 253382 [details] pushed as c251ab5 - shaped-texture: Use non-deprecated cogl APIs
Attachment 253383 [details] pushed as ab4c929 - shaped-texture: Turn blending off when drawing entirely opaque regions


Fixed the g_clear_pointer issue in a separate commit.

For the if check around the compositor, most other places have it, and while we do not support running uncomposited, I'd rather not remove a bunch of these checks before a release. We should probably do it at the beginning of next cycle.