GDI+ take 3

GDI+ take 3

[Juan José García-Ripoll]

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

After some consideration and washing off the feeling that I am wasting
my time, I have reworked my previous patch to test at boot time whether
GDI+ is available (gdiplus_v3.diff). It implements a generic
"native-image-api" option which can be extended to OSX (I have a patch,
but I have no machines to test it so it is not attached).

It does not implement a new image type. Instead, it places the native
backends at the front and continues searching the image list when those
backends are not active. I find that treating the native backend as a
second class citizen, as ImageMagick, is not ok. But most important, I
find that the abstraction of backend = image-type is broken.

Regarding previous discussions, I believe the concerns about GDI+
becoming deprecated are irrelevant in comparison to the fact that Emacs
is using already a deprecated API, namely GDI, which is older and
equally prone to disappear in favor of Direct2d/WIC.

Regarding unicode, GDI+ requires WCHAR. Therefore the backend refuses to
initialize if w32-unicode-filenames is NIL, defaulting to other
backends. However, if the user changes this variable from T to NIL after
the backend has been initialized, the backend will remain active but
refuse to load images.

I have fixed the use of terminal hooks, due to insistance. However, that
meant one function declaration had to be added to w32term.h

I have not changed the use of SSDATA in the :data field of images. If
this is a problem, it is so in all of image.c.

I have touch my Emacs installation to avoid removing the double spaces
after a dot, but I am not 100% sure all is Kosher.

I go to bed. If you still find that this patch is not acceptable, I am
not going to work any more on it. I developed it to minimize the number
of dependencies of Emacs on the Windows platform, but that does not seem
to be a priority around here.

Best,

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gdiplus_v3.diff --]
[-- Type: text/x-patch, Size: 13958 bytes --]

diff --git a/src/image.c b/src/image.c
index 65d59254f0..2ef51a87f2 100644
--- a/src/image.c
+++ b/src/image.c
@@ -751,7 +751,7 @@ x_create_bitmap_mask (struct frame *f, ptrdiff_t id)

   /* Check that SPEC is a valid image specification for the given
      image type.  Value is true if SPEC is valid.  */
-  bool (*valid_p) (Lisp_Object spec);
+  bool (*valid_p) (Lisp_Object spec, Lisp_Object type);

   /* Load IMG which is used on frame F from information contained in
      IMG->spec.  Value is true if successful.  */
@@ -807,7 +807,7 @@ valid_image_p (Lisp_Object object)
 	      {
 		struct image_type const *type = lookup_image_type (XCAR (tail));
 		if (type)
-		  return type->valid_p (object);
+		  return type->valid_p (object, builtin_lisp_symbol (type->type));
 	      }
 	    break;
 	  }
@@ -3144,12 +3144,12 @@ slurp_file (int fd, ptrdiff_t *size)
    displayed is used.  */

 static bool
-xbm_image_p (Lisp_Object object)
+xbm_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword kw[XBM_LAST];

   memcpy (kw, xbm_format, sizeof kw);
-  if (!parse_image_spec (object, kw, XBM_LAST, Qxbm))
+  if (!parse_image_spec (object, kw, XBM_LAST, type))
     return 0;

   eassert (EQ (kw[XBM_TYPE].value, Qxbm));
@@ -3697,7 +3697,7 @@ xbm_load (struct frame *f, struct image *img)
   bool success_p = 0;
   Lisp_Object file_name;

-  eassert (xbm_image_p (img->spec));
+  eassert (xbm_image_p (img->spec, Qxbm));

   /* If IMG->spec specifies a file name, create a non-file spec from it.  */
   file_name = image_spec_value (img->spec, QCfile, NULL);
@@ -4155,11 +4155,11 @@ xpm_valid_color_symbols_p (Lisp_Object color_symbols)
 /* Value is true if OBJECT is a valid XPM image specification.  */

 static bool
-xpm_image_p (Lisp_Object object)
+xpm_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[XPM_LAST];
   memcpy (fmt, xpm_format, sizeof fmt);
-  return (parse_image_spec (object, fmt, XPM_LAST, Qxpm)
+  return (parse_image_spec (object, fmt, XPM_LAST, type)
 	  /* Either `:file' or `:data' must be present.  */
 	  && fmt[XPM_FILE].count + fmt[XPM_DATA].count == 1
 	  /* Either no `:color-symbols' or it's a list of conses
@@ -5882,13 +5882,13 @@ image_build_heuristic_mask (struct frame *f, struct image *img,
 /* Return true if OBJECT is a valid PBM image specification.  */

 static bool
-pbm_image_p (Lisp_Object object)
+pbm_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[PBM_LAST];

   memcpy (fmt, pbm_format, sizeof fmt);

-  if (!parse_image_spec (object, fmt, PBM_LAST, Qpbm))
+  if (!parse_image_spec (object, fmt, PBM_LAST, type))
     return 0;

   /* Must specify either :data or :file.  */
@@ -6230,6 +6230,83 @@ pbm_load (struct frame *f, struct image *img)
   return 1;
 }

+\f
+/***********************************************************************
+			    NATIVE IMAGE HANDLING
+ ***********************************************************************/
+#if defined(HAVE_NATIVE_IMAGE_API) && defined(HAVE_NTGUI)
+/*
+ * These functions are actually defined in the OS-native implementation
+ * file.  Currently, for Windows GDI+ interface, w32image.c, but other
+ * operating systems can follow suit.
+ */
+
+static bool
+init_native_image_functions (void)
+{
+  return w32_gdiplus_startup ();
+}
+
+/* Indices of image specification fields in native format, below.  */
+
+enum native_image_keyword_index
+{
+  NATIVE_IMAGE_TYPE,
+  NATIVE_IMAGE_DATA,
+  NATIVE_IMAGE_FILE,
+  NATIVE_IMAGE_ASCENT,
+  NATIVE_IMAGE_MARGIN,
+  NATIVE_IMAGE_RELIEF,
+  NATIVE_IMAGE_ALGORITHM,
+  NATIVE_IMAGE_HEURISTIC_MASK,
+  NATIVE_IMAGE_MASK,
+  NATIVE_IMAGE_BACKGROUND,
+  NATIVE_IMAGE_INDEX,
+  NATIVE_IMAGE_LAST
+};
+
+/* Vector of image_keyword structures describing the format
+   of valid user-defined image specifications.  */
+
+static const struct image_keyword native_image_format[] =
+{
+  {":type",		IMAGE_SYMBOL_VALUE,			1},
+  {":data",		IMAGE_STRING_VALUE,			0},
+  {":file",		IMAGE_STRING_VALUE,			0},
+  {":ascent",		IMAGE_ASCENT_VALUE,			0},
+  {":margin",		IMAGE_NON_NEGATIVE_INTEGER_VALUE_OR_PAIR, 0},
+  {":relief",		IMAGE_INTEGER_VALUE,			0},
+  {":conversion",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":heuristic-mask",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":mask",		IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":background",	IMAGE_STRING_OR_NIL_VALUE,		0},
+  {":index",		IMAGE_NON_NEGATIVE_INTEGER_VALUE,	0}
+};
+
+/* Return true if OBJECT is a valid native API image specification.  */
+
+static bool
+native_image_p (Lisp_Object object, Lisp_Object type)
+{
+  struct image_keyword fmt[NATIVE_IMAGE_LAST];
+  memcpy (fmt, native_image_format, sizeof fmt);
+
+  if (!parse_image_spec (object, fmt, 10, type))
+    return 0;
+
+  /* Must specify either the :data or :file keyword.  */
+  return fmt[NATIVE_IMAGE_FILE].count + fmt[NATIVE_IMAGE_DATA].count == 1;
+}
+
+static bool
+native_image_load (struct frame *f, struct image *img)
+{
+  return w32_load_image (f, img,
+                         image_spec_value (img->spec, QCfile, NULL),
+                         image_spec_value (img->spec, QCdata, NULL));
+}
+#endif
+
 \f
 /***********************************************************************
 				 PNG
@@ -6274,12 +6351,12 @@ pbm_load (struct frame *f, struct image *img)
 /* Return true if OBJECT is a valid PNG image specification.  */

 static bool
-png_image_p (Lisp_Object object)
+png_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[PNG_LAST];
   memcpy (fmt, png_format, sizeof fmt);

-  if (!parse_image_spec (object, fmt, PNG_LAST, Qpng))
+  if (!parse_image_spec (object, fmt, PNG_LAST, type))
     return 0;

   /* Must specify either the :data or :file keyword.  */
@@ -6889,7 +6966,6 @@ png_load (struct frame *f, struct image *img)
                         image_spec_value (img->spec, QCdata, NULL));
 }

-
 #endif /* HAVE_NS */


@@ -6937,13 +7013,13 @@ png_load (struct frame *f, struct image *img)
 /* Return true if OBJECT is a valid JPEG image specification.  */

 static bool
-jpeg_image_p (Lisp_Object object)
+jpeg_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[JPEG_LAST];

   memcpy (fmt, jpeg_format, sizeof fmt);

-  if (!parse_image_spec (object, fmt, JPEG_LAST, Qjpeg))
+  if (!parse_image_spec (object, fmt, JPEG_LAST, type))
     return 0;

   /* Must specify either the :data or :file keyword.  */
@@ -7513,12 +7589,12 @@ jpeg_load (struct frame *f, struct image *img)
 /* Return true if OBJECT is a valid TIFF image specification.  */

 static bool
-tiff_image_p (Lisp_Object object)
+tiff_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[TIFF_LAST];
   memcpy (fmt, tiff_format, sizeof fmt);

-  if (!parse_image_spec (object, fmt, TIFF_LAST, Qtiff))
+  if (!parse_image_spec (object, fmt, TIFF_LAST, type))
     return 0;

   /* Must specify either the :data or :file keyword.  */
@@ -7961,19 +8037,19 @@ gif_clear_image (struct frame *f, struct image *img)
 /* Return true if OBJECT is a valid GIF image specification.  */

 static bool
-gif_image_p (Lisp_Object object)
+gif_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[GIF_LAST];
   memcpy (fmt, gif_format, sizeof fmt);

-  if (!parse_image_spec (object, fmt, GIF_LAST, Qgif))
+  if (!parse_image_spec (object, fmt, GIF_LAST, type))
     return 0;

   /* Must specify either the :data or :file keyword.  */
   return fmt[GIF_FILE].count + fmt[GIF_DATA].count == 1;
 }

-#endif /* HAVE_GIF */
+#endif /* HAVE_GIF || HAVE_NS */

 #ifdef HAVE_GIF

