Re: Using the ImageMagick backend seems to leak memory

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

joakim@verona.se writes:

Hi Joakim,

>> this is driving me nuts.  I've already tried modifying
>> imagemagick_load_image() with some messages, and so that it always
>> does MagickWandGenesis() first and MagickWandTerminus() after
>> destroying the last MagickWand.  It does so, but that didn't help...

[patch below]

>> One strangeness I found is the line
>>
>>     image_error ("im read failed", Qnil, Qnil);
>>
>> which is called unconditionally in imagemagick_load_image().  Why?
>
> Thats supposed to be an error condition exit only.
> Isnt there a return statement before?

No, neither before nor after.

> I havent yet tried to recreate your problem, but my experience when I
> worked on the patch was that I got memory issues when working with
> image bundle files.

That's not the case here.  I load one single PNG (or whatever) file, or
in the case of doc-view, several PNG files one after the other.

> Also the imagemagick team was very helpful in providing fixes in their
> code.

I tried to get in contact, but my posting via Gmane was rejected and I
got no response on ##imagemagick.  Then I tried to register with their
web forum, but I still haven't got the confirmation mail...

> Have you tried to install a development version of imagemagick?

No, I'm running the released version 6.6.5.6.  And as I've said in
another mail, the MagickWand example code with my small changes works
just fine with that version, i.e., memory is given back...

And the basic order of MagickWand function invocations applied in emacs
seems to match the example.  Maybe some emacs/X structures share data
with MagickWand structures, so that the latter cannot be freed when
destroying the MagickWand or calling MagickWandTerminus?

Bye,
Tassilo

Here's a patch with my unsuccessful changes to image.c.  Maybe it's of
some use...

--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 08:11:07 +0000
@@ -7521,9 +7521,12 @@
      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 ();
+  message ("MagickWandGenesis ();");
   image = image_spec_value (img->spec, QCindex, NULL);
   ino = INTEGERP (image) ? XFASTINT (image) : 0;
   ping_wand = NewMagickWand ();
+  message ("ping_wand = NewMagickWand ();");
   MagickSetResolution (ping_wand, 2, 2);
   if (filename != NULL)
     {
@@ -7539,6 +7542,7 @@
       image_error ("Invalid image number `%s' in image `%s'",
 		   image, img->spec);
       DestroyMagickWand (ping_wand);
+      message ("DestroyMagickWand (ping_wand);");
       return 0;
     }
 
@@ -7549,6 +7553,9 @@
                     img->data.lisp_val));
 
   DestroyMagickWand (ping_wand);
+  message ("DestroyMagickWand (ping_wand);");
+
+
   /* Now, after pinging, we know how many images are inside the
      file. If its not a bundle, just one.  */
 
@@ -7566,6 +7573,7 @@
       if (im_image != NULL)
 	{
 	  image_wand = NewMagickWandFromImage (im_image);
+	  message ("image_wand = NewMagickWand ();");
 	  status = MagickTrue;
 	}
       else
@@ -7574,6 +7582,7 @@
   else
     {
       image_wand = NewMagickWand ();
+      message ("image_wand = NewMagickWand ();");
       status = MagickReadImageBlob (image_wand, contents, size);
     }
   image_error ("im read failed", Qnil, Qnil);
@@ -7805,11 +7814,18 @@
 
   /* Final cleanup. image_wand should be the only resource left. */
   DestroyMagickWand (image_wand);
+  message ("DestroyMagickWand (image_wand);");
+  MagickWandTerminus ();
+  message ("MagickWandTerminus ();");
 
   return 1;
 
  imagemagick_error:
   DestroyMagickWand (image_wand);
+  message ("Error case: DestroyMagickWand (image_wand);");
+  MagickWandTerminus ();
+  message ("MagickWandTerminus ();");
+
   /* TODO more cleanup.  */
   image_error ("Error parsing IMAGEMAGICK image `%s'", img->spec, Qnil);
   return 0;
@@ -8681,8 +8697,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---