GNOME Bugzilla – Bug 706930
shaped-texture: Turn blending off when drawing entirely opaque regions
Last modified: 2013-08-28 15:17:17 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.
Created attachment 253310 [details] [review] Refactor how shapes are done
Created attachment 253312 [details] [review] shaped-texture: Simplify pipeline creation
Created attachment 253313 [details] [review] shaped-texture: Use non-deprecated cogl APIs
Created attachment 253314 [details] [review] shaped-texture: Turn blending off when drawing entirely opaque regions
(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.
Review of attachment 253310 [details] [review]: This commit message is useless.
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?
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) ?
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.
(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.
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.
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.
Created attachment 253322 [details] [review] shaped-texture: Use non-deprecated cogl APIs
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.
(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.
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?
(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)
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.
Created attachment 253382 [details] [review] shaped-texture: Use non-deprecated cogl APIs
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.
Now with segfaults and assertion fails fixed!
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?
Review of attachment 253321 [details] [review]: OK.
Review of attachment 253382 [details] [review]: OK.
Review of attachment 253383 [details] [review]: Looks good. Did some test could not spot any noticeable difference on this hardware but no regressions either.
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.