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 735637 - Rewrite background code
Rewrite background code
Status: RESOLVED FIXED
Product: mutter
Classification: Core
Component: general
unspecified
Other All
: Normal normal
: ---
Assigned To: mutter-maint
mutter-maint
: 695928 (view as bug list)
Depends on: 735632
Blocks: 735638
 
 
Reported: 2014-08-28 20:33 UTC by Owen Taylor
Modified: 2014-09-04 18:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Rewrite background code (114.09 KB, patch)
2014-08-28 20:33 UTC, Owen Taylor
committed Details | Review
Here's a fixup for the texture creation, adding docs and also fixing a problem (10.14 KB, patch)
2014-09-02 20:35 UTC, Owen Taylor
committed Details | Review
MetaBackgroundActor: make drawing more like MetaShapedTexture (5.09 KB, patch)
2014-09-02 21:26 UTC, Owen Taylor
committed Details | Review
Cleanups (5.31 KB, patch)
2014-09-02 21:37 UTC, Owen Taylor
committed Details | Review
MetaBackgroundActor: match total dimming if GLSL is not present (1.49 KB, patch)
2014-09-02 22:11 UTC, Owen Taylor
committed Details | Review
MetaBackground: add properties to set vignette settings (7.96 KB, patch)
2014-09-03 16:24 UTC, Owen Taylor
committed Details | Review

Description Owen Taylor 2014-08-28 20:33:49 UTC
The old requirement that multiple MetaBackgroundActor objects be
layered on top of each to produce blended backgrounds resulted in
extremely inefficient drawing since the entire framebuffer had
to be read and written multiple times.

 * Replace the MetaBackground ClutterContent with a plain GObject
   that serves to hold the background parameters and prerender
   textures to be used to draw the background. It handles
   colors, gradients, and blended images, but does not handle
   vignetting

 * Add vignetting to MetaBackgroundActor directly.

 * Add MetaBackgroundImage and MetaBackgroundImageCache to allow
   multiple MetaBackground objects to share the same images

By removing the usage of ClutterContent, the following optimizations
were easy to add:

 Blending is turned off when the actor is fully opaque
 Nearest-neighbour filtering is used when drawing 1:1

The GLSL vignette code is slightly improved to use a vertex shader
snippet for computing the texture coordinate => position in actor
mapping.
Comment 1 Owen Taylor 2014-08-28 20:33:52 UTC
Created attachment 284753 [details] [review]
Rewrite background code
Comment 2 drago01 2014-08-28 20:44:47 UTC
*** Bug 695928 has been marked as a duplicate of this bug. ***
Comment 3 Jasper St. Pierre (not reading bugmail) 2014-08-29 16:52:58 UTC
Review of attachment 284753 [details] [review]:

::: src/compositor/cogl-utils.c
@@ +74,3 @@
+meta_create_large_texture (int                   width,
+                           int                   height,
+                           CoglTextureComponents components)

I'm not exactly sure what a "large" texture is.

