Re: Using the ImageMagick backend seems to leak memory

[Tassilo Horn <tassilo@member.fsf.org>]

joakim@verona.se writes:

Hi Joakim,

> I've looked at this a little bit further.
>
> The error is that the image read with ReadImage is cloned by
> NewMagickWandFromImage not refered as I believed when I wrote the
> code.
>
> The code was written that way to avoid another memory problem I
> experienced with image bundles. If the current strategy remains,
> DestroyImage(im_image); should be the right fix.

I've tested that, and now memory is freed as expected.  Still, when I
used doc-view to view a 30 pages PDF (aka 30 large PNGs), the emacs
process started with ~11 MB when showing the first page and was at 12.2
MB after viewing all pages in sequence, killing the doc-view buffer, and
doing (clear-image-cache) and (garbage-collect).  But that might be no
real leak.

> However, the strategy should probably be changed to avoid the cloning.
> This will result in a more complex code path.

Well, you are the ImageMagick master. ;-)

Anyway, should I commit the following patch which includes your
DestroyImage() call that fixes the issue, and I additionally surrounded
image loading with calls to MagickWandGenesis()/MagickWandTerminus(),
because that's the way the API is used throughout the example codes in
the MagickWand API docs?

--8<---------------cut here---------------start------------->8---
=== modified file 'src/image.c'
--- src/image.c	2011-01-07 22:33:32 +0000
+++ src/image.c	2011-01-11 19:02:33 +0000
@@ -7521,6 +7521,9 @@
      image.  Interface :index is same as for GIF.  First we "ping" the
      image to see how many sub-images it contains. Pinging is faster
      than loading the image to find out things about it.  */
+
+  /* MagickWandGenesis() initializes the imagemagick library.  */
+  MagickWandGenesis ();
   image = image_spec_value (img->spec, QCindex, NULL);
   ino = INTEGERP (image) ? XFASTINT (image) : 0;
   ping_wand = NewMagickWand ();
@@ -7549,6 +7552,7 @@
                     img->data.lisp_val));
 
   DestroyMagickWand (ping_wand);
+
   /* Now, after pinging, we know how many images are inside the
      file. If its not a bundle, just one.  */
 
@@ -7566,6 +7570,7 @@
       if (im_image != NULL)
 	{
 	  image_wand = NewMagickWandFromImage (im_image);
+          DestroyImage(im_image);
 	  status = MagickTrue;
 	}
       else
@@ -7576,7 +7581,7 @@
       image_wand = NewMagickWand ();
       status = MagickReadImageBlob (image_wand, contents, size);
     }
-  image_error ("im read failed", Qnil, Qnil);
+
   if (status == MagickFalse) goto imagemagick_error;
 
   /* If width and/or height is set in the display spec assume we want
@@ -7805,11 +7810,13 @@
 
   /* Final cleanup. image_wand should be the only resource left. */
   DestroyMagickWand (image_wand);
+  MagickWandTerminus ();
 
   return 1;
 
  imagemagick_error:
   DestroyMagickWand (image_wand);
+  MagickWandTerminus ();
   /* TODO more cleanup.  */
   image_error ("Error parsing IMAGEMAGICK image `%s'", img->spec, Qnil);
   return 0;
@@ -8681,8 +8688,6 @@
 #if defined (HAVE_IMAGEMAGICK)
   if (EQ (type, Qimagemagick))
     {
-      /* MagickWandGenesis() initializes the imagemagick library.  */
-      MagickWandGenesis ();
       return CHECK_LIB_AVAILABLE (&imagemagick_type, init_imagemagick_functions,
 				  libraries);
     }

--8<---------------cut here---------------end--------------->8---

Bye,
Tassilo