bug#57691: 29.0.50; image-tests failures

bug#57691: 29.0.50; image-tests failures

[Eli Zaretskii]

Running test/manual/image-tests.el, as in

  ./src/emacs -Q -l test/manual/image-tests.el --eval "(ert t)"

produces 2 failures:

Selector: t
Passed:  38
Failed:  2 (2 unexpected)
Skipped: 0
Total:   40/40

Started at:   2022-09-09 09:22:13+0300
Finished.
Finished at:  2022-09-09 09:22:19+0300

..........F........................F....

F image-tests-image-metadata/gif
    (ert-test-failed
     ((should
       (memq 'delay
	      (image-metadata
	       (create-image ...))))
      :form
      (memq delay nil)
      :value nil))

F image-tests-load-image/svg-invalid
    (ert-test-failed
     ((should
       (string-search "XML parse error"
		      (buffer-string)))
      :form
      (string-search "XML parse error" "For information about GNU Emacs and the GNU system, type C-h C-a.\nType C-c C-c to view the image as text\nError parsing SVG image: Error domain 1 code 4 on line 1 column 1 of data: Start tag expected, '<' not found [3 times]\n")
      :value nil))

Let me know if I can help in debugging this.

In GNU Emacs 29.0.50 (build 1848, i686-pc-mingw32) of 2022-09-09 built
 on HOME-C4E4A596F7
Repository revision: adc8344e759b0aa3c919df9bdff1232d89b2e6b4
Repository branch: master
Windowing system distributor 'Microsoft Corp.', version 5.1.2600
System Description: Microsoft Windows XP Service Pack 3 (v5.1.0.2600)

Configured using:
 'configure -C --prefix=/d/usr --with-wide-int
 --enable-checking=yes,glyphs 'CFLAGS=-O0 -gdwarf-4 -g3''

Configured features:
ACL GIF GMP GNUTLS HARFBUZZ JPEG JSON LCMS2 LIBXML2 MODULES NOTIFY
W32NOTIFY PDUMPER PNG RSVG SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS WEBP XPM ZLIB

Important settings:
  value of $LANG: ENU
  locale-coding-system: cp1255

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date subr-x mm-decode
mm-bodies mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader
cl-loaddefs cl-lib sendmail rfc2047 rfc2045 ietf-drums mm-util
mail-prsvr mail-utils rmc iso-transl tooltip eldoc paren electric
uniquify ediff-hook vc-hooks lisp-float-type elisp-mode mwheel dos-w32
ls-lisp disp-table term/w32-win w32-win w32-vars term/common-win
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
simple cl-generic indonesian philippine cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
loaddefs faces cus-face macroexp files window text-properties overlay
sha1 md5 base64 format env code-pages mule custom widget keymap
hashtable-print-readable backquote threads w32notify w32 lcms2 multi-tty
make-network-process emacs)

Memory information:
((conses 16 42423 9500)
 (symbols 48 6244 0)
 (strings 16 16532 2998)
 (string-bytes 1 397900)
 (vectors 16 9231)
 (vector-slots 8 144103 13397)
 (floats 8 23 45)
 (intervals 40 274 97)
 (buffers 896 10))

bug#57691: 29.0.50; image-tests failures

[Stefan Kangas]

