Re: [PATCH] Use GdkPixbufLoader for thumbnailer to reduce memory consumption

[Christian Neumair <chris gnome-de org>]

Am Samstag, den 08.04.2006, 01:59 +0200 schrieb Martin Wehner:
> On Fri, 2006-04-07 at 15:30 +0200, Christian Neumair wrote:
> > The attached patch ports the nautilus image loading code to
> > GdkPixbufLoader, which ensures that less memory is consumed for large
> > images. The old thumbnailing code was also not very traceable wrt frame
> > addition.
> 
> It looks good in general, but:
> 
> > if (!gdk_pixbuf_loader_write (loader, buffer, buflen, &error)) {                                             
> >         /* error might be NULL although FALSE is returned, bug #337611 */                                    
> 
> I'm not sure after reading the bug: Is that just an assumption based on reading the code or did you actually get an NULL error?

It happened at least once, but I don't remember the culprit image, and I
fail to reproduce it now, so I'll leave it out for now and let our users
provide a testcase ;).
                                                                 
> >         if (error != NULL);                                                                                  
> >                 g_error_free (error);
> 
> This looks like it doesn't do what you intended.

Why?

> > scale = (double)args->nominal_size/args->base_size;
> 
> Missing whitespaces. (Granted, in the current version too)
> 
> > GError *error = NULL;
> 
> Styleguide.

I'm attaching a new patch with the suggested fixes and a bit more
whitespace cleanup.

-- 
Christian Neumair <chris gnome-de org>

Index: libnautilus-private/nautilus-icon-factory.c
===================================================================
RCS file: /cvs/gnome/nautilus/libnautilus-private/nautilus-icon-factory.c,v
retrieving revision 1.325
diff -u -p -r1.325 nautilus-icon-factory.c
--- libnautilus-private/nautilus-icon-factory.c	22 Mar 2006 17:41:32 -0000	1.325
+++ libnautilus-private/nautilus-icon-factory.c	8 Apr 2006 11:16:26 -0000
@@ -1110,30 +1110,6 @@ path_represents_svg_image (const char *p
 }
 
 static GdkPixbuf *
