GNOME Bugzilla – Bug 682427
Handle animated backgrounds in the shell
Last modified: 2013-02-19 23:38:27 UTC
It probably makes sense to handle animated backgrounds as a special case, where we animate actors directly rather than have gnome-settings-daemon manually set a root pixmap when crossfading, which hurts us a lot.
Actually, the idea was to move GnomeBG in mutter directly, and kill the whole _XROOTPMAP_ID thing (it's just legacy cruft), as that would allows to have different backgrounds in different modes. But this is 3.8 material, at this point.
(In reply to comment #1) > Actually, the idea was to move GnomeBG in mutter directly, and kill the whole > _XROOTPMAP_ID thing As I mention in another bug, the one use of _XROOTPMAP_ID is to capture the contents of what Plymouth stuck up on the heads before Xorg took it over.
Yeah I saw that after this one, although in that bug you don't use MetaBackgroundActor to wrap _XROOTPMAP_ID, you use your own ClutterX11Texture, so they're not necessarily related.
Did we ever have a patch here ? Now that we've decided to go ahead and drop fallback mode, we need one...
Well, first of all, this needs to be reassigned to mutter, as that's where the background is rendered.
Created attachment 228650 [details] [review] MetaBackgroundActor: import background rendering using libgnome-desktop Instead of relying on gnome-settings-daemon to set up _XROOTPMAP_ID properly, do rendering in process and stuff the background into a normal texture. This will allow to do more fancy transitions in the future, using Clutter to drive them. This can be considered a preliminary patch. Background is rendered fine, and with the right amount of patching in libgnome-desktop it doesn't even crash. Next step is to cross-fade changes using Clutter, then we need to expose slideshows from GnomeBG to mutter (so they can be prerendered and blended in GPU). Another interesting thing to do would be making meta-background.c public, so that gnome-shell could use that to render a texture that's backed by lock screen background settings.
Created attachment 228662 [details] [review] Reintroduce background crossfading ... but do it right, using Clutter animation framework for vblank syncing and Cogl to generate GLSL shaders that do the hard work of blending the old and the new background in HW.
Created attachment 228666 [details] [review] Reintroduce background crossfading ... but do it right, using Clutter animation framework for vblank syncing and Cogl to generate GLSL shaders that do the hard work of blending the old and the new background in HW. Previous patch regressed GLSL snippets.
How does this affect the ability to have nautilus handle the desktop (i.e. draw files and folders there) My anecdotal experience with g-t-t users is that this option is very popular.
(In reply to comment #9) > How does this affect the ability to have nautilus handle the desktop (i.e. draw > files and folders there) > > My anecdotal experience with g-t-t users is that this option is very popular. I tried it here, it works fine: the desktop window is composited on top of the MetaBackgroundActor like any other window. As an existing optimization, only the visible region of MBA is repainted, so if nautilus is drawing the desktop there should be no performance loss. Different would be if nautilus removed code to handle drawing the desktop, which is something they want to do, but not the scope of this bug.
Note that cross-fade would be slow because they're doing software compositing and everything. I don't know if we'll cross-fade actors at specified times with timeline management, too. It might be worth it to make Nautilus have an ARGB window and not draw the background on its own.
(In reply to comment #11) > Note that cross-fade would be slow because they're doing software compositing > and everything. I don't know if we'll cross-fade actors at specified times with > timeline management, too. And by 'they', you mean nautilus here, I assume ? > It might be worth it to make Nautilus have an ARGB window and not draw the > background on its own. We can treat that as an optimization to investigate when the nautilus desktop handling gets moved to its own process. Do the patches here look good, otherwise ?
Created attachment 230360 [details] [review] MetaBackgroundActor: import background rendering using libgnome-desktop Instead of relying on gnome-settings-daemon to set up _XROOTPMAP_ID properly, do rendering in process and stuff the background into a normal texture. This will allow to do more fancy transitions in the future, using Clutter to drive them. Fixed a few Cogl warnings and fixed a race condition at startup. Uses g_task_wait_sync() from bug 689393.
Created attachment 230361 [details] [review] Reintroduce background crossfading ... but do it right, using Clutter animation framework for vblank syncing and Cogl to generate GLSL shaders that do the hard work of blending the old and the new background in HW.
Created attachment 231272 [details] [review] MetaBackgroundActor: import background rendering using libgnome-desktop Instead of relying on gnome-settings-daemon to set up _XROOTPMAP_ID properly, do rendering in process and stuff the background into a normal texture. This will allow to do more fancy transitions in the future, using Clutter to drive them. Fixed a leak.
Review of attachment 231272 [details] [review]: I'll give this a go soon and see how it works, but first a few comments: ::: configure.ac @@ +76,3 @@ $CLUTTER_PACKAGE >= 1.9.10 cogl-1.0 >= 1.9.6 + gnome-desktop-3.0 So this adds a dependency on gnome-desktop and also needs to add dependency on glib 2.36 because you use GTask. Is that something we're okay with? One thought is, ultimately there's only going to be one thing drawing backgrounds anymore (this), so maybe the background drawing code doesn't need to be in a shared library. We could just drop it from gnome-desktop. Well, I guess there's the control-center thumbnail too. ::: src/compositor/meta-background-actor.c @@ +302,3 @@ int width, height; + meta_background_ensure_rendered (priv->background); So this synchronization point partially removes the advantage of your helper thread right? I mean things could "stall" here. ::: src/compositor/meta-background.c @@ +63,3 @@ + TRUE, + td->monitors, + td->num_monitors); So this seems very round about. We're loading backgrounds from disk to a pixbuf, then redrawing them again to a larger pixbuf, then rendering the pixbuf, right? We should be able to side step a lot of the processing by moving this to the shell. Granted, we would still need a stitched together image if we want to support pseudo-transparency on legacy terminals, but if we care about that (which isn't a given) it shouldn't be at the expense of the "hot" path.
(In reply to comment #16) > Review of attachment 231272 [details] [review]: > > I'll give this a go soon and see how it works, but first a few comments: > > ::: configure.ac > @@ +76,3 @@ > $CLUTTER_PACKAGE >= 1.9.10 > cogl-1.0 >= 1.9.6 > + gnome-desktop-3.0 > > So this adds a dependency on gnome-desktop and also needs to add dependency on > glib 2.36 because you use GTask. Is that something we're okay with? One thought > is, ultimately there's only going to be one thing drawing backgrounds anymore > (this), so maybe the background drawing code doesn't need to be in a shared > library. We could just drop it from gnome-desktop. Well, I guess there's the > control-center thumbnail too. The control-center pokes gnome-shell to get a screenshot of the screen, and merge that in with a preview of the background. I think we could just as well ask gnome-shell to create the preview for us...
(In reply to comment #16) > Review of attachment 231272 [details] [review]: > > I'll give this a go soon and see how it works, but first a few comments: > > ::: configure.ac > @@ +76,3 @@ > $CLUTTER_PACKAGE >= 1.9.10 > cogl-1.0 >= 1.9.6 > + gnome-desktop-3.0 > > So this adds a dependency on gnome-desktop and also needs to add dependency on > glib 2.36 because you use GTask. Is that something we're okay with? One thought > is, ultimately there's only going to be one thing drawing backgrounds anymore > (this), so maybe the background drawing code doesn't need to be in a shared > library. We could just drop it from gnome-desktop. Well, I guess there's the > control-center thumbnail too. I'm not the maintainer of nautilus, and it's not my decision to keep code for drawing the desktop there, but I understand if people would be upset by the removal of this feature. And every GNOME user I know (outside of the GNOME community itself) has icons on the desktop. OTOH, given that the compositor is always there, nautilus can just paint a transparent ARGB window, and forget about backgrounds. > ::: src/compositor/meta-background-actor.c > @@ +302,3 @@ > int width, height; > > + meta_background_ensure_rendered (priv->background); > > So this synchronization point partially removes the advantage of your helper > thread right? I mean things could "stall" here. Yes, the first frame stalls loading the background. The helper thread becomes useful when later changing the background, or when an animation kicks in, because we don't block if we already have a texture to use. I don't like this solution either, but we need to paint something there... > ::: src/compositor/meta-background.c > @@ +63,3 @@ > + TRUE, > + td->monitors, > + td->num_monitors); > > So this seems very round about. We're loading backgrounds from disk to a > pixbuf, then redrawing them again to a larger pixbuf, then rendering the > pixbuf, right? We should be able to side step a lot of the processing by moving > this to the shell. Granted, we would still need a stitched together image if > we want to support pseudo-transparency on legacy terminals, but if we care > about that (which isn't a given) it shouldn't be at the expense of the "hot" > path. I agree that all the drawing that happens in GnomeBG could be done more efficiently in the GPU by Cogl. I decided to do it this way for now because I wanted to reuse GnomeBG as much as possible, and not rewrite the xml parser and handling of strecthing and gradients. This can change if we decide GnomeBG is legacy and not needed. Btw, this already drops support for legacy terminals, because I never set _XROOTPMAP_ID with the rendered pixbuf. I want to use _XROOTPMAP_ID for plymouth->gdm->session communication, and I think noone will be upset if xterm doesn't do transparency.
(In reply to comment #18) > I agree that all the drawing that happens in GnomeBG could be done more > efficiently in the GPU by Cogl. I decided to do it this way for now because I > wanted to reuse GnomeBG as much as possible, and not rewrite the xml parser and > handling of strecthing and gradients. This can change if we decide GnomeBG is > legacy and not needed. GnomeBG should be phased out. It doesn't match the designs for background manipulation options, and we probably want to cut down on the number of options for backgrounds (you can bin "stretch"/"scale"/etc. if you allow the user to crop the photo themselves for example).
(In reply to comment #18) > > control-center thumbnail too. > > I'm not the maintainer of nautilus, and it's not my decision to keep code for > drawing the desktop there, but I understand if people would be upset by the > removal of this feature. And every GNOME user I know (outside of the GNOME > community itself) has icons on the desktop. > OTOH, given that the compositor is always there, nautilus can just paint a > transparent ARGB window, and forget about backgrounds. That sounds about right. We do need to keep the desktop icons for classic mode, I'm afraid. But we should do the argb thing. I've filed bug 691359 for it.
*** Bug 660837 has been marked as a duplicate of this bug. ***
i'm going to a post a patchset here that addresses this bug and part of bug 682429 here.
Created attachment 235987 [details] [review] compositor: do sync actor stack in one pass This refactor will simplify a subsequent commit that introduces more than one background actor to the window group.
Created attachment 235988 [details] [review] compositor: rework how backgrounds are managed Background handling in GNOME is very roundabout at the moment. gnome-settings-daemon uses gnome-desktop to read the background from disk into a screen-sized pixmap. It then sets the XID of that pixmap on the _XROOTPMAP_ID root window property. mutter puts that pixmap into a texture/actor which gnome-shell then uses. Having the gnome-settings-daemon detour from disk to screen means we can't easily let the compositor handle transition effects when switching backgrounds. Also, having the background actor be per-screen instead of per-monitor means we may have oversized textures in certain multihead setups. This commit changes mutter to read backgrounds from disk itself, and it changes backgrounds to be per-monitor. This way background handling/compositing is left to the compositor.
Created attachment 235989 [details] [review] compositor: remove the hidden group It is unused and always empty. https://bugzilla.gnome.org/show_bug.cgi?id=688210
Review of attachment 235989 [details] [review]: I was planning on making the same cleanup.
(In reply to comment #22) > i'm going to a post a patchset here that addresses this bug and part of bug > 682429 here. well actually, let's put the mutter patches here, and the gnome-shell patches there.
Comment on attachment 235989 [details] [review] compositor: remove the hidden group Attachment 235989 [details] pushed as fb0cd80 - compositor: remove the hidden group
Created attachment 236005 [details] [review] compositor: do sync actor stack in one pass This refactor will simplify a subsequent commit that introduces more than one background actor to the window group.
Created attachment 236006 [details] [review] compositor: rework how backgrounds are managed Background handling in GNOME is very roundabout at the moment. gnome-settings-daemon uses gnome-desktop to read the background from disk into a screen-sized pixmap. It then sets the XID of that pixmap on the _XROOTPMAP_ID root window property. mutter puts that pixmap into a texture/actor which gnome-shell then uses. Having the gnome-settings-daemon detour from disk to screen means we can't easily let the compositor handle transition effects when switching backgrounds. Also, having the background actor be per-screen instead of per-monitor means we may have oversized textures in certain multihead setups. This commit changes mutter to read backgrounds from disk itself, and it changes backgrounds to be per-monitor. This way background handling/compositing is left to the compositor.
Review of attachment 236006 [details] [review]: ::: src/compositor/meta-background.c @@ +955,3 @@ + set_filename (self, filename); + + texture = cogl_texture_new_from_file (filename, I don't like the idea of reading potentially large files in the compositor. Can we move that to a thread?
(In reply to comment #31) > I don't like the idea of reading potentially large files in the compositor. Can > we move that to a thread? Makes sense. I can probably borrow some code from gcampax's branch to help here.
Review of attachment 236006 [details] [review]: OK so setting this to needs-work based on the above comments.
Created attachment 236523 [details] [review] compositor: rework how backgrounds are managed Background handling in GNOME is very roundabout at the moment. gnome-settings-daemon uses gnome-desktop to read the background from disk into a screen-sized pixmap. It then sets the XID of that pixmap on the _XROOTPMAP_ID root window property. mutter puts that pixmap into a texture/actor which gnome-shell then uses. Having the gnome-settings-daemon detour from disk to screen means we can't easily let the compositor handle transition effects when switching backgrounds. Also, having the background actor be per-screen instead of per-monitor means we may have oversized textures in certain multihead setups. This commit changes mutter to read backgrounds from disk itself, and it changes backgrounds to be per-monitor. This way background handling/compositing is left to the compositor.
Review of attachment 236523 [details] [review]: This will break background handling in the default plugin. If we don't care about the default plugin, that's fine, but we need to make that decision first. Initial code review looks fine, it's just *a lot* of code.
Review of attachment 236005 [details] [review]: OK.
Review of attachment 236523 [details] [review]: ::: src/compositor/meta-background.c @@ +154,3 @@ + +static void +get_texture_area_and_scale (MetaBackground *self, I wonder if some of this might be better done with cogl_scale / cogl_translate. @@ +198,3 @@ + */ + image_area = actor_pixel_rect; + *texture_x_scale = 1.0 / texture_width; "unscaled"? Shouldn't this be "1.0" then, or do I not understand how things are scaled? Usually "1.0" is "unscaled" @@ +397,3 @@ + /* And then cut out any parts occluded by window actors + */ + if (META_IS_BACKGROUND_ACTOR (actor) && FALSE) ??? @@ +412,3 @@ + * and paint each area one by one + */ + n_texture_subareas = cairo_region_num_rectangles (paintable_region); Should we do the same logic as the stex, which is to do a full repaint if we have greater than some N rectangles, to prevent slow multitexturing? @@ +560,3 @@ + code = DESATURATE_CODE; + + cogl_snippet_set_replace (snippet, code); Isn't the point of the snippet API that you can mix multiple snippets efficiently? Is it hard / impossible to have the blur and desaturate have separate snippets, like vignettes?
(In reply to comment #37) > Review of attachment 236523 [details] [review]: > > ::: src/compositor/meta-background.c > @@ +154,3 @@ > + > +static void > +get_texture_area_and_scale (MetaBackground *self, > > I wonder if some of this might be better done with cogl_scale / cogl_translate. > > @@ +198,3 @@ > + */ > + image_area = actor_pixel_rect; > + *texture_x_scale = 1.0 / texture_width; > > "unscaled"? Shouldn't this be "1.0" then, or do I not understand how things are > scaled? Usually "1.0" is "unscaled" different coordinates systems. texels are 0.0-1.0, the image data is 0-texture_size. > @@ +412,3 @@ > + * and paint each area one by one > + */ > + n_texture_subareas = cairo_region_num_rectangles (paintable_region); > > Should we do the same logic as the stex, which is to do a full repaint if we > have greater than some N rectangles, to prevent slow repeated texturing? Not sure. We're using paint nodes, so clutter might already do the right thing for us. > > @@ +560,3 @@ > + code = DESATURATE_CODE; > + > + cogl_snippet_set_replace (snippet, code); > > Isn't the point of the snippet API that you can mix multiple snippets > efficiently? Is it hard / impossible to have the blur and desaturate have > separate snippets, like vignettes? They probably could be separate snippets. I think I did it that way originally and hit a hitch, but I don't remember what it was.
Created attachment 236681 [details] [review] compositor: do sync actor stack in one pass This refactor will simplify a subsequent commit that introduces more than one background actor to the window group.
Created attachment 236682 [details] [review] compositor: export actor_is_untransformed function actor_is_untransformed is a function meta-window-group uses to determine if an actor is relatively pixel aligned and not contorted. It then returns the coordinates of the actor. In a subsequent commit will need the function in a different file, so this commit separates it out.
Created attachment 236683 [details] [review] compositor: rework how backgrounds are managed Background handling in GNOME is very roundabout at the moment. gnome-settings-daemon uses gnome-desktop to read the background from disk into a screen-sized pixmap. It then sets the XID of that pixmap on the _XROOTPMAP_ID root window property. mutter puts that pixmap into a texture/actor which gnome-shell then uses. Having the gnome-settings-daemon detour from disk to screen means we can't easily let the compositor handle transition effects when switching backgrounds. Also, having the background actor be per-screen instead of per-monitor means we may have oversized textures in certain multihead setups. This commit changes mutter to read backgrounds from disk itself, and it changes backgrounds to be per-monitor. This way background handling/compositing is left to the compositor.
Review of attachment 236682 [details] [review]: ::: src/compositor/meta-window-group.c @@ +108,3 @@ */ +gboolean +meta_window_group_actor_is_untransformed (ClutterActor *actor, Having meta_window_group_actor_is_untransformed that doesn't take a MetaWindowGroup will confuse the hell out of introspection (and also me, I guess). meta_actor_is_untransformed in cogl-utils.[ch] I guess? Maybe make a clutter-utils.[ch] ?
Review of attachment 236683 [details] [review]: This is a big patch, but it looks OK to me.
Attachment 236681 [details] pushed as aba8740 - compositor: do sync actor stack in one pass Attachment 236682 [details] pushed as 842bc44 - compositor: export actor_is_untransformed function Attachment 236683 [details] pushed as 580feb0 - compositor: rework how backgrounds are managed