GNOME Bugzilla – Bug 735637
Rewrite background code
Last modified: 2014-09-04 18:20: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.
Created attachment 284753 [details] [review] Rewrite background code
*** Bug 695928 has been marked as a duplicate of this bug. ***
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?
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.
(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
meta-background.c looks fine to me. Thanks.
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.
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.)
Created attachment 285207 [details] [review] Cleanups - Fix line breaking - Join duplicate blocks - Change add/remove_vignette to set/unset_vignette - Fix up comment
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?
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.
Review of attachment 285207 [details] [review]: Looks good.
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.
(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.
(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.
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.
Review of attachment 285266 [details] [review]: This looks good.
All pushed.