Re: Using the ImageMagick backend seems to leak memory

[joakim@verona.se]

Tassilo Horn <tassilo@member.fsf.org> writes:

> 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?

The patch seems fine. Im not entirely sure about genesis/terminus
though, I interpreted the docs to mean genesis=library load.


>
> === 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);
>      }
>
>
> Bye,
> Tassilo

-- 
Joakim Verona