GNU bug report logs - #39994
27.0.90; Broken image-converter probe for imagemagick

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 39994 in the body.
You can then email your comments to 39994 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: bug-gnu-emacs <at> gnu.org
Subject: 27.0.90; Broken image-converter probe for imagemagick
Date: Mon, 09 Mar 2020 02:09:05 +0200

Tags: patch

I tried to visit an .ico file using graphicsmagick image-converter,
but it failed with the error:

  Cannot display image: (/usr/bin/gm convert: Unexpected end-of-file ().

Probably a bug in graphicsmagick, so we can do nothing to fix this.

Then I tried imagemagick, but image-converter said .ico format is unsupported.
Whereas running `convert -list format` outputs:

   Format  Module    Mode  Description
-------------------------------------------------------------------------------
      ICO* ICON      rw+   Microsoft icon

So this patch adds the support for the 'Module' column to imagemagick probe:

diff --git a/lisp/image/image-converter.el b/lisp/image/image-converter.el
index 0488a13d41..5843b2a399 100644
--- a/lisp/image/image-converter.el
+++ b/lisp/image/image-converter.el
@@ -44,8 +44,8 @@ image-converter-regexp
 
 (defvar image-converter--converters
   '((graphicsmagick :command ("gm" "convert") :probe ("-list" "format"))
-    (ffmpeg :command "ffmpeg" :probe "-decoders")
-    (imagemagick :command "convert" :probe ("-list" "format")))
+    (imagemagick :command "convert" :probe ("-list" "format"))
+    (ffmpeg :command "ffmpeg" :probe "-decoders"))
   "List of supported image converters to try.")
 
 (defun image-convert-p (source &optional data-p)
@@ -150,7 +150,7 @@ image-converter--probe
         (forward-line 1)
         ;; Lines look like
         ;; "      WPG* r--   Word Perfect Graphics".
-        (while (re-search-forward "^ *\\([A-Z0-9]+\\)\\*? +r" nil t)
+        (while (re-search-forward "^ *\\([A-Z0-9]+\\)\\*?\\(?: +[A-Z0-9]+\\)? +r" nil t)
           (push (downcase (match-string 1)) formats)))
       (nreverse formats))))
 

Message #8 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Mon, 09 Mar 2020 10:15:03 +0100

Juri Linkov <juri <at> linkov.net> writes:

> Then I tried imagemagick, but image-converter said .ico format is unsupported.
> Whereas running `convert -list format` outputs:
>
>    Format  Module    Mode  Description
> -------------------------------------------------------------------------------
>       ICO* ICON      rw+   Microsoft icon
>
> So this patch adds the support for the 'Module' column to imagemagick probe:

Ah, so some versions have an additional column in there?  My convert
-list format outputs:

      ICO* rw+   Microsoft icon

>  (defvar image-converter--converters
>    '((graphicsmagick :command ("gm" "convert") :probe ("-list" "format"))
> -    (ffmpeg :command "ffmpeg" :probe "-decoders")
> -    (imagemagick :command "convert" :probe ("-list" "format")))
> +    (imagemagick :command "convert" :probe ("-list" "format"))
> +    (ffmpeg :command "ffmpeg" :probe "-decoders"))
>    "List of supported image converters to try.")

Was this part included by mistake?  It changes the order the converters
are tested.

>          ;; Lines look like
>          ;; "      WPG* r--   Word Perfect Graphics".
> -        (while (re-search-forward "^ *\\([A-Z0-9]+\\)\\*? +r" nil t)
> +        (while (re-search-forward "^ *\\([A-Z0-9]+\\)\\*?\\(?: +[A-Z0-9]+\\)? +r" nil t)

Look OK to me, but the comment should be amended to reflect the two
different line formats it's now matching.

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

Message #11 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Tue, 10 Mar 2020 00:43:44 +0200

>> Then I tried imagemagick, but image-converter said .ico format is unsupported.
>> Whereas running `convert -list format` outputs:
>>
>>    Format  Module    Mode  Description
>> -------------------------------------------------------------------------------
>>       ICO* ICON      rw+   Microsoft icon
>>
>> So this patch adds the support for the 'Module' column to imagemagick probe:
>
> Ah, so some versions have an additional column in there?  My convert
> -list format outputs:
>
>       ICO* rw+   Microsoft icon

Maybe because my version is too old:
ImageMagick 6.9.7-4 Q16 x86_64 20170114

>>  (defvar image-converter--converters
>>    '((graphicsmagick :command ("gm" "convert") :probe ("-list" "format"))
>> -    (ffmpeg :command "ffmpeg" :probe "-decoders")
>> -    (imagemagick :command "convert" :probe ("-list" "format")))
>> +    (imagemagick :command "convert" :probe ("-list" "format"))
>> +    (ffmpeg :command "ffmpeg" :probe "-decoders"))
>>    "List of supported image converters to try.")
>
> Was this part included by mistake?  It changes the order the converters
> are tested.