@@ -8573,12 +8649,12 @@ imagemagick_clear_image (struct frame *f,
    identify the IMAGEMAGICK format.   */

 static bool
-imagemagick_image_p (Lisp_Object object)
+imagemagick_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[IMAGEMAGICK_LAST];
   memcpy (fmt, imagemagick_format, sizeof fmt);

-  if (!parse_image_spec (object, fmt, IMAGEMAGICK_LAST, Qimagemagick))
+  if (!parse_image_spec (object, fmt, IMAGEMAGICK_LAST, type))
     return 0;

   /* Must specify either the :data or :file keyword.  */
@@ -9368,12 +9444,12 @@ DEFUN ("imagemagick-types", Fimagemagick_types, Simagemagick_types, 0, 0, 0,
    identify the SVG format.   */

 static bool
-svg_image_p (Lisp_Object object)
+svg_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[SVG_LAST];
   memcpy (fmt, svg_format, sizeof fmt);

-  if (!parse_image_spec (object, fmt, SVG_LAST, Qsvg))
+  if (!parse_image_spec (object, fmt, SVG_LAST, type))
     return 0;

   /* Must specify either the :data or :file keyword.  */
@@ -9836,7 +9912,7 @@ #define HAVE_GHOSTSCRIPT 1
    specification.  */

 static bool
-gs_image_p (Lisp_Object object)
+gs_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[GS_LAST];
   Lisp_Object tem;
@@ -9844,7 +9920,7 @@ gs_image_p (Lisp_Object object)

   memcpy (fmt, gs_format, sizeof fmt);

-  if (!parse_image_spec (object, fmt, GS_LAST, Qpostscript))
+  if (!parse_image_spec (object, fmt, GS_LAST, type))
     return 0;

   /* Bounding box must be a list or vector containing 4 integers.  */
@@ -10131,13 +10207,20 @@ DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 1, 1, 0,
 initialize_image_type (struct image_type const *type)
 {
 #ifdef WINDOWSNT
-  Lisp_Object typesym = builtin_lisp_symbol (type->type);
-  Lisp_Object tested = Fassq (typesym, Vlibrary_cache);
+  Lisp_Object typesym, tested;
+  bool (*init) (void) = type->init;
+
+#ifdef HAVE_NATIVE_IMAGE_API
+  if (init == init_native_image_functions)
+    return init();
+#endif
+
+  typesym = builtin_lisp_symbol (type->type);
+  tested = Fassq (typesym, Vlibrary_cache);
   /* If we failed to load the library before, don't try again.  */
   if (CONSP (tested))
     return !NILP (XCDR (tested)) ? true : false;

-  bool (*init) (void) = type->init;
   if (init)
     {
       bool type_valid = init ();
@@ -10164,6 +10247,16 @@ initialize_image_type (struct image_type const *type)
  { SYMBOL_INDEX (Qsvg), svg_image_p, svg_load, image_clear_image,
    IMAGE_TYPE_INIT (init_svg_functions) },
 #endif
+#if defined HAVE_NATIVE_IMAGE_API
+ { SYMBOL_INDEX (Qjpeg), native_image_p, native_image_load, image_clear_image,
+   IMAGE_TYPE_INIT (init_native_image_functions) },
+ { SYMBOL_INDEX (Qpng), native_image_p, native_image_load, image_clear_image,
+   IMAGE_TYPE_INIT (init_native_image_functions) },
+ { SYMBOL_INDEX (Qgif), native_image_p, native_image_load, image_clear_image,
+   IMAGE_TYPE_INIT (init_native_image_functions) },
+ { SYMBOL_INDEX (Qtiff), native_image_p, native_image_load, image_clear_image,
+   IMAGE_TYPE_INIT (init_native_image_functions) },
+#endif
 #if defined HAVE_PNG || defined HAVE_NS
  { SYMBOL_INDEX (Qpng), png_image_p, png_load, image_clear_image,
    IMAGE_TYPE_INIT (init_png_functions) },
@@ -10198,7 +10291,13 @@ lookup_image_type (Lisp_Object type)
     {
       struct image_type const *r = &image_types[i];
       if (EQ (type, builtin_lisp_symbol (r->type)))
+#ifdef HAVE_NATIVE_IMAGE_API
+        /* We can have more than one backend for one image type.  */
+        if (initialize_image_type (r))
+          return r;
+#else
 	return initialize_image_type (r) ? r : NULL;
+#endif
     }
   return NULL;
 }
@@ -10315,22 +10414,22 @@ syms_of_image (void)
   add_image_type (Qxpm);
 #endif

-#if defined (HAVE_JPEG) || defined (HAVE_NS)
+#if defined (HAVE_JPEG) || defined (HAVE_NS) || defined (HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qjpeg, "jpeg");
   add_image_type (Qjpeg);
 #endif

-#if defined (HAVE_TIFF) || defined (HAVE_NS)
+#if defined (HAVE_TIFF) || defined (HAVE_NS) || defined (HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qtiff, "tiff");
   add_image_type (Qtiff);
 #endif

-#if defined (HAVE_GIF) || defined (HAVE_NS)
+#if defined (HAVE_GIF) || defined (HAVE_NS) || defined (HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qgif, "gif");
   add_image_type (Qgif);
 #endif

-#if defined (HAVE_PNG) || defined (HAVE_NS)
+#if defined (HAVE_PNG) || defined (HAVE_NS) || defined(HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qpng, "png");
   add_image_type (Qpng);
 #endif
diff --git a/src/w32.c b/src/w32.c
index 698e10e234..1d2a52b6df 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -10225,6 +10225,10 @@ term_ntproc (int ignored)
   term_winsock ();

   term_w32select ();
+
+#ifdef HAVE_GDIPLUS
+  w32_gdiplus_shutdown ();
+#endif
 }

 void
diff --git a/src/w32term.c b/src/w32term.c
index 5fa77d58e1..f19754df02 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -1529,7 +1529,7 @@ w32_query_colors (struct frame *f, Emacs_Color *colors, int ncolors)

 /* Store F's background color into *BGCOLOR.  */

-static void
+void
 w32_query_frame_background_color (struct frame *f, Emacs_Color *bgcolor)
 {
   bgcolor->pixel = FRAME_BACKGROUND_PIXEL (f);
diff --git a/src/w32term.h b/src/w32term.h
index f8a8a727e8..d44c6f9b83 100644
--- a/src/w32term.h
+++ b/src/w32term.h
@@ -75,7 +75,10 @@ #define CP_DEFAULT 1004
 extern void w32_regenerate_palette (struct frame *f);
 extern void w32_fullscreen_rect (HWND hwnd, int fsmode, RECT normal,
                                  RECT *rect);
-
+extern int w32_load_image (struct frame *f, struct image *img,
+                           Lisp_Object spec_file, Lisp_Object spec_data);
+extern bool w32_gdiplus_startup (void);
+extern void w32_gdiplus_shutdown (void);
 \f
 /* For each display (currently only one on w32), we have a structure that
    records information about it.  */
@@ -248,6 +251,8 @@ #define CP_DEFAULT 1004
 extern int w32_display_pixel_width (struct w32_display_info *);
 extern void initialize_frame_menubar (struct frame *);
 extern void w32_dialog_in_progress (Lisp_Object in_progress);
+extern void w32_query_frame_background_color (struct frame *f,
+                                              Emacs_Color *bgcolor);

 extern void w32_make_frame_visible (struct frame *f);
 extern void w32_make_frame_invisible (struct frame *f);

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Sat, 04 Apr 2020 23:25:09 +0200
> 
> After some consideration and washing off the feeling that I am wasting
> my time, I have reworked my previous patch to test at boot time whether
> GDI+ is available (gdiplus_v3.diff). It implements a generic
> "native-image-api" option which can be extended to OSX (I have a patch,
> but I have no machines to test it so it is not attached).

Thank you very much for working on this.  You haven't wasted your
time, I can assure you.

The patches you sent don't include some of the bits in the previous
version: the changes in configure.ac, src/Makefile.in, etc/NEWS, and
src/w32image.c.  Did I miss something?

Re: GDI+ take 3

[Eli Zaretskii]

> Date: Sun, 05 Apr 2020 15:58:21 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> The patches you sent don't include some of the bits in the previous
> version: the changes in configure.ac, src/Makefile.in, etc/NEWS, and
> src/w32image.c.  Did I miss something?

Ping!

I'd like to install these changes, but some parts seem to be missing,
see above.  Could you please post the missing parts (or the entire
changeset, if that is more convenient)?

Thanks.

Re: GDI+ take 3

[Juan José García-Ripoll]

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

Eli Zaretskii <eliz@gnu.org> writes:
> I'd like to install these changes, but some parts seem to be missing,
> see above.  Could you please post the missing parts (or the entire
> changeset, if that is more convenient)?

Apologies for the corrupted patch. Here it goes, I hope, in a complete form.

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-gdi+-v3.diff --]
[-- Type: text/x-patch, Size: 27566 bytes --]

diff --git a/configure.ac b/configure.ac
index 9907160482..41a1860493 100644
--- a/configure.ac
+++ b/configure.ac
@@ -433,6 +433,7 @@ AC_DEFUN
 OPTION_DEFAULT_ON([cairo],[don't compile with Cairo drawing])
 OPTION_DEFAULT_ON([xml2],[don't compile with XML parsing support])
 OPTION_DEFAULT_OFF([imagemagick],[compile with ImageMagick image support])
+OPTION_DEFAULT_OFF([native-image-api], [use native API's (GDI+ on Windows) for JPEG/TIFF/GIFF/PNG])
 OPTION_DEFAULT_ON([json], [don't compile with native JSON support])
 
 OPTION_DEFAULT_ON([xft],[don't use XFT for anti aliased fonts])
@@ -2126,6 +2127,7 @@ AC_DEFUN
 NTLIB=
 CM_OBJ="cm.o"
 XARGS_LIMIT=
+HAVE_NATIVE_IMAGE_API=no
 if test "${HAVE_W32}" = "yes"; then
   AC_DEFINE(HAVE_NTGUI, 1, [Define to use native MS Windows GUI.])
   if test "$with_toolkit_scroll_bars" = "no"; then
@@ -2154,7 +2156,12 @@ AC_DEFUN
     # the rc file), not a linker script.
     W32_RES_LINK="-Wl,emacs.res"
   else
-    W32_OBJ="$W32_OBJ w32.o w32console.o w32heap.o w32inevt.o w32proc.o"
+    if test "${with_native_image_api}" = yes; then
+      AC_DEFINE(HAVE_NATIVE_IMAGE_API, 1, [Define to use MS Windows GDI+ for images.])
+      HAVE_NATIVE_IMAGE_API=yes
+      W32_NATIVE_IMAGE_API="w32image.o"
+    fi
+    W32_OBJ="$W32_OBJ w32.o w32console.o w32heap.o w32inevt.o w32proc.o $W32_NATIVE_IMAGE_API"
     W32_LIBS="$W32_LIBS -lwinmm -lusp10 -lgdi32 -lcomdlg32"
     W32_LIBS="$W32_LIBS -lmpr -lwinspool -lole32 -lcomctl32"
     W32_RES_LINK="\$(EMACSRES)"
@@ -5703,6 +5710,7 @@ AC_DEFUN
   Does Emacs use cairo?                                   ${HAVE_CAIRO}
   Does Emacs use -llcms2?                                 ${HAVE_LCMS2}
   Does Emacs use imagemagick?                             ${HAVE_IMAGEMAGICK}
+  Does Emacs use native API for images?                   ${HAVE_NATIVE_IMAGE_API}
   Does Emacs support sound?                               ${HAVE_SOUND}
   Does Emacs use -lgpm?                                   ${HAVE_GPM}
   Does Emacs use -ldbus?                                  ${HAVE_DBUS}
diff --git a/etc/NEWS b/etc/NEWS
index 7a7f11f507..847ecffec2 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -372,6 +372,12 @@ coding-system-for-{read,write} or call 'set-process-coding-system'.
 'module-file-suffix' now has the value ".dylib" on macOS, but the
 ".so" suffix is supported as well.
 
++++
+** Emacs can now use Microsoft Windows GDI+ library to load bitmap images in
+JPEG, PNG, GIF and TIFF formats.  This support is enabled with
+--with-native-image-api, which automatically disables the use of third party
+libraries for those formats.
+
 \f
 ----------------------------------------------------------------------
 This file is part of GNU Emacs.
diff --git a/lisp/term/w32-win.el b/lisp/term/w32-win.el
index 3e932c7593..5901e0295e 100644
--- a/lisp/term/w32-win.el
+++ b/lisp/term/w32-win.el
@@ -231,6 +231,8 @@ libgnutls-version
 ;;; Set default known names for external libraries
 (setq dynamic-library-alist
       (list
+       '(gdiplus "gdiplus.dll")
+       '(shlwapi "shlwapi.dll")
        '(xpm "libxpm.dll" "xpm4.dll" "libXpm-nox4.dll")
        ;; Versions of libpng 1.4.x and later are incompatible with
        ;; earlier versions.  Set up the list of libraries according to
diff --git a/src/image.c b/src/image.c
index c98ca291ca..ff2d12fa1a 100644
--- a/src/image.c
+++ b/src/image.c
@@ -751,7 +751,7 @@ x_create_bitmap_mask (struct frame *f, ptrdiff_t id)
 
   /* Check that SPEC is a valid image specification for the given
      image type.  Value is true if SPEC is valid.  */
-  bool (*valid_p) (Lisp_Object spec);
+  bool (*valid_p) (Lisp_Object spec, Lisp_Object type);
 
   /* Load IMG which is used on frame F from information contained in
      IMG->spec.  Value is true if successful.  */
@@ -807,7 +807,7 @@ valid_image_p (Lisp_Object object)
 	      {
 		struct image_type const *type = lookup_image_type (XCAR (tail));
 		if (type)
-		  return type->valid_p (object);
+		  return type->valid_p (object, builtin_lisp_symbol (type->type));
 	      }
 	    break;
 	  }
@@ -3144,12 +3144,12 @@ slurp_file (int fd, ptrdiff_t *size)
    displayed is used.  */
 
 static bool
-xbm_image_p (Lisp_Object object)
+xbm_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword kw[XBM_LAST];
 
   memcpy (kw, xbm_format, sizeof kw);
-  if (!parse_image_spec (object, kw, XBM_LAST, Qxbm))
+  if (!parse_image_spec (object, kw, XBM_LAST, type))
     return 0;
 
   eassert (EQ (kw[XBM_TYPE].value, Qxbm));
@@ -3697,7 +3697,7 @@ xbm_load (struct frame *f, struct image *img)
   bool success_p = 0;
   Lisp_Object file_name;
 
-  eassert (xbm_image_p (img->spec));
+  eassert (xbm_image_p (img->spec, Qxbm));
 
   /* If IMG->spec specifies a file name, create a non-file spec from it.  */
   file_name = image_spec_value (img->spec, QCfile, NULL);
@@ -4155,11 +4155,11 @@ xpm_valid_color_symbols_p (Lisp_Object color_symbols)
 /* Value is true if OBJECT is a valid XPM image specification.  */
 
 static bool
-xpm_image_p (Lisp_Object object)
+xpm_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[XPM_LAST];
   memcpy (fmt, xpm_format, sizeof fmt);
-  return (parse_image_spec (object, fmt, XPM_LAST, Qxpm)
+  return (parse_image_spec (object, fmt, XPM_LAST, type)
 	  /* Either `:file' or `:data' must be present.  */
 	  && fmt[XPM_FILE].count + fmt[XPM_DATA].count == 1
 	  /* Either no `:color-symbols' or it's a list of conses
@@ -5883,13 +5883,13 @@ image_build_heuristic_mask (struct frame *f, struct image *img,
 /* Return true if OBJECT is a valid PBM image specification.  */
 
 static bool
-pbm_image_p (Lisp_Object object)
+pbm_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[PBM_LAST];
 
   memcpy (fmt, pbm_format, sizeof fmt);
 
-  if (!parse_image_spec (object, fmt, PBM_LAST, Qpbm))
+  if (!parse_image_spec (object, fmt, PBM_LAST, type))
     return 0;
 
   /* Must specify either :data or :file.  */
@@ -6231,6 +6231,83 @@ pbm_load (struct frame *f, struct image *img)
   return 1;
 }
 
+\f
+/***********************************************************************
+			    NATIVE IMAGE HANDLING
+ ***********************************************************************/
+#if defined(HAVE_NATIVE_IMAGE_API) && defined(HAVE_NTGUI)
+/*
+ * These functions are actually defined in the OS-native implementation
+ * file.  Currently, for Windows GDI+ interface, w32image.c, but other
+ * operating systems can follow suit.
+ */
+
+static bool
+init_native_image_functions (void)
+{
+  return w32_gdiplus_startup ();
+}
+
+/* Indices of image specification fields in native format, below.  */
+
+enum native_image_keyword_index
+{
+  NATIVE_IMAGE_TYPE,
+  NATIVE_IMAGE_DATA,
+  NATIVE_IMAGE_FILE,
+  NATIVE_IMAGE_ASCENT,
+  NATIVE_IMAGE_MARGIN,
+  NATIVE_IMAGE_RELIEF,
+  NATIVE_IMAGE_ALGORITHM,
+  NATIVE_IMAGE_HEURISTIC_MASK,
+  NATIVE_IMAGE_MASK,
+  NATIVE_IMAGE_BACKGROUND,
+  NATIVE_IMAGE_INDEX,
+  NATIVE_IMAGE_LAST
+};
+
+/* Vector of image_keyword structures describing the format
+   of valid user-defined image specifications.  */
+
+static const struct image_keyword native_image_format[] =
+{
+  {":type",		IMAGE_SYMBOL_VALUE,			1},
+  {":data",		IMAGE_STRING_VALUE,			0},
+  {":file",		IMAGE_STRING_VALUE,			0},
+  {":ascent",		IMAGE_ASCENT_VALUE,			0},
+  {":margin",		IMAGE_NON_NEGATIVE_INTEGER_VALUE_OR_PAIR, 0},
+  {":relief",		IMAGE_INTEGER_VALUE,			0},
+  {":conversion",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":heuristic-mask",	IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":mask",		IMAGE_DONT_CHECK_VALUE_TYPE,		0},
+  {":background",	IMAGE_STRING_OR_NIL_VALUE,		0},
+  {":index",		IMAGE_NON_NEGATIVE_INTEGER_VALUE,	0}
+};
+
+/* Return true if OBJECT is a valid native API image specification.  */
+
+static bool
+native_image_p (Lisp_Object object, Lisp_Object type)
+{
+  struct image_keyword fmt[NATIVE_IMAGE_LAST];
+  memcpy (fmt, native_image_format, sizeof fmt);
+
+  if (!parse_image_spec (object, fmt, 10, type))
+    return 0;
+
+  /* Must specify either the :data or :file keyword.  */
+  return fmt[NATIVE_IMAGE_FILE].count + fmt[NATIVE_IMAGE_DATA].count == 1;
+}
+
+static bool
+native_image_load (struct frame *f, struct image *img)
+{
+  return w32_load_image (f, img,
+                         image_spec_value (img->spec, QCfile, NULL),
+                         image_spec_value (img->spec, QCdata, NULL));
+}
+#endif
+
 \f
 /***********************************************************************
 				 PNG
@@ -6275,12 +6352,12 @@ pbm_load (struct frame *f, struct image *img)
 /* Return true if OBJECT is a valid PNG image specification.  */
 
 static bool
-png_image_p (Lisp_Object object)
+png_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[PNG_LAST];
   memcpy (fmt, png_format, sizeof fmt);
 
-  if (!parse_image_spec (object, fmt, PNG_LAST, Qpng))
+  if (!parse_image_spec (object, fmt, PNG_LAST, type))
     return 0;
 
   /* Must specify either the :data or :file keyword.  */
@@ -6890,7 +6967,6 @@ png_load (struct frame *f, struct image *img)
                         image_spec_value (img->spec, QCdata, NULL));
 }
 
-
 #endif /* HAVE_NS */
 
 
@@ -6938,13 +7014,13 @@ png_load (struct frame *f, struct image *img)
 /* Return true if OBJECT is a valid JPEG image specification.  */
 
 static bool
-jpeg_image_p (Lisp_Object object)
+jpeg_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[JPEG_LAST];
 
   memcpy (fmt, jpeg_format, sizeof fmt);
 
-  if (!parse_image_spec (object, fmt, JPEG_LAST, Qjpeg))
+  if (!parse_image_spec (object, fmt, JPEG_LAST, type))
     return 0;
 
   /* Must specify either the :data or :file keyword.  */
@@ -7514,12 +7590,12 @@ jpeg_load (struct frame *f, struct image *img)
 /* Return true if OBJECT is a valid TIFF image specification.  */
 
 static bool
-tiff_image_p (Lisp_Object object)
+tiff_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[TIFF_LAST];
   memcpy (fmt, tiff_format, sizeof fmt);
 
-  if (!parse_image_spec (object, fmt, TIFF_LAST, Qtiff))
+  if (!parse_image_spec (object, fmt, TIFF_LAST, type))
     return 0;
 
   /* Must specify either the :data or :file keyword.  */
@@ -7962,19 +8038,19 @@ gif_clear_image (struct frame *f, struct image *img)
 /* Return true if OBJECT is a valid GIF image specification.  */
 
 static bool
-gif_image_p (Lisp_Object object)
+gif_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[GIF_LAST];
   memcpy (fmt, gif_format, sizeof fmt);
 
-  if (!parse_image_spec (object, fmt, GIF_LAST, Qgif))
+  if (!parse_image_spec (object, fmt, GIF_LAST, type))
     return 0;
 
   /* Must specify either the :data or :file keyword.  */
   return fmt[GIF_FILE].count + fmt[GIF_DATA].count == 1;
 }
 
-#endif /* HAVE_GIF */
+#endif /* HAVE_GIF || HAVE_NS */
 
 #ifdef HAVE_GIF
 
@@ -8574,12 +8650,12 @@ imagemagick_clear_image (struct frame *f,
    identify the IMAGEMAGICK format.   */
 
 static bool
-imagemagick_image_p (Lisp_Object object)
+imagemagick_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[IMAGEMAGICK_LAST];
   memcpy (fmt, imagemagick_format, sizeof fmt);
 
-  if (!parse_image_spec (object, fmt, IMAGEMAGICK_LAST, Qimagemagick))
+  if (!parse_image_spec (object, fmt, IMAGEMAGICK_LAST, type))
     return 0;
 
   /* Must specify either the :data or :file keyword.  */
@@ -9369,12 +9445,12 @@ DEFUN ("imagemagick-types", Fimagemagick_types, Simagemagick_types, 0, 0, 0,
    identify the SVG format.   */
 
 static bool
-svg_image_p (Lisp_Object object)
+svg_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[SVG_LAST];
   memcpy (fmt, svg_format, sizeof fmt);
 
-  if (!parse_image_spec (object, fmt, SVG_LAST, Qsvg))
+  if (!parse_image_spec (object, fmt, SVG_LAST, type))
     return 0;
 
   /* Must specify either the :data or :file keyword.  */
@@ -9837,7 +9913,7 @@ #define HAVE_GHOSTSCRIPT 1
    specification.  */
 
 static bool
-gs_image_p (Lisp_Object object)
+gs_image_p (Lisp_Object object, Lisp_Object type)
 {
   struct image_keyword fmt[GS_LAST];
   Lisp_Object tem;
@@ -9845,7 +9921,7 @@ gs_image_p (Lisp_Object object)
 
   memcpy (fmt, gs_format, sizeof fmt);
 
-  if (!parse_image_spec (object, fmt, GS_LAST, Qpostscript))
+  if (!parse_image_spec (object, fmt, GS_LAST, type))
     return 0;
 
   /* Bounding box must be a list or vector containing 4 integers.  */
@@ -10132,13 +10208,20 @@ DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 1, 1, 0,
 initialize_image_type (struct image_type const *type)
 {
 #ifdef WINDOWSNT
-  Lisp_Object typesym = builtin_lisp_symbol (type->type);
-  Lisp_Object tested = Fassq (typesym, Vlibrary_cache);
+  Lisp_Object typesym, tested;
+  bool (*init) (void) = type->init;
+
+#ifdef HAVE_NATIVE_IMAGE_API
+  if (init == init_native_image_functions)
+    return init();
+#endif
+
+  typesym = builtin_lisp_symbol (type->type);
+  tested = Fassq (typesym, Vlibrary_cache);
   /* If we failed to load the library before, don't try again.  */
   if (CONSP (tested))
     return !NILP (XCDR (tested)) ? true : false;
 
-  bool (*init) (void) = type->init;
   if (init)
     {
       bool type_valid = init ();
@@ -10165,6 +10248,16 @@ initialize_image_type (struct image_type const *type)
  { SYMBOL_INDEX (Qsvg), svg_image_p, svg_load, image_clear_image,
    IMAGE_TYPE_INIT (init_svg_functions) },
 #endif
+#if defined HAVE_NATIVE_IMAGE_API
+ { SYMBOL_INDEX (Qjpeg), native_image_p, native_image_load, image_clear_image,
+   IMAGE_TYPE_INIT (init_native_image_functions) },
+ { SYMBOL_INDEX (Qpng), native_image_p, native_image_load, image_clear_image,
+   IMAGE_TYPE_INIT (init_native_image_functions) },
+ { SYMBOL_INDEX (Qgif), native_image_p, native_image_load, image_clear_image,
+   IMAGE_TYPE_INIT (init_native_image_functions) },
+ { SYMBOL_INDEX (Qtiff), native_image_p, native_image_load, image_clear_image,
+   IMAGE_TYPE_INIT (init_native_image_functions) },
+#endif
 #if defined HAVE_PNG || defined HAVE_NS
  { SYMBOL_INDEX (Qpng), png_image_p, png_load, image_clear_image,
    IMAGE_TYPE_INIT (init_png_functions) },
@@ -10199,7 +10292,13 @@ lookup_image_type (Lisp_Object type)
     {
       struct image_type const *r = &image_types[i];
       if (EQ (type, builtin_lisp_symbol (r->type)))
+#ifdef HAVE_NATIVE_IMAGE_API
+        /* We can have more than one backend for one image type.  */
+        if (initialize_image_type (r))
+          return r;
+#else
 	return initialize_image_type (r) ? r : NULL;
+#endif
     }
   return NULL;
 }
@@ -10316,22 +10415,22 @@ syms_of_image (void)
   add_image_type (Qxpm);
 #endif
 
-#if defined (HAVE_JPEG) || defined (HAVE_NS)
+#if defined (HAVE_JPEG) || defined (HAVE_NS) || defined (HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qjpeg, "jpeg");
   add_image_type (Qjpeg);
 #endif
 
-#if defined (HAVE_TIFF) || defined (HAVE_NS)
+#if defined (HAVE_TIFF) || defined (HAVE_NS) || defined (HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qtiff, "tiff");
   add_image_type (Qtiff);
 #endif
 
-#if defined (HAVE_GIF) || defined (HAVE_NS)
+#if defined (HAVE_GIF) || defined (HAVE_NS) || defined (HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qgif, "gif");
   add_image_type (Qgif);
 #endif
 
-#if defined (HAVE_PNG) || defined (HAVE_NS)
+#if defined (HAVE_PNG) || defined (HAVE_NS) || defined(HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qpng, "png");
   add_image_type (Qpng);
 #endif
diff --git a/src/w32.c b/src/w32.c
index 698e10e234..1d2a52b6df 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -10225,6 +10225,10 @@ term_ntproc (int ignored)
   term_winsock ();
 
   term_w32select ();
+
+#ifdef HAVE_GDIPLUS
+  w32_gdiplus_shutdown ();
+#endif
 }
 
 void
diff --git a/src/w32image.c b/src/w32image.c
new file mode 100644
index 0000000000..28aa8637b6
--- /dev/null
+++ b/src/w32image.c
@@ -0,0 +1,304 @@
+/* Implementation of GUI terminal on the Microsoft Windows API.
+
+Copyright (C) 1989, 1993-2020 Free Software Foundation, Inc.
+
+This file is part of GNU Emacs.
+
+GNU Emacs is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 3 of the License, or (at
+your option) any later version.
+
+GNU Emacs is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.  */
+
+#include <config.h>
+#include "lisp.h"
+#include "dispextern.h"
+#define COBJMACROS
+#include <objidl.h>
+#include <wtypes.h>
+#include <gdiplus.h>
+#include <shlwapi.h>
+#include "w32common.h"
+#include "w32term.h"
+#include "frame.h"
+#include "coding.h"
+
+/*#define LINK_GDIPLUS_STATICALLY 1*/
+
+#ifndef LINK_GDIPLUS_STATICALLY
+DEF_DLL_FN (GpStatus, GdiplusStartup, (ULONG_PTR *, GdiplusStartupInput *, GdiplusStartupOutput *));
+DEF_DLL_FN (VOID, GdiplusShutdown, (ULONG_PTR));
+DEF_DLL_FN (GpStatus, GdipGetPropertyItemSize, (GpImage *, PROPID, UINT *));
+DEF_DLL_FN (GpStatus, GdipGetPropertyItem, (GpImage *, PROPID, UINT, PropertyItem *));
+DEF_DLL_FN (GpStatus, GdipImageGetFrameDimensionsCount, (GpImage *, UINT *));
+DEF_DLL_FN (GpStatus, GdipImageGetFrameDimensionsList, (GpImage *, GUID *, UINT));
+DEF_DLL_FN (GpStatus, GdipImageGetFrameCount, (GpImage *, GDIPCONST GUID *, UINT *));
+DEF_DLL_FN (GpStatus, GdipImageSelectActiveFrame, (GpImage*, GDIPCONST GUID *, UINT));
+DEF_DLL_FN (GpStatus, GdipCreateBitmapFromFile, (WCHAR *, GpBitmap **));
+DEF_DLL_FN (GpStatus, GdipCreateBitmapFromStream, (IStream *, GpBitmap **));
+DEF_DLL_FN (IStream *, SHCreateMemStream, (const BYTE *pInit, UINT cbInit));
+DEF_DLL_FN (GpStatus, GdipCreateHBITMAPFromBitmap, (GpBitmap *, HBITMAP *, ARGB));
+DEF_DLL_FN (GpStatus, GdipDisposeImage, (GpImage *));
+DEF_DLL_FN (GpStatus, GdipGetImageHeight, (GpImage *, UINT *));
+DEF_DLL_FN (GpStatus, GdipGetImageWidth, (GpImage *, UINT *));
+#endif
+
+static int gdip_initialized = 0;
+static ULONG_PTR token;
+static GdiplusStartupInput input;
+static GdiplusStartupOutput output;
+
+bool
+w32_gdiplus_startup (void)
+{
+  HANDLE gdiplus_lib, shlwapi_lib;
+  GpStatus status;
+
+  if (gdip_initialized < 0)
+      return 0;
+  else if (gdip_initialized)
+      return 1;
+
+#ifndef LINK_GDIPLUS_STATICALLY
+  DEFSYM (Qgdiplus, "gdiplus");
+  DEFSYM (Qshlwapi, "shlwapi");
+  if (!(gdiplus_lib = w32_delayed_load (Qgdiplus))) {
+    gdip_initialized = -1;
+    return 0;
+  }
+  if (!(shlwapi_lib = w32_delayed_load (Qshlwapi))) {
+    gdip_initialized = -1;
+    return 0;
+  }
+
+  LOAD_DLL_FN (gdiplus_lib, GdiplusStartup);
+  LOAD_DLL_FN (gdiplus_lib, GdiplusShutdown);
+  LOAD_DLL_FN (gdiplus_lib, GdipGetPropertyItemSize);
+  LOAD_DLL_FN (gdiplus_lib, GdipGetPropertyItem);
+  LOAD_DLL_FN (gdiplus_lib, GdipImageGetFrameDimensionsCount);
+  LOAD_DLL_FN (gdiplus_lib, GdipImageGetFrameDimensionsList);
+  LOAD_DLL_FN (gdiplus_lib, GdipImageGetFrameCount);
+  LOAD_DLL_FN (gdiplus_lib, GdipImageSelectActiveFrame);
+  LOAD_DLL_FN (gdiplus_lib, GdipCreateBitmapFromFile);
+  LOAD_DLL_FN (gdiplus_lib, GdipCreateBitmapFromStream);
+  LOAD_DLL_FN (gdiplus_lib, GdipCreateHBITMAPFromBitmap);
+  LOAD_DLL_FN (gdiplus_lib, GdipDisposeImage);
+  LOAD_DLL_FN (gdiplus_lib, GdipGetImageHeight);
+  LOAD_DLL_FN (gdiplus_lib, GdipGetImageWidth);
+  LOAD_DLL_FN (shlwapi_lib, SHCreateMemStream);
+
+# define GdiplusStartup fn_GdiplusStartup
+# define GdiplusShutdown fn_GdiplusShutdown
+# define GdipGetPropertyItemSize fn_GdipGetPropertyItemSize
+# define GdipGetPropertyItem fn_GdipGetPropertyItem
+# define GdipImageGetFrameDimensionsCount fn_GdipImageGetFrameDimensionsCount
+# define GdipImageGetFrameDimensionsList fn_GdipImageGetFrameDimensionsList
+# define GdipImageGetFrameCount fn_GdipImageGetFrameCount
+# define GdipImageSelectActiveFrame fn_GdipImageSelectActiveFrame
+# define GdipCreateBitmapFromFile fn_GdipCreateBitmapFromFile
+# define GdipCreateBitmapFromStream fn_GdipCreateBitmapFromStream
+# define SHCreateMemStream fn_SHCreateMemStream
+# define GdipCreateHBITMAPFromBitmap fn_GdipCreateHBITMAPFromBitmap
+# define GdipDisposeImage fn_GdipDisposeImage
+# define GdipGetImageHeight fn_GdipGetImageHeight
+# define GdipGetImageWidth fn_GdipGetImageWidth
+#endif
+
+  input.GdiplusVersion = 1;
+  input.DebugEventCallback = NULL;
+  input.SuppressBackgroundThread = FALSE;
+  input.SuppressExternalCodecs = FALSE;
+
+  status = GdiplusStartup (&token, &input, &output);
+  if (status == Ok)
+    {
+      gdip_initialized = 1;
+      return 1;
+    }
+  else
+    {
+      gdip_initialized = -1;
+      return 0;
+    }
+}
+
+void
+w32_gdiplus_shutdown (void)
+{
+  GdiplusShutdown (token);
+}
+
+
+static double
+w32_frame_delay (GpBitmap *pBitmap, int frame)
+{
+  UINT size;
+  PropertyItem *propertyItem;
+  double delay = 0.0;
+
+  /* Assume that the image has a property item of type PropertyItemEquipMake.
+     Get the size of that property item.  */
+  GdipGetPropertyItemSize (pBitmap, PropertyTagFrameDelay, &size);
+
+  /* Allocate a buffer to receive the property item.  */
+  propertyItem = (PropertyItem*)malloc (size);
+  if (propertyItem != NULL)
+    {
+      /* Get the property item.  */
+      GdipGetPropertyItem (pBitmap, PropertyTagFrameDelay, size, propertyItem);
+      delay = ((double)propertyItem[frame].length) / 100;
+      if (delay == 0)
+        {
+          /* In GIF files, unfortunately, delay is only specified for the first
+             frame.  */
+          delay = ((double)propertyItem[0].length) / 100;
+        }
+      free (propertyItem);
+    }
+  return delay;
+}
+
+static UINT
+w32_select_active_frame (GpBitmap *pBitmap, int frame, int *nframes, double *delay)
+{
+  UINT count, frameCount;
+  GUID pDimensionIDs[1];
+  GpStatus status = Ok;
+
+  status = GdipImageGetFrameDimensionsCount (pBitmap, &count);
+  frameCount = *nframes = 0;
+  *delay = 0.0;
+  if (count)
+    {
+      status = GdipImageGetFrameDimensionsList (pBitmap, pDimensionIDs, 1);
+      status = GdipImageGetFrameCount (pBitmap, &pDimensionIDs[0], &frameCount);
+      if ((status == Ok) && (frameCount > 1))
+        {
+          if (frame < 0 || frame >= frameCount)
+            {
+              status = GenericError;
+            }
+          else
+            {
+              status = GdipImageSelectActiveFrame (pBitmap, &pDimensionIDs[0], frame);
+              *delay = w32_frame_delay (pBitmap, frame);
+              *nframes = frameCount;
+            }
+        }
+    }
+  return status;
+}
+
+static ARGB
+w32_image_bg_color (struct frame *f, struct image *img)
+{
+  /* png_color_16 *image_bg; */
+  Lisp_Object specified_bg
+    = Fplist_get (XCDR (img->spec), QCbackground);
+  Emacs_Color color;
+
+  /* If the user specified a color, try to use it; if not, use the
+     current frame background, ignoring any default background
+     color set by the image.  */
+  if (STRINGP (specified_bg)
+      ? w32_defined_color (f, SSDATA (specified_bg), &color, false, false)
+      : (w32_query_frame_background_color (f, &color), true))
+    /* The user specified `:background', use that.  */
+    {
+      DWORD red = (((DWORD) color.red) & 0xff00) << 8;
+      DWORD green = ((DWORD) color.green) & 0xff00;
+      DWORD blue = ((DWORD) color.blue) >> 8;
+      return red | green | blue;
+    }
+  return ((DWORD) 0xff000000);
+}
+
+int
+w32_load_image (struct frame *f, struct image *img,
+                Lisp_Object spec_file, Lisp_Object spec_data)
+{
+  Emacs_Pixmap pixmap;
+  GpStatus status = GenericError;
+  GpBitmap *pBitmap;
+  wchar_t filename[MAX_PATH];
+  ARGB bg_color;
+  Lisp_Object lisp_index, metadata;
+  unsigned int index, nframes;
+  double delay;
+
+  eassert (valid_image_p (img->spec));
+
+  /* This function only gets called if init_w32_gdiplus () was invoked. We have
+     a valid token and GDI+ is active.  */
+  if (STRINGP (spec_file))
+    {
+      if (w32_unicode_filenames)
+        {
+          filename_to_utf16 (SSDATA (spec_file) , filename);
+          status = GdipCreateBitmapFromFile (filename, &pBitmap);
+        }
+      else
+        {
+          add_to_log ("GDI+ requires w32-unicode-filenames to be T");
+          status = GenericError;
+        }
+    }
+  else if (STRINGP (spec_data))
+    {
+      IStream *pStream = SHCreateMemStream ((BYTE *) SSDATA (spec_data),
+                                            SBYTES (spec_data));
+      if (pStream != NULL)
+        {
+          status = GdipCreateBitmapFromStream (pStream, &pBitmap);
+          IStream_Release (pStream);
+        }
+    }
+
+  metadata = Qnil;
+  if (status == Ok)
+    {
+      /* In multiframe pictures, select the first one */
+      lisp_index = Fplist_get (XCDR (img->spec), QCindex);
+      index = FIXNUMP (lisp_index) ? XFIXNAT (lisp_index) : 0;
+      status = w32_select_active_frame (pBitmap, index, &nframes, &delay);
+      if ((status == Ok))
+        {
+          if (nframes > 1)
+            metadata = Fcons (Qcount, Fcons (make_fixnum (nframes), metadata));
+          if (delay)
+            metadata = Fcons (Qdelay, Fcons (make_float (delay), metadata));
+        }
+    }
+
+  if (status == Ok)
+    {
+      bg_color = w32_image_bg_color (f, img);
+      status = GdipCreateHBITMAPFromBitmap (pBitmap, &pixmap, bg_color);
+      if (status == Ok)
+        {
+          UINT width, height;
+          GdipGetImageWidth (pBitmap, &width);
+          GdipGetImageHeight (pBitmap, &height);
+          img->width = width;
+          img->height = height;
+          img->pixmap = pixmap;
+          img->lisp_data = metadata;
+        }
+
+      GdipDisposeImage (pBitmap);
+    }
+
+  if (status != Ok)
+    {
+      add_to_log ("Unable to load image %s", img->spec);
+      return 0;
+    }
+  return 1;
+}
diff --git a/src/w32term.c b/src/w32term.c
index 5fa77d58e1..f19754df02 100644
--- a/src/w32term.c
+++ b/src/w32term.c
@@ -1529,7 +1529,7 @@ w32_query_colors (struct frame *f, Emacs_Color *colors, int ncolors)
 
 /* Store F's background color into *BGCOLOR.  */
 
-static void
+void
 w32_query_frame_background_color (struct frame *f, Emacs_Color *bgcolor)
 {
   bgcolor->pixel = FRAME_BACKGROUND_PIXEL (f);
diff --git a/src/w32term.h b/src/w32term.h
index f8a8a727e8..d44c6f9b83 100644
--- a/src/w32term.h
+++ b/src/w32term.h
@@ -75,7 +75,10 @@ #define CP_DEFAULT 1004
 extern void w32_regenerate_palette (struct frame *f);
 extern void w32_fullscreen_rect (HWND hwnd, int fsmode, RECT normal,
                                  RECT *rect);
-
+extern int w32_load_image (struct frame *f, struct image *img,
+                           Lisp_Object spec_file, Lisp_Object spec_data);
+extern bool w32_gdiplus_startup (void);
+extern void w32_gdiplus_shutdown (void);
 \f
 /* For each display (currently only one on w32), we have a structure that
    records information about it.  */
@@ -248,6 +251,8 @@ #define CP_DEFAULT 1004
 extern int w32_display_pixel_width (struct w32_display_info *);
 extern void initialize_frame_menubar (struct frame *);
 extern void w32_dialog_in_progress (Lisp_Object in_progress);
+extern void w32_query_frame_background_color (struct frame *f,
+                                              Emacs_Color *bgcolor);
 
 extern void w32_make_frame_visible (struct frame *f);
 extern void w32_make_frame_invisible (struct frame *f);

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Mon, 13 Apr 2020 12:04:39 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > I'd like to install these changes, but some parts seem to be missing,
> > see above.  Could you please post the missing parts (or the entire
> > changeset, if that is more convenient)?
> 
> Apologies for the corrupted patch. Here it goes, I hope, in a complete form.

Thanks, I've installed this now on the master branch, with some
changes to enable switching this feature on and off at runtime.
Building with this feature available is now ON by default, but to
actually use the feature, one needs to turn it on at runtime by
setting the new variable w32-use-native-image-API to a non-nil value.

There's a FIXME in w32image.c: in my testing the call to
GdipImageGetFrameCount in w32_select_active_frame always fails with
status Win32Err, I couldn't understand why.  Did I goof with some of
my changes on top of yours?  For now, I simply disregarded that
particular error; the code seems to work fine otherwise (but
multiframe images will probably not work as expected).  Could you
please look into this and see what could be causing this?

There's another FIXME in w32image.c that asks about including the
objidl.h header.  I didn't need that in my mingw.org's MinGW build,
but maybe MinGW64 builds do.

Testing and bug reports about this new feature are welcome.

Thanks again for working on this and for persevering.

Re: GDI+ take 3

[Eli Zaretskii]

> Date: Tue, 14 Apr 2020 18:33:59 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> Thanks, I've installed this now on the master branch, with some
> changes to enable switching this feature on and off at runtime.
> Building with this feature available is now ON by default, but to
> actually use the feature, one needs to turn it on at runtime by
> setting the new variable w32-use-native-image-API to a non-nil value.

One more thing: Alan, as you asked, I tried to keep the changes
extensible to NS native image display, I hope I succeeded.  Please
take a look; if something else needs to be done to make NS use the
framework, let's talk.

Thanks.

Re: GDI+ take 3

[Dmitry Gutov]

On 14.04.2020 18:33, Eli Zaretskii wrote:
> Building with this feature available is now ON by default, but to
> actually use the feature, one needs to turn it on at runtime by
> setting the new variable w32-use-native-image-API to a non-nil value.

I would suggest turning it on to collect valuable feedback from early 
testers, since master is pretty far from a release.

Re: GDI+ take 3

[Eli Zaretskii]

> Cc: emacs-devel@gnu.org
> From: Dmitry Gutov <dgutov@yandex.ru>
> Date: Tue, 14 Apr 2020 21:38:33 +0300
> 
> I would suggest turning it on to collect valuable feedback from early 
> testers, since master is pretty far from a release.

As soon as I'm confident it's reliable enough.  You didn't have the
few hours I had stepping through the code...

Re: GDI+ take 3

[Basil L. Contovounesios]

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

Eli Zaretskii <eliz@gnu.org> writes:

> Testing and bug reports about this new feature are welcome.

No testing, just a compiler warning:

  image.c:6240:1: warning: ‘image_can_use_native_api’ defined but not used [-Wunused-function]
   6240 | image_can_use_native_api (Lisp_Object type)
        | ^~~~~~~~~~~~~~~~~~~~~~~~

Is the following okay as a fix?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Fix-warning-from-recent-native-image-API-changes.patch --]
[-- Type: text/x-diff, Size: 1008 bytes --]

From 31ba44075d766e3ccf0f0bf65c17e1d9944b03f5 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
Date: Tue, 14 Apr 2020 19:37:28 +0100
Subject: [PATCH] Fix warning from recent native image API changes

* src/image.c (image_can_use_native_api) [HAVE_NATIVE_IMAGE_API]:
Define static function only if it will be used.
---
 src/image.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/image.c b/src/image.c
index aacaf0b734..039fd1bd63 100644
--- a/src/image.c
+++ b/src/image.c
@@ -6236,19 +6236,17 @@ pbm_load (struct frame *f, struct image *img)
 			    NATIVE IMAGE HANDLING
  ***********************************************************************/
 
+#if HAVE_NATIVE_IMAGE_API
 static bool
 image_can_use_native_api (Lisp_Object type)
 {
-#if HAVE_NATIVE_IMAGE_API
 # ifdef HAVE_NTGUI
   return w32_can_use_native_image_api (type);
 # else
   return false;
 # endif
-#else
-  return false;
-#endif
 }
+#endif
 
 #if HAVE_NATIVE_IMAGE_API
 
-- 
2.25.1


[-- Attachment #3: Type: text/plain, Size: 829 bytes --]


Thanks,

-- 
Basil

In GNU Emacs 28.0.50 (build 3, x86_64-pc-linux-gnu, X toolkit, cairo version 1.16.0, Xaw3d scroll bars)
 of 2020-04-14 built on thunk
Repository revision: 439e578532279507f45519c67eb84d5b7bbba8b6
Repository branch: blc/image-native-warning
Windowing system distributor 'The X.Org Foundation', version 11.0.12008000
System Description: Debian GNU/Linux bullseye/sid

Configured using:
 'configure 'CC=ccache gcc' 'CFLAGS=-O2 -march=native' --config-cache
 --prefix=/home/blc/.local --with-x-toolkit=lucid
 --with-file-notification=yes --with-x'

Configured features:
XAW3D XPM JPEG TIFF GIF PNG RSVG CAIRO SOUND GPM DBUS GSETTINGS GLIB
NOTIFY INOTIFY ACL LIBSELINUX GNUTLS LIBXML2 FREETYPE HARFBUZZ M17N_FLT
LIBOTF ZLIB TOOLKIT_SCROLL_BARS LUCID X11 XDBE XIM MODULES THREADS
LIBSYSTEMD JSON PDUMPER LCMS2 GMP

Re: GDI+ take 3

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Cc: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>,
>   emacs-devel@gnu.org
> Date: Tue, 14 Apr 2020 20:08:45 +0100
> 
> No testing, just a compiler warning:
> 
>   image.c:6240:1: warning: ‘image_can_use_native_api’ defined but not used [-Wunused-function]
>    6240 | image_can_use_native_api (Lisp_Object type)
>         | ^~~~~~~~~~~~~~~~~~~~~~~~

Ugh!

> Is the following okay as a fix?

No, that's not right, because it will break the WINDOWSNT build.  I
fixed it slightly differently, please take a look.

Thanks.

Re: GDI+ take 3

[Dmitry Gutov]

On 14.04.2020 21:43, Eli Zaretskii wrote:
> As soon as I'm confident it's reliable enough.  You didn't have the
> few hours I had stepping through the code...

Yes, of course.

Re: GDI+ take 3

[Basil L. Contovounesios]

Eli Zaretskii <eliz@gnu.org> writes:

>> Is the following okay as a fix?
>
> No, that's not right, because it will break the WINDOWSNT build.  I
> fixed it slightly differently, please take a look.

LGTM, thanks.

-- 
Basil

Re: GDI+ take 3

[Alan Third]

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

On Tue, Apr 14, 2020 at 08:43:52PM +0300, Eli Zaretskii wrote:
> > Date: Tue, 14 Apr 2020 18:33:59 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> > Cc: emacs-devel@gnu.org
> > 
> > Thanks, I've installed this now on the master branch, with some
> > changes to enable switching this feature on and off at runtime.
> > Building with this feature available is now ON by default, but to
> > actually use the feature, one needs to turn it on at runtime by
> > setting the new variable w32-use-native-image-API to a non-nil value.
> 
> One more thing: Alan, as you asked, I tried to keep the changes
> extensible to NS native image display, I hope I succeeded.  Please
> take a look; if something else needs to be done to make NS use the
> framework, let's talk.

Looks good, I’ve implemented the NS equivalent. The only thing missing
is the ability to force the native API off at run‐time, but I’m not
sure it’s important.

-- 
Alan Third

[-- Attachment #2: 0001-Use-native-image-API-for-NS.patch --]
[-- Type: text/plain, Size: 12858 bytes --]

From b21f001a21fa3f5ea6f6ffb0a2c4a753faf420b4 Mon Sep 17 00:00:00 2001
From: Alan Third <alan@idiocy.org>
Date: Tue, 14 Apr 2020 22:56:06 +0100
Subject: [PATCH] Use native image API for NS

* configure.ac (NATIVE_IMAGE_API): Move above NS definitions.
(HAVE_NATIVE_IMAGE_API): Set for NS.
(HAVE_PNG, HAVE_JPEG, HAVE_GIF, HAVE_TIFF): Enable on NS builds.
* src/image.c (HAVE_NS): Fix a number of #if's so they no longer rely
on HAVE_NS.
(PIX_MASK_DRAW): Add for HAVE_NS so libpng support will compile.
(image_can_use_native_api):
(native_image_load): Add NS support.
(png_load):
(jpeg_load):
(tiff_load):
(gif_load): Remove NS specific definitions.
* src/nsimage.m (ns_can_use_native_image_api): New function.
* src/nsterm.h: (ns_can_use_native_image_api): New function.
---
 configure.ac  | 25 +++++++++------
 src/image.c   | 89 +++++++++++++++------------------------------------
 src/nsimage.m | 49 ++++++++++++++++++++++++++++
 src/nsterm.h  |  1 +
 4 files changed, 91 insertions(+), 73 deletions(-)

diff --git a/configure.ac b/configure.ac
index b0a2cc466b..cdb8378abe 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1910,6 +1910,8 @@ AC_DEFUN
   bitmapdir=${bmd_acc#:}
 fi
 
+NATIVE_IMAGE_API=no
+
 test "${with_ns}" = maybe && test "${opsys}" != darwin && with_ns=no
 HAVE_NS=no
 NS_GNUSTEP_CONFIG=no
@@ -2021,6 +2023,11 @@ AC_DEFUN
        AC_MSG_ERROR([Mac OS X 10.6 or newer is required]);
     fi
   fi
+
+  if test "${with_native_image_api}" = yes; then
+     AC_DEFINE(HAVE_NATIVE_IMAGE_API, 1, [Define to use native OS APIs for images.])
+     NATIVE_IMAGE_API="yes (ns)"
+  fi
 fi
 
 AC_SUBST(LIBS_GNUSTEP)
@@ -2127,7 +2134,6 @@ AC_DEFUN
 NTLIB=
 CM_OBJ="cm.o"
 XARGS_LIMIT=
-NATIVE_IMAGE_API=no
 if test "${HAVE_W32}" = "yes"; then
   AC_DEFINE(HAVE_NTGUI, 1, [Define to use native MS Windows GUI.])
   if test "$with_toolkit_scroll_bars" = "no"; then
@@ -3575,9 +3581,8 @@ AC_DEFUN
 ### Use -ljpeg if available, unless '--with-jpeg=no'.
 HAVE_JPEG=no
 LIBJPEG=
-if test "${NS_IMPL_COCOA}" = yes; then
-  : # Cocoa provides its own jpeg support, so do nothing.
-elif test "${HAVE_X11}" = "yes" || test "${HAVE_W32}" = "yes"; then
+if test "${HAVE_X11}" = "yes" || test "${HAVE_W32}" = "yes" \
+   || test "${HAVE_NS}" = "yes"; then
   if test "${with_jpeg}" != "no"; then
     AC_CACHE_CHECK([for jpeglib 6b or later],
       [emacs_cv_jpeglib],
@@ -3726,13 +3731,12 @@ AC_DEFUN
 HAVE_PNG=no
 LIBPNG=
 PNG_CFLAGS=
-if test "${NS_IMPL_COCOA}" = yes; then
-  : # Cocoa provides its own png support, so do nothing.
-elif test "${with_png}" != no; then
+if test "${with_png}" != no; then
   # mingw32 loads the library dynamically.
   if test "$opsys" = mingw32; then
     AC_CHECK_HEADER([png.h], [HAVE_PNG=yes])
-  elif test "${HAVE_X11}" = "yes" || test "${HAVE_W32}" = "yes"; then
+  elif test "${HAVE_X11}" = "yes" || test "${HAVE_W32}" = "yes" \
+       || test "${HAVE_NS}" = "yes"; then
     EMACS_CHECK_MODULES([PNG], [libpng >= 1.0.0])
     if test $HAVE_PNG = yes; then
       LIBPNG=$PNG_LIBS
@@ -3806,7 +3810,8 @@ AC_DEFUN
   if test "${HAVE_TIFF}" = "yes"; then
     AC_DEFINE(HAVE_TIFF, 1, [Define to 1 if you have the tiff library (-ltiff).])
   fi
-elif test "${HAVE_X11}" = "yes" || test "${HAVE_W32}" = "yes"; then
+elif test "${HAVE_X11}" = "yes" || test "${HAVE_W32}" = "yes" \
+     || test "${HAVE_NS}" = "yes"; then
   if test "${with_tiff}" != "no"; then
     AC_CHECK_HEADER(tiffio.h,
       [tifflibs="-lz -lm"
@@ -3835,7 +3840,7 @@ AC_DEFUN
     AC_DEFINE(HAVE_GIF, 1, [Define to 1 if you have a gif (or ungif) library.])
   fi
 elif test "${HAVE_X11}" = "yes" && test "${with_gif}" != "no" \
-        || test "${HAVE_W32}" = "yes"; then
+        || test "${HAVE_W32}" = "yes" || test "${HAVE_NS}" = "yes"; then
   AC_CHECK_HEADER(gif_lib.h,
 # EGifPutExtensionLast only exists from version libungif-4.1.0b1.
 # Earlier versions can crash Emacs, but version 5.0 removes EGifPutExtensionLast.
diff --git a/src/image.c b/src/image.c
index aacaf0b734..210c83a5fe 100644
--- a/src/image.c
+++ b/src/image.c
@@ -24,7 +24,7 @@ Copyright (C) 1989, 1992-2020 Free Software Foundation, Inc.
 
 /* Include this before including <setjmp.h> to work around bugs with
    older libpng; see Bug#17429.  */
-#if defined HAVE_PNG && !defined HAVE_NS
+#if defined HAVE_PNG
 # include <png.h>
 #endif
 
@@ -125,6 +125,7 @@ #define PUT_PIXEL XPutPixel
 #define NO_PIXMAP 0
 
 #define PIX_MASK_RETAIN	0
+#define PIX_MASK_DRAW	1
 
 #endif /* HAVE_NS */
 
@@ -6242,6 +6243,8 @@ image_can_use_native_api (Lisp_Object type)
 #if HAVE_NATIVE_IMAGE_API
 # ifdef HAVE_NTGUI
   return w32_can_use_native_image_api (type);
+# elif defined HAVE_NS
+  return ns_can_use_native_image_api (type);
 # else
   return false;
 # endif
@@ -6315,6 +6318,10 @@ native_image_load (struct frame *f, struct image *img)
   return w32_load_image (f, img,
                          image_spec_value (img->spec, QCfile, NULL),
                          image_spec_value (img->spec, QCdata, NULL));
+# elif defined HAVE_NS
+  return ns_load_image (f, img,
+                        image_spec_value (img->spec, QCfile, NULL),
+                        image_spec_value (img->spec, QCdata, NULL));
 # else
   return 0;
 # endif
@@ -6327,7 +6334,7 @@ native_image_load (struct frame *f, struct image *img)
 				 PNG
  ***********************************************************************/
 
-#if defined (HAVE_PNG) || defined (HAVE_NS)
+#if defined (HAVE_PNG)
 
 /* Indices of image specification fields in png_format, below.  */
 
@@ -6378,10 +6385,10 @@ png_image_p (Lisp_Object object)
   return fmt[PNG_FILE].count + fmt[PNG_DATA].count == 1;
 }
 
-#endif /* HAVE_PNG || HAVE_NS */
+#endif /* HAVE_PNG */
 
 
-#if defined HAVE_PNG && !defined HAVE_NS
+#ifdef HAVE_PNG
 
 # ifdef WINDOWSNT
 /* PNG library details.  */
@@ -6971,17 +6978,7 @@ png_load (struct frame *f, struct image *img)
   return png_load_body (f, img, &c);
 }
 
-#elif defined HAVE_NS
-
-static bool
-png_load (struct frame *f, struct image *img)
-{
-  return ns_load_image (f, img,
-                        image_spec_value (img->spec, QCfile, NULL),
-                        image_spec_value (img->spec, QCdata, NULL));
-}
-
-#endif /* HAVE_NS */
+#endif /* HAVE_PNG */
 
 
 \f
@@ -6989,7 +6986,7 @@ png_load (struct frame *f, struct image *img)
 				 JPEG
  ***********************************************************************/
 
-#if defined (HAVE_JPEG) || defined (HAVE_NS)
+#if defined (HAVE_JPEG)
 
 /* Indices of image specification fields in gs_format, below.  */
 
@@ -7041,7 +7038,7 @@ jpeg_image_p (Lisp_Object object)
   return fmt[JPEG_FILE].count + fmt[JPEG_DATA].count == 1;
 }
 
-#endif /* HAVE_JPEG || HAVE_NS */
+#endif /* HAVE_JPEG */
 
 #ifdef HAVE_JPEG
 
@@ -7543,18 +7540,6 @@ jpeg_load (struct frame *f, struct image *img)
   return jpeg_load_body (f, img, &mgr);
 }
 
-#else /* HAVE_JPEG */
-
-#ifdef HAVE_NS
-static bool
-jpeg_load (struct frame *f, struct image *img)
-{
-  return ns_load_image (f, img,
-			image_spec_value (img->spec, QCfile, NULL),
-			image_spec_value (img->spec, QCdata, NULL));
-}
-#endif  /* HAVE_NS */
-
 #endif /* !HAVE_JPEG */
 
 
@@ -7563,7 +7548,7 @@ jpeg_load (struct frame *f, struct image *img)
 				 TIFF
  ***********************************************************************/
 
-#if defined (HAVE_TIFF) || defined (HAVE_NS)
+#if defined (HAVE_TIFF)
 
 /* Indices of image specification fields in tiff_format, below.  */
 
@@ -7616,7 +7601,7 @@ tiff_image_p (Lisp_Object object)
   return fmt[TIFF_FILE].count + fmt[TIFF_DATA].count == 1;
 }
 
-#endif /* HAVE_TIFF || HAVE_NS */
+#endif /* HAVE_TIFF */
 
 #ifdef HAVE_TIFF
 
@@ -7984,16 +7969,6 @@ tiff_load (struct frame *f, struct image *img)
   return 1;
 }
 
-#elif defined HAVE_NS
-
-static bool
-tiff_load (struct frame *f, struct image *img)
-{
-  return ns_load_image (f, img,
-                        image_spec_value (img->spec, QCfile, NULL),
-                        image_spec_value (img->spec, QCdata, NULL));
-}
-
 #endif
 
 
@@ -8002,7 +7977,7 @@ tiff_load (struct frame *f, struct image *img)
 				 GIF
  ***********************************************************************/
 
-#if defined (HAVE_GIF) || defined (HAVE_NS)
+#if defined (HAVE_GIF)
 
 /* Indices of image specification fields in gif_format, below.  */
 
@@ -8064,7 +8039,7 @@ gif_image_p (Lisp_Object object)
   return fmt[GIF_FILE].count + fmt[GIF_DATA].count == 1;
 }
 
-#endif /* HAVE_GIF || HAVE_NS */
+#endif /* HAVE_GIF */
 
 #ifdef HAVE_GIF
 
@@ -8581,18 +8556,6 @@ gif_load (struct frame *f, struct image *img)
   return 1;
 }
 
-#else  /* !HAVE_GIF */
-
-#ifdef HAVE_NS
-static bool
-gif_load (struct frame *f, struct image *img)
-{
-  return ns_load_image (f, img,
-                        image_spec_value (img->spec, QCfile, NULL),
-			image_spec_value (img->spec, QCdata, NULL));
-}
-#endif /* HAVE_NS */
-
 #endif /* HAVE_GIF */
 
 
@@ -10259,19 +10222,19 @@ initialize_image_type (struct image_type const *type)
  { SYMBOL_INDEX (Qsvg), svg_image_p, svg_load, image_clear_image,
    IMAGE_TYPE_INIT (init_svg_functions) },
 #endif
-#if defined HAVE_PNG || defined HAVE_NS
+#if defined HAVE_PNG
  { SYMBOL_INDEX (Qpng), png_image_p, png_load, image_clear_image,
    IMAGE_TYPE_INIT (init_png_functions) },
 #endif
-#if defined HAVE_GIF || defined HAVE_NS
+#if defined HAVE_GIF
  { SYMBOL_INDEX (Qgif), gif_image_p, gif_load, gif_clear_image,
    IMAGE_TYPE_INIT (init_gif_functions) },
 #endif
-#if defined HAVE_TIFF || defined HAVE_NS
+#if defined HAVE_TIFF
  { SYMBOL_INDEX (Qtiff), tiff_image_p, tiff_load, image_clear_image,
    IMAGE_TYPE_INIT (init_tiff_functions) },
 #endif
-#if defined HAVE_JPEG || defined HAVE_NS
+#if defined HAVE_JPEG
  { SYMBOL_INDEX (Qjpeg), jpeg_image_p, jpeg_load, image_clear_image,
    IMAGE_TYPE_INIT (init_jpeg_functions) },
 #endif
@@ -10421,22 +10384,22 @@ syms_of_image (void)
   add_image_type (Qxpm);
 #endif
 
-#if defined (HAVE_JPEG) || defined (HAVE_NS) || defined (HAVE_NATIVE_IMAGE_API)
+#if defined (HAVE_JPEG) || defined (HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qjpeg, "jpeg");
   add_image_type (Qjpeg);
 #endif
 
-#if defined (HAVE_TIFF) || defined (HAVE_NS) || defined (HAVE_NATIVE_IMAGE_API)
+#if defined (HAVE_TIFF) || defined (HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qtiff, "tiff");
   add_image_type (Qtiff);
 #endif
 
-#if defined (HAVE_GIF) || defined (HAVE_NS) || defined (HAVE_NATIVE_IMAGE_API)
+#if defined (HAVE_GIF) || defined (HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qgif, "gif");
   add_image_type (Qgif);
 #endif
 
-#if defined (HAVE_PNG) || defined (HAVE_NS) || defined(HAVE_NATIVE_IMAGE_API)
+#if defined (HAVE_PNG) || defined (HAVE_NATIVE_IMAGE_API)
   DEFSYM (Qpng, "png");
   add_image_type (Qpng);
 #endif
diff --git a/src/nsimage.m b/src/nsimage.m
index 3cccc984ca..07750de95f 100644
--- a/src/nsimage.m
+++ b/src/nsimage.m
@@ -45,6 +45,55 @@ Updated by Christian Limpach (chris@nice.ch)
 
    ========================================================================== */
 
+bool
+ns_can_use_native_image_api (Lisp_Object type)
+{
+  NSString *imageType = @"unknown";
+  NSArray *types;
+
+  NSTRACE ("ns_can_use_native_image_api");
+
+  if (EQ (type, Qnative_image))
+    return YES;
+
+#ifdef NS_IMPL_COCOA
+  /* Work out the UTI of the image type.  */
+  if (EQ (type, Qjpeg))
+    imageType = @"public.jpeg";
+  else if (EQ (type, Qpng))
+    imageType = @"public.png";
+  else if (EQ (type, Qgif))
+    imageType = @"com.compuserve.gif";
+  else if (EQ (type, Qtiff))
+    imageType = @"public.tiff";
+  else if (EQ (type, Qsvg))
+    imageType = @"public.svg-image";
+
+  /* NSImage also supports a host of other types such as PDF and BMP,
+     but we don't yet support these in image.c.  */
+
+  types = [NSImage imageTypes];
+#else
+  /* Work out the image type.  */
+  if (EQ (type, Qjpeg))
+    imageType = @"jpeg";
+  else if (EQ (type, Qpng))
+    imageType = @"png";
+  else if (EQ (type, Qgif))
+    imageType = @"gif";
+  else if (EQ (type, Qtiff))
+    imageType = @"tiff";
+
+  types = [NSImage imageFileTypes];
+#endif
+
+  /* Check if the type is supported on this system.  */
+  if ([types indexOfObject:imageType] != NSNotFound)
+    return YES;
+  else
+    return NO;
+}
+
 void *
 ns_image_from_XBM (char *bits, int width, int height,
                    unsigned long fg, unsigned long bg)
diff --git a/src/nsterm.h b/src/nsterm.h
index f5d3c32b8b..8d5371c8f2 100644
--- a/src/nsterm.h
+++ b/src/nsterm.h
@@ -1189,6 +1189,7 @@ #define NSAPP_DATA2_RUNFILEDIALOG 11
 
 /* From nsimage.m, needed in image.c */
 struct image;
+extern bool ns_can_use_native_image_api (Lisp_Object type);
 extern void *ns_image_from_XBM (char *bits, int width, int height,
                                 unsigned long fg, unsigned long bg);
 extern void *ns_image_for_XPM (int width, int height, int depth);
-- 
2.26.1

Re: GDI+ take 3

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Tue, 14 Apr 2020 22:57:19 +0100
> Cc: juanjose.garciaripoll@gmail.com, emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> Is the following okay as a fix?
> >
> > No, that's not right, because it will break the WINDOWSNT build.  I
> > fixed it slightly differently, please take a look.
> 
> LGTM, thanks.

Thanks for testing.

Re: GDI+ take 3

[Juanma Barranquero]

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

Builds and runs ok here.  (MSYS2 build, Windows 10.)

BTW, NEWS says

  This support is enabled
  with --with-native-image-api, which automatically disables the use of
  optional third party libraries for those formats.

but I think it is enabled by default if supported on the build environment,
isn't it?

[-- Attachment #2: Type: text/html, Size: 410 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 15:40:49 +0200
> Cc: "Basil L. Contovounesios" <contovob@tcd.ie>, juanjose.garciaripoll@gmail.com, 
> 	Emacs developers <emacs-devel@gnu.org>
> 
> Builds and runs ok here.  (MSYS2 build, Windows 10.)

Thanks for testing.

Could you perhaps step with a debugger through w32_select_active_frame
and see if this call:

   status = GdipImageGetFrameCount (pBitmap, &pDimensionIDs[0], &frameCount);

returns a value of 'status' that is something other than 'Ok'?  It
returns Win32Error here, for some reason.  If it does return
Win32Error for you as well, what does this show if you type it in GDB
immediately after the call:

  (gdb) p w32_last_error ()

Here it shows zero, which is... not helpful :-(

> BTW, NEWS says 
> 
>   This support is enabled
>   with --with-native-image-api, which automatically disables the use of
>   optional third party libraries for those formats.
> 
> but I think it is enabled by default if supported on the build environment, isn't it?

Right, and "./configure --help" also had it backwards.  Fixed, thanks.

re: GDI+ take 3

[Angelo Graziosi]

Eli Zaretskii wrote:
> Building with this feature available is now ON by default

From master it says:

[...]
  Does Emacs use -ljpeg?                                  yes
  Does Emacs use -ltiff?                                  yes
  Does Emacs use a gif library?                           yes
  Does Emacs use a png library?                           yes
[...]
  Does Emacs use native APIs for images?                  yes (w32)
[...]

Maybe I am wrong, but the discussion started with the goal to make the Windows build self-contained without PNG, JPEG etc.. Or Not?

So I do not understand why this should be better...

 Angelo

Re: GDI+ take 3

[Juanma Barranquero]

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

On Wed, Apr 15, 2020 at 4:00 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Could you perhaps step with a debugger through w32_select_active_frame
> and see if this call:
>
>    status = GdipImageGetFrameCount (pBitmap, &pDimensionIDs[0],
&frameCount);
>
> returns a value of 'status' that is something other than 'Ok'?  It
> returns Win32Error here, for some reason.

Thread 1 hit Breakpoint 2, w32_select_active_frame (pBitmap=0xf522f0,
    frame=0, nframes=nframes@entry=0xbfcdbc, delay=delay@entry=0xbfcdc0)
    at w32image.c:235
235     {
(gdb) n
240       status = GdipImageGetFrameDimensionsCount (pBitmap, &count);
(gdb) n
241       frameCount = *nframes = 0;
(gdb) n
242       *delay = 0.0;
(gdb) n
243       if (count)
(gdb) n
245           status = GdipImageGetFrameDimensionsList (pBitmap,
pDimensionIDs, 1);
(gdb) n
246           status = GdipImageGetFrameCount (pBitmap, &pDimensionIDs[0],
&frameCount);
(gdb) n
247           if (status == Ok && frameCount > 1)
(gdb) p status
$2 = Ok
(gdb)

[-- Attachment #2: Type: text/html, Size: 1323 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> Date: Wed, 15 Apr 2020 16:07:46 +0200 (CEST)
> From: Angelo Graziosi <angelo.g0@libero.it>
> 
>   Does Emacs use -ljpeg?                                  yes
>   Does Emacs use -ltiff?                                  yes
>   Does Emacs use a gif library?                           yes
>   Does Emacs use a png library?                           yes
> [...]
>   Does Emacs use native APIs for images?                  yes (w32)
> [...]
> 
> Maybe I am wrong, but the discussion started with the goal to make the Windows build self-contained without PNG, JPEG etc.. Or Not?

You could try building --without-jpeg etc., if you want.  It's
supposed to work (except that w32-use-native-image-API still has to be
set to a non-nil value for it to actually do it).  I didn't yet try
such a build myself, so if there are problems, please be sure to
report them.

Re: GDI+ take 3

[Juanma Barranquero]

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

I've tried with gif, png, jpeg and tiff images and I get Ok in every case.

[-- Attachment #2: Type: text/html, Size: 100 bytes --]

Re: GDI+ take 3

[Angelo Graziosi]

> Il 15 aprile 2020 alle 16.15 Eli Zaretskii ha scritto:
> 
> You could try building --without-jpeg etc., if you want. 

If I understand, GDI does not replace rsvg and Xpm so we have to allow the system to include them. Right?

Re: GDI+ take 3

[Eli Zaretskii]

> Date: Wed, 15 Apr 2020 16:22:38 +0200 (CEST)
> From: Angelo Graziosi <angelo.g0@libero.it>
> Cc: emacs-devel@gnu.org
> 
> 
> > Il 15 aprile 2020 alle 16.15 Eli Zaretskii ha scritto:
> > 
> > You could try building --without-jpeg etc., if you want. 
> 
> If I understand, GDI does not replace rsvg and Xpm so we have to allow the system to include them. Right?

Yes.

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 16:12:01 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> 246           status = GdipImageGetFrameCount (pBitmap, &pDimensionIDs[0], &frameCount);
> (gdb) n
> 247           if (status == Ok && frameCount > 1)
> (gdb) p status
> $2 = Ok

Thanks.  This probably means the problem, whatever it is, is either
specific to XP or to mingw.org's MinGW.

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 16:17:49 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> I've tried with gif, png, jpeg and tiff images and I get Ok in every case.

What about GIF and TIFF images that have more than one frame?  Does
that work as expected?

Re: GDI+ take 3

[Juanma Barranquero]

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

On Wed, Apr 15, 2020 at 4:28 PM Eli Zaretskii <eliz@gnu.org> wrote:

> What about GIF and TIFF images that have more than one frame?  Does
> that work as expected?

I don't have a multi-frame tiff around, but I just tested with an animated
gif:

(gdb) n
246           status = GdipImageGetFrameCount (pBitmap, &pDimensionIDs[0],
&frameCount);
(gdb) n
247           if (status == Ok && frameCount > 1)
(gdb) p status
$7 = Ok
(gdb) p frameCount
$8 = 150
(gdb)

[-- Attachment #2: Type: text/html, Size: 628 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 16:35:43 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> > What about GIF and TIFF images that have more than one frame?  Does
> > that work as expected?
> 
> I don't have a multi-frame tiff around, but I just tested with an animated gif:
> 
> (gdb) n
> 246           status = GdipImageGetFrameCount (pBitmap, &pDimensionIDs[0], &frameCount);
> (gdb) n
> 247           if (status == Ok && frameCount > 1)
> (gdb) p status
> $7 = Ok
> (gdb) p frameCount
> $8 = 150
> (gdb)

Does this display as expected, when w32-use-native-image-API is
non-nil?

And is there some place I can obtain this GIF file, to try it here?

Thanks.

Re: GDI+ take 3

[Juanma Barranquero]

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

On Wed, Apr 15, 2020 at 4:44 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Does this display as expected, when w32-use-native-image-API is
> non-nil?

I'm rebuilding, will test shortly.

> And is there some place I can obtain this GIF file, to try it here?

It's the animated gif in this BoingBoing article:

https://boingboing.net/2018/08/22/caucasian-ovcharka-determines.html

[-- Attachment #2: Type: text/html, Size: 625 bytes --]

Re: GDI+ take 3

[Juanma Barranquero]

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

On Wed, Apr 15, 2020 at 4:44 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Does this display as expected, when w32-use-native-image-API is
> non-nil?

Animation is choppy and stops well before the end.

However, I can use f and b to move back and forth as expected. It's just
the animation that apparently fails.

This is my default checking, non-optimized build, BTW, so perhaps that's a
factor.

[-- Attachment #2: Type: text/html, Size: 522 bytes --]

Re: GDI+ take 3

[Juanma Barranquero]

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

Hmm.

When I disable the native API and use libgif-7, the animation works just
fine.

[-- Attachment #2: Type: text/html, Size: 115 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 17:02:46 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> When I disable the native API and use libgif-7, the animation works just fine. 

Yes, works here as well in that case.

Are the values of delay returned by w32_frame_delay similar to what
you get with libgif animation?

Re: GDI+ take 3

[Angelo Graziosi]

I have built --without-jpeg,tiff,png,gif and enabled the API variable:

(setq w32-use-native-image-API t)

for the JPEG and PNG it seems there are no issue.

I tested also with the CavernousFeistyArachnid-size_restricted GIF from  Juanma link. The GIF si loaded but not animated (the same occurs with the build I did on April 06 which include JPEG, PNG, GIF etc..)

I don't have a TIFF image...

Re: GDI+ take 3

[Eli Zaretskii]

> Date: Wed, 15 Apr 2020 17:25:15 +0200 (CEST)
> From: Angelo Graziosi <angelo.g0@libero.it>
> Cc: emacs-devel@gnu.org
> 
> I tested also with the CavernousFeistyArachnid-size_restricted GIF from  Juanma link. The GIF si loaded but not animated (the same occurs with the build I did on April 06 which include JPEG, PNG, GIF etc..)

You need to type RET to start animation.  When I do that in a build
with libgif, it starts animation for me.

Re: GDI+ take 3

[Juanma Barranquero]

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

On Wed, Apr 15, 2020 at 5:10 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Are the values of delay returned by w32_frame_delay similar to what
> you get with libgif animation?

Sorry, I don't understand. With the native animation w32_frame_delay
returns 6.

253                   status = GdipImageSelectActiveFrame (pBitmap,
&pDimensionIDs[0],
(gdb) n
255                   *delay = w32_frame_delay (pBitmap, frame);
(gdb) n
256                   *nframes = frameCount;
(gdb) p *delay
$1 = 6
(gdb)

How do I get the equivalent value when using libgif?

[-- Attachment #2: Type: text/html, Size: 722 bytes --]

Re: GDI+ take 3

[Angelo Graziosi]

> Il 15 aprile 2020 alle 17.27 Eli Zaretskii ha scritto:
> 
> 
> You need to type RET to start animation.  When I do that in a build
> with libgif, it starts animation for me.

Sometimes it starts also in the build with GDI and without PNG etc.. but at steps, it is not fluid and stops then restarts slowly etc.

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 17:31:40 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> > Are the values of delay returned by w32_frame_delay similar to what
> > you get with libgif animation?
> 
> Sorry, I don't understand. With the native animation w32_frame_delay returns 6.
> 
> 253                   status = GdipImageSelectActiveFrame (pBitmap, &pDimensionIDs[0],
> (gdb) n
> 255                   *delay = w32_frame_delay (pBitmap, frame);
> (gdb) n
> 256                   *nframes = frameCount;
> (gdb) p *delay
> $1 = 6

6 is too much, I think.  Are you saying that inside w32_frame_delay,
before division by 100, the value is 600?

> How do I get the equivalent value when using libgif?

Put a breakpoint inside gif_load, here:

      img->lisp_data = list2 (Qextension_data, img->lisp_data);
      if (delay)         <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
	img->lisp_data
	  = Fcons (Qdelay,
		   Fcons (make_float (delay / 100.0),
			  img->lisp_data));

and look at the value.

Re: GDI+ take 3

[Eli Zaretskii]

> Date: Wed, 15 Apr 2020 18:46:32 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: juanjose.garciaripoll@gmail.com, emacs-devel@gnu.org
> 
> > How do I get the equivalent value when using libgif?
> 
> Put a breakpoint inside gif_load, here:
> 
>       img->lisp_data = list2 (Qextension_data, img->lisp_data);
>       if (delay)         <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<
> 	img->lisp_data
> 	  = Fcons (Qdelay,
> 		   Fcons (make_float (delay / 100.0),
> 			  img->lisp_data));
> 
> and look at the value.

Alternatively, this should be easier:

 (image-metadata
  (find-image
   '((:type gif :file "/path/to/CavernousFeistyArachnid-size_restricted.gif"))))

Re: GDI+ take 3

[Eli Zaretskii]

> Date: Wed, 15 Apr 2020 18:56:18 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: juanjose.garciaripoll@gmail.com, emacs-devel@gnu.org
> 
>  (image-metadata
>   (find-image
>    '((:type gif :file "/path/to/CavernousFeistyArachnid-size_restricted.gif"))))

Btw, if you want to do this again, after changing the value of
w32-use-native-image-API, you need to do the following, to force Emacs
to forget the image it already loaded:

  M-: (clear-image-cache t) RET

Re: GDI+ take 3

[Juanma Barranquero]

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

On Wed, Apr 15, 2020 at 5:46 PM Eli Zaretskii <eliz@gnu.org> wrote:

> 6 is too much, I think.  Are you saying that inside w32_frame_delay,
> before division by 100, the value is 600?

Yep.


Thread 1 hit Breakpoint 2, w32_frame_delay (pBitmap=pBitmap@entry=0x5af22f0,
    frame=frame@entry=0) at w32image.c:205
205     {
(gdb) n
212       GdipGetPropertyItemSize (pBitmap, PropertyTagFrameDelay, &size);
(gdb) n
215       propertyItem = malloc (size);
(gdb) n
216       if (propertyItem != NULL)
(gdb) n
[New Thread 18512.0x55f4]
219           GdipGetPropertyItem (pBitmap, PropertyTagFrameDelay, size,
propertyItem);
(gdb) n
220           delay = propertyItem[frame].length / 100.0;
(gdb) p frame
$1 = 0
(gdb) p propertyItem[0].length
$2 = 600
(gdb) n
221           if (delay == 0)
(gdb) p delay
$3 = 6
(gdb)


After setting w32-use-native-image-API to nil and clearing the cache,

 (image-metadata
  (find-image
   '((:type gif :file "/path/to/CavernousFeistyArach
nid-size_restricted.gif"))))

=>

(count 150 delay 0.1 extension-data
       (249 "^E^@Y" 0 "^A^@^@" 255 "NETSCAPE2.0"))

[-- Attachment #2: Type: text/html, Size: 1780 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 17:31:40 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> > Are the values of delay returned by w32_frame_delay similar to what
> > you get with libgif animation?
> 
> Sorry, I don't understand. With the native animation w32_frame_delay returns 6.
> 
> 253                   status = GdipImageSelectActiveFrame (pBitmap, &pDimensionIDs[0],
> (gdb) n
> 255                   *delay = w32_frame_delay (pBitmap, frame);
> (gdb) n
> 256                   *nframes = frameCount;
> (gdb) p *delay
> $1 = 6
> (gdb)

Btw, isn't the code wrong?  AFAIU by reading relevant discussions on
the Internet, we should be looking at

   *(long *)propertyItem[frame].value

instead of at propertyItem[frame].length.  The latter gives the length
of the value in bytes or something.  Hmm...

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 18:50:05 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> 220           delay = propertyItem[frame].length / 100.0;
> (gdb) p frame
> $1 = 0
> (gdb) p propertyItem[0].length
> $2 = 600

I think I understand what happens here.  600 = 150 * 4, so the .length
member tells us the size of the array in bytes.

Can you see what is the value of propertyItem[0].type?
I expect it to be 4 or maybe 7, which means the value is a 32-bit
integer.

And if it is indeed 4 or 7, what does this produce:

  (gdb) p ((int *)propertyItem[frame].value)[0]

> After setting w32-use-native-image-API to nil and clearing the cache, 
> 
>  (image-metadata
>   (find-image
>    '((:type gif :file "/path/to/CavernousFeistyArachnid-size_restricted.gif"))))
> 
> =>
> 
> (count 150 delay 0.1 extension-data
>        (249 "^E^@Y" 0 "^A^@^@" 255 "NETSCAPE2.0"))

Yeah, that's what I get as well.  So the delay is 0.1 sec.

Thanks.

Re: GDI+ take 3

[Juanma Barranquero]

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

On Wed, Apr 15, 2020 at 6:59 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Can you see what is the value of propertyItem[0].type?
> I expect it to be 4 or maybe 7, which means the value is a 32-bit
> integer.
>
> And if it is indeed 4 or 7, what does this produce:
>
>   (gdb) p ((int *)propertyItem[frame].value)[0]


Thread 1 hit Breakpoint 2, w32_frame_delay (pBitmap=pBitmap@entry=0x8de22f0,
    frame=frame@entry=0) at w32image.c:205
205     {
(gdb) n
[New Thread 12320.0x50a4]
212       GdipGetPropertyItemSize (pBitmap, PropertyTagFrameDelay, &size);
(gdb) n
[New Thread 12320.0x508c]
215       propertyItem = malloc (size);
(gdb) n
216       if (propertyItem != NULL)
(gdb) n
219           GdipGetPropertyItem (pBitmap, PropertyTagFrameDelay, size,
propertyItem);
(gdb) n
220           delay = propertyItem[frame].length / 100.0;
(gdb) p frame
$1 = 0
(gdb) p propertyItem[0].type
$2 = 4
(gdb) p ((int *)propertyItem[0].value)[0]
$3 = 10
(gdb)

[-- Attachment #2: Type: text/html, Size: 1235 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 19:24:38 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> (gdb) p frame
> $1 = 0
> (gdb) p propertyItem[0].type
> $2 = 4
> (gdb) p ((int *)propertyItem[0].value)[0]
> $3 = 10
> (gdb)

Great, thanks.  10 is indeed 0.1 sec, so I think I know how to fix the
code.  But I will need you to test it :-(

Re: GDI+ take 3

[Juanma Barranquero]

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

On Wed, Apr 15, 2020 at 7:34 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Great, thanks.  10 is indeed 0.1 sec, so I think I know how to fix the
> code.  But I will need you to test it :-(

Go ahead. There's nothing if not free time in this covid-19 inflicted times
;-)

[-- Attachment #2: Type: text/html, Size: 375 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 19:49:25 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> > Great, thanks.  10 is indeed 0.1 sec, so I think I know how to fix the
> > code.  But I will need you to test it :-(
> 
> Go ahead. There's nothing if not free time in this covid-19 inflicted times ;-)

Thanks.  The patch is below.  It's a bit overboard, but hey! COVID-19
justifies that, no?

diff --git a/src/w32image.c b/src/w32image.c
index 6a3e37c..20fc94f 100644
--- a/src/w32image.c
+++ b/src/w32image.c
@@ -200,6 +200,43 @@ w32_can_use_native_image_api (Lisp_Object type)
   return gdiplus_startup ();
 }
 
+enum PropertyItem_type {
+  PI_BYTE = 1,
+  PI_ASCIIZ = 2,
+  PI_USHORT = 3,
+  PI_ULONG = 4,
+  PI_ULONG_PAIR = 5,
+  PI_BYTE_ANY = 6,
+  PI_LONG = 7,
+  PI_LONG_PAIR = 10
+};
+
+static unsigned long
+decode_delay (PropertyItem *propertyItem, int frame)
+{
+  enum PropertyItem_type type = propertyItem[frame].type;
+  unsigned delay;
+
+  switch (type)
+    {
+    case PI_BYTE:
+    case PI_BYTE_ANY:
+      delay = *((unsigned char *)propertyItem[frame].value);
+      break;
+    case PI_USHORT:
+      delay = *((unsigned short *)propertyItem[frame].value);
+      break;
+    case PI_ULONG:
+    case PI_LONG:	/* delay shuold always be positive */
+      delay = *((unsigned long *)propertyItem[frame].value);
+      break;
+    default:
+      emacs_abort ();
+    }
+
+  return delay;
+}
+
 static double
 w32_frame_delay (GpBitmap *pBitmap, int frame)
 {
@@ -217,13 +254,14 @@ w32_frame_delay (GpBitmap *pBitmap, int frame)
     {
       /* Get the property item.  */
       GdipGetPropertyItem (pBitmap, PropertyTagFrameDelay, size, propertyItem);
-      delay = propertyItem[frame].length / 100.0;
-      if (delay == 0)
+      delay = decode_delay (propertyItem, frame);
+      if (delay <= 0)
         {
           /* In GIF files, unfortunately, delay is only specified for the first
              frame.  */
-          delay = propertyItem[0].length / 100.0;
+          delay = decode_delay (propertyItem, 0);
         }
+      delay /= 100.0;
       free (propertyItem);
     }
   return delay;

Re: GDI+ take 3

[Juanma Barranquero]

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

Alas, it crashes.


Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
0x00007fff6f4b0bb3 in KERNELBASE!DebugBreak ()
   from C:\WINDOWS\System32\KernelBase.dll
(gdb) bt
#0  0x00007fff6f4b0bb3 in KERNELBASE!DebugBreak ()
   from C:\WINDOWS\System32\KernelBase.dll
#1  0x000000040026c5cf in emacs_abort () at w32fns.c:10980
#2  0x00000004002b0c0e in decode_delay (
    propertyItem=propertyItem@entry=0x561a690, frame=frame@entry=1)
    at w32image.c:234
#3  0x00000004002b0c65 in w32_frame_delay (pBitmap=pBitmap@entry=0x59822f0,
    frame=frame@entry=1) at w32image.c:257
#4  0x00000004002b0d58 in w32_select_active_frame (pBitmap=0x59822f0,
    frame=1, nframes=nframes@entry=0xbfde7c, delay=delay@entry=0xbfde80)
    at w32image.c:293
#5  0x00000004002b14ab in w32_load_image (f=f@entry=0x55f39e0,
    img=img@entry=0x3e50fa0, spec_file=<optimized out>,
    spec_data=spec_data@entry=XIL(0)) at w32image.c:368
#6  0x00000004002c8e67 in native_image_load (f=0x55f39e0, img=0x3e50fa0)
    at lisp.h:1042
#7  0x00000004002cbbe4 in lookup_image (f=f@entry=0x55f39e0,
    spec=spec@entry=XIL(0x560f503)) at image.c:2302
#8  0x00000004002cc3a5 in Fimage_metadata (spec=XIL(0x560f503),
    frame=<optimized out>) at image.c:1121
#9  0x00000004001c74f7 in funcall_subr (subr=0x4006ef8c0 <Simage_metadata>,
    numargs=numargs@entry=1, args=args@entry=0xbfe2d8) at eval.c:2869
#10 0x00000004001c5355 in Ffuncall (nargs=2, args=args@entry=0xbfe2d0)
    at lisp.h:2113
#11 0x0000000400215b49 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(257), nargs=nargs@entry=1,
    args=<optimized out>, args@entry=0xbfe530) at bytecode.c:633
#12 0x00000004001c8e86 in funcall_lambda (fun=XIL(0x41bf775),
    nargs=nargs@entry=1, arg_vector=arg_vector@entry=0xbfe530) at
lisp.h:1862
#13 0x00000004001c546f in Ffuncall (nargs=2, args=args@entry=0xbfe528)
    at eval.c:2796
#14 0x0000000400215b49 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(1542), nargs=nargs@entry
=6,
    args=<optimized out>, args@entry=0xbfe828) at bytecode.c:633
#15 0x00000004001c8e86 in funcall_lambda (fun=XIL(0x41bf605),
    nargs=nargs@entry=6, arg_vector=arg_vector@entry=0xbfe828) at
lisp.h:1862
#16 0x00000004001c546f in Ffuncall (nargs=nargs@entry=7,
    args=args@entry=0xbfe820) at eval.c:2796
#17 0x00000004001c5aa2 in Fapply (nargs=<optimized out>, args=0xbfea08)
    at eval.c:2424
#18 0x00000004001c74a2 in funcall_subr (subr=0x4006e8040 <Sapply>,
    numargs=numargs@entry=2, args=args@entry=0xbfea08) at eval.c:2847
#19 0x00000004001c5355 in Ffuncall (nargs=3, args=args@entry=0xbfea00)
    at lisp.h:2113
#20 0x0000000400215b49 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(257), nargs=nargs@entry=1,
    args=<optimized out>, args@entry=0xbfeca8) at bytecode.c:633
#21 0x00000004001c8e86 in funcall_lambda (fun=XIL(0x452905d),
    nargs=nargs@entry=1, arg_vector=arg_vector@entry=0xbfeca8) at
lisp.h:1862
#22 0x00000004001c546f in Ffuncall (nargs=nargs@entry=2,
    args=args@entry=0xbfeca0) at eval.c:2796
#23 0x00000004001c5622 in call1 (fn=<optimized out>,
    arg1=arg1@entry=XIL(0x81248c5)) at eval.c:2654
#24 0x0000000400117127 in timer_check_2 (timers=<optimized out>,
    timers@entry=XIL(0x55b5e03), idle_timers=<optimized out>,
    idle_timers@entry=XIL(0x55b5dd3)) at lisp.h:1042
#25 0x0000000400119cd7 in timer_check () at keyboard.c:4402
#26 0x0000000400119d33 in readable_events (flags=1) at keyboard.c:3401
#27 0x000000040011f80d in get_input_pending (flags=flags@entry=1)
    at keyboard.c:6810
#28 0x000000040011f964 in detect_input_pending_run_timers (
    do_display=do_display@entry=true) at keyboard.c:10361
#29 0x0000000400225c6d in wait_reading_process_output (
    time_limit=<optimized out>, nsecs=<optimized out>, read_kbd=-1,
    do_display=do_display@entry=true, wait_for_cell=<optimized out>,
    wait_for_cell@entry=XIL(0), wait_proc=wait_proc@entry=0x0,
    just_wait_proc=just_wait_proc@entry=0) at process.c:5701
#30 0x000000040000eeb3 in sit_for (timeout=make_fixnum(150),
    reading=reading@entry=true, display_option=display_option@entry=1)
    at lisp.h:1042
#31 0x00000004001216f7 in read_char (commandflag=135378451, map=XIL(0),
    map@entry=XIL(0x811b613), prev_event=XIL(0xfffffffc00000000),
    prev_event@entry=XIL(0), used_mouse_menu=0xbff5b0,
    used_mouse_menu@entry=0xbff4ab, end_time=end_time@entry=0x0)
    at keyboard.c:2738
#32 0x00000004001228fd in read_key_sequence (keybuf=keybuf@entry=0xbff5b0,
    prompt=XIL(0), dont_downcase_last=dont_downcase_last@entry=false,
    can_return_switch_frame=can_return_switch_frame@entry=true,
    fix_current_buffer=fix_current_buffer@entry=true,
    prevent_redisplay=prevent_redisplay@entry=false) at keyboard.c:9547
#33 0x00000004001243b6 in command_loop_1 () at lisp.h:1042
#34 0x00000004001c4446 in internal_condition_case (
    bfun=bfun@entry=0x400123fbf <command_loop_1>,
    handlers=handlers@entry=XIL(0x90),
    hfun=hfun@entry=0x400118042 <cmd_error>) at eval.c:1355
#35 0x000000040010f3c0 in command_loop_2 (ignore=<optimized out>)
    at lisp.h:1042
#36 0x00000004001c437e in internal_catch (tag=<optimized out>,
    func=func@entry=0x40010f3a4 <command_loop_2>, arg=arg@entry=XIL(0))
    at eval.c:1116
#37 0x000000040011095b in command_loop () at lisp.h:1042
#38 0x0000000000000000 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Lisp Backtrace:
"image-metadata" (0xbfe2d8)
"image-multi-frame-p" (0xbfe530)
"image-animate-timeout" (0xbfe828)
"apply" (0xbfea08)
"timer-event-handler" (0xbfeca8)
(gdb) frame 3
#3  0x00000004002b0c65 in w32_frame_delay (pBitmap=pBitmap@entry=0x59822f0,
    frame=frame@entry=1) at w32image.c:257
257           delay = decode_delay (propertyItem, frame);
(gdb) p frame
$1 = 1
(gdb) p propertyItem
$2 = (PropertyItem *) 0x561a690
(gdb) p *propertyItem
$3 = {
  id = 20736,
  length = 600,
  type = 4,
  value = 0x561a6a8
}
(gdb)

[-- Attachment #2: Type: text/html, Size: 6975 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 20:45:30 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> Alas, it crashes.
> 
> Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
> 0x00007fff6f4b0bb3 in KERNELBASE!DebugBreak ()
>    from C:\WINDOWS\System32\KernelBase.dll
> (gdb) bt
> #0  0x00007fff6f4b0bb3 in KERNELBASE!DebugBreak ()
>    from C:\WINDOWS\System32\KernelBase.dll
> #1  0x000000040026c5cf in emacs_abort () at w32fns.c:10980
> #2  0x00000004002b0c0e in decode_delay (
>     propertyItem=propertyItem@entry=0x561a690, frame=frame@entry=1)
>     at w32image.c:234

With Juanma's generous help off-list, I've succeeded to find a way of
preventing the crashes.  The changes are now installed on master.  GIF
animation still doesn't work 100% correctly, you need to hit RET twice
to get the full animation (according to Juanma).  Further debugging is
required to clean this up.

Re: GDI+ take 3

[Juanma Barranquero]

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

On Wed, Apr 15, 2020 at 10:21 PM Eli Zaretskii <eliz@gnu.org> wrote:

> GIF
> animation still doesn't work 100% correctly, you need to hit RET twice
> to get the full animation (according to Juanma).

I just tested with a much smaller GIF image (20 KiB, vs 4 MiB of the other
test image), and the result is the same:

- Loads correctly
- Hitting RET animates the image for a short while, then jumps back to the
first image and stops.
- Hitting RET a second time animates the image correctly, and it stops at
the last frame.

[-- Attachment #2: Type: text/html, Size: 674 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Wed, 15 Apr 2020 22:31:40 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> I just tested with a much smaller GIF image (20 KiB, vs 4 MiB of the other test image), and the result is the
> same:
> 
> - Loads correctly
> - Hitting RET animates the image for a short while, then jumps back to the first image and stops.
> - Hitting RET a second time animates the image correctly, and it stops at the last frame.

I spent some time trying to make this code work reliably on my XP
system here, but finally gave up.  This mail is mainly a record of
what I tried and what I observed.

The GdipImageGetFrameCount call always returns status = Win32Error
here, for any image file I tried to load.  I have no idea why.  I
added code to work around that by calling GdipGetPropertyItem and then
using the propertyItem[0].length member, but then
GdipImageGetFrameCount began mysteriously to succeed (?? which already
smells of some bug somewhere), and w32_select_active_frame started
behaving erratically: various local variables change values during the
execution of the function, GDI+ calls sometimes return
status = InvalidParameter, etc.  Those are telltale signs of some code
smashing the stack, but I couldn't spot any source-level problems.  So
I ended up tossing that workaround code, as it doesn't seem to improve
the situation, maybe even make it worse.

In addition, multi-page TIFF files I tried (from
https://www.nightprogrammer.org/development/multipage-tiff-example-download-test-image-file/)
fail to load: GdipCreateBitmapFromFile returns status = InvalidParameter,
for no clear reason.  Single-page TIFF images do load successfully.

I guess I'm missing something here, or maybe I'm just too stupid for
this stuff, sigh...

Re: GDI+ take 3

[Eli Zaretskii]

> Date: Tue, 14 Apr 2020 23:19:45 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: juanjose.garciaripoll@gmail.com, emacs-devel@gnu.org
> 
> > One more thing: Alan, as you asked, I tried to keep the changes
> > extensible to NS native image display, I hope I succeeded.  Please
> > take a look; if something else needs to be done to make NS use the
> > framework, let's talk.
> 
> Looks good, I’ve implemented the NS equivalent. The only thing missing
> is the ability to force the native API off at run‐time, but I’m not
> sure it’s important.

Fine with me, thanks.

Re: GDI+ take 3

[Juanma Barranquero]

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

On Thu, Apr 16, 2020 at 12:04 PM Eli Zaretskii <eliz@gnu.org> wrote:

> In addition, multi-page TIFF files I tried (from
>
https://www.nightprogrammer.org/development/multipage-tiff-example-download-test-image-file/
)
> fail to load: GdipCreateBitmapFromFile returns status = InvalidParameter,
> for no clear reason.  Single-page TIFF images do load successfully.

These two images crash my emacs with w32-use-native-image-API = t. Single
page tifs work as expected.

Thread 1 received signal SIGTRAP, Trace/breakpoint trap.
0x00007fff8e530aa3 in KERNELBASE!DebugBreak ()
   from C:\WINDOWS\System32\KernelBase.dll
(gdb) bt
#0  0x00007fff8e530aa3 in KERNELBASE!DebugBreak ()
   from C:\WINDOWS\System32\KernelBase.dll
#1  0x000000040026c72f in emacs_abort () at w32fns.c:10979
#2  0x00000004002b0d72 in decode_delay (propertyItem=0xd40000,
    propertyItem@entry=0x585b820, frame=13893632, frame@entry=0)
    at w32image.c:237
#3  0x00000004002b0dc9 in w32_frame_delay (pBitmap=pBitmap@entry=0x5d622f0,
    frame=frame@entry=0) at w32image.c:260
#4  0x00000004002b0ebc in w32_select_active_frame (pBitmap=0x5d622f0,
    frame=0, nframes=nframes@entry=0xbfcdbc, delay=delay@entry=0xbfcdc0)
    at w32image.c:301
#5  0x00000004002b160d in w32_load_image (f=f@entry=0x58439e0,
    img=img@entry=0xcdece0, spec_file=<optimized out>,
    spec_data=spec_data@entry=XIL(0)) at w32image.c:370
#6  0x00000004002c8fc7 in native_image_load (f=0x58439e0, img=0xcdece0)
    at lisp.h:1042
#7  0x00000004002cbd44 in lookup_image (f=f@entry=0x58439e0,
    spec=spec@entry=XIL(0x897aab3)) at image.c:2303
#8  0x00000004002cc505 in Fimage_metadata (spec=XIL(0x897aab3),
    frame=<optimized out>) at image.c:1122
#9  0x00000004001c7617 in funcall_subr (subr=0x4006ef8c0 <Simage_metadata>,
    numargs=numargs@entry=1, args=args@entry=0xbfd218) at eval.c:2869
#10 0x00000004001c5475 in Ffuncall (nargs=2, args=args@entry=0xbfd210)
    at lisp.h:2113
#11 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(257), nargs=nargs@entry=1,
    args=<optimized out>, args@entry=0xbfd448) at bytecode.c:633
#12 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x430f775),
    nargs=nargs@entry=1, arg_vector=arg_vector@entry=0xbfd448) at
lisp.h:1862
#13 0x00000004001c558f in Ffuncall (nargs=2, args=args@entry=0xbfd440)
    at eval.c:2796
#14 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(0), nargs=nargs@entry=0,
    args=<optimized out>, args@entry=0xbfd6d0) at bytecode.c:633
#15 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x59a3805),
    nargs=nargs@entry=0, arg_vector=arg_vector@entry=0xbfd6d0) at
lisp.h:1862
#16 0x00000004001c558f in Ffuncall (nargs=1, args=args@entry=0xbfd6c8)
    at eval.c:2796
#17 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(0), nargs=nargs@entry=0,
    args=<optimized out>, args@entry=0xbfd930) at bytecode.c:633
#18 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x59a3665),
    nargs=nargs@entry=0, arg_vector=arg_vector@entry=0xbfd930) at
lisp.h:1862
#19 0x00000004001c558f in Ffuncall (nargs=1, args=args@entry=0xbfd928)
    at eval.c:2796
#20 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(513), nargs=nargs@entry=2,
    args=<optimized out>, args@entry=0xbfdb68) at bytecode.c:633
#21 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x40a0625),
    nargs=nargs@entry=2, arg_vector=arg_vector@entry=0xbfdb68) at
lisp.h:1862
#22 0x00000004001c558f in Ffuncall (nargs=3, args=args@entry=0xbfdb60)
    at eval.c:2796
#23 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(256), nargs=nargs@entry=0,
    args=<optimized out>, args@entry=0xbfdf48) at bytecode.c:633
#24 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x402cb4d),
    nargs=nargs@entry=0, arg_vector=arg_vector@entry=0xbfdf48) at
