Created attachment 174445 [details] [review]
Refactor "texture material" creation from MetaShadowFactory

Create new cogl-utils.[ch] and move a helper function from
MetaShadowFactory there as meta_create_texture_material(); this
allows us to create single-layer materials from different parts of
Mutter and have them share the same template material.

Also expose a function for creating a 1x1 texture of a given
color meta_create_color_texture_4ub().

Created attachment 174446 [details] [review]
Draw the root window background

Comment on attachment 174445 [details] [review]
Refactor "texture material" creation from MetaShadowFactory

>+ * _meta_create_color_texture_4ub:
>+meta_create_color_texture_4ub (guint8 red,

The name in the doc has an extra "_" prefixed to it

>+/* Based on gnome-shell/src/st/_st_create_texture_material.c */

gnome-shell/src/st/st-private.c:_st_create_texture_material

>+ * allows sharing a shader for different uses in Mutter. To share the same
>+ * shader with all other materials that are just texture plus opacity
>+ * would require Cogl fixes.

Are there bugs about that? Would be nice to reference them if so, so that in 10 years, someone can figure out whether or not the comment still applies.

Created attachment 174544 [details] [review]
Turn off background repeats for a full-size image

Small followup patch for an issue that was bugging me in testing

==
If we have repeats on for a full-sized image, then if the background
is displayed scaled (for example, in a desktop preview mode) then we
can get artifacts along the edge of the background where the repeat
of the opposite edge is blended in by bilinear scaling. So turn off
repeats when the screen and background image sizes match.

(In reply to comment #3)

> >+ * allows sharing a shader for different uses in Mutter. To share the same
> >+ * shader with all other materials that are just texture plus opacity
> >+ * would require Cogl fixes.
> 
> Are there bugs about that? Would be nice to reference them if so, so that in 10
> years, someone can figure out whether or not the comment still applies.

Filed http://bugzilla.clutter-project.org/show_bug.cgi?id=2425 and added a reference to it in the comment locally.

Comment on attachment 174446 [details] [review]
Draw the root window background

>@@ -862,7 +905,9 @@ sync_actor_stacking (MetaCompScreen *info,
>       old = old->next;
>     }
> 
>-  if (old != NULL) /* An extra MetaWindowActor??? .... restack */
>+  while (old && !META_IS_BACKGROUND_ACTOR (old->data))
>+    old = old->next;
>+  if (old == NULL || old->data != info->background_actor)
>     reordered = TRUE;

This is wrong; @old is stacked bottom to top, so the background actor will be at the beginning, not the end. (So reordered will always get set TRUE.)

The original check seems to have been confused too; it was saying "if there is any ClutterActor stacked above the topmost MetaWindowActor, then restack". But the restacking procedure just pushed all the MetaWindowActors down, so the end result would be identical to what you started with; a non-window ClutterActor above the topmost MetaWindowActor. So anyway, the fact that we're getting rid of that check isn't a problem at least.

I guess the new check is supposed to mean "if info->background_actor is somewhere other than where it's supposed to be in the list, then restack", but it belongs before the loop, not after. Also, you should change the !META_IS_BACKGROUND_ACTOR to !=info->background_actor, for both speed and consistency.

>+struct _MetaBackgroundActorClass
>+{
>+  ClutterGroupClass parent_class;
>+};

ClutterActorClass

>+static void
>+set_texture (MetaBackgroundActor *self,
>+             CoglHandle           texture)

you should call clutter_actor_queue_redraw() here rather than calling it in each place that calls set_texture().

>+/* Sets our material to paint with a 1x1 texture of the stage's background
>+ * color; doing this when we have no pixmap allows the application to turn
>+ * off painting the stage. There might be a performance benefit to
>+ * painting in this case with a solid color, but the normal solid color
>+ * case is a 1x1 root pixmap, so we'd have to reverse-engineer that to
>+ * actually pick up the (small?) performance win. This is just a fallback.
>+ */

OK, but I don't know what that means. :)

>+          cogl_rectangle_with_texture_coords (rect.x, rect.y,

Should we be using cogl_rectangle*s*_with_texture_coords() since "In some situations it can give a significant performance boost rather than calling cogl_rectangle_with_texture_coords() separately for each rectangle." ?

>+  meta_screen_get_size (self->screen, &width, &height);
>+
>+  clutter_paint_volume_set_width (volume, width);
>+  clutter_paint_volume_set_height (volume, height);

I don't know the details of the paint volume stuff, but does this actually result in any optimization relative to just not implementing the method? Or should we be taking the visible region into account?

>+/**
>+ * meta_background_actor_new:

_update. (_new and _update both claim to be _new in their doc comment)

Comment on attachment 174544 [details] [review]
Turn off background repeats for a full-size image

looks like it makes sense

Created attachment 174796 [details] [review]
Draw the root window background

Add code to track and draw the root window background. The advantage of doing
it here as compared to in a plugin is that we can use the visiblity smarts
of MetaWindowGroup to optimize out drawing the background when obscured.

If handling other than tracking the _XROOTPMAP_ID property is desired in the
future, more functionality like setting the background from a file or doing
cross-fades can be added.

The new background actor is exposed to plugins via meta_plugin_get_background_actor()
similar to other exposed actors to allow cloning the background for use in
other displays. The actual class is not installed for public consumption at
the moment since it has no useful methods.

(In reply to comment #6)
> (From update of attachment 174446 [details] [review])
> >@@ -862,7 +905,9 @@ sync_actor_stacking (MetaCompScreen *info,
> >       old = old->next;
> >     }
> > 
> >-  if (old != NULL) /* An extra MetaWindowActor??? .... restack */
> >+  while (old && !META_IS_BACKGROUND_ACTOR (old->data))
> >+    old = old->next;
> >+  if (old == NULL || old->data != info->background_actor)
> >     reordered = TRUE;
> 
> This is wrong; @old is stacked bottom to top, so the background actor will be
> at the beginning, not the end. (So reordered will always get set TRUE.)
>
> The original check seems to have been confused too; it was saying "if there is
> any ClutterActor stacked above the topmost MetaWindowActor, then restack". But
> the restacking procedure just pushed all the MetaWindowActors down, so the end
> result would be identical to what you started with; a non-window ClutterActor
> above the topmost MetaWindowActor. So anyway, the fact that we're getting rid
> of that check isn't a problem at least.

The check was meant to be on a stray MetaWindowActor - to check that the list of MetaWindowActor children was what we expectd. But it wasn't quite right, and didn't serve an obvious purpose, so removed.

> I guess the new check is supposed to mean "if info->background_actor is
> somewhere other than where it's supposed to be in the list, then restack", but
> it belongs before the loop, not after. Also, you should change the
> !META_IS_BACKGROUND_ACTOR to !=info->background_actor, for both speed and
> consistency.

Yeah. I've attached a version with a somewhat rewritten check. Can you look it over? (The rewritten check, tested, does get hit and decide that sometimes there was no reorder, as I would expect.)

> >+struct _MetaBackgroundActorClass
> >+{
> >+  ClutterGroupClass parent_class;
> >+};
> 
> ClutterActorClass
> 
> >+static void
> >+set_texture (MetaBackgroundActor *self,
> >+             CoglHandle           texture)
> 
> you should call clutter_actor_queue_redraw() here rather than calling it in
> each place that calls set_texture().

That's better. Fixed.

> >+/* Sets our material to paint with a 1x1 texture of the stage's background
> >+ * color; doing this when we have no pixmap allows the application to turn
> >+ * off painting the stage. There might be a performance benefit to
> >+ * painting in this case with a solid color, but the normal solid color
> >+ * case is a 1x1 root pixmap, so we'd have to reverse-engineer that to
> >+ * actually pick up the (small?) performance win. This is just a fallback.
> >+ */
> 
> OK, but I don't know what that means. :)

Seems to have all complete sentences and no missing words, so I'll leave it, will help me anyways :-)

> >+          cogl_rectangle_with_texture_coords (rect.x, rect.y,
> 
> Should we be using cogl_rectangle*s*_with_texture_coords() since "In some
> situations it can give a significant performance boost rather than calling
> cogl_rectangle_with_texture_coords() separately for each rectangle." ?

Will quote what I said to Florian earlier to day in regard to MetaShadow:

 > Why not cogl_rectangles_with_texture_coords()?

 Offhand, I don't think it's a significant win - because of the Cogl journal,
 the main difference is that we save a little bit of validation work per
 rectangle.... so in general doesn't seem worth having to manage a temporary
 array of float rectangles. I could be wrong ... hard to know without writing
 some benchmarks.

But I filed https://bugzilla.gnome.org/show_bug.cgi?id=635206 in case someone wants to try using with_texture_coords() in all three place in Mutter and seeing if it helps.

> >+  meta_screen_get_size (self->screen, &width, &height);
> >+
> >+  clutter_paint_volume_set_width (volume, width);
> >+  clutter_paint_volume_set_height (volume, height);
> 
> I don't know the details of the paint volume stuff, but does this actually
> result in any optimization relative to just not implementing the method? Or
> should we be taking the visible region into account?

For the fullscreen case, I think there is very little difference. The main reason I did this was for something like GNOME Shell's non-relayed-out overview where we have a clone of the actor taking up only part of the screen.

Taking the visible region into account doens't make sense since the paint volume should be a property of where the actor paints, and not dependent on the position of other actor. (Plus, we only set the visible region during the actual paint call, so it won't be set when the paint volume is queried)
 
> >+/**
> >+ * meta_background_actor_new:
> 
> _update. (_new and _update both claim to be _new in their doc comment)

Fixed.

All patches pushed