bug#39994: 27.0.90; Broken image-converter probe for imagemagick

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Juri Linkov]

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

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Lars Ingebrigtsen]

Juri Linkov <juri@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

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Juri Linkov]

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

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Lars Ingebrigtsen]

Juri Linkov <juri@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

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Juri Linkov]

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.

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Juri Linkov]

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

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Juri Linkov]

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

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Juri Linkov]

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').

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Lars Ingebrigtsen]

Juri Linkov <juri@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

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Juri Linkov]

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

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Lars Ingebrigtsen]

Juri Linkov <juri@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

bug#39994: 27.0.90; Broken image-converter probe for imagemagick

[Juri Linkov]

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