lisp.h:1862
#25 0x00000004001c558f in Ffuncall (nargs=1, args=args@entry=0xbfdf40)
    at eval.c:2796
#26 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(256), nargs=nargs@entry=1,
    args=<optimized out>, args@entry=0xbfe1a8) at bytecode.c:633
#27 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x40e5e95),
    nargs=nargs@entry=1, arg_vector=arg_vector@entry=0xbfe1a8) at
lisp.h:1862
#28 0x00000004001c558f in Ffuncall (nargs=2, args=args@entry=0xbfe1a0)
    at eval.c:2796
#29 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(1280), nargs=nargs@entry
=2,
    args=<optimized out>, args@entry=0xbfe4f8) at bytecode.c:633
#30 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x40e5bad),
    nargs=nargs@entry=2, arg_vector=arg_vector@entry=0xbfe4f8) at
lisp.h:1862
#31 0x00000004001c558f in Ffuncall (nargs=3, args=args@entry=0xbfe4f0)
    at eval.c:2796
#32 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(1542), nargs=nargs@entry
=6,
    args=<optimized out>, args@entry=0xbfe808) at bytecode.c:633
#33 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x418ab45),
    nargs=nargs@entry=6, arg_vector=arg_vector@entry=0xbfe808) at
lisp.h:1862
#34 0x00000004001c558f in Ffuncall (nargs=7, args=args@entry=0xbfe800)
    at eval.c:2796