-scale_icon (GdkPixbuf *pixbuf,
-	    double *scale)
-{
-	guint width, height;
-
-	width = gdk_pixbuf_get_width (pixbuf);
-	height = gdk_pixbuf_get_height (pixbuf);
-
-	if ((int) (width * *scale) > NAUTILUS_ICON_MAXIMUM_SIZE ||
-	    (int) (height * *scale) > NAUTILUS_ICON_MAXIMUM_SIZE) {
-		*scale = MIN ((double) NAUTILUS_ICON_MAXIMUM_SIZE / width,
-			      (double) NAUTILUS_ICON_MAXIMUM_SIZE / height);
-	}
-
-	width = floor (width * *scale + 0.5);
-	height = floor (height * *scale + 0.5);
-	
-	return gdk_pixbuf_scale_simple (pixbuf,
-					width == 0 ? 1 : width,
-					height == 0 ? 1 : height,
-					GDK_INTERP_BILINEAR);
-}
-
-static GdkPixbuf *
 load_icon_file (const char    *filename,
 		guint          base_size,
 		guint          nominal_size,
@@ -1141,11 +1117,10 @@ load_icon_file (const char    *filename,
 		double        *scale_x,
 		double        *scale_y)
 {
-	GdkPixbuf *pixbuf, *scaled_pixbuf;
-	int width, height, size;
-	double scale;
-	gboolean is_thumbnail;
-        gboolean add_frame = FALSE;
+	GdkPixbuf *pixbuf;
+	gboolean add_frame;
+
+	add_frame = FALSE;
 
 	*scale_x = 1.0;
 	*scale_y = 1.0;
@@ -1156,59 +1131,35 @@ load_icon_file (const char    *filename,
 					  force_nominal ? 0 : base_size,
 					  scale_x, scale_y);
 	} else {
-		is_thumbnail = strstr (filename, "/.thumbnails/")  != NULL;
+		int size;
+		gboolean is_thumbnail;
 
 		/* FIXME: Maybe we shouldn't have to load the file each time
 		 * Not sure if that is important */
-		if (is_thumbnail) {
-			pixbuf = nautilus_thumbnail_load_framed_image (filename);
-		} else {
-			pixbuf = gdk_pixbuf_new_from_file (filename, NULL);
-		}
+		pixbuf = nautilus_thumbnail_load_image (filename,
+							base_size,
+							nominal_size,
+							force_nominal,
+							scale_x,
+							scale_y);
 
 		if (pixbuf == NULL) {
 			return NULL;
 		}
-		
-		if (force_nominal) {
-			width = gdk_pixbuf_get_width (pixbuf); 
-			height = gdk_pixbuf_get_height (pixbuf);
-			base_size = MAX (width, height);                        
-		} else if (base_size == 0) {
-			if (is_thumbnail) {
-				base_size = 128 * NAUTILUS_ICON_SIZE_STANDARD / NAUTILUS_ICON_SIZE_THUMBNAIL;
-			} else {
-				width = gdk_pixbuf_get_width (pixbuf); 
-				height = gdk_pixbuf_get_height (pixbuf);
-				size = MAX (width, height);
-                                if (size > NAUTILUS_ICON_SIZE_THUMBNAIL && !gdk_pixbuf_get_has_alpha(pixbuf)) {
-                                        add_frame=TRUE;
-                                }
-                                if (size >  nominal_size * NAUTILUS_ICON_SIZE_THUMBNAIL / NAUTILUS_ICON_SIZE_STANDARD) {
-                                        base_size = size * NAUTILUS_ICON_SIZE_STANDARD / NAUTILUS_ICON_SIZE_THUMBNAIL;
-                                }
-                                else if (size > NAUTILUS_ICON_SIZE_STANDARD) {
-					base_size = nominal_size;
-				} else {
-					/* Don't scale up small icons */
-					base_size = NAUTILUS_ICON_SIZE_STANDARD;
-				}
-			}
-		}
-		
-		if (base_size != nominal_size) {
-			scale = (double)nominal_size/base_size;
-			scaled_pixbuf = scale_icon (pixbuf, &scale);
-			*scale_x = scale;
-			*scale_y = scale;
-			g_object_unref (pixbuf);
-			pixbuf = scaled_pixbuf;
+
+		is_thumbnail = strstr (filename, "/.thumbnails/") != NULL;
+
+		size = MAX (gdk_pixbuf_get_width (pixbuf), gdk_pixbuf_get_height (pixbuf));
+
+		if ((is_thumbnail || size > NAUTILUS_ICON_SIZE_THUMBNAIL) && !gdk_pixbuf_get_has_alpha (pixbuf)) {
+			add_frame = TRUE;
 		}
 	}
 
-        if (add_frame){
-                nautilus_thumbnail_frame_image(&pixbuf);
-        }
+	if (add_frame) {
+		nautilus_thumbnail_frame_image(&pixbuf);
+	}
+
 	return pixbuf;
 }
 
Index: libnautilus-private/nautilus-thumbnails.c
===================================================================
RCS file: /cvs/gnome/nautilus/libnautilus-private/nautilus-thumbnails.c,v
retrieving revision 1.56
diff -u -p -r1.56 nautilus-thumbnails.c
--- libnautilus-private/nautilus-thumbnails.c	23 Nov 2005 13:29:34 -0000	1.56
+++ libnautilus-private/nautilus-thumbnails.c	8 Apr 2006 11:16:34 -0000
@@ -30,6 +30,7 @@
 #include "nautilus-global-preferences.h"
 #include "nautilus-icon-factory-private.h"
 #include "nautilus-icon-factory.h"
+#include <math.h>
 #include <eel/eel-gdk-pixbuf-extensions.h>
 #include <eel/eel-graphic-effects.h>
 #include <eel/eel-string.h>
@@ -263,22 +264,145 @@ nautilus_thumbnail_frame_image (GdkPixbu
 	*pixbuf=pixbuf_with_frame;
 }
 
-/* routine to load an image from the passed-in path, and then embed it in
- * a frame if necessary
+typedef struct {
+	gboolean is_thumbnail;
+	guint base_size;
+	guint nominal_size;
+	gboolean force_nominal;
+	int original_height;
+	int original_width;
+	double *scale_x_out;
+	double *scale_y_out;
+} ThumbnailLoadArgs;
+
+static void
+thumbnail_loader_size_prepared (GdkPixbufLoader *loader,
+				int width,
+				int height,
+				ThumbnailLoadArgs *args)
+{
+	int size = MAX (width, height);
+
+	args->original_width = width;
+	args->original_height = height;
+
+	if (args->force_nominal) {
+		args->base_size = MAX (width, height);                        
+	} else if (args->base_size == 0) {
+		if (args->is_thumbnail) {
+			args->base_size = 128 * NAUTILUS_ICON_SIZE_STANDARD / NAUTILUS_ICON_SIZE_THUMBNAIL;
+		} else {
+			if (size > args->nominal_size * NAUTILUS_ICON_SIZE_THUMBNAIL / NAUTILUS_ICON_SIZE_STANDARD) {
+				args->base_size = size * NAUTILUS_ICON_SIZE_STANDARD / NAUTILUS_ICON_SIZE_THUMBNAIL;
+			} else if (size > NAUTILUS_ICON_SIZE_STANDARD) {
+				args->base_size = args->nominal_size;
+			} else {
+				/* Don't scale up small icons */
+				args->base_size = NAUTILUS_ICON_SIZE_STANDARD;
+			}
+		}
+	}
+
+	if (args->base_size != args->nominal_size) {
+		double scale;
+
+		scale = (double) args->nominal_size / args->base_size;
+
+		if ((int) (width * scale) > NAUTILUS_ICON_MAXIMUM_SIZE ||
+		    (int) (height * scale) > NAUTILUS_ICON_MAXIMUM_SIZE) {
+			scale = MIN ((double) NAUTILUS_ICON_MAXIMUM_SIZE / width,
+				     (double) NAUTILUS_ICON_MAXIMUM_SIZE / height);
+		}
+
+		width = MAX (1, floor (width * scale + 0.5));
+		height = MAX (1, floor (height * scale + 0.5));
+
+		gdk_pixbuf_loader_set_size (loader, width, height);
+	}
+
+}
+
+static void
+thumbnail_loader_area_prepared (GdkPixbufLoader *loader,
+				ThumbnailLoadArgs *args)
+{
+	GdkPixbuf *pixbuf;
+
+	pixbuf = gdk_pixbuf_loader_get_pixbuf (loader);
+
+	*args->scale_x_out = gdk_pixbuf_get_width (pixbuf) / args->original_width;
+	*args->scale_y_out = gdk_pixbuf_get_height (pixbuf) / args->original_height;
+}
+
+/* routine to load an image from the passed-in path
  */
 GdkPixbuf *