@@ +86,3 @@
+    {
+      if (cogl_has_feature (ctx, COGL_FEATURE_ID_TEXTURE_RECTANGLE))
+        should_use_rectangle = TRUE;

else
  g_warning ("Unsupported configuration");

?

@@ +91,3 @@
+  if (should_use_rectangle)
+    texture = COGL_TEXTURE (cogl_texture_rectangle_new_with_size (ctx,
+                                                                  width, height));

Can you put this on one line? Same with below.

::: src/compositor/cogl-utils.h
@@ +26,3 @@
 CoglPipeline * meta_create_texture_pipeline (CoglTexture *texture);
 
+CoglTexture *meta_create_large_texture (int                   width,

Nit: "CoglTexture * meta_create_large_texture"

::: src/compositor/meta-background-actor.c
@@ +51,3 @@
+ *     background can't be expressed with Cogl's fixed-function layer
+ *     combining (which is confined to what GL's texture environment
+ *     combining can do) So we can only handle the above directly if

Doesn't ARB_multitexture allow for three-way blending? Personally, I don't really care about this. I'd rather just mandate support for GLSL.

@@ +184,3 @@
+  MetaRectangle monitor_geometry;
+
+  meta_screen_get_monitor_geometry (priv->screen, priv->monitor, &monitor_geometry);

I'm not a fan of making this tied to the monitor geometry: I imagine a BackgroundActor being a generic actor, but it's just a purity thing. Since we always use the allocation, actually, it shouldn't really matter, since we should set a monitor constraint on the background container anyway.

@@ +252,3 @@
+        }
+
+      if ((pipeline_flags & PIPELINE_VIGNETTE) != 0)

Can you merge this with the other branch above?

@@ +394,3 @@
+  paintable_region = cairo_region_create_rectangle (&actor_pixel_rect);
+  if (priv->clip_region != NULL)
+    cairo_region_intersect (paintable_region, priv->clip_region);

We do this in meta-shaped-texture.c by intersecting every rectangle instead of allocating a new region (look for gdk_rectangle_intersect). I'd prefer to keep the code for this fairly similar.

@@ +405,3 @@
+   * and paint each area one by one
+   */
+  n_texture_subareas = cairo_region_num_rectangles (paintable_region);

Should we bail out and do a full paint if it's greater than N rects as well here?

@@ +416,3 @@
+      ty1 = (rect.y - actor_pixel_rect.y - priv->texture_area.y) / (float)priv->texture_area.height;
+      tx2 = (rect.x + rect.width - actor_pixel_rect.x - priv->texture_area.x) / (float)priv->texture_area.width;
+      ty2 = (rect.y + rect.height - actor_pixel_rect.y - priv->texture_area.y) / (float)priv->texture_area.height;

It would be nice if this looked more like meta-shaped-texture.c (perhaps copy over paint_clipped_rectangle?)

@@ +647,3 @@
+
+void
+meta_background_actor_add_vignette (MetaBackgroundActor *self,

Can you name this "set_vignette" or similar? "add_vignette" implies to me you shouldn't call it when you don't already have a vignette.

::: src/compositor/meta-background.c
@@ +17,1 @@
  * along with this program; if not, see <http://www.gnu.org/licenses/>.

This file is effectively unreadable in Splinter. Can you push a branch so I can just review the new file?
Comment 4 drago01 2014-08-29 18:05:44 UTC
Review of attachment 284753 [details] [review]:

Seems like Jasper reviewed this before I was done with mine so here are a few additional comments.

::: src/compositor/meta-background-actor.c
@@ +418,3 @@
+      ty2 = (rect.y + rect.height - actor_pixel_rect.y - priv->texture_area.y) / (float)priv->texture_area.height;
+
+      cogl_framebuffer_draw_textured_rectangle (cogl_get_draw_framebuffer (),

Could move this before the loop to save a few function call.

::: src/compositor/meta-background-image.c
@@ +170,3 @@
+
+  texture = meta_create_large_texture (width, height,
+                                       has_alpha ? COGL_TEXTURE_COMPONENTS_RGBA : COGL_TEXTURE_COMPONENTS_RGB);

Do we have to check endianess here?

@@ +315,3 @@
+ * This function is a convenience function for checking for success,
+ * without having to call meta_background_image_get_success() and
+ * handle the return of a Cogl type.

I can't parse that. Seems like that "meta_background_image_get_success()" should be something else.
Comment 5 Owen Taylor 2014-08-29 20:20:40 UTC
(In reply to comment #3)
> ::: src/compositor/cogl-utils.c
> @@ +74,3 @@
> +meta_create_large_texture (int                   width,
> +                           int                   height,
> +                           CoglTextureComponents components)
> 
> I'm not exactly sure what a "large" texture is.

It's really "create a texture in a way that we want to use in some places in the code but not others" and "large" was the best I could come up with.

Could do with a doc comment :-) "large" here means basically that a) it's too big to atlas effectively and b) there might be a lot of waste if rounding up to a power of 2. Basically these are conditions where using a rectangular texture makes sense if NPOT textures aren't available.
 
> @@ +86,3 @@
> +    {
> +      if (cogl_has_feature (ctx, COGL_FEATURE_ID_TEXTURE_RECTANGLE))
> +        should_use_rectangle = TRUE;
> 
> else
>   g_warning ("Unsupported configuration");

Yeah, wouldn't hurt though no NPOT and no Rectangular implies no TFP.

> @@ +91,3 @@
> +  if (should_use_rectangle)
> +    texture = COGL_TEXTURE (cogl_texture_rectangle_new_with_size (ctx,
> +                                                                  width,
> height));
> 
> Can you put this on one line? Same with below.

If you like.

> ::: src/compositor/cogl-utils.h
> @@ +26,3 @@
>  CoglPipeline * meta_create_texture_pipeline (CoglTexture *texture);
> 
> +CoglTexture *meta_create_large_texture (int                   width,
> 
> Nit: "CoglTexture * meta_create_large_texture"

Not according to me :-) It's not lined up with the function above so is independently indented, and I would not put a space there.
 
> ::: src/compositor/meta-background-actor.c
> @@ +51,3 @@
> + *     background can't be expressed with Cogl's fixed-function layer
> + *     combining (which is confined to what GL's texture environment
> + *     combining can do) So we can only handle the above directly if
> 
> Doesn't ARB_multitexture allow for three-way blending? Personally, I don't
> really care about this. I'd rather just mandate support for GLSL.

ARB_multitexture doesn't allow expressing the OVER operation - in limited circumstances you can work around that, but it just gets really complicated and inefficient, and you'd still need code like the current code for the cases you can't express. 

But really, GLSL is only one aspect of this. Sliced textures are another important one - the GNOME 2560x1440 default backgrounds will be sliced on some hardware we support. And simply efficiency - if I'm drawing, say, a 1920x1080 screen (2 megapixel), with the standard two-background blend that GNOME uses for most of the day, do you want to, per frame:

 Read 8mb, write 8mb
Or:
 Read 28mb, write 8mb

Avoiding the math of bilinear texturing is also a huge win on LLVMpipe, though not much of a deal on real hardware.

Finally, a GLSL-only one pass version is not simple at all when you try to handle everything (different texture coordinates) and also write out shaders that don't do a ton of extra work (important to LLVMPIPE) ... I had a not-quite-working version at one point, and it was not easier to read than this - rather the opposite.

> @@ +184,3 @@
> +  MetaRectangle monitor_geometry;
> +
> +  meta_screen_get_monitor_geometry (priv->screen, priv->monitor,
> &monitor_geometry);
> 
> I'm not a fan of making this tied to the monitor geometry: I imagine a
> BackgroundActor being a generic actor, but it's just a purity thing. Since we
> always use the allocation, actually, it shouldn't really matter, since we
> should set a monitor constraint on the background container anyway.

MetaBackground/MetaBackgroundActor, both in the new and old version really are assuming that they are drawing to an actor that is the size of the monitor. For this reason, it makes a ton of sense that the preferred size is the size of the monitor.

There's a weird exception to this that I preserved, but swept a bit under the rug to avoid blowing the reader's mind - the GNOME Shell SystemBackground assumes that a wallpapered background can be drawn at any size, and the wallpaper keeps on tiling. The first version of my rewrite broke this by scaling the drawn monitor to the size of the actor ... it is now *extended* to that size, as with the old code, which will not make sense for most things other than wallpaper. Since in the normal case, the actor is sized to the monitor size, it doesn't matter that.

> @@ +252,3 @@
> +        }
> +
> +      if ((pipeline_flags & PIPELINE_VIGNETTE) != 0)
> 
> Can you merge this with the other branch above?

It was kept separate so that everything stayed more parallel and I didn't need differently named sets of variables. But I can merge it.

> @@ +394,3 @@
> +  paintable_region = cairo_region_create_rectangle (&actor_pixel_rect);
> +  if (priv->clip_region != NULL)
> +    cairo_region_intersect (paintable_region, priv->clip_region);
> 
> We do this in meta-shaped-texture.c by intersecting every rectangle instead of
> allocating a new region (look for gdk_rectangle_intersect). I'd prefer to keep
> the code for this fairly similar.
> 
> @@ +405,3 @@
> +   * and paint each area one by one
> +   */
> +  n_texture_subareas = cairo_region_num_rectangles (paintable_region);
> 
> Should we bail out and do a full paint if it's greater than N rects as well
> here?
> 
> @@ +416,3 @@
> +      ty1 = (rect.y - actor_pixel_rect.y - priv->texture_area.y) /
> (float)priv->texture_area.height;
> +      tx2 = (rect.x + rect.width - actor_pixel_rect.x - priv->texture_area.x)
> / (float)priv->texture_area.width;
> +      ty2 = (rect.y + rect.height - actor_pixel_rect.y - priv->texture_area.y)
> / (float)priv->texture_area.height;
> 
> It would be nice if this looked more like meta-shaped-texture.c (perhaps copy
> over paint_clipped_rectangle?)

OK, I'll try to coordinate this code with MetaShapedTexture ... maybe even see if there's a helper function that can be extracted into cogl-utils. (Note that this code is essentially the same as it was before my rewrite .... the region arithmetic is not new.)

> @@ +647,3 @@
> +
> +void
> +meta_background_actor_add_vignette (MetaBackgroundActor *self,
> 
> Can you name this "set_vignette" or similar? "add_vignette" implies to me you
> shouldn't call it when you don't already have a vignette.

set_blah() is a bit weird for me for a function that isn't a setter, but I guess set_vigentte() and unset_vignette() is fine. Breaking this up into three properties vignette/vignette_sharpness/vignette_brightness is possible, but seemed like a lot of boilerplate for an API that is used minimally.

> ::: src/compositor/meta-background.c
> @@ +17,1 @@
>   * along with this program; if not, see <http://www.gnu.org/licenses/>.
> 
> This file is effectively unreadable in Splinter. Can you push a branch so I can
> just review the new file?

You just have to read down one column and ignore the other column and the blank lines :-) wip/background-rework branches pushed to mutter and gnome-shell
Comment 6 Jasper St. Pierre (not reading bugmail) 2014-08-29 20:49:10 UTC
meta-background.c looks fine to me. Thanks.
Comment 7 Owen Taylor 2014-09-02 20:35:13 UTC
Created attachment 285188 [details] [review]
Here's a fixup for the texture creation, adding docs and also fixing a problem

noticed that we weren't actually handling background images greater than the
texture size limit. (Now tested with a 100000x10 background on my system.)

I plan to squash this, but attaching for now.
Comment 8 Owen Taylor 2014-09-02 21:26:15 UTC
Created attachment 285206 [details] [review]
MetaBackgroundActor: make drawing more like MetaShapedTexture

Instead of allocating a new region and intersecting it with the paint
bounds rectangle, intersect each rectangle separately.

Also, draw the whole region as a single texture if there are more than
64 rectangles. (Increased from 16 for MetaShapedTexture, since it
doesn't take a lot of windows to make the remaining desktop area
have more than 16 rectangles.)
Comment 9 Owen Taylor 2014-09-02 21:37:13 UTC
Created attachment 285207 [details] [review]
Cleanups

 - Fix line breaking
 - Join duplicate blocks
 - Change add/remove_vignette to set/unset_vignette
 - Fix up comment
Comment 10 Jasper St. Pierre (not reading bugmail) 2014-09-02 21:42:48 UTC
Review of attachment 285188 [details] [review]:

::: src/compositor/cogl-utils.c
@@ +116,3 @@
+          g_warning ("Cannot create texture. Support for GL_ARB_texture_non_power_of_two or "
+                     "ARB_texture_rectangle is required");
+          return NULL; /* Will probably cause caller to crash */

If this can never realistically happen on any supported system, I'd be fine with doing a g_assert_not_reached () here.

@@ +136,3 @@
+      if (!cogl_texture_allocate (texture, &catch_error))
+        {
+          cogl_error_free (catch_error);

Can't you pass NULL for the catch_error param if you're never going to use it?
Comment 11 Jasper St. Pierre (not reading bugmail) 2014-09-02 21:44:32 UTC
Review of attachment 285206 [details] [review]:

::: src/compositor/meta-background-actor.c
@@ +442,1 @@
+           return;

Hm, this was probably copied from an old version of meta-shaped-texture.c. After my current rewrite to fix a few issues, it's no longer structured like this, with this return here.

I don't really care that much, just pointing it out.
Comment 12 Jasper St. Pierre (not reading bugmail) 2014-09-02 21:44:44 UTC
Review of attachment 285207 [details] [review]:

Looks good.
Comment 13 Owen Taylor 2014-09-02 22:11:33 UTC
Created attachment 285208 [details] [review]
MetaBackgroundActor: match total dimming if GLSL is not present

Without GLSL, we didn't apply the vignetting, which not only made the
background uniform in color, it made it much lighter. Adjust for this
and make the average brightness with the vignette effect the same
with or without GLSL.
Comment 14 Owen Taylor 2014-09-02 22:13:52 UTC
(In reply to comment #10)
> Review of attachment 285188 [details] [review]:
> 
> ::: src/compositor/cogl-utils.c
> @@ +116,3 @@
> +          g_warning ("Cannot create texture. Support for
> GL_ARB_texture_non_power_of_two or "
> +                     "ARB_texture_rectangle is required");
> +          return NULL; /* Will probably cause caller to crash */
> 
> If this can never realistically happen on any supported system, I'd be fine
> with doing a g_assert_not_reached () here.

g_error() is probably clearer than g_warning/comment-that-the-caller-will-crash :-)

> @@ +136,3 @@
> +      if (!cogl_texture_allocate (texture, &catch_error))
> +        {
> +          cogl_error_free (catch_error);
> 
> Can't you pass NULL for the catch_error param if you're never going to use it?

Passing null produces a warning to stderr, which we want to suppress here.
Comment 15 Owen Taylor 2014-09-02 22:14:53 UTC
(In reply to comment #11)
> Review of attachment 285206 [details] [review]:
> 
> ::: src/compositor/meta-background-actor.c
> @@ +442,1 @@
> +           return;
> 
> Hm, this was probably copied from an old version of meta-shaped-texture.c.
> After my current rewrite to fix a few issues, it's no longer structured like
> this, with this return here.
> 
> I don't really care that much, just pointing it out.

Rather, I just wrote it as it made sense, which, without all the newer complications of MetaShapedTexture was just a return here.
Comment 16 Owen Taylor 2014-09-03 16:24:23 UTC
Created attachment 285266 [details] [review]
MetaBackground: add properties to set vignette settings

Make the vignette options properties so they can be animated;
modify the function-call API for meta_background_actor_set_vignette()
to correspond more closely to the 3 properties.
Comment 17 Jasper St. Pierre (not reading bugmail) 2014-09-03 16:38:55 UTC
Review of attachment 285266 [details] [review]:

This looks good.
Comment 18 Owen Taylor 2014-09-04 18:18:26 UTC
All pushed.