#35 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(1025), nargs=nargs@entry
=4,
    args=<optimized out>, args@entry=0xbfecb0) at bytecode.c:633
#36 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x3fffa7d),
    nargs=nargs@entry=4, arg_vector=arg_vector@entry=0xbfecb0) at
lisp.h:1862
#37 0x00000004001c558f in Ffuncall (nargs=5, args=args@entry=0xbfeca8)
    at eval.c:2796
#38 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(513), nargs=nargs@entry=2,
    args=<optimized out>, args@entry=0xbfeff0) at bytecode.c:633
#39 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x40d4e4d),
    nargs=nargs@entry=2, arg_vector=arg_vector@entry=0xbfeff0) at
lisp.h:1862
#40 0x00000004001c558f in Ffuncall (nargs=nargs@entry=3,
    args=args@entry=0xbfefe8) at eval.c:2796
#41 0x00000004001bff13 in Ffuncall_interactively (nargs=3, args=0xbfefe8)
    at callint.c:254
#42 0x00000004001c75c2 in funcall_subr (
    subr=0x4006e7bc0 <Sfuncall_interactively>, numargs=numargs@entry=3,
    args=args@entry=0xbfefe8) at eval.c:2847
#43 0x00000004001c5475 in Ffuncall (nargs=nargs@entry=4,
    args=args@entry=0xbfefe0) at lisp.h:2113