-nautilus_thumbnail_load_framed_image (const char *path)
+nautilus_thumbnail_load_image (const char *path,
+			       guint base_size,
+			       guint nominal_size,
+			       gboolean force_nominal,
+			       double *scale_x_out,
+			       double *scale_y_out)
 {
+	guchar *buffer;
+	GdkPixbufLoader *loader;
 	GdkPixbuf *pixbuf;
-	
-	pixbuf = gdk_pixbuf_new_from_file (path, NULL);
-	if (pixbuf == NULL) {
+	GError *error;
+	gsize buflen;
+	ThumbnailLoadArgs args;
+
+	error = NULL;
+
+	if (!g_file_get_contents (path, (gchar **) &buffer, &buflen, &error)) {
+		g_message ("Failed to load %s into memory: %s", path, error->message);
+
+		g_error_free (error);
+
 		return NULL;
 	}
-	if (!gdk_pixbuf_get_has_alpha (pixbuf)) {
-		nautilus_thumbnail_frame_image (&pixbuf);
+
+	loader = gdk_pixbuf_loader_new ();
+	g_signal_connect (loader, "size-prepared",
+			  G_CALLBACK (thumbnail_loader_size_prepared),
+			  &args);
+	g_signal_connect (loader, "area-prepared",
+			  G_CALLBACK (thumbnail_loader_area_prepared),
+			  &args);
+
+	args.is_thumbnail = strstr (path, "/.thumbnails/") != NULL;
+	args.base_size = base_size;
+	args.nominal_size = nominal_size;
+	args.force_nominal = force_nominal;
+	args.scale_x_out = scale_x_out;
+	args.scale_y_out = scale_y_out;
+
+	if (!gdk_pixbuf_loader_write (loader, buffer, buflen, &error)) {
+		g_message ("Failed to write %s to thumbnail pixbuf loader: %s", path, error->message);
+
+		gdk_pixbuf_loader_close (loader, NULL);
+		g_object_unref (G_OBJECT (loader));
+		g_error_free (error);
+		g_free (buffer);
+
+		return NULL;
+	}
+
+	if (!gdk_pixbuf_loader_close (loader, &error)) {
+		g_message ("Failed to close thumbnail pixbuf loader for %s: %s", path, error->message);
+
+		g_object_unref (G_OBJECT (loader));
+		g_error_free (error);
+		g_free (buffer);
+
+		return NULL;
 	}
-        return pixbuf;
+
+	pixbuf = g_object_ref (gdk_pixbuf_loader_get_pixbuf (loader));
+
+	g_object_unref (G_OBJECT (loader));
+	g_free (buffer);
+
+	return pixbuf;
 }
 
 void
Index: libnautilus-private/nautilus-thumbnails.h
===================================================================
RCS file: /cvs/gnome/nautilus/libnautilus-private/nautilus-thumbnails.h,v
retrieving revision 1.12
diff -u -p -r1.12 nautilus-thumbnails.h
--- libnautilus-private/nautilus-thumbnails.h	19 Sep 2005 13:51:16 -0000	1.12
+++ libnautilus-private/nautilus-thumbnails.h	8 Apr 2006 11:16:34 -0000
@@ -31,7 +31,12 @@
 /* Returns NULL if there's no thumbnail yet. */
 void       nautilus_create_thumbnail                (NautilusFile *file);
 void       nautilus_thumbnail_frame_image           (GdkPixbuf **pixbuf);
-GdkPixbuf *nautilus_thumbnail_load_framed_image     (const char   *path);
+GdkPixbuf *nautilus_thumbnail_load_image            (const char *path,
+						     guint       base_size,
+						     guint       nominal_size,
+						     gboolean    force_nominal,
+						     double     *scale_x_out,
+						     double     *scale_y_out);
 void       nautilus_update_thumbnail_file_copied    (const char   *source_file_uri,
 						     const char   *destination_file_uri);
 void       nautilus_update_thumbnail_file_renamed   (const char   *source_file_uri,