As for the CPU impact, it's fairly dramatic. On a Dell Mini Ten, replacing the standard 1920x1400 wallpaper (which we have to ship in that size to suit all possible resolutions) with a size fitting the netbook (1024x600) takes away some 85% of g-s-d's total CPU time at startup. On a bootchart [1], almost all of g-s-d's blue CPU usage goes away.

As for the cache clean policy, I think this cache should only ever have one image, the one currently set. If you change it, then the image is set on the fly, and this operation could/should regenerate the cache. (If that's inconvenient because it uses a different API, then it will just be regenerated at the next desktop startup). I wouldn't like to see the cache pile up many images over time.

I don't think we should try to cover use cases like "slideshow" here. If you use slideshow, you just have to live with the extra .8 s of boot time.

[1] http://people.canonical.com/~pitti/bootcharts/daniel-lucid-20100129-1.png

(In reply to comment #1)
> As for the cache clean policy, I think this cache should only ever have one
> image, the one currently set.

Mostly agree here, except that...

> I don't think we should try to cover use cases like "slideshow" here. If you
> use slideshow, you just have to live with the extra .8 s of boot time.

I think this could be handled too.

Also, it might be a nice touch to cache the image for the second monitor (in case your're using randr).

Review of attachment 152554 [details] [review]:

I don't know the code very well, but here's a first review.

::: gnome-desktop-2.29.4/libgnome-desktop/gnome-bg.c
@@ +588,3 @@
+                              const char        *filename,
+                              int                width,
+                              int                height)

Maybe name the function get_wallpaper_cache_path?

@@ +597,3 @@
+	changed_filename = g_strdup (filename);
+	changed_filename = g_strdelimit (changed_filename, G_DIR_SEPARATOR_S, '_');
+	cache_filename = g_strdup_printf ("%i_%i_%i_%s", placement, width, height, changed_filename);

It's probably better to use the string from placement_lookup instead of %i for placement.

Also, what about doing the g_strdup_printf first on filename, and then doing the g_strdelimit. You can get rid of changed_filename this way.

And probably name the variable cache_basename.

@@ +618,3 @@
+	if (mtime < cache_mtime)
+		return TRUE;
+	return FALSE;

return (mtime < cache_mtime)

@@ +631,3 @@
+	GdkPixbufFormat *format;
+
+	cache_filename = get_wallpaper_cache_filename (bg->placement, bg->filename, width, height);

Note that you're using filename with different meanings (basename and full path). It might be already like this in the code, but still something that you can fix in the new code ;-)

@@ +864,3 @@
 	}
+
+	refresh_cache_file(bg, scaled, dest_width, dest_height);

Missing space before "(" :-)

@@ +1717,3 @@
+		/* Try to hit local cache first */
+		cache_filename = get_wallpaper_cache_filename (bg->placement, filename, best_width, best_height);
+		if (cache_file_is_valid(filename, cache_filename))

Missing space before "("

Created attachment 152760 [details] [review]
Add cache for wallpapers

Here is a new version of the patch with previously requests normally fulfilled (checking is always good :)).

Concerning the cache. I was thinking to add monX for monitor num X to cache one background only for each monitor.

I just have to change some internal functions prototype to add either screen or monitor num, but I'm concerned by that exported function:
gboolean
gnome_bg_is_dark (GnomeBG *bg,
                  int      width,
                  int      height)

to add either a GDKScreen or a monitor number. But this breaks the API…
What's your thought on that? (unfortunately, GFileMonitor of GnomeBG bg doesn't have any identifier)

Using monitor numbers is not really going to work. They are not guaranteed to be stable

Created attachment 153837 [details] [review]
Add cache for wallpapers with clean policy

Here is the last version of the patch.
Talking to Vuntz, it seems that there is no more the capacity of setting one wallpaper by monitor, so we went to the "just cache one wallpaper by user track" as a clean policy.

> Talking to Vuntz, it seems that there is no more the capacity of setting one
> wallpaper by monitor, so we went to the "just cache one wallpaper by user
> track" as a clean policy.

There has never been a way to set per-monitor backgrounds. What we do though, is to render the one selected background for each monitor, at different resolutions.
So you may want to reconsider that.

Review of attachment 153837 [details] [review]:

I think you really want to allow multiple sizes of the same background image to be cached, to handle the multi-monitor scenario. And clear the cache when the selected background image changes.

::: gnome-desktop-2.29.90/libgnome-desktop/gnome-bg.c
@@ +656,3 @@
+				gdk_pixbuf_save (new_pixbuf, cache_filename, format_name, NULL, "quality", "100", NULL);
+			else
+				gdk_pixbuf_save (new_pixbuf, cache_filename, format_name, NULL, NULL);

If you want to take the cycle saving seriously, you should look at gdk_pixdata_serialize etc, which will let you mmap the pixbuf data without any parsing or copying. Doesn't make much sense to covert the pixbuf to a jpeg for saving on disk and then convert it back to pixbuf when loading it.

@@ +881,3 @@
 	}
+
+	if ((dest_width > 300) && (dest_height > 300))

This strikes me as pointless. Just do it always.

@@ +1735,2 @@
+		/* Try to hit local cache first */
+		cache_filename = get_wallpaper_cache_filename (bg->placement, filename, best_width, best_height);

load_from_cache should probably be factored out as a separate function here.

What I do not understand though, is what the mtime of the original image has to do with the validity of the cache...

(In reply to comment #8)
> If you want to take the cycle saving seriously, you should look at
> gdk_pixdata_serialize etc, which will let you mmap the pixbuf data without any
> parsing or copying. Doesn't make much sense to covert the pixbuf to a jpeg for
> saving on disk and then convert it back to pixbuf when loading it.

Thanks for pointing out!

> +
> +    if ((dest_width > 300) && (dest_height > 300))
> 
> This strikes me as pointless. Just do it always.

The CPU overhead for small (tiled) backgrounds is negligible, thus this would save the overhead of cache processing.

> What I do not understand though, is what the mtime of the original image has to
> do with the validity of the cache...

You certainly need a way to see whether the background image changed underneath you? E. g. a distro default background which is shipped in a package might change, or you might have a program/script which fetches a daily image and sticks it into ~/wallpapers/mybackground.jpg. So the mtime comparison seems sensible to me?

If the user selects a different background, I would expect that to usually have an older mtime than the cache file. But its still different and needs to trigger cleaning the cache.

(In reply to comment #10)
> If the user selects a different background, I would expect that to usually have
> an older mtime than the cache file. But its still different and needs to
> trigger cleaning the cache.

Sure. In that case the cache file name won't match any more, and it gets cleaned.

This implementation is actually not that obvious: cache_file_is_valid() does not explicitly test for existence. It just seems that get_mtime() "magically" returns something like "plus infinity" for nonexisting files?

Anyway, I'd also like to see this file existence test being more explicit.

Didier provided an alternative patch which uses serializing. I did three boot charts for both cases (.jpg cache, with above patch, and the serialized cache with the new patch):

http://people.canonical.com/~pitti/bootcharts/wallpaper-cache/

This shows that with the uncompressed (serialized) g-s-d indeed needs a little less CPU time. The total boot time difference is below bootchart's accuracy though, so it's hard to quantify the savings in that, but from my experience with bootchart so far (interpreting the blue CPU activity areas) I'd say that the uncompressed image saves some 0.1 s.

However, the price for this is of course that there is now a lot more data to be read from the disk: The jpg cache was ~ 20 kB, the uncompressed cache is 1 MB.

So on the mini (utterly slow Atom CPU, fast SSD disk) compressed vs. uncompressed does not make a measurable total difference, while on more "typical" machines (fast CPU, slow disk) the cost of the bigger cache ought to outweigh the CPU saving by far.

So in summary I'd rather recommend to not use a raw cache.

Created attachment 154021 [details] [review]
Add cache for wallpapers with multiple monitor support and refactoring

Following Martin's remark, find attached a new version of the patch, without serialization. (I can still provide you that version if you need it, but on my 1920x1200, the serialized version is 2.5MiB whereas the initial file is 19KiB)

This one contains now:
- cleanage policy a little bit smarter: only clean every other files with same transformation x resolution. This enable the cache to support multiple monitors resolution. The "unused cache file" if you change your resolution and never come back to it seems acceptable.
- refactoring of loading from cache in its own function
- explicit file existence test for cache_file_is_valid()

Created attachment 164296 [details] [review]
Add cache for wallpapers with multiple monitors support and refactoring

Review of attachment 164296 [details] [review]:

Can you fix the tabs vs spaces issue? :-) See other comments below:

::: libgnome-desktop/gnome-bg.c
@@ +609,3 @@
+	cache_prefix_name = get_wallpaper_cache_prefix_name (placement, width, height);
+	cache_basename = g_strdup_printf ("%s_%s", cache_prefix_name, filename);
+	cache_basename = g_strdelimit (cache_basename, G_DIR_SEPARATOR_S, '_');

We really want to use a hash (md5 should be enough) to avoid clashes between /tmp/a_1.png and /tmp/a/1.png.

@@ +629,3 @@
+
+	if (!g_file_test (cache_filename, G_FILE_TEST_IS_REGULAR))
+		return FALSE;

This test should be done before get_mtime()

@@ +651,3 @@
+	cache_dir = get_wallpaper_cache_dir ();
+
+	/* Only refresh scaled file on disk if useful (and don't cache slideshow) */

It'd be really great to also be able to cache slideshows. This would imply refreshing the cache file only when the new image in the slide show is needed. And suffixing the basename of the cache file with a number, I guess (well, before the file extension).

The code needs to be restructured a bit, but this should be doable.

@@ +656,3 @@
+		if (format != NULL) {
+			if (!g_file_test (cache_dir, G_FILE_TEST_IS_DIR))
+				g_mkdir (cache_dir, 0700);

You want g_mkdir_with_parents()

@@ +657,3 @@
+			if (!g_file_test (cache_dir, G_FILE_TEST_IS_DIR))
+				g_mkdir (cache_dir, 0700);
+			else {

I'd create cache_prefix_name here and not earlier; I was wondering why you needed it earlier.

@@ +663,3 @@
+				while (file != NULL) {
+					path = g_build_filename (cache_dir, file, NULL);
+					/* only purge files with same transformation and resolution */

Apparently, this is needed because we don't have the monitor number and we support different backgrounds on different monitors. If we can't update the code to have the monitor parameter here, at least the comment should mention this :-)

But a quick look at the code makes me think we can have it. That's actually easy: either we care about monitors and we have an argument, or we don't care about it and that's why you don't have any mention of a monitor in the callers of this function.

@@ +664,3 @@
+					path = g_build_filename (cache_dir, file, NULL);
+					/* only purge files with same transformation and resolution */
+					if (strstr (file, cache_prefix_name) && g_file_test (path, G_FILE_TEST_IS_REGULAR))

g_str_has_prefix() instead of strstr()?

@@ +667,3 @@
+						g_unlink (path);
+					file = g_dir_read_name (g_cache_dir);
+					g_free (path);

Those two lines should be in the reverse order for clarity :-)

@@ +1778,2 @@
+		/* Try to hit local cache first */
+		pixbuf = load_from_cache_file(bg, filename, best_width, best_height);

Missing space before parenthesis

Created attachment 172883 [details] [review]
git format patch for adding cache

I think I fixed the concerns there, as discussed IRL.

There are still space vs tabs issue in the file, but I didn't fix that everywhere, just the part in the patch.

I renamed draw_image to draw_image_for_thumb as well.

Created attachment 174155 [details] [review]
Wallpaper patch against 2.91.1

I've rebased again the patch against 2.91.1. Can you please have a look at it? Otherwise, no ice cream at my "pendaison crémaillère" vuntz! :)

If you need a git-format patch, I can do that in a second.

Created attachment 174156 [details] [review]
Wallpaper patch against 2.91.1

oupss, gconf_enum_to_string () doesn't really exists now that the branch is using gsettings, removing that and use the integer equivalent for the patch.

Created attachment 174700 [details] [review]
Wallpaper patch against 2.91.2

Created attachment 175232 [details] [review]
gnome-bg: Cache resized wallpaper to help CPU at startup

Resized and transformed wallpapers are stored in ~/.cache/wallpaper/.
The cache is refreshed for each new current wallpaper depending on the
resolution or transformation, and each time the source image is
refreshed.

The source image format is preserved.

Pushed. After talking with Didier, it probably needs some testing, though :-)