#44 0x00000004001c5bc2 in Fapply (nargs=nargs@entry=3, args=0xbff120,
    args@entry=0xbff1c0) at eval.c:2424
#45 0x00000004001c09c6 in Fcall_interactively (function=XIL(0xbff2f0),
    record_flag=XIL(0x4001c5475), keys=XIL(0x40d4bc8)) at lisp.h:1042
#46 0x00000004001c7627 in funcall_subr (
    subr=0x4006e7b80 <Scall_interactively>, numargs=numargs@entry=3,
    args=args@entry=0xbff2f0) at eval.c:2872
#47 0x00000004001c5475 in Ffuncall (nargs=4, args=args@entry=0xbff2e8)
    at lisp.h:2113
#48 0x0000000400215ca9 in exec_byte_code (bytestr=<optimized out>,
    vector=<optimized out>, maxdepth=<optimized out>,
    args_template=args_template@entry=make_fixnum(1025), nargs=nargs@entry
=1,
    args=<optimized out>, args@entry=0xbff568) at bytecode.c:633
#49 0x00000004001c8fa6 in funcall_lambda (fun=XIL(0x40cf625),
    nargs=nargs@entry=1, arg_vector=arg_vector@entry=0xbff568) at
lisp.h:1862
#50 0x00000004001c558f in Ffuncall (nargs=nargs@entry=2,
    args=args@entry=0xbff560) at eval.c:2796