[-- Attachment #1: Type: text/plain, Size: 1980 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

> Running test/manual/image-tests.el, as in
>
>   ./src/emacs -Q -l test/manual/image-tests.el --eval "(ert t)"
>
> produces 2 failures:

Thanks for reporting this.

> F image-tests-image-metadata/gif
>     (ert-test-failed
>      ((should
>        (memq 'delay
> 	      (image-metadata
> 	       (create-image ...))))
>       :form
>       (memq delay nil)
>       :value nil))

That doesn't happen here, so maybe it's MS-Windows specific?

Could you build with --enable-checking="glyphs", and see what you get
from evaluating:

  (let ((img (create-image
              (expand-file-name "test/data/image/black.gif"
                                source-directory))))
    (cons (lookup-image img) (image-metadata img)))

It should print some output to the terminal too, which is also
interesting.

For reference, I get:

  (10 delay 0.06666666666666667 extension-data (249 "\^A\0\0\^A"))

Note that the above 1/15=0.066 delay is what we expect (see
image.c:9452).  I also get this terminal output:

  (image :type gif :file
  "/home/skangas/wip/emacs/test/data/image/black.gif"
  :scale 1.0031746031746032 :transform-smoothing t)

I'm not sure if it matters or not, but do you happen to know which
version of giflib you're using?

> F image-tests-load-image/svg-invalid
>     (ert-test-failed
>      ((should
>        (string-search "XML parse error"
> 		      (buffer-string)))
>       :form
>       (string-search "XML parse error" "For information about GNU Emacs and the GNU system, type C-h C-a.\nType C-c C-c to view the image as text\nError parsing SVG image: Error domain 1 code 4 on line 1 column 1 of data: Start tag expected, '<' not found [3 times]\n")
>       :value nil))

This looks like some version of librsvg giving a different error
message.  Does the attached patch fix this for you?

Also, out of interest, do you know which version of librsvg you're
using?

[-- Attachment #2: 0001-Fix-SVG-test-for-old-librsvg.patch --]
[-- Type: text/x-diff, Size: 1065 bytes --]

From 9e7ceaa93ab2a794bd0223bbec0d40a90aecc09c Mon Sep 17 00:00:00 2001
From: Stefan Kangas <stefankangas@gmail.com>
Date: Fri, 9 Sep 2022 10:31:25 +0200
Subject: [PATCH] Fix SVG test for old librsvg

* test/manual/image-tests.el (image-tests-load-image/svg-invalid):
Match error message in old versions of librsvg.  (Bug#57691)
---
 test/manual/image-tests.el | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/test/manual/image-tests.el b/test/manual/image-tests.el
index c726845bd3..8f6476f3e2 100644
--- a/test/manual/image-tests.el
+++ b/test/manual/image-tests.el
@@ -86,8 +86,9 @@ image-tests-load-image/svg-invalid
                                          "invalid foo bar"
                                          :type svg)))
     (redisplay))
+  ;; librsvg error: "... Start tag expected, '<' not found [3 times]"
   (with-current-buffer "*Messages*"
-    (should (string-search "XML parse error" (buffer-string)))))
+    (should (string-match "[Ee]rror.+Start tag expected" (buffer-string)))))
 
 \f
 ;;;; image-test-size
-- 
2.30.2

bug#57691: 29.0.50; image-tests failures

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 9 Sep 2022 05:03:00 -0400
> 
> Could you build with --enable-checking="glyphs", and see what you get
> from evaluating:
> 
>   (let ((img (create-image
>               (expand-file-name "test/data/image/black.gif"
>                                 source-directory))))
>     (cons (lookup-image img) (image-metadata img)))
> 
> It should print some output to the terminal too, which is also
> interesting.

Evaluation returns (21), and prints this on stderr:

  (image :type gif :file "d:/gnu/git/emacs/trunk/test/data/image/black.gif" :scale 1 :transform-smoothing t)

> For reference, I get:
> 
>   (10 delay 0.06666666666666667 extension-data (249 "\^A\0\0\^A"))

I get something similar if I disable w32-use-native-image-API (which
is ON by default in Emacs 29):

  (21 delay 0.06666666666666667 extension-data (249 "^A\0\0^A"))

So I guess the test assumes something non-portable?  If I invoke the
test with --eval "(setq w32-use-native-image-API nil)", the
image-tests-image-metadata/gif test passes.

> I'm not sure if it matters or not, but do you happen to know which
> version of giflib you're using?

If w32-use-native-image-API is nil, my Emacs uses libgif v5.1.0.  By
default, libgif is not used at all; instead, Emacs uses MS-Windows
native APIs in the system library gdiplus.dll to display GIF images.

> > F image-tests-load-image/svg-invalid
> >     (ert-test-failed
> >      ((should
> >        (string-search "XML parse error"
> > 		      (buffer-string)))
> >       :form
> >       (string-search "XML parse error" "For information about GNU Emacs and the GNU system, type C-h C-a.\nType C-c C-c to view the image as text\nError parsing SVG image: Error domain 1 code 4 on line 1 column 1 of data: Start tag expected, '<' not found [3 times]\n")
> >       :value nil))
> 
> This looks like some version of librsvg giving a different error
> message.  Does the attached patch fix this for you?

