bug#10112: ImageMagick doesn't display some image formats

bug#10112: ImageMagick doesn't display some image formats

[Juri Linkov]

Visiting images whose format is supported by ImageMagick displays
a small empty box.

Read-only formats like .dot have only read-functions, but not write-functions
(the error being "no encode delegate for this image format").

`imagemagick_load_image' calls `MagickPingImage' to read image attributes.
But since before this calls there is an other call to `MagickSetResolution',
`MagickPingImage' assumes that we are going to modify the image, and returns
the `MagickFalse' status.

Removing `MagickSetResolution' cures this problem.  but I hesitate to remove it,
because I don't know why it's here.  So at least moving `MagickSetResolution'
a few lines below and calling after `MagickPingImage' will allow the read-only
images to be correctly displayed:

=== modified file 'src/image.c'
--- src/image.c	2011-11-21 22:14:28 +0000
+++ src/image.c	2011-11-22 22:01:00 +0000
@@ -7618,7 +7618,6 @@ imagemagick_load_image
   image = image_spec_value (img->spec, QCindex, NULL);
   ino = INTEGERP (image) ? XFASTINT (image) : 0;
   ping_wand = NewMagickWand ();
-  MagickSetResolution (ping_wand, 2, 2);
   if (filename != NULL)
     {
       status = MagickPingImage (ping_wand, filename);
@@ -7628,6 +7627,8 @@ (at your option) any later version.
       status = MagickPingImageBlob (ping_wand, contents, size);
     }
 