I propose to change the order because ffmpeg doesn't support too much
image formats, so it is less useful than imagemagick when both
imagemagick and ffmpeg are installed at the same time.

>>          ;; Lines look like
>>          ;; "      WPG* r--   Word Perfect Graphics".
>> -        (while (re-search-forward "^ *\\([A-Z0-9]+\\)\\*? +r" nil t)
>> +        (while (re-search-forward "^ *\\([A-Z0-9]+\\)\\*?\\(?: +[A-Z0-9]+\\)? +r" nil t)
>
> Look OK to me, but the comment should be amended to reflect the two
> different line formats it's now matching.

Oh, I haven't noticed the comment, I don't know why, now updated:

diff --git a/lisp/image/image-converter.el b/lisp/image/image-converter.el
index 0488a13d41..3d74b8b30c 100644
--- a/lisp/image/image-converter.el
+++ b/lisp/image/image-converter.el
@@ -149,8 +149,9 @@ image-converter--probe
       (when (re-search-forward "^-" nil t)
         (forward-line 1)
         ;; Lines look like
-        ;; "      WPG* r--   Word Perfect Graphics".
-        (while (re-search-forward "^ *\\([A-Z0-9]+\\)\\*? +r" nil t)
+        ;; "      WPG* r--   Word Perfect Graphics" or
+        ;; "      WPG* WPG       r--   Word Perfect Graphics".
+        (while (re-search-forward "^ *\\([A-Z0-9]+\\)\\*?\\(?: +[A-Z0-9]+\\)? +r" nil t)
           (push (downcase (match-string 1)) formats)))
       (nreverse formats))))
 

Message #14 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Sat, 14 Mar 2020 13:02:03 +0100

Juri Linkov <juri <at> linkov.net> writes:

> I propose to change the order because ffmpeg doesn't support too much
> image formats, so it is less useful than imagemagick when both
> imagemagick and ffmpeg are installed at the same time.

We put imagemagick at the end because of the same reason we're
deprecating libmagick in Emacs -- imagemagick has had en enormous number
of exploitable bugs over the years.  Running external programs mitigates
this slightly, but it should remain the last option.

> Oh, I haven't noticed the comment, I don't know why, now updated:

Looks good to me; please apply.

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

Message #17 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Sun, 15 Mar 2020 02:00:12 +0200

tags 39994 fixed
close 39994 27.1
quit

>> I propose to change the order because ffmpeg doesn't support too much
>> image formats, so it is less useful than imagemagick when both
>> imagemagick and ffmpeg are installed at the same time.
>
> We put imagemagick at the end because of the same reason we're
> deprecating libmagick in Emacs -- imagemagick has had en enormous number
> of exploitable bugs over the years.  Running external programs mitigates
> this slightly, but it should remain the last option.

Right.  I moved it up only because I was lazy to restart Emacs -
currently customization of image-converter requires restarting Emacs,
but it's not a big problem.

>> Oh, I haven't noticed the comment, I don't know why, now updated:
>
> Looks good to me; please apply.

Done.

Message #24 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Mon, 16 Mar 2020 02:04:58 +0200

reopen 39994
tags 39994 - fixed
quit

>> We put imagemagick at the end because of the same reason we're
>> deprecating libmagick in Emacs -- imagemagick has had en enormous number
>> of exploitable bugs over the years.  Running external programs mitigates
>> this slightly, but it should remain the last option.
>
> Right.  I moved it up only because I was lazy to restart Emacs -
> currently customization of image-converter requires restarting Emacs,
> but it's not a big problem.

Sorry, I misremembered - actually it *is* a problem,
since handling customization is not yet implemented.

Here is a possible implementation:

diff --git a/lisp/image/image-converter.el b/lisp/image/image-converter.el
index 0488a13d41..4efae5c202 100644
--- a/lisp/image/image-converter.el
+++ b/lisp/image/image-converter.el
@@ -57,6 +57,10 @@ image-convert-p
   ;; Find an installed image converter.
   (unless image-converter
     (image-converter--find-converter))