#51 0x00000004001c5742 in call1 (fn=<optimized out>,
    arg1=arg1@entry=XIL(0xfffffffc0390ba28)) at eval.c:2654
#52 0x00000004001246ec in command_loop_1 () at lisp.h:1042
#53 0x00000004001c4566 in internal_condition_case (
    bfun=bfun@entry=0x400123fbf <command_loop_1>,
    handlers=handlers@entry=XIL(0x90),
    hfun=hfun@entry=0x400118042 <cmd_error>) at eval.c:1355
#54 0x000000040010f3c0 in command_loop_2 (ignore=<optimized out>)
    at lisp.h:1042
#55 0x00000004001c449e in internal_catch (tag=<optimized out>,
    func=func@entry=0x40010f3a4 <command_loop_2>, arg=arg@entry=XIL(0))
    at eval.c:1116
#56 0x000000040011095b in command_loop () at lisp.h:1042
#57 0x0000000000000000 in ?? ()
Backtrace stopped: previous frame inner to this frame (corrupt stack?)

Lisp Backtrace:
"image-metadata" (0xbfd218)
"image-multi-frame-p" (0xbfd448)
"image-mode--setup-mode" (0xbfd6d0)
"image-mode" (0xbfd930)
"set-auto-mode-0" (0xbfdb68)
"set-auto-mode" (0xbfdf48)
"normal-mode" (0xbfe1a8)
"after-find-file" (0xbfe4f8)
"find-file-noselect-1" (0xbfe808)
"find-file-noselect" (0xbfecb0)
"find-file" (0xbfeff0)
"funcall-interactively" (0xbfefe8)
"call-interactively" (0xbff2f0)
"command-execute" (0xbff568)
(gdb) frame 3
#3  0x00000004002b0dc9 in w32_frame_delay (pBitmap=pBitmap@entry=0x5d622f0,
    frame=frame@entry=0) at w32image.c:260
260           delay = decode_delay (propertyItem, frame);
(gdb) p frame
$1 = 0
(gdb) p propertyItem[0]
$3 = {
  id = 2880154539,
  length = 2880154539,
  type = 43947,
  value = 0xfeeefeeefeeefeee
}
(gdb)

[-- Attachment #2: Type: text/html, Size: 11206 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 17 Apr 2020 01:49:46 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> #3  0x00000004002b0dc9 in w32_frame_delay (pBitmap=pBitmap@entry=0x5d622f0,
>     frame=frame@entry=0) at w32image.c:260
> 260           delay = decode_delay (propertyItem, frame);
> (gdb) p frame
> $1 = 0
> (gdb) p propertyItem[0]
> $3 = {
>   id = 2880154539,
>   length = 2880154539,
>   type = 43947,
>   value = 0xfeeefeeefeeefeee
> }

So I guess instead of crashing, we should in such cases log an error
message and pretend the image has only one frame/page?

Thanks.

Re: GDI+ take 3

[Juan José García-Ripoll]

Eli Zaretskii <eliz@gnu.org> writes:
> So I guess instead of crashing, we should in such cases log an error
> message and pretend the image has only one frame/page?

Can you please send me the offending TIFF file? I tested the code with
multipage TIFF's and multipage GIF's and it worked well. It did not get
the delays right, but was never crashing on a Windows 10 computer.

-- 
Juan José García Ripoll

Quantum Information and Foundations Group
Institute of Fundamental Physics IFF-CSIC
Calle Serrano 113b, Madrid 28006 Spain
http://quinfog.hbar.es - http://juanjose.garcia.ripoll

Re: GDI+ take 3

[Juanma Barranquero]

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