Yes, thanks.

> Also, out of interest, do you know which version of librsvg you're
> using?

2.40.1.

bug#57691: 29.0.50; image-tests failures

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> So I guess the test assumes something non-portable?  If I invoke the
> test with --eval "(setq w32-use-native-image-API nil)", the
> image-tests-image-metadata/gif test passes.
>
>> I'm not sure if it matters or not, but do you happen to know which
>> version of giflib you're using?
>
> If w32-use-native-image-API is nil, my Emacs uses libgif v5.1.0.  By
> default, libgif is not used at all; instead, Emacs uses MS-Windows
> native APIs in the system library gdiplus.dll to display GIF images.

Ah, I forgot about the native image stuff.

It seems to me that the test currently assumes that we do NOT use a
native image API.

I don't know if we (1) need to adjust the delay on MS-Windows along the
lines of what we do in image.c [see commit 3de2462da7], or (2) should
change the test to set w32-use-native-image-API to nil, or (3) if we
should just skip the test on MS-Windows.

If we would like to change this like in (1) above, I guess the place to
do so is w32image.c:448.  Perhaps similarly for nsimage.m:546.
I couldn't find anything related to Qdelay in haikuimage.c, but maybe
there too.

bug#57691: 29.0.50; image-tests failures

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> This looks like some version of librsvg giving a different error
>> message.  Does the attached patch fix this for you?
>
> Yes, thanks.
>
>> Also, out of interest, do you know which version of librsvg you're
>> using?
>
> 2.40.1.

Mine is 2.50.3, so I guess the message format has changed.  The fix is
pushed to master, thanks for testing.

bug#57691: 29.0.50; image-tests failures

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 9 Sep 2022 08:44:01 -0400
> Cc: 57691@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If w32-use-native-image-API is nil, my Emacs uses libgif v5.1.0.  By
> > default, libgif is not used at all; instead, Emacs uses MS-Windows
> > native APIs in the system library gdiplus.dll to display GIF images.
> 
> Ah, I forgot about the native image stuff.
> 
> It seems to me that the test currently assumes that we do NOT use a
> native image API.
> 
> I don't know if we (1) need to adjust the delay on MS-Windows along the
> lines of what we do in image.c [see commit 3de2462da7], or (2) should
> change the test to set w32-use-native-image-API to nil, or (3) if we
> should just skip the test on MS-Windows.

What does that test check? why does it want to see the delay?

The native APIs support animated GIFs better than with libgif, so
artificially making changes of the delay for the single benefit of
running this test is not the best idea, IMO.

bug#57691: 29.0.50; image-tests failures

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 9 Sep 2022 08:44:19 -0400
> Cc: 57691@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> This looks like some version of librsvg giving a different error
> >> message.  Does the attached patch fix this for you?
> >
> > Yes, thanks.
> >
> >> Also, out of interest, do you know which version of librsvg you're
> >> using?
> >
> > 2.40.1.
> 
> Mine is 2.50.3

librsvg 2.40.x is the last version that doesn't need a Rust compiler
to build, so I think we should keep it supported for the observable
future.

bug#57691: 29.0.50; image-tests failures

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> What does that test check? why does it want to see the delay?