+  ;; When image-converter was customized
+  (if (and image-converter (not image-converter-regexp))
+      (when-let ((formats (image-converter--probe image-converter)))
+        (setq image-converter-regexp (concat "\\." (regexp-opt formats) "\\'"))))
   (and image-converter
        (or (and (not data-p)
                 (string-match image-converter-regexp source))

Message #27 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Mon, 16 Mar 2020 02:23:44 +0200

Since now it's pretest time, I gave it more testing, and found more problems:

1. AFAIR one of the goals for creating image-converter.el
was to handle such rare image formats as WEBP,
but I tried to open a webp file, and image-converter failed
because it doesn't recognize WEBP.

There is no WEBP mentioned in the output of "identify -list format".
After installing `apt-get install webp`, another command
"identify -list delegate" reports its support with:

  Delegate                Command
  -------------------------------------------------------------------------------
         webp =>          "dwebp' -pam '%i' -o '%o"
          png<= webp      "cwebp' -quiet %Q '%i' -o '%o"

2. After adding manually webp to image-converter-regexp,
there is another problem: image-converter--convert-magick
calls the command with

  (apply #'call-process (car command)
         nil t nil

where the arg 't' means to mix standard error output with ordinary output,
but ImageMagick outputs some info messages to stderr, e.g.:

  Decoded /tmp/magick-20114vaPD-fxUjRW4. Dimensions: 320 x 214 . Format: lossy. Now saving...
  Saved file /tmp/magick-20114h1Jh0D04beDR

thus breaking the image output.

3. Visiting an image file from an archive signals the error

  Cannot display image: (IMAGE-FORMAT should be a symbol like ‘image/png’)

4. Exif fails to visit images with the error:

  Cannot display image: (sequencep 122)

Shouldn't exif code be called with ignore-errors, so its errors won't
affect the image displaying?

Test case:

  exif --output=blackz.jpg --tag=Artist --ifd=0 --set-value='z' test/data/image/black.jpg

Exif fails to handle ASCII field values whose length is less than 4.
In the above example the length of the 'Artist' field is 1 ('z').

Message #34 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Mon, 30 Mar 2020 02:16:28 +0300

> diff --git a/lisp/image/image-converter.el b/lisp/image/image-converter.el
> index 0488a13d41..4efae5c202 100644
> --- a/lisp/image/image-converter.el
> +++ b/lisp/image/image-converter.el
> @@ -57,6 +57,10 @@ image-convert-p
>    ;; Find an installed image converter.
>    (unless image-converter
>      (image-converter--find-converter))
> +  ;; When image-converter was customized
> +  (if (and image-converter (not image-converter-regexp))
> +      (when-let ((formats (image-converter--probe image-converter)))
> +        (setq image-converter-regexp (concat "\\." (regexp-opt formats) "\\'"))))
>    (and image-converter
>         (or (and (not data-p)
>                  (string-match image-converter-regexp source))

Pushed this fix to emacs-27.

Other (apparently more minor) problems could be fixed later.

Message #37 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Sun, 02 Aug 2020 09:56:33 +0200

Juri Linkov <juri <at> linkov.net> writes:

> Since now it's pretest time, I gave it more testing, and found more problems:
>
> 1. AFAIR one of the goals for creating image-converter.el
> was to handle such rare image formats as WEBP,
> but I tried to open a webp file, and image-converter failed
> because it doesn't recognize WEBP.
>
> There is no WEBP mentioned in the output of "identify -list format".
> After installing `apt-get install webp`, another command
> "identify -list delegate" reports its support with:
>
>   Delegate                Command
>   -------------------------------------------------------------------------------
>          webp =>          "dwebp' -pam '%i' -o '%o"
>           png<= webp      "cwebp' -quiet %Q '%i' -o '%o"

My imagemagick says:

$ identify -list format | grep -i webp
     WEBP* WEBP      rw-   WebP Image Format (libwebp 0.6.1[0208])

So I guess that you have a too-old imagemagick installation?  I don't
really think this is an image-converter.el bug, though -- it's best
effort, and if you don't have external programs to display these things,
then it fails.

> 2. After adding manually webp to image-converter-regexp,
> there is another problem: image-converter--convert-magick
> calls the command with
>
>   (apply #'call-process (car command)
>          nil t nil
>
> where the arg 't' means to mix standard error output with ordinary output,
> but ImageMagick outputs some info messages to stderr, e.g.:
>
>   Decoded /tmp/magick-20114vaPD-fxUjRW4. Dimensions: 320 x 214 . Format: lossy. Now saving...
>   Saved file /tmp/magick-20114h1Jh0D04beDR
>
> thus breaking the image output.

Yes, it's a pain that we can't direct stderr to its own buffer.  Is
there any reason why?  We don't want to write this stuff to a file
(which is allowed), because of the problems with clean-up.

> 3. Visiting an image file from an archive signals the error
>
>   Cannot display image: (IMAGE-FORMAT should be a symbol like ‘image/png’)

Do you have a test case for this?

> 4. Exif fails to visit images with the error:
>
>   Cannot display image: (sequencep 122)
>
> Shouldn't exif code be called with ignore-errors, so its errors won't
> affect the image displaying?
>
> Test case:
>
>   exif --output=blackz.jpg --tag=Artist --ifd=0 --set-value='z' test/data/image/black.jpg

I seem to recall fixing this, and this test case doesn't fail for me (in
Emacs 28.1).

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

Message #40 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Mon, 03 Aug 2020 02:31:39 +0300

>> 1. AFAIR one of the goals for creating image-converter.el
>> was to handle such rare image formats as WEBP,
>> but I tried to open a webp file, and image-converter failed
>> because it doesn't recognize WEBP.
>>
>> There is no WEBP mentioned in the output of "identify -list format".
>> After installing `apt-get install webp`, another command
>> "identify -list delegate" reports its support with:
>>
>>   Delegate                Command
>>   -------------------------------------------------------------------------------
>>          webp =>          "dwebp' -pam '%i' -o '%o"
>>           png<= webp      "cwebp' -quiet %Q '%i' -o '%o"
>
> My imagemagick says:
>
> $ identify -list format | grep -i webp
>      WEBP* WEBP      rw-   WebP Image Format (libwebp 0.6.1[0208])
>
> So I guess that you have a too-old imagemagick installation?  I don't
> really think this is an image-converter.el bug, though -- it's best
> effort, and if you don't have external programs to display these things,
> then it fails.

I already upgraded to the latest version, and now the output is exactly
the same as yours above.  Then even without installing the Debian
package 'webp', visiting a webp file shows it just fine as an image with
"Image[image-convert]" in the mode-line.  So there is no problem anymore.

>> 2. After adding manually webp to image-converter-regexp,
>> there is another problem: image-converter--convert-magick
>> calls the command with
>>
>>   (apply #'call-process (car command)
>>          nil t nil
>>
>> where the arg 't' means to mix standard error output with ordinary output,
>> but ImageMagick outputs some info messages to stderr, e.g.:
>>
>>   Decoded /tmp/magick-20114vaPD-fxUjRW4. Dimensions: 320 x 214 . Format: lossy. Now saving...
>>   Saved file /tmp/magick-20114h1Jh0D04beDR
>>
>> thus breaking the image output.
>
> Yes, it's a pain that we can't direct stderr to its own buffer.  Is
> there any reason why?  We don't want to write this stuff to a file
> (which is allowed), because of the problems with clean-up.

I can't reproduce this problem anymore since webp opens without an error.

>> 3. Visiting an image file from an archive signals the error
>>
>>   Cannot display image: (IMAGE-FORMAT should be a symbol like ‘image/png’)
>
> Do you have a test case for this?

The test case is to zip a png file and a webp file.
Then visiting a png file in the archive displays the image,
whereas visiting a webp file signals the error:

  "Unknown image type; consider switching ‘image-use-external-converter’ on"

But the value of 'image-use-external-converter' is already 't'.

>> 4. Exif fails to visit images with the error:
>>
>>   Cannot display image: (sequencep 122)
>>
>> Shouldn't exif code be called with ignore-errors, so its errors won't
>> affect the image displaying?
>>
>> Test case:
>>
>>   exif --output=blackz.jpg --tag=Artist --ifd=0 --set-value='z' test/data/image/black.jpg
>
> I seem to recall fixing this, and this test case doesn't fail for me (in
> Emacs 28.1).

Yes, you already fixed it.  So it looks like everything is fixed here,
and the feature request to support image-converter in archives
could be moved to a separate bug#.

PS: Also 'image-next-file' currently ignores webp files in Dired-mode.

Message #43 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Lars Ingebrigtsen <larsi <at> gnus.org>
To: Juri Linkov <juri <at> linkov.net>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Mon, 03 Aug 2020 09:15:42 +0200

Juri Linkov <juri <at> linkov.net> writes:

>>> 3. Visiting an image file from an archive signals the error
>>>
>>>   Cannot display image: (IMAGE-FORMAT should be a symbol like ‘image/png’)
>>
>> Do you have a test case for this?
>
> The test case is to zip a png file and a webp file.
> Then visiting a png file in the archive displays the image,
> whereas visiting a webp file signals the error:
>
>   "Unknown image type; consider switching ‘image-use-external-converter’ on"
>
> But the value of 'image-use-external-converter' is already 't'.

I've now fixed the message (there may be image formats even the external
converter can't do, and in that case the message was wrong), and I also
fixed it so that it works (i.e., you can now visit .webp files from .zip
files).

> PS: Also 'image-next-file' currently ignores webp files in Dired-mode.

Fixed now.  :-)

So I think everything here has been fixed (mostly), so I'm closing this
bug report.

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

Message #50 received at 39994 <at> debbugs.gnu.org (full text, mbox):

From: Juri Linkov <juri <at> linkov.net>
To: Lars Ingebrigtsen <larsi <at> gnus.org>
Cc: 39994 <at> debbugs.gnu.org
Subject: Re: bug#39994: 27.0.90; Broken image-converter probe for imagemagick
Date: Tue, 04 Aug 2020 02:42:00 +0300

> So I think everything here has been fixed (mostly), so I'm closing this
> bug report.

Thanks, I tested all your changes and confirm there are no more problems.

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.