On Fri, Apr 17, 2020 at 9:27 AM Juan José García-Ripoll <
juanjose.garciaripoll@gmail.com> wrote:

> Can you please send me the offending TIFF file?

Try the two examples linked from
https://www.nightprogrammer.org/development/multipage-tiff-example-download-test-image-file/

These are the ones that crash my Emacs.

[-- Attachment #2: Type: text/html, Size: 538 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Fri, 17 Apr 2020 09:27:54 +0200
> Cc: Juanma Barranquero <lekktu@gmail.com>, juanjose.garciaripoll@gmail.com,
>  emacs-devel@gnu.org
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> > So I guess instead of crashing, we should in such cases log an error
> > message and pretend the image has only one frame/page?
> 
> Can you please send me the offending TIFF file? I tested the code with
> multipage TIFF's and multipage GIF's and it worked well. It did not get
> the delays right, but was never crashing on a Windows 10 computer.

It could be that my changes caused this.  Apologies if that's the
reason.

Re: GDI+ take 3

[Juan José García-Ripoll]

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

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Juan José García-Ripoll <juanjose.garciaripoll@gmail.com>:
>> Can you please send me the offending TIFF file? I tested the code with
>> multipage TIFF's and multipage GIF's and it worked well. It did not get
>> the delays right, but was never crashing on a Windows 10 computer.
>
> It could be that my changes caused this.  Apologies if that's the
> reason.

No, it is not your fault. You actually almost fixed my delay decoding
routine. The problem is that the TIFF file has no delay tag. This is
seen if you deactivate the native API and use libtiff. I have fixed this
using the attached patch. I use the following code to test it.

(setq w32-use-native-image-API t)
(defvar test-image nil)
(setq debug-on-error t)
(setq test-image (create-image "image-test.tiff"))
(message "Delay: %S" (image-multi-frame-p test-image))
(insert-image test-image)
(image-animate test-image)

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: w32image.diff --]
[-- Type: text/x-patch, Size: 1101 bytes --]

diff --git a/src/w32image.c b/src/w32image.c
index 0a2a55d..24970ae 100644
--- a/src/w32image.c
+++ b/src/w32image.c
@@ -246,10 +246,15 @@ w32_frame_delay (GpBitmap *pBitmap, int frame)
   UINT size;
   PropertyItem *propertyItem;
   double delay = 0.0;
+  GpStatus status = Ok;
 
   /* Assume that the image has a property item of type PropertyItemEquipMake.
      Get the size of that property item.  */
-  GdipGetPropertyItemSize (pBitmap, PropertyTagFrameDelay, &size);
+  status = GdipGetPropertyItemSize (pBitmap, PropertyTagFrameDelay, &size);
+  if (status != Ok) {
+    /* Property not found.  */
+    return -1;
+  }
 
   /* Allocate a buffer to receive the property item.  */
   propertyItem = malloc (size);