It used to just check that no metadata was returned for these images.
Then Lars changed the GIF and WebP loading code to always add a `delay',
so I adapted the tests to that.

(More work is needed to make these tests check for more substantial
things.  See the TODO where I say that we could expand them to test
using files that actually do contain metadata.)

> The native APIs support animated GIFs better than with libgif, so
> artificially making changes of the delay for the single benefit of
> running this test is not the best idea, IMO.

Gotcha.  So it sounds like we should leave that alone, and adapt the
test to Windows instead.

Does the below workaround make sense?  That way, at least we test
something.

diff --git a/test/manual/image-tests.el b/test/manual/image-tests.el
index c66080cf02..4b1b122309 100644
--- a/test/manual/image-tests.el
+++ b/test/manual/image-tests.el
@@ -220,11 +220,13 @@ image-tests-image-mask-p/error-on-invalid-spec
 ;; TODO: These tests could be expanded with files that actually
 ;;       contain metadata.

+(defvar w32-use-native-image-API)
 (ert-deftest image-tests-image-metadata/gif ()
   (image-skip-unless 'gif)
-  (should (memq 'delay
-                (image-metadata
-                 (create-image (cdr (assq 'gif image-tests--images)))))))
+  (let ((w32-use-native-image-API nil))
+    (should (memq 'delay
+                  (image-metadata
+                   (create-image (cdr (assq 'gif image-tests--images))))))))

 (ert-deftest image-tests-image-metadata/jpeg ()
   (image-skip-unless 'jpeg)

bug#57691: 29.0.50; image-tests failures

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

> librsvg 2.40.x is the last version that doesn't need a Rust compiler
> to build, so I think we should keep it supported for the observable
> future.

Interesting.

I guess the likelihood of more libraries going in that direction will
tend to increase over time, a development that might be encouraged by
things like Rust in Linux kernel modules.

bug#57691: 29.0.50; image-tests failures

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Fri, 9 Sep 2022 18:37:20 -0400
> Cc: 57691@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What does that test check? why does it want to see the delay?
> 
> It used to just check that no metadata was returned for these images.
> Then Lars changed the GIF and WebP loading code to always add a `delay',
> so I adapted the tests to that.

Is it important to verify that the 'delay' stuff is in metadata?  If
not, how about just removing that part of the test?

> > The native APIs support animated GIFs better than with libgif, so
> > artificially making changes of the delay for the single benefit of
> > running this test is not the best idea, IMO.
> 
> Gotcha.  So it sounds like we should leave that alone, and adapt the
> test to Windows instead.
> 
> Does the below workaround make sense?  That way, at least we test
> something.

I'd rather we skipped the test if w32-use-native-image-API is non-nil.
That's because my expectations are that once Emacs 29 hits the street,
people will stop building Emacs on Windows with libgif (and a few
other image libraries which the native APIs can replace), so setting
the variable to nil will probably mean you'd need to check
image-type-available-p again etc.  That sounds like an unnecessary
complication.

And once again, perhaps that part of the test should simply be
removed, as I don't see why we'd need to verify the metadata.

Thanks.

bug#57691: 29.0.50; image-tests failures

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> It used to just check that no metadata was returned for these images.
>> Then Lars changed the GIF and WebP loading code to always add a `delay',
>> so I adapted the tests to that.
>
> Is it important to verify that the 'delay' stuff is in metadata?  If
> not, how about just removing that part of the test?

It's not crucial to check that part, I'm just not sure what to do
instead.

Ideally, we would test for something more substantial, of course, and I
hope to get to that eventually.  Maybe it's okay to leave it as a
wishlist item for now (i.e. the "TODO" comment in
manual/image-tests.el).

> I'd rather we skipped the test if w32-use-native-image-API is non-nil.

This should now be done on master.  Could you please test?

bug#57691: 29.0.50; image-tests failures

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Mon, 12 Sep 2022 09:23:54 -0400
> Cc: 57691@debbugs.gnu.org
> 
> > I'd rather we skipped the test if w32-use-native-image-API is non-nil.
> 
> This should now be done on master.  Could you please test?

Thanks, but the condition is wrong, so it still fails.  The test is
skipped unless either one of two conditions hold, and the first one
does hold even if w32-use-native-image-API is non-nil.

bug#57691: 29.0.50; image-tests failures

[Stefan Kangas]

Eli Zaretskii <eliz@gnu.org> writes:

>> This should now be done on master.  Could you please test?
>
> Thanks, but the condition is wrong, so it still fails.  The test is
> skipped unless either one of two conditions hold, and the first one
> does hold even if w32-use-native-image-API is non-nil.

Well, that was a brain fart.  Should be fixed now.

bug#57691: 29.0.50; image-tests failures

[Eli Zaretskii]

> From: Stefan Kangas <stefankangas@gmail.com>
> Date: Tue, 13 Sep 2022 05:40:00 -0400
> Cc: 57691@debbugs.gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> This should now be done on master.  Could you please test?
> >
> > Thanks, but the condition is wrong, so it still fails.  The test is
> > skipped unless either one of two conditions hold, and the first one
> > does hold even if w32-use-native-image-API is non-nil.
> 
> Well, that was a brain fart.  Should be fixed now.

Thanks, it works now.