+  MagickSetResolution (ping_wand, 2, 2);
+
   if (! (0 <= ino && ino < MagickGetNumberImages (ping_wand)))
     {
       image_error ("Invalid image number `%s' in image `%s'",

bug#10112: ImageMagick doesn't display some image formats

[Juri Linkov]

> So at least moving `MagickSetResolution' a few lines below and calling after
> `MagickPingImage' will allow the read-only images to be correctly displayed:

Patch installed.

> Read-only formats like .dot have only read-functions, but not write-functions
> (the error being "no encode delegate for this image format").

I think we should expose ImageMagick error messages to Emacs.
The patch below adds a new function `imagemagick_error' that is
called in places where the ImageMagick returns `MagickFalse' status.
It puts the error message to the *Messages* buffer.

This function helps to understand why ImageMagick fails to display
images.  For instance, now with more informative error message
it reveals why Gnus can't display some ImageMagick formats
demonstrated at http://debbugs.gnu.org/9044#41
The error message is:

  no decode delegate for this image format `' @ error/blob.c/BlobToImage/349

This means that ImageMagick is unable to read a TGA image from Blob,
i.e. when the image is defined by the :data tag like Gnus does.
OTOH, when a TGA image is defined by the :file tag, then ImageMagick
can read it and display correctly.

We could try to write an image to a temporary file before displaying it,
but I'm not sure if it's worth the trouble to overcome shortcomings
of ImageMagick.

=== modified file 'src/image.c'
--- src/image.c	2011-11-24 19:02:39 +0000
+++ src/image.c	2011-11-24 19:08:18 +0000
@@ -7564,6 +7564,19 @@ (at your option) any later version.
 					    MagickPixelPacket *);
 #endif
 
+static void
+imagemagick_error (MagickWand *wand)
+{
+  char *description;
+  ExceptionType severity;
+
+  description = MagickGetException (wand,&severity);
+  image_error ("ImageMagick error: %s",
+	       make_string (description, strlen (description)),
+	       Qnil);
+  description = (char *) MagickRelinquishMemory (description);
+}
+
 /* Helper function for imagemagick_load, which does the actual loading
    given contents and size, apart from frame and image structures,
    passed from imagemagick_load.  Uses librimagemagick to do most of
@@ -7628,6 +7641,13 @@ (at your option) any later version.
       status = MagickPingImageBlob (ping_wand, contents, size);
     }
 
+  if (status == MagickFalse)
+    {
+      imagemagick_error (ping_wand);
+      DestroyMagickWand (ping_wand);
+      return 0;
+    }
+
   MagickSetResolution (ping_wand, 2, 2);
 
   if (! (0 <= ino && ino < MagickGetNumberImages (ping_wand)))
@@ -7669,7 +7689,10 @@ (at your option) any later version.
     {
       image_wand = NewMagickWand ();
       if (MagickReadImageBlob (image_wand, contents, size) == MagickFalse)
-	goto imagemagick_error;
+	{
+	  imagemagick_error (image_wand);
+	  goto imagemagick_error;
+	}
     }
 
   /* If width and/or height is set in the display spec assume we want
@@ -7697,6 +7720,7 @@ (at your option) any later version.
       if (status == MagickFalse)
 	{
 	  image_error ("Imagemagick scale failed", Qnil, Qnil);
+	  imagemagick_error (image_wand);
 	  goto imagemagick_error;
 	}
     }
@@ -7751,6 +7775,7 @@ (at your option) any later version.
       if (status == MagickFalse)
         {
           image_error ("Imagemagick image rotate failed", Qnil, Qnil);
+	  imagemagick_error (image_wand);
           goto imagemagick_error;
         }
     }

bug#10112: ImageMagick doesn't display some image formats

[Wolfgang Jenkner]

Juri Linkov <juri@jurta.org> writes:

> This means that ImageMagick is unable to read a TGA image from Blob,
> i.e. when the image is defined by the :data tag like Gnus does.
> OTOH, when a TGA image is defined by the :file tag, then ImageMagick
> can read it and display correctly.

Isn't this simply because image-type-header-regexps doesn't contain an
entry to detect that image format?  E.g., I have to explicitly
(add-to-list 'image-type-header-regexps '("\\`\377\330" . imagemagick))
to have image-mode use imagemagick to display a jpeg file.

Wolfgang

bug#10112: ImageMagick doesn't display some image formats

[Wolfgang Jenkner]

Wolfgang Jenkner <wjenkner@inode.at> writes:

>  E.g., I have to explicitly
> (add-to-list 'image-type-header-regexps '("\\`\377\330" . imagemagick))
> to have image-mode use imagemagick to display a jpeg file.

Sorry, this seems to be another problem...

Wolfgang

bug#10112: ImageMagick doesn't display some image formats

[Juri Linkov]

>>  E.g., I have to explicitly
>> (add-to-list 'image-type-header-regexps '("\\`\377\330" . imagemagick))
>> to have image-mode use imagemagick to display a jpeg file.
>
> Sorry, this seems to be another problem...

Yes, this is another problem.  Gnus doesn't use `image-type-header-regexps'.
mm-decode.el creates an image by calling `create-image' with `imagemagick'
and `data-p' arguments, so `image-type' doesn't call `image-type-from-data'.

But this is still a problem.  Currently we can't view images supported
by ImageMagick when they are visited from archives.  That's because of
the problem you mentioned: there are no ImageMagick headers in
`image-type-header-regexps'.

Another problem is that we can't use ImageMagick transformations on
default image types JPEG, GIF, PNG because they are not visited
with the help of ImageMagick.

bug#10112: ImageMagick doesn't display some image formats

[Juri Linkov]

> But this is still a problem.  Currently we can't view images supported
> by ImageMagick when they are visited from archives.  That's because of
> the problem you mentioned: there are no ImageMagick headers in
> `image-type-header-regexps'.

Since we have only filename extensions to detect ImageMagick-only formats,
but not file headers, one way to solve this problem is to try matching
buffer names when matching file names fails.  This can't be done without
affecting other functionality, so perhaps this should be postponed to 24.2.

> Another problem is that we can't use ImageMagick transformations on
> default image types JPEG, GIF, PNG because they are not visited
> with the help of ImageMagick.

This problem can be fixed without affecting other functionality.
It is localized to just one function `imagemagick-register-types'
so it's safe to install for 24.1.

The patch below does for `image-type-header-regexps' exactly the same thing
as already is done for `image-type-file-name-regexps' - adding entries
that match ImageMagick image formats.  (However, there is one difference
where unlike grouping filename regexps into one regexp with `regexp-opt'
can't be used to group file header regexps; I don't why but the result of
`regexp-opt' matches nothing; maybe because these regexps are quite complicated).

=== modified file 'lisp/image.el'
--- lisp/image.el	2011-09-16 13:46:42 +0000
+++ lisp/image.el	2011-12-15 23:48:03 +0000
@@ -32,7 +32,7 @@ (defgroup image ()
 
 (defalias 'image-refresh 'image-flush)
 
-(defconst image-type-header-regexps
+(defvar image-type-header-regexps
   `(("\\`/[\t\n\r ]*\\*.*XPM.\\*/" . xpm)
     ("\\`P[1-6][[:space:]]+\\(?:#.*[[:space:]]+\\)*[0-9]+[[:space:]]+[0-9]+" . pbm)
     ("\\`GIF8[79]a" . gif)
@@ -702,7 +704,14 @@ (defun imagemagick-register-types ()
       (let ((extension (concat "\\." (regexp-opt im-types) "\\'")))
         (push (cons extension 'image-mode) auto-mode-alist)
         (push (cons extension 'imagemagick)
-              image-type-file-name-regexps)))))
+              image-type-file-name-regexps))
+      (dolist (header-regexp image-type-header-regexps)
+        (let ((im-type (cdr header-regexp)))
+	  (when (and (member (symbol-name im-type) im-types)
+		     (not (memq (intern (upcase (symbol-name im-type)))
+				imagemagick-types-inhibit)))
+	    (push (cons (car header-regexp) 'imagemagick)
+		  image-type-header-regexps)))))))
 
 (provide 'image)

bug#10112: ImageMagick doesn't display some image formats

[Juri Linkov]

I'm starting to adapt this patch to the recent changes in bug#11557.

I propose the following modifications:

1. Currently the priority of specialized image libraries over
ImageMagick is hard-coded in `imagemagick-register-types' as:

	  ;; Append to `image-type-file-name-regexps', so that we
	  ;; preferentially use specialized image libraries.
	  (add-to-list 'image-type-file-name-regexps
	  	       (cons re 'imagemagick) t)

It could use a new customizable variable where the user can specify
whether to use ImageMagick for all enabled image types, or use
available specialized libraries.  The same option could also specify the
priority of `imagemagick' elements in `image-type-header-regexps'.

Or maybe this new option is not necessary, and `imagemagick'
should have the highest priority.  When the user wants to use
specialized image libraries, it's possible to remove conflicting
image types (such as jpeg/png/gif) from `imagemagick-enabled-types'.

2. The most user-friendly UI to enable image types
is to allow the user to select a set of enabled formats
from the list of all available formats displayed as checkboxes.
This is like during installation of some graphical programs
when they present a list of available image format to allow the user
to associate them with file extensions.

It's possible to do this in Customization UI using the following type
for `imagemagick-enabled-types':

  :type (cons 'set (mapcar
		    (lambda (type)
		      (list 'const type))
		    (sort (imagemagick-types) 'string<)))

3. `imagemagick-types-inhibit' could use the same customization type,
and it should be renamed to `imagemagick-disabled-types' or completely
removed, with adding a new element to `imagemagick-enabled-types'
that will specify whether the list excludes/includes the defined types.

bug#10112: ImageMagick doesn't display some image formats

[Lars Magne Ingebrigtsen]

Juri Linkov <juri@jurta.org> writes:

> -(defconst image-type-header-regexps
> +(defvar image-type-header-regexps
>    `(("\\`/[\t\n\r ]*\\*.*XPM.\\*/" . xpm)
>      ("\\`P[1-6][[:space:]]+\\(?:#.*[[:space:]]+\\)*[0-9]+[[:space:]]+[0-9]+" . pbm)
>      ("\\`GIF8[79]a" . gif)
> @@ -702,7 +704,14 @@ (defun imagemagick-register-types ()
>        (let ((extension (concat "\\." (regexp-opt im-types) "\\'")))
>          (push (cons extension 'image-mode) auto-mode-alist)
>          (push (cons extension 'imagemagick)
> -              image-type-file-name-regexps)))))
> +              image-type-file-name-regexps))
> +      (dolist (header-regexp image-type-header-regexps)
> +        (let ((im-type (cdr header-regexp)))
> +	  (when (and (member (symbol-name im-type) im-types)
> +		     (not (memq (intern (upcase (symbol-name im-type)))
> +				imagemagick-types-inhibit)))
> +	    (push (cons (car header-regexp) 'imagemagick)
> +		  image-type-header-regexps)))))))
>
>  (provide 'image)

This bug was not installed after Emacs 24.1 was released, but it was
marked as "pending".  Is it still applicable? 

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#10112: ImageMagick doesn't display some image formats

[Juri Linkov]

> This bug was not installed after Emacs 24.1 was released, but it was
> marked as "pending".  Is it still applicable?

I see that this patch was obsoleted by your revision 81d0eae7
that forces the imagemagick type in image-mode.  At the moment
I'm not sure whether the same handling is needed outside of image-mode.

bug#10112: ImageMagick doesn't display some image formats

[Lars Magne Ingebrigtsen]

Juri Linkov <juri@jurta.org> writes:

>> This bug was not installed after Emacs 24.1 was released, but it was
>> marked as "pending".  Is it still applicable?
>
> I see that this patch was obsoleted by your revision 81d0eae7
> that forces the imagemagick type in image-mode.  At the moment
> I'm not sure whether the same handling is needed outside of image-mode.

That forcing will probably go away once I implement the general image
rescaling code I have planned...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no

bug#10112: ImageMagick doesn't display some image formats

[Juri Linkov]

> The patch below adds a new function `imagemagick_error' that is
> called in places where the ImageMagick returns `MagickFalse' status.

Patch installed.

> Removing `MagickSetResolution' cures this problem.  but I hesitate to
> remove it, because I don't know why it's here.  So at least moving
> `MagickSetResolution' a few lines below and calling after
> `MagickPingImage' will allow the read-only images to be correctly
> displayed:

Looking closer at the current situation, I discovered that
using `MagickSetResolution' is suggested by
http://www.imagemagick.org/discourse-server/viewtopic.php?f=2&t=16502
But this hack precludes from displaying some other image formats.
It is intended for loading multi-page djvu images.  Since the
functionality for browsing them is not implemented anyway, it can be
commented out until implemented in 24.2.

The relevant info from etc/TODO:

  *** For some reason its unbearably slow to look at a page in a large
  image bundle using the :index feature.  The ImageMagick "display"
  command is also a bit slow, but nowhere near as slow as the Emacs
  code.  It seems ImageMagick tries to unpack every page when loading the
  bundle.  This feature is not the primary usecase in Emacs though.

  ImageMagick 6.6.2-9 introduced a bugfix for single page djvu load.  It
  is now much faster to use the :index feature, but still not very fast.