@@ -372,7 +377,7 @@ w32_load_image (struct frame *f, struct image *img,
         {
           if (nframes > 1)
             metadata = Fcons (Qcount, Fcons (make_fixnum (nframes), metadata));
-          if (delay)
+          if (delay >= 0)
             metadata = Fcons (Qdelay, Fcons (make_float (delay), metadata));
         }
       else if (status == Win32Error) /* FIXME! */

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Fri, 17 Apr 2020 01:49:46 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> On Thu, Apr 16, 2020 at 12:04 PM Eli Zaretskii <eliz@gnu.org> wrote:
> 
> > In addition, multi-page TIFF files I tried (from
> > https://www.nightprogrammer.org/development/multipage-tiff-example-download-test-image-file/)
> > fail to load: GdipCreateBitmapFromFile returns status = InvalidParameter,
> > for no clear reason.  Single-page TIFF images do load successfully.
> 
> These two images crash my emacs with w32-use-native-image-API = t. Single page tifs work as expected.

Thanks, I hope with Juan's latest change this is prevented.  In any
case, I replaced the emacs_abort call with returning a negative value,
which should avoid using the invalid data and add a log message.

Re: GDI+ take 3

[Juanma Barranquero]

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

Tiffs no longer crash for me and are correctly animated.

GIFs still fail to animate correctly the first time. Juan José, are you
planning to look into it?

[-- Attachment #2: Type: text/html, Size: 203 bytes --]

Re: GDI+ take 3

[Juan José García-Ripoll]

Juanma Barranquero <lekktu@gmail.com> writes:
> Tiffs no longer crash for me and are correctly animated
> GIFs still fail to animate correctly the first time. Juan José, are you
> planning to look into it?

With my last mini-patch, I tried exactly the same lisp code with this image
https://upload.wikimedia.org/wikipedia/commons/2/2c/Rotating_earth_%28large%29.gif
and it animates just fine, exactly as emacs 26.3 with libgif.

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Sat, 18 Apr 2020 14:38:58 +0200
> 
> Juanma Barranquero <lekktu@gmail.com> writes:
> > Tiffs no longer crash for me and are correctly animated
> > GIFs still fail to animate correctly the first time. Juan José, are you
> > planning to look into it?
> 
> With my last mini-patch, I tried exactly the same lisp code with this image
> https://upload.wikimedia.org/wikipedia/commons/2/2c/Rotating_earth_%28large%29.gif
> and it animates just fine, exactly as emacs 26.3 with libgif.

I think I've just seen what Juanma describes -- but with libgif.  I
saw it only once, and couldn't reproduce afterwards.  But I think this
means that the problem is not with the w32image.c code, it's somewhere
else.

Juanma, if you still can reproduce the problem at will, please step
through the code which performs the animation, and tell what you see
there that causes the animation to stop prematurely.  Some of the
animation happens in Lisp, so it 's a bit tricky to "step through it",
but if you can reliably reproduce, just discovering what stops the
animation would be an important step forward.

TIA

Re: GDI+ take 3

[Juanma Barranquero]

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

On Sat, Apr 18, 2020 at 3:39 PM Eli Zaretskii <eliz@gnu.org> wrote:

> I think I've just seen what Juanma describes -- but with libgif.  I
> saw it only once, and couldn't reproduce afterwards.  But I think this
> means that the problem is not with the w32image.c code, it's somewhere
> else.

I haven't yet seen it with libgif, only with the native API. But, on the
other hand, that's not entirely unexpected because I've tried the image
many more times with the native API.

I thought it always failed, but testing it now I realized it fails only
some times. Still, at least in my case, it fails more often than not. But
I've been surprised a few times by the animation working right after the
first <RET>.

That happening / not happening suggest we're accessing some uninitialized
memory or some such, I suspect.

> Juanma, if you still can reproduce the problem at will, please step
> through the code which performs the animation, and tell what you see
> there that causes the animation to stop prematurely.  Some of the
> animation happens in Lisp, so it 's a bit tricky to "step through it",
> but if you can reliably reproduce, just discovering what stops the
> animation would be an important step forward.

I'll do.

[-- Attachment #2: Type: text/html, Size: 1972 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Sat, 18 Apr 2020 17:56:41 +0200
> Cc: Juan José García-Ripoll <juanjose.garciaripoll@gmail.com>, 
> 	Emacs developers <emacs-devel@gnu.org>
> 
> That happening / not happening suggest we're accessing some uninitialized
> memory or some such, I suspect.

That'd be my guess, yes.

> > Juanma, if you still can reproduce the problem at will, please step
> > through the code which performs the animation, and tell what you see
> > there that causes the animation to stop prematurely.  Some of the
> > animation happens in Lisp, so it 's a bit tricky to "step through it",
> > but if you can reliably reproduce, just discovering what stops the
> > animation would be an important step forward.
> 
> I'll do.

Thanks.

Re: GDI+ take 3

[Juan José García-Ripoll]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Juanma Barranquero <lekktu@gmail.com>
>> Date: Sat, 18 Apr 2020 17:56:41 +0200
>> Cc: Juan José García-Ripoll <juanjose.garciaripoll@gmail.com>, 
>> 	Emacs developers <emacs-devel@gnu.org>
>> 
>> That happening / not happening suggest we're accessing some uninitialized
>> memory or some such, I suspect.
>
> That'd be my guess, yes.

Don't think so. If there was corruption, you would see a crash, as it
already happened with TIFF; from what I gather, it simply stops
animating but Emacs keeps working. Couldn't it be just a problem of
latency?

Emacs is doing something rather ugly with animated images: it reloads
them every time a new frame is requested. This means decoding the entire
image; extracting the frame; displaying it; dropping the image. Indeed,
the continuous reloading of images and associated slowdown is one of the
reasons I stopped using equation previews in LaTeX.

My laptop is old, but I have an SSD and never see any problem. When
Emacs runs on a hard disk or the computer is more busy, maybe the timer
cannot catch up with the speed of the animation past some frames and the
image is not updated. Or maybe the timer gets triggered while
decoding.

Probably one does not need to "step" through all the code, but just make
sure w32_load_image gets called, logging those calls and the time they
happen. If they are equally spaced and all succeed, maybe you are right,
but I would guess there is a bottleneck somewhere.

Cheers

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Sat, 18 Apr 2020 19:51:01 +0200
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> That happening / not happening suggest we're accessing some uninitialized
> >> memory or some such, I suspect.
> >
> > That'd be my guess, yes.
> 
> Don't think so. If there was corruption, you would see a crash, as it
> already happened with TIFF; from what I gather, it simply stops
> animating but Emacs keeps working. Couldn't it be just a problem of
> latency?

Yes, that could also be the reason.

> Emacs is doing something rather ugly with animated images: it reloads
> them every time a new frame is requested. This means decoding the entire
> image; extracting the frame; displaying it; dropping the image.

The image shouldn't be dropped, it should be cached, AFAIU.  Although
maybe we cache each frame separately, in which case the caching will
not help, indeed, at least not the first time we play the animation.

Re: GDI+ take 3

[Eli Zaretskii]

> Date: Sat, 18 Apr 2020 21:01:11 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: emacs-devel@gnu.org
> 
> > Don't think so. If there was corruption, you would see a crash, as it
> > already happened with TIFF; from what I gather, it simply stops
> > animating but Emacs keeps working. Couldn't it be just a problem of
> > latency?
> 
> Yes, that could also be the reason.

Btw, one reason for latency could be GC.  Juanma, if you turn on
garbage-collection-messages, do you see that GC runs in the middle of
an animation, when we stop the animation too early?

Re: GDI+ take 3

[Juanma Barranquero]

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

On Sat, Apr 18, 2020 at 8:04 PM Eli Zaretskii <eliz@gnu.org> wrote:

> Btw, one reason for latency could be GC.  Juanma, if you turn on
> garbage-collection-messages, do you see that GC runs in the middle of
> an animation, when we stop the animation too early?

There seems to be no clear correlation.

I've tested a few times, and in the cases where the animation stops early,
the gc message is shown after the animation stops. In the occasions where
the animation runs correctly up to the end, there are gc messages while
the animation is running, but they don't seem to affect it.

[-- Attachment #2: Type: text/html, Size: 1033 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juanma Barranquero <lekktu@gmail.com>
> Date: Sat, 18 Apr 2020 20:49:52 +0200
> Cc: juanjose.garciaripoll@gmail.com, Emacs developers <emacs-devel@gnu.org>
> 
> > Btw, one reason for latency could be GC.  Juanma, if you turn on
> > garbage-collection-messages, do you see that GC runs in the middle of
> > an animation, when we stop the animation too early?
> 
> There seems to be no clear correlation.
> 
> I've tested a few times, and in the cases where the animation stops early,
> the gc message is shown after the animation stops. In the occasions where
> the animation runs correctly up to the end, there are gc messages while
> the animation is running, but they don't seem to affect it.

Thanks.  I guess one theory eats dust.

Re: GDI+ take 3

[Alan Third]

On Sat, Apr 18, 2020 at 04:38:32PM +0300, Eli Zaretskii wrote:
> > From: Juan José García-Ripoll
> >  <juanjose.garciaripoll@gmail.com>
> > Date: Sat, 18 Apr 2020 14:38:58 +0200
> > 
> > Juanma Barranquero <lekktu@gmail.com> writes:
> > > Tiffs no longer crash for me and are correctly animated
> > > GIFs still fail to animate correctly the first time. Juan José, are you
> > > planning to look into it?
> > 
> > With my last mini-patch, I tried exactly the same lisp code with this image
> > https://upload.wikimedia.org/wikipedia/commons/2/2c/Rotating_earth_%28large%29.gif
> > and it animates just fine, exactly as emacs 26.3 with libgif.
> 
> I think I've just seen what Juanma describes -- but with libgif.  I
> saw it only once, and couldn't reproduce afterwards.  But I think this
> means that the problem is not with the w32image.c code, it's somewhere
> else.

FWIW, with a big enough gif[1] I can see the same behaviour using
giflib on an NS build. The native macOS image backend seems to be able
to handle the same gif, but it’s still displayed at a far slower frame
rate than my web browser can handle.

[1] https://drive.google.com/file/d/0B35XTfGL8WmuN2xXcUk4Z3MtMDQ/view?usp=sharing
-- 
Alan Third

Re: GDI+ take 3

[Juan José García-Ripoll]

Alan Third <alan@idiocy.org> writes:
> FWIW, with a big enough gif[1] I can see the same behaviour using
> giflib on an NS build. The native macOS image backend seems to be able
> to handle the same gif, but it’s still displayed at a far slower frame
> rate than my web browser can handle.
>
> [1] https://drive.google.com/file/d/0B35XTfGL8WmuN2xXcUk4Z3MtMDQ/view?usp=sharing

I think we may be onto something, but first let me distinguish four ways
to deal with this animated gif.

Method A (image-mode + giflib):
1. Open with emacs -Q image-test-large.gif
2. Press ENTER

Method B (pure elisp + giflib):
1. Write down this lisp code into image-test.el
    (defvar test-image (create-image "image-test-large.gif"))
    (insert-image test-image)
    (image-animate test-image)
2. Execute with emacs -Q image-test-large.gif

Method A' and B' (same but gdi+):
- Same, but add --eval '(setq w32-use-native-image-API t)' when invoking
  emacs

* With emacs 26.3, A and B seem to work, but it plays the gif file slower than
  a browser.
* With emacs-28, methods A and B (use libgif) do not work. It slows down, skip
  frames because the timer is triggered out of time.
* With emacs-28, methods A' and B' (active gdi+) both work at the same pace
  as emacs-26.3.

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

Re: GDI+ take 3

[Eli Zaretskii]

I finally figured out why w32image.c worked erratically for me.  We
cannot use DEF_DLL_FN and LOAD_DLL_FN for GDI+ functions, because GDI+
functions use STDCALL calling conventions, and the above 2 macros are
defined for functions using CDECL calling conventions.  I guess on
64-bit Windows this is less important, but on 32-bit Windows it's very
important.

I pushed the changes to load GDI+ functions correctly.  Now everything
works for me like Juanma reported, including multi-page GIF and TIFF
images.  Please test again that I didn't break anything in the 64-bit
builds.

Thanks.

Re: GDI+ take 3

[Juan José García-Ripoll]

Folloup on the previous studies. Note that I am using emacs on a 64-bit
machine, not 32 bit. So I am unable to comment on Juanma's statistics or
Eli's more recent build. However, I hooked into IMAGE-ANIMATE-TIMEOUT
and it confirms what I suspected: the load times surpass the scheduled
time of the timer.

The image that was supplied by Alan has a nominal delay time of 0.03
seconds between frames. Emacs 26.3 takes about 0.14 seconds on average
to load each frame of the gif file, using giflib. Emacs 28 as I built it
from git source right now, takes 0.3 seconds with giflib and a time that
grows from 0.01 up to 0.16 seconds (probably because frames have to be
read sequentially, there is no index).

Emacs 26.3
Time: (24220 44200 793183 0), time-to-load-image: (0 0 159487 0), stated-delay-time: 0.03, speed: 1
Time: (24220 44201 27584 0), time-to-load-image: (0 0 167416 0), stated-delay-time: 0.03, speed: 1
Time: (24220 44201 207428 0), time-to-load-image: (0 0 166559 0), stated-delay-time: 0.03, speed: 1
...
Time: (24220 44234 713581 0), time-to-load-image: (0 0 183838 0), stated-delay-time: 0.03, speed: 1
Time: (24220 44234 908589 0), time-to-load-image: (0 0 186141 0), stated-delay-time: 0.03, speed: 1
Time: (24220 44235 107525 0), time-to-load-image: (0 0 187332 0), stated-delay-time: 0.03, speed: 1

Emacs 28 (giflib)
Time: (24220 43970 179318 0), time-to-load-image: (0 0 365329 0), stated-delay-time: 0.03, speed: 1
Time: (24220 43970 572005 0), time-to-load-image: (0 0 356802 0), stated-delay-time: 0.03, speed: 1
Time: (24220 43970 939828 0), time-to-load-image: (0 0 367949 0), stated-delay-time: 0.03, speed: 1
...
Time: (24220 44038 598226 0), time-to-load-image: (0 0 366024 0), stated-delay-time: 0.03, speed: 1
Time: (24220 44038 980329 0), time-to-load-image: (0 0 365757 0), stated-delay-time: 0.03, speed: 1
Time: (24220 44039 356957 0), time-to-load-image: (0 0 368651 0), stated-delay-time: 0.03, speed: 1


Emacs 28 (gdi+)
Time: (24220 44147 897292 0), time-to-load-image: (0 0 13333 0), stated-delay-time: 0.03, speed: 1
Time: (24220 44147 939953 0), time-to-load-image: (0 0 17375 0), stated-delay-time: 0.03, speed: 1
Time: (24220 44147 971464 0), time-to-load-image: (0 0 15131 0), stated-delay-time: 0.03, speed: 1
...
Time: (24220 44164 698744 0), time-to-load-image: (0 0 137093 0), stated-delay-time: 0.03, speed: 1
Time: (24220 44164 846623 0), time-to-load-image: (0 0 137473 0), stated-delay-time: 0.03, speed: 1
Time: (24220 44164 994965 0), time-to-load-image: (0 0 139321 0), stated-delay-time: 0.03, speed: 1


-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

Re: GDI+ take 3

[Juan José García-Ripoll]

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

Eli Zaretskii <eliz@gnu.org> writes:
> I pushed the changes to load GDI+ functions correctly.  Now everything
> works for me like Juanma reported, including multi-page GIF and TIFF
> images.  Please test again that I didn't break anything in the 64-bit
> builds.

Two minor changes attached. First, the introduction of the Qnative_image
type makes one call to image_can_use_native_api irrelevant.

Regarding this, I think it is a bad idea to introduce Qnative_image,
because that is not an image format and users cannot recognize the
actual image type from the image descriptor.

The second fix makes w32image.c behave like nsimage.c, in that a delay=0
is regarded as non-existant, thus returning nil. This makes animations
default to the minimum animation delay, which currently is 0.01, and
more useful than a delay of 0.

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 1097 bytes --]

diff --git a/src/image.c b/src/image.c
index ffe2f607e5..c5835a0623 100644
--- a/src/image.c
+++ b/src/image.c
@@ -10182,11 +10182,6 @@ initialize_image_type (struct image_type const *type)
 #ifdef WINDOWSNT
   Lisp_Object typesym = builtin_lisp_symbol (type->type);

-# if HAVE_NATIVE_IMAGE_API
-  if (image_can_use_native_api (typesym))
-    return true;
-# endif
-
   Lisp_Object tested = Fassq (typesym, Vlibrary_cache);
   /* If we failed to load the library before, don't try again.  */
   if (CONSP (tested))
diff --git a/src/w32image.c b/src/w32image.c
index 085a5db3ab..11d96a123e 100644
--- a/src/w32image.c
+++ b/src/w32image.c
@@ -444,7 +444,8 @@ w32_load_image (struct frame *f, struct image *img,
         {
           if (nframes > 1)
             metadata = Fcons (Qcount, Fcons (make_fixnum (nframes), metadata));
-          if (delay >= 0)
+          /* We follow nsimage.c: if the delay is zero, return nil.  */
+          if (delay > 0)
             metadata = Fcons (Qdelay, Fcons (make_float (delay), metadata));
         }
       else if (status == Win32Error) /* FIXME! */

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Sun, 19 Apr 2020 22:08:06 +0200
> 
> The image that was supplied by Alan has a nominal delay time of 0.03
> seconds between frames. Emacs 26.3 takes about 0.14 seconds on average
> to load each frame of the gif file, using giflib. Emacs 28 as I built it
> from git source right now, takes 0.3 seconds with giflib and a time that
> grows from 0.01 up to 0.16 seconds (probably because frames have to be
> read sequentially, there is no index).

Can you show the Lisp you used to time this?  I'd like to see what
times I get here.

Also, do you have any suggestions how to fix this?  Perhaps we should
first create the images and cache them, and only then start the
animation?  Some other ideas?

Thanks.

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Sun, 19 Apr 2020 22:28:24 +0200
> 
> Two minor changes attached. First, the introduction of the Qnative_image
> type makes one call to image_can_use_native_api irrelevant.

That makes some assumptions I felt uneasy about.  First, it assumes
that the native_image entry will never have an init method; this could
become wrong at some future point, at which time someone will have to
remember to adapt initialize_image_type to handle that (since
native_image doesn't really have a library to load).

Second, what happens if the TYPE argument specifies an image type the
native_image cannot handle (e.g., SVG), and the corresponding optional
image library is not linked in or is unavailable at run time?  With
your patch, we would return true, right?

So, all things considered, I think it is more future-proof to leave
that call in place, at least until we make up our minds regarding the
default configuration on MS-Windows.

> Regarding this, I think it is a bad idea to introduce Qnative_image,
> because that is not an image format and users cannot recognize the
> actual image type from the image descriptor.

I'm not sure I understand the issue.  When you say "users", what do
you mean in this context?  If you mean users like me and you, then how
and where would we see Qnative_image?

The technical reason I introduced Qnative_image is that look
up_image_type wants to return a struct, and its callers depend on
that.  Also, I thought about the use case where none of the optional
image libraries are compiled in, in which case this will be the only
element of that array.  I shortly entertained the idea to put the
requested image symbol (like Qpng etc.) there, but I didn't like the
idea of changing a const struct.

What would you propose to do instead?

> The second fix makes w32image.c behave like nsimage.c, in that a delay=0
> is regarded as non-existant, thus returning nil. This makes animations
> default to the minimum animation delay, which currently is 0.01, and
> more useful than a delay of 0.

You are right.  However, I believe I already fixed that, albeit a bit
differently, as part of commit 423089d (after bumping into the same
problem during testing my recent changes).  We can use your change
instead, if you think it is better for some reason.

Thanks.

Re: GDI+ take 3

[Alan Third]

On Sun, Apr 19, 2020 at 10:08:06PM +0200, Juan José García-Ripoll wrote:
> The image that was supplied by Alan has a nominal delay time of 0.03
> seconds between frames. Emacs 26.3 takes about 0.14 seconds on average
> to load each frame of the gif file, using giflib. Emacs 28 as I built it
> from git source right now, takes 0.3 seconds with giflib and a time that
> grows from 0.01 up to 0.16 seconds (probably because frames have to be
> read sequentially, there is no index).

So Emacs 28 takes twice as long to load any given gif frame as Emacs
26 does? When both are using giflib?

Have I misunderstood you?
-- 
Alan Third

Re: GDI+ take 3

[Juan José García-Ripoll]

Alan Third <alan@idiocy.org> writes:
> On Sun, Apr 19, 2020 at 10:08:06PM +0200, Juan José García-Ripoll wrote:
>> The image that was supplied by Alan has a nominal delay time of 0.03
>> seconds between frames. Emacs 26.3 takes about 0.14 seconds on average
>> to load each frame of the gif file, using giflib. Emacs 28 as I built it
>> from git source right now, takes 0.3 seconds with giflib and a time that
>> grows from 0.01 up to 0.16 seconds (probably because frames have to be
>> read sequentially, there is no index).
>
> So Emacs 28 takes twice as long to load any given gif frame as Emacs
> 26 does? When both are using giflib?

Yes, but please understand I am using the default build flags for Emacs
(just plain ./configure + make) and the giflib version Emacs 28 is using
is the one shipped with latest msys64. In contrast, for Emacs 26.3 I
already use a preinstalled distribution, which may not have the same
version of giflib and may have been built with different flags.

In any case GDI+ is faster than both of them on my system and, while
slower than the desired 0.03 s / frame specified in the GIF file, it
still looks ok.

Cheers,

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

Re: GDI+ take 3

[Juan José García-Ripoll]

Eli Zaretskii <eliz@gnu.org> writes:
>> From: Juan José García-Ripoll
>>  <juanjose.garciaripoll@gmail.com>
>> Date: Sun, 19 Apr 2020 22:28:24 +0200
>> 
>> Two minor changes attached. First, the introduction of the Qnative_image
>> type makes one call to image_can_use_native_api irrelevant.
>
> That makes some assumptions I felt uneasy about.  First, it assumes
> that the native_image entry will never have an init method; this could
> become wrong at some future point, at which time someone will have to
> remember to adapt initialize_image_type to handle that (since
> native_image doesn't really have a library to load).

I do not think this is right. The current implementation is

- Something wants to load an image and invokes lookup_image_type()
- lookup_image_type() calls image_can_use_native_api
  + within image_can_use_native_api, the type of image is verified
     - if it is right, it _initializes_ gdi+ (= init method)
     - otherwise returns false
  + when image_can_use_native_api returns false, lookup_image_type
     loops over image type
     - for each image type it calls initialize_image_type()
       * initialize_image_type aclls image_can_use_native_api again!
       * it then invokes the initialization method

So, in the current architecture, image_can_use_native_api() is called
redundantly. The second call can be eliminated because it will always
return false. Moreover, image_can_use_native_api() always must implement
an init method, because failed initialization is a cause for not
supporting the API.

> Second, what happens if the TYPE argument specifies an image type the
> native_image cannot handle (e.g., SVG), and the corresponding optional
> image library is not linked in or is unavailable at run time?  With
> your patch, we would return true, right?

No. It would call the initialization method.

>> Regarding this, I think it is a bad idea to introduce Qnative_image,
>> because that is not an image format and users cannot recognize the
>> actual image type from the image descriptor.
>
> I'm not sure I understand the issue.  When you say "users", what do
> you mean in this context?  If you mean users like me and you, then how
> and where would we see Qnative_image?

By "users" I mean anyone using the image library, from image.el,
subcompoments or third-party libraries that use that. I assumed that the
internal type in image.c was also the type returned by functions such as
IMAGE-TYPE and the like. I am not sure any more.

> What would you propose to do instead?

To separate the notion of image type from the notion of image
handler. The former is a unique identifier for the type of image or
format, the latter is which library handles it. But, as I said above, if
img->type.type is not exposed, it does not matter.

>> The second fix makes w32image.c behave like nsimage.c, in that a delay=0
>> is regarded as non-existant, thus returning nil. This makes animations
>> default to the minimum animation delay, which currently is 0.01, and
>> more useful than a delay of 0.
>
> You are right.  However, I believe I already fixed that, albeit a bit
> differently, as part of commit 423089d (after bumping into the same
> problem during testing my recent changes).  We can use your change
> instead, if you think it is better for some reason.

I am not sure. I pulled latest emacs and w32_frame_delay() currently has
this

	  delay = decode_delay (propertyItem, frame);
	  if (delay <= 0)
	    {
	      /* In GIF files, unfortunately, delay is only specified
		 for the first frame.  */
	      delay = decode_delay (propertyItem, 0);
	    }

This code is wrong. It originates in my wrong decoding from
PropertyItem. I was testing the GDI+ library with various GIF files and
they returned 0 for the delay at later frames, but it was because I
misunderstood how Property Item works. It should read

	  delay = decode_delay (propertyItem, frame);

Now, in both cases (before and with this change) the output of this
function is used here (this is the code in master).

          if (delay >= 0)
            metadata = Fcons (Qdelay, Fcons (make_float (delay), metadata));

If delay == 0, it should not be returned. But I suppose it is just
nitpicking.

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

Re: GDI+ take 3

[Juan José García-Ripoll]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Juan José García-Ripoll
>>  <juanjose.garciaripoll@gmail.com>
>> Date: Sun, 19 Apr 2020 22:08:06 +0200
>>
>> The image that was supplied by Alan has a nominal delay time of 0.03
>> seconds between frames. Emacs 26.3 takes about 0.14 seconds on average
>> to load each frame of the gif file, using giflib. Emacs 28 as I built it
>> from git source right now, takes 0.3 seconds with giflib and a time that
>> grows from 0.01 up to 0.16 seconds (probably because frames have to be
>> read sequentially, there is no index).
>
> Can you show the Lisp you used to time this?  I'd like to see what
> times I get here.

It is attached. Note that it is an ugly hack. I am rewriting
image-animate-timeout because I have to get access to the internal variables.

> Also, do you have any suggestions how to fix this?  Perhaps we should first
> create the images and cache them, and only then start the animation?  Some
> other ideas?

That is one option. I am not sure about the logic on caching, and whether it
warrants that all frames would be kept in memory.

Other than that, one might have to reconsider the current mechanism how images
are built. In all platforms, when using giflib, images cleared various times
and built using PUT_PIXEL. I think this is behind the slowdown compared to
GDI+.

To make an informed decision, it would be appropriate to know what happens on
other platforms. In OSX it seems that loading of gifs is fast enough that, like
GDI+, no fix would be required. What about Xwindows / Cairo? Does it vary much?
I do not have machines to gather this information

Cheers

--
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

[-- Attachment #2: image-test.el --]
[-- Type: application/emacs-lisp, Size: 2791 bytes --]

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Tue, 21 Apr 2020 08:44:13 +0200
> 
> > That makes some assumptions I felt uneasy about.  First, it assumes
> > that the native_image entry will never have an init method; this could
> > become wrong at some future point, at which time someone will have to
> > remember to adapt initialize_image_type to handle that (since
> > native_image doesn't really have a library to load).
> 
> I do not think this is right. The current implementation is
> 
> - Something wants to load an image and invokes lookup_image_type()
> - lookup_image_type() calls image_can_use_native_api
>   + within image_can_use_native_api, the type of image is verified
>      - if it is right, it _initializes_ gdi+ (= init method)
>      - otherwise returns false
>   + when image_can_use_native_api returns false, lookup_image_type
>      loops over image type
>      - for each image type it calls initialize_image_type()
>        * initialize_image_type aclls image_can_use_native_api again!
>        * it then invokes the initialization method
> 
> So, in the current architecture, image_can_use_native_api() is called
> redundantly.

Yes, but it's very fast.  And also, is the above flow the only one?
Can we ever call initialize_image_type directly?  Doesn't that happen
when we first time call image-type-available-p, for example?  Such a
call can be issued by Lisp which tries to see if a certain type of
images can be displayed, and if not, use some fallback.

> > Second, what happens if the TYPE argument specifies an image type the
> > native_image cannot handle (e.g., SVG), and the corresponding optional
> > image library is not linked in or is unavailable at run time?  With
> > your patch, we would return true, right?
> 
> No. It would call the initialization method.

I think we may be miscommunicating.  If Emacs is built without
librsvg, the HAVE_RSVG macro is not defined, and the SVG part of the
image_types[] array and init_svg_functions are not compiled into
Emacs.  So there's no initialization method to call for SVG images,
and initialize_image_type should return false.

> > You are right.  However, I believe I already fixed that, albeit a bit
> > differently, as part of commit 423089d (after bumping into the same
> > problem during testing my recent changes).  We can use your change
> > instead, if you think it is better for some reason.
> 
> I am not sure. I pulled latest emacs and w32_frame_delay() currently has
> this
> 
> 	  delay = decode_delay (propertyItem, frame);
> 	  if (delay <= 0)
> 	    {
> 	      /* In GIF files, unfortunately, delay is only specified
> 		 for the first frame.  */
> 	      delay = decode_delay (propertyItem, 0);
> 	    }
> 
> This code is wrong. It originates in my wrong decoding from
> PropertyItem. I was testing the GDI+ library with various GIF files and
> they returned 0 for the delay at later frames, but it was because I
> misunderstood how Property Item works. It should read
> 
> 	  delay = decode_delay (propertyItem, frame);

OK, I'll remove that part.

> Now, in both cases (before and with this change) the output of this
> function is used here (this is the code in master).
> 
>           if (delay >= 0)
>             metadata = Fcons (Qdelay, Fcons (make_float (delay), metadata));
> 
> If delay == 0, it should not be returned. But I suppose it is just
> nitpicking.

I thought a delay of zero was a valid value.  But if it isn't, you
are, of course, right, and we should change the inequality to use >.

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Tue, 21 Apr 2020 09:35:05 +0200
> 
> > Can you show the Lisp you used to time this?  I'd like to see what
> > times I get here.
> 
> It is attached. Note that it is an ugly hack. I am rewriting
> image-animate-timeout because I have to get access to the internal variables.

That's okay, I just wanted to repeat the experiment.

> To make an informed decision, it would be appropriate to know what happens on
> other platforms. In OSX it seems that loading of gifs is fast enough that, like
> GDI+, no fix would be required. What about Xwindows / Cairo? Does it vary much?
> I do not have machines to gather this information

I hope others will chime in with that info.

Re: GDI+ take 3

[Juan José García-Ripoll]

Eli Zaretskii <eliz@gnu.org> writes:
>> - Something wants to load an image and invokes lookup_image_type()
>> - lookup_image_type() calls image_can_use_native_api
>>   + within image_can_use_native_api, the type of image is verified
>>      - if it is right, it _initializes_ gdi+ (= init method)
>>      - otherwise returns false
>>   + when image_can_use_native_api returns false, lookup_image_type
>>      loops over image type
>>      - for each image type it calls initialize_image_type()
>>        * initialize_image_type aclls image_can_use_native_api again!
>>        * it then invokes the initialization method
>> 
>> So, in the current architecture, image_can_use_native_api() is called
>> redundantly.
>
> Yes, but it's very fast.  And also, is the above flow the only one?
> Can we ever call initialize_image_type directly?

No. It is a static function and it is only called from lookup_image_type()

> I think we may be miscommunicating.  If Emacs is built without
> librsvg, the HAVE_RSVG macro is not defined, and the SVG part of the
> image_types[] array and init_svg_functions are not compiled into
> Emacs.  So there's no initialization method to call for SVG images,
> and initialize_image_type should return false.

My patch only removes the call to image_can_use_native_api () from
initialize_image_type() because that function was already used in the
only function that invokes it. The output of initialize_image_type() is
not changed for other libraries.

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es

Re: GDI+ take 3

[Alan Third]

On Tue, Apr 21, 2020 at 09:35:05AM +0200, Juan José García-Ripoll wrote:
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Also, do you have any suggestions how to fix this?  Perhaps we should first
> > create the images and cache them, and only then start the animation?  Some
> > other ideas?
> 
> That is one option. I am not sure about the logic on caching, and whether it
> warrants that all frames would be kept in memory.
> 
> Other than that, one might have to reconsider the current mechanism how images
> are built. In all platforms, when using giflib, images cleared various times
> and built using PUT_PIXEL. I think this is behind the slowdown compared to
> GDI+.
> 
> To make an informed decision, it would be appropriate to know what happens on
> other platforms. In OSX it seems that loading of gifs is fast enough that, like
> GDI+, no fix would be required. What about Xwindows / Cairo? Does it vary much?
> I do not have machines to gather this information

Loading of gifs on macOS may be fast enough because the image library
does its own caching, so may not be regenerating the images from
scratch each time.

XWindows (and I think Cairo) uses giflib exclusively, so shouldn’t
perform substantially different from either Windows or macOS when
using giflib instead of the native image library.

I agree with you, though, that fixing this particular performance
problem would require us to rethink how we load and cache images.

-- 
Alan Third

Re: GDI+ take 3

[Eli Zaretskii]

> Date: Tue, 21 Apr 2020 19:17:34 +0100
> From: Alan Third <alan@idiocy.org>
> Cc: emacs-devel@gnu.org
> 
> I agree with you, though, that fixing this particular performance
> problem would require us to rethink how we load and cache images.

I don't think our image caching mechanism was designed to show
animations.  It was designed to show a small number of static images
within mostly-textual content.

Re: GDI+ take 3

[Eli Zaretskii]

> From: Juan José García-Ripoll
>  <juanjose.garciaripoll@gmail.com>
> Date: Sun, 19 Apr 2020 22:08:06 +0200
> 
> Folloup on the previous studies. Note that I am using emacs on a 64-bit
> machine, not 32 bit. So I am unable to comment on Juanma's statistics or
> Eli's more recent build. However, I hooked into IMAGE-ANIMATE-TIMEOUT
> and it confirms what I suspected: the load times surpass the scheduled
> time of the timer.
> 
> The image that was supplied by Alan has a nominal delay time of 0.03
> seconds between frames. Emacs 26.3 takes about 0.14 seconds on average
> to load each frame of the gif file, using giflib. Emacs 28 as I built it
> from git source right now, takes 0.3 seconds with giflib and a time that
> grows from 0.01 up to 0.16 seconds (probably because frames have to be
> read sequentially, there is no index).

I see similar timings here.

I wonder why the times go up with GDI+, but not with giflib.  Maybe
the answer hides in what you say by "there's no index" -- what do you
mean by that?

P.S. Your test code allowed me to find and fix a bug in how images are
loaded natively, now fixed in the repository.

Re: GDI+ take 3

[Alan Third]

On Tue, Apr 21, 2020 at 08:25:45AM +0200, Juan José García-Ripoll wrote:
> Yes, but please understand I am using the default build flags for Emacs
> (just plain ./configure + make) and the giflib version Emacs 28 is using
> is the one shipped with latest msys64. In contrast, for Emacs 26.3 I
> already use a preinstalled distribution, which may not have the same
> version of giflib and may have been built with different flags.
> 
> In any case GDI+ is faster than both of them on my system and, while
> slower than the desired 0.03 s / frame specified in the GIF file, it
> still looks ok.

Thanks for your explanation. I was concerned I’d introduced some huge
slowdown between 26 and 28.
-- 
Alan Third

Re: GDI+ take 3

[Alan Third]

On Tue, Apr 21, 2020 at 09:34:56PM +0300, Eli Zaretskii wrote:
> > Date: Tue, 21 Apr 2020 19:17:34 +0100
> > From: Alan Third <alan@idiocy.org>
> > Cc: emacs-devel@gnu.org
> > 
> > I agree with you, though, that fixing this particular performance
> > problem would require us to rethink how we load and cache images.
> 
> I don't think our image caching mechanism was designed to show
> animations.  It was designed to show a small number of static images
> within mostly-textual content.

I’ve been thinking a little about if it’s possible to move image
loading into modules to be loaded at runtime, and one of the ideas I
had to support that may be helpful here. I don’t know too much about
Emacs Lisp’s internals, so feel free to shoot this idea down.

I believe we could add a new method of defining images which would be,
in effect, a pointer to a decoded image. Perhaps a system dependent
format, like pixmap on X, or an NSImage object on macOS, but it could
easily be some system independent format we devise ourselves.

We would have to provide a function (or perhaps modules could provide
it) where we pass in a filename, or data, and get back this pointer.
The function could also take a range of frame numbers, and return a
list of image pointers all decoded at once.

When displaying the image instead of passing a filename or data to
create-image, we pass in the image pointer, and everything should work
more or less the same from there.

We would have to rewrite how animations are displayed to update the
image from a list of image pointers instead of incrementing the frame
count.

The image pointer would, presumably, be subject to garbage collection,
so we may have to be able to clear any related images from the image
cache when it’s garbage collected. I don’t know how practical this is.

-- 
Alan Third

Re: GDI+ take 3

[Juan José García-Ripoll]

Eli Zaretskii <eliz@gnu.org> writes:
> I wonder why the times go up with GDI+, but not with giflib.  Maybe
> the answer hides in what you say by "there's no index" -- what do you
> mean by that?

Sorry, I was not too clear, but AFAIK GIF is a sequential format for a
composite image written down as a chained list of compressed blocks. I
have a feeling that libgif reads _all_ of the image, while GDI+ only
reads up to the frame we select.

Cheers,

-- 
Juan José García Ripoll
http://juanjose.garciaripoll.com
http://quinfog.hbar.es