[PATCH] Delayed loading of image libraries

[PATCH] Delayed loading of image libraries

[Juanma Barranquero]

OK, this is the (hopefully) final version of the "delayed loading" patch.

It is almost identical to the last one, except for renaming a few things
and adding a missing staticpro that was causing problems on
bootstrapping.  I've added docs to display.texi, and an entry to
etc/NEWS (though I'm, as always, unsure about the right place to put the
entry).  The only thing lacking is ChangeLog entries.

As I said a few days ago, I'd like to install this on trunk, if no one
has objections.

Any comments welcome, of course.

                                                                Juanma




--- old/etc/NEWS	2004-06-10 09:27:32.000000000 +0200
+++ new/etc/NEWS	2004-06-10 13:32:53.000000000 +0200
@@ -17,4 +17,8 @@
 * Installation Changes in Emacs 21.4
 
+** On MS Windows, Emacs now loads the image libraries on demand.
+You can configure the supported image types and their associated dynamic
+libraries by setting the variable `image-library-alist'.
+
 ---
 ** A Bulgarian translation of the Emacs Tutorial is available.
diff -Bru2 old/lisp/image.el new/lisp/image.el
--- old/lisp/image.el	2004-06-10 09:27:54.000000000 +0200
+++ new/lisp/image.el	2004-06-10 11:46:40.000000000 +0200
@@ -49,4 +49,15 @@
 a non-nil value, TYPE is the image's type.")
 
+;;;###autoload
+(defvar image-library-alist nil
+  "Alist of image types vs external libraries needed to display them.
+
+Each element is a list (IMAGE-TYPE LIBRARY...), where the car is a symbol
+representing a supported image type, and the rest are strings giving
+alternate filenames for the corresponding external libraries to load.
+They are tried in the order they appear on the list; if none of them can
+be loaded, the running session of Emacs won't display the image type.
+No entries are needed for pbm and xbm images; they're always supported.")
+;;;###autoload (put 'image-library-alist 'risky-local-variable t)
 
 (defun image-jpeg-p (data)
@@ -112,6 +123,6 @@
   "Value is non-nil if image type TYPE is available.
 Image types are symbols like `xbm' or `jpeg'."
-  (and (boundp 'image-types) (not (null (memq type image-types)))))
-
+  (and (fboundp 'init-image-library)
+       (init-image-library type image-library-alist)))
 
 ;;;###autoload
diff -Bru2 old/lisp/term/w32-win.el new/lisp/term/w32-win.el
--- old/lisp/term/w32-win.el	2004-06-10 09:28:04.000000000 +0200
+++ new/lisp/term/w32-win.el	2004-06-10 11:46:41.000000000 +0200
@@ -1262,4 +1262,12 @@
 	    (error "Font not found")))))
 
+;;; Set default known names for image libraries
+(setq image-library-alist
+      '((xpm "libXpm-nox4.dll" "libxpm.dll")
+        (png "libpng13d.dll" "libpng13.dll" "libpng12d.dll" "libpng12.dll" "libpng.dll")
+        (jpeg "jpeg62.dll" "libjpeg.dll" "jpeg-62.dll" "jpeg.dll")
+        (tiff "libtiff3.dll" "libtiff.dll")
+        (gif "libungif.dll")))
+
 ;;; arch-tag: 69fb1701-28c2-4890-b351-3d1fe4b4f166
 ;;; w32-win.el ends here
diff -Bru2 old/lispref/display.texi new/lispref/display.texi
--- old/lispref/display.texi	2004-06-10 09:28:10.000000000 +0200
+++ new/lispref/display.texi	2004-06-10 13:01:02.000000000 +0200
@@ -2876,6 +2876,11 @@
 
   Emacs can display a number of different image formats; some of them
-are supported only if particular support libraries are installed on your
-machine.  The supported image formats include XBM, XPM (needing the
+are supported only if particular support libraries are installed on
+your machine.  In some environments, Emacs allows loading image
+libraries on demand; if so, the variable @code{image-library-alist}
+can be used to modify the set of known names for these dynamic
+libraries (though it is not posible to add new image formats).
+
+  The supported image formats include XBM, XPM (needing the
 libraries @code{libXpm} version 3.4k and @code{libz}), GIF (needing
 @code{libungif} 4.1.0), Postscript, PBM, JPEG (needing the
@@ -2888,9 +2893,47 @@
 
 @defvar image-types
+@vindex image-types
 This variable contains a list of those image type symbols that are
-supported in the current configuration.
+potentially supported in the current configuration.
+@emph{Potentially} here means that Emacs knows about the image types,
+not necessarily that they can be loaded (they could depend on
+unavailable dynamic libraries, for example).
+
+To know which image types are really available, use
+@code{image-type-available-p}.
+@end defvar
+
+@defvar image-library-alist
+@vindex image-library-alist
+This in an alist of image types vs external libraries needed to
+display them.
+
+Each element is a list @code{(@var{IMAGE-TYPE} @var{LIBRARY}...)},
+where the car is a supported image format from @code{image-types}, and
+the rest are strings giving alternate filenames for the corresponding
+external libraries to load.
+
+They are tried in the order they appear on the list; if none of them
+can be loaded, the running session of Emacs won't support the image
+type.  No entries are needed for @code{pbm} and @code{xbm} images;
+they're always supported.
+
+This variable is ignored if the image libraries are statically linked
+into Emacs.
 @end defvar
 
+@defun  image-type-available-p type
+@findex image-type-available-p
+
+This function returns non-nil if image type TYPE is available, i.e.,
+if images of this type can be loaded and displayed in Emacs.  TYPE
+should be one of the types contained in @code{image-types}.
+
+For image types whose support libraries are statically linked, this
+function always returns @code{t}; for other image types, it returns
+@code{t} if the dynamic library could be loaded, @code{nil} otherwise.
+@end defun
+
 @menu
 * Image Descriptors::   How to specify an image for use in @code{:display}.
--- old/src/dispextern.h	2004-06-10 09:28:19.000000000 +0200
+++ new/src/dispextern.h	2004-06-10 11:46:41.000000000 +0200
@@ -2569,5 +2569,4 @@
 extern int mode_line_in_non_selected_windows;
 extern int redisplaying_p;
-extern Lisp_Object Vimage_types;
 extern void add_to_log P_ ((char *, Lisp_Object, Lisp_Object));
 extern int help_echo_showing_p;
@@ -2655,4 +2654,6 @@
 #ifdef HAVE_WINDOW_SYSTEM
 
+extern Lisp_Object Vimage_types;
+
 extern int x_bitmap_height P_ ((struct frame *, int));
 extern int x_bitmap_width P_ ((struct frame *, int));
diff -Bru2 old/src/image.c new/src/image.c
--- old/src/image.c	2004-06-10 09:28:20.000000000 +0200
+++ new/src/image.c	2004-06-10 11:51:49.000000000 +0200
@@ -606,4 +606,12 @@
 static struct image_type *image_types;
 
+/* A list of symbols, one for each supported image type.  */
+
+Lisp_Object Vimage_types;
+
+/* Cache for delayed-loading image types.  */
+
+static Lisp_Object Vimage_type_cache;
+
 /* The symbol `xbm' which is used as the type symbol for XBM images.  */
 
@@ -630,5 +638,5 @@
 /* Function prototypes.  */
 
-static void define_image_type P_ ((struct image_type *type));
+static Lisp_Object define_image_type P_ ((struct image_type *type, int loaded));
 static struct image_type *lookup_image_type P_ ((Lisp_Object symbol));
 static void image_error P_ ((char *format, Lisp_Object, Lisp_Object));
@@ -638,19 +646,35 @@
 				       Lisp_Object));
 
+#define CACHE_IMAGE_TYPE(type, status) \
+  do { Vimage_type_cache = Fcons (Fcons (type, status), Vimage_type_cache); } while (0)
+
+#define ADD_IMAGE_TYPE(type) \
+  do { Vimage_types = Fcons (type, Vimage_types); } while (0)
 
 /* Define a new image type from TYPE.  This adds a copy of TYPE to
-   image_types and adds the symbol *TYPE->type to Vimage_types.  */
+   image_types and caches the loading status of TYPE.  */
 
-static void
-define_image_type (type)
+static Lisp_Object
+define_image_type (type, loaded)
      struct image_type *type;
+     int loaded;
 {
-  /* Make a copy of TYPE to avoid a bus error in a dumped Emacs.
-     The initialized data segment is read-only.  */
-  struct image_type *p = (struct image_type *) xmalloc (sizeof *p);
-  bcopy (type, p, sizeof *p);
-  p->next = image_types;
-  image_types = p;
-  Vimage_types = Fcons (*p->type, Vimage_types);
+  Lisp_Object success;
+
+  if (!loaded)
+    success = Qnil;
+  else
+    {
+      /* Make a copy of TYPE to avoid a bus error in a dumped Emacs.
+         The initialized data segment is read-only.  */
+      struct image_type *p = (struct image_type *) xmalloc (sizeof *p);
+      bcopy (type, p, sizeof *p);
+      p->next = image_types;
+      image_types = p;
+      success = Qt;
+    }
+
+  CACHE_IMAGE_TYPE(*type->type, success);
+  return success;
 }
 
@@ -1789,4 +1813,31 @@
   }
 
+/* Load a DLL implementing an image type.
+   The `image-library-alist' variable associates a symbol,
+   identifying  an image type, to a list of possible filenames.
+   The function returns NULL if no library could be loaded for
+   the given image type, or if the library was previously loaded;
+   else the handle of the DLL.  */
+static HMODULE
+w32_delayed_load (Lisp_Object libraries, Lisp_Object type)
+{
+  HMODULE library = NULL;
+
+  if (CONSP (libraries) && NILP (Fassq (type, Vimage_type_cache)))
+    {
+      Lisp_Object dlls = Fassq (type, libraries);
+
+      if (CONSP (dlls))
+        for (dlls = XCDR (dlls); CONSP (dlls); dlls = XCDR (dlls))
+          {
+            CHECK_STRING_CAR (dlls);
+            if (library = LoadLibrary (SDATA (XCAR (dlls))))
+              break;
+          }
+    }
+
+  return library;
+}
+
 #endif /* HAVE_NTGUI */
 
@@ -3489,11 +3540,10 @@
 DEF_IMGLIB_FN (XImageFree);
 
-
 static int
-init_xpm_functions (void)
+init_xpm_functions (Lisp_Object libraries)
 {
   HMODULE library;
 
-  if (!(library = LoadLibrary ("libXpm.dll")))
+  if (!(library = w32_delayed_load (libraries, Qxpm)))
     return 0;
 
@@ -5589,19 +5639,10 @@
 
 static int
-init_png_functions (void)
+init_png_functions (Lisp_Object libraries)
 {
   HMODULE library;
 
-  /* Ensure zlib is loaded.  Try debug version first.  */
-  if (!LoadLibrary ("zlibd.dll")
-      && !LoadLibrary ("zlib.dll"))
-    return 0;
-
   /* Try loading libpng under probable names.  */
-  if (!(library = LoadLibrary ("libpng13d.dll"))
-      && !(library = LoadLibrary ("libpng13.dll"))
-      && !(library = LoadLibrary ("libpng12d.dll"))
-      && !(library = LoadLibrary ("libpng12.dll"))
-      && !(library = LoadLibrary ("libpng.dll")))
+  if (!(library = w32_delayed_load (libraries, Qpng)))
     return 0;
 
@@ -6247,11 +6288,9 @@
 
 static int
-init_jpeg_functions (void)
+init_jpeg_functions (Lisp_Object libraries)
 {
   HMODULE library;
 
-  if (!(library = LoadLibrary ("libjpeg.dll"))
-      && !(library = LoadLibrary ("jpeg-62.dll"))
-      && !(library = LoadLibrary ("jpeg.dll")))
+  if (!(library = w32_delayed_load (libraries, Qjpeg)))
     return 0;
 
@@ -6684,9 +6723,9 @@
 
 static int
-init_tiff_functions (void)
+init_tiff_functions (Lisp_Object libraries)
 {
   HMODULE library;
 
-  if (!(library = LoadLibrary ("libtiff.dll")))
+  if (!(library = w32_delayed_load (libraries, Qtiff)))
     return 0;
 
@@ -7104,9 +7143,9 @@
 
 static int
-init_gif_functions (void)
+init_gif_functions (Lisp_Object libraries)
 {
   HMODULE library;
 
-  if (!(library = LoadLibrary ("libungif.dll")))
+  if (!(library = w32_delayed_load (libraries, Qgif)))
     return 0;
 
@@ -7881,7 +7920,79 @@
  ***********************************************************************/
 
+#ifdef HAVE_NTGUI
+/* Image types that rely on external libraries are loaded dynamically
+   if the library is available.  */
+#define CHECK_LIB_AVAILABLE(image_type, init_lib_fn) \
+  define_image_type (image_type, init_lib_fn (libraries))
+#else
+#define CHECK_LIB_AVAILABLE(image_type, init_lib_fn) \
+  define_image_type (image_type, TRUE)
+#endif /* HAVE_NTGUI */
+
+DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 2, 2, 0,
+       doc: /* Initialize image library implementing image type TYPE.
+Return non-nil if TYPE is a supported image type.
+
+Image types pbm and xbm are prebuilt; other types are loaded here.
+Libraries to load are specified in alist LIBRARIES (usually, the value
+of `image-library-alist', which see.  */)
+  (type, libraries)
+{
+  Lisp_Object tested;
+
+  /* Don't try to reload the library.  */
+  tested = Fassq (type, Vimage_type_cache);
+  if (CONSP (tested))
+    return XCDR (tested);
+
+#if defined (HAVE_XPM) || defined (MAC_OS)
+  if (EQ (type, Qxpm))
+    return CHECK_LIB_AVAILABLE(&xpm_type, init_xpm_functions);
+#endif
+
+#if defined (HAVE_JPEG) || defined (MAC_OS)
+  if (EQ (type, Qjpeg))
+    return CHECK_LIB_AVAILABLE(&jpeg_type, init_jpeg_functions);
+#endif
+
+#if defined (HAVE_TIFF) || defined (MAC_OS)
+  if (EQ (type, Qtiff))
+    return CHECK_LIB_AVAILABLE(&tiff_type, init_tiff_functions);
+#endif
+
+#if defined (HAVE_GIF) || defined (MAC_OS)
+  if (EQ (type, Qgif))
+    return CHECK_LIB_AVAILABLE(&gif_type, init_gif_functions);
+#endif
+
+#if defined (HAVE_PNG) || defined (MAC_OS)
+  if (EQ (type, Qpng))
+    return CHECK_LIB_AVAILABLE(&png_type, init_png_functions);
+#endif
+
+#ifdef HAVE_GHOSTSCRIPT
+  if (EQ (type, Qpostscript))
+    return CHECK_LIB_AVAILABLE(&gs_type, init_gs_functions);
+#endif
+
+  /* If the type is not recognized, avoid testing it ever again.  */
+  CACHE_IMAGE_TYPE(type, Qnil);
+  return Qnil;
+}
+
 void
 syms_of_image ()
 {
+  /* Must be defined now becase we're going to update it below, while
+     defining the supported image types.  */
+  DEFVAR_LISP ("image-types", &Vimage_types,
+    doc: /* List of potentially supported image types.
+Each element of the list is a symbol for a image type, like 'jpeg or 'png.
+To check whether it is really supported, use `image-type-available-p'.  */);
+  Vimage_types = Qnil;
+
+  Vimage_type_cache = Qnil;
+  staticpro (&Vimage_type_cache);
+
   QCascent = intern (":ascent");
   staticpro (&QCascent);
@@ -7917,4 +8028,5 @@
   staticpro (&Qpostscript);
 #ifdef HAVE_GHOSTSCRIPT
+  ADD_IMAGE_TYPE(Qpostscript);
   QCloader = intern (":loader");
   staticpro (&QCloader);
@@ -7929,11 +8041,14 @@
   Qpbm = intern ("pbm");
   staticpro (&Qpbm);
+  ADD_IMAGE_TYPE(Qpbm);
 
   Qxbm = intern ("xbm");
   staticpro (&Qxbm);
+  ADD_IMAGE_TYPE(Qxbm);
 
 #if defined (HAVE_XPM) || defined (MAC_OS)
   Qxpm = intern ("xpm");
   staticpro (&Qxpm);
+  ADD_IMAGE_TYPE(Qxpm);
 #endif
 
@@ -7941,4 +8056,5 @@
   Qjpeg = intern ("jpeg");
   staticpro (&Qjpeg);
+  ADD_IMAGE_TYPE(Qjpeg);
 #endif
 
@@ -7946,4 +8062,5 @@
   Qtiff = intern ("tiff");
   staticpro (&Qtiff);
+  ADD_IMAGE_TYPE(Qtiff);
 #endif
 
@@ -7951,4 +8068,5 @@
   Qgif = intern ("gif");
   staticpro (&Qgif);
+  ADD_IMAGE_TYPE(Qgif);
 #endif
 
@@ -7956,6 +8074,8 @@
   Qpng = intern ("png");
   staticpro (&Qpng);
+  ADD_IMAGE_TYPE(Qpng);
 #endif
 
+  defsubr (&Sinit_image_library);
   defsubr (&Sclear_image_cache);
   defsubr (&Simage_size);
@@ -7985,50 +8105,11 @@
 }
 
-
-#ifdef HAVE_NTGUI
-/* Image types that rely on external libraries are loaded dynamically
-   if the library is available.  */
-#define IF_LIB_AVAILABLE(init_lib_fn)  if (init_lib_fn())
-#else
-#define IF_LIB_AVAILABLE(init_func)    /* Load unconditionally */
-#endif /* HAVE_NTGUI */
-
 void
 init_image ()
 {
   image_types = NULL;
-  Vimage_types = Qnil;
 
-  define_image_type (&xbm_type);
-  define_image_type (&pbm_type);
-
-#if defined (HAVE_XPM) || defined (MAC_OS)
-  IF_LIB_AVAILABLE(init_xpm_functions)
-    define_image_type (&xpm_type);
-#endif
-
-#if defined (HAVE_JPEG) || defined (MAC_OS)
-  IF_LIB_AVAILABLE(init_jpeg_functions)
-    define_image_type (&jpeg_type);
-#endif
-
-#if defined (HAVE_TIFF) || defined (MAC_OS)
-  IF_LIB_AVAILABLE(init_tiff_functions)
-    define_image_type (&tiff_type);
-#endif
-
-#if defined (HAVE_GIF) || defined (MAC_OS)
-  IF_LIB_AVAILABLE(init_gif_functions)
-    define_image_type (&gif_type);
-#endif
-
-#if defined (HAVE_PNG) || defined (MAC_OS)
-  IF_LIB_AVAILABLE(init_png_functions)
-    define_image_type (&png_type);
-#endif
-
-#ifdef HAVE_GHOSTSCRIPT
-  define_image_type (&gs_type);
-#endif
+  define_image_type (&xbm_type, TRUE);
+  define_image_type (&pbm_type, TRUE);
 
 #ifdef MAC_OS
diff -Bru2 old/src/xdisp.c new/src/xdisp.c
--- old/src/xdisp.c	2004-06-10 09:28:26.000000000 +0200
+++ new/src/xdisp.c	2004-06-10 11:46:41.000000000 +0200
@@ -671,8 +671,4 @@
 Lisp_Object Vhscroll_step;
 
-/* A list of symbols, one for each supported image type.  */
-
-Lisp_Object Vimage_types;
-
 /* The variable `resize-mini-windows'.  If nil, don't resize
    mini-windows.  If t, always resize them to fit the text they
@@ -22260,9 +22256,4 @@
   Vhscroll_step = make_number (0);
 
-  DEFVAR_LISP ("image-types", &Vimage_types,
-    doc: /* List of supported image types.
-Each element of the list is a symbol for a supported image type.  */);
-  Vimage_types = Qnil;
-
   DEFVAR_BOOL ("message-truncate-lines", &message_truncate_lines,
     doc: /* If non-nil, messages are truncated instead of resizing the echo area.

Re: [PATCH] Delayed loading of image libraries

[Andreas Schwab]

Juanma Barranquero <jmbarranquero@wke.es> writes:

> +DEFUN ("init-image-library", Finit_image_library, Sinit_image_library, 2, 2, 0,
> +       doc: /* Initialize image library implementing image type TYPE.
> +Return non-nil if TYPE is a supported image type.
> +
> +Image types pbm and xbm are prebuilt; other types are loaded here.
> +Libraries to load are specified in alist LIBRARIES (usually, the value
> +of `image-library-alist', which see.  */)
> +  (type, libraries)
> +{
> +  Lisp_Object tested;
> +
> +  /* Don't try to reload the library.  */
> +  tested = Fassq (type, Vimage_type_cache);
> +  if (CONSP (tested))
> +    return XCDR (tested);
> +
> +#if defined (HAVE_XPM) || defined (MAC_OS)
> +  if (EQ (type, Qxpm))
> +    return CHECK_LIB_AVAILABLE(&xpm_type, init_xpm_functions);
> +#endif

Please make libraries an explicit parameter of CHECK_LIB_AVAILABLE, so
that it does not look like it is completely ignored.  Also, you are
missing to declare the parameters.  Please always compile with -Wall.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Delayed loading of image libraries

[Juanma Barranquero]

On Wed, 30 Jun 2004 12:15:29 +0200
Andreas Schwab <schwab@suse.de> wrote:

> Please make libraries an explicit parameter of CHECK_LIB_AVAILABLE, so
> that it does not look like it is completely ignored.

Well, it is a matter of taste.  Hidding things is not good, but
redundant info is not good either, and in this case, there's no way
CHECK_LIB_AVAILABLE is ever gonna get passed anything but `libraries'. 
Also, CHECK_LIB_AVAILABLE is defined just above init-image-library; it
takes only a cursory look to notice that `libraries' is being used.  I'd
agree with you if CHECK_LIB_AVAILABLE were defined in an include file,
or at the top of image.c.

> Also, you are
> missing to declare the parameters.

You're right; thanks for noticing.

> Please always compile with -Wall.

I'll try to remember, if I ever compile with GCC.  For MSVC, I'm using
the default warnings (default, as set up by nt/configure.bat, I mean);
with these, there's no warning for the missing parameter declarations.

                                                                Juanma

Re: [PATCH] Delayed loading of image libraries

[Andreas Schwab]

Juanma Barranquero <jmbarranquero@wke.es> writes:

> On Wed, 30 Jun 2004 12:15:29 +0200
> Andreas Schwab <schwab@suse.de> wrote:
>
>> Please make libraries an explicit parameter of CHECK_LIB_AVAILABLE, so
>> that it does not look like it is completely ignored.
>
> Well, it is a matter of taste.  Hidding things is not good, but
> redundant info is not good either, and in this case, there's no way
> CHECK_LIB_AVAILABLE is ever gonna get passed anything but `libraries'. 

Implicit parameters are bad when you want to find out what's going on and
can hide hard to track bugs.

On a related note, what is Finit_image_library supposed to be passed in
lookup_image_type?  image-library-alist is not available on the C level.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Delayed loading of image libraries

[Juanma Barranquero]

On Wed, 30 Jun 2004 13:24:18 +0200
Andreas Schwab <schwab@suse.de> wrote:

> Implicit parameters are bad when you want to find out what's going on and
> can hide hard to track bugs.

As I've said, I agree on the general principle but I don't think it's
very significant in this case.  But please, do change it if it really
bothers you.

> On a related note, what is Finit_image_library supposed to be passed in
> lookup_image_type?

I'm not sure.  Miles added that code, so I suppose he found some problem
that I did overlook.

>  image-library-alist is not available on the C level.

Perhaps it should?

                                                                Juanma

Re: [PATCH] Delayed loading of image libraries

[Andreas Schwab]

Juanma Barranquero <jmbarranquero@wke.es> writes:

>> On a related note, what is Finit_image_library supposed to be passed in
>> lookup_image_type?
>
> I'm not sure.  Miles added that code, so I suppose he found some problem
> that I did overlook.
>
>>  image-library-alist is not available on the C level.
>
> Perhaps it should?

Would Qnil be valid in this context?  Certainly for !HAVE_NTGUI, but would
that work for HAVE_NTGUI?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Delayed loading of image libraries

[Juanma Barranquero]

On Wed, 30 Jun 2004 13:41:51 +0200
Andreas Schwab <schwab@suse.de> wrote:

> Would Qnil be valid in this context?  Certainly for !HAVE_NTGUI, but would
> that work for HAVE_NTGUI?

Passing Qnil should work.  However, Miles' change doesn't seem to be
right, even when passing Qnil to Finit_image_library.  make_image does:

  img->type = lookup_image_type (image_spec_value (spec, QCtype, NULL));
  xassert (img->type != NULL);

and, from what I gather, xassert can be a null op**, in which case an
image is built with a non-valid type.

In fact, I'm not sure why lookup_image_type is supposed to allow
returning NULL.  It is an internal function to map image type symbols to
image type structs, so the image type symbol validation should be done
at a higher level (before calling lookup_image_type).

**BTW, what is xassert supposed to do?  On dispextern.h it is a null op
unless GLYPH_DEBUG is defined; and fontset.c has its own definition.

                                                                Juanma

Re: [PATCH] Delayed loading of image libraries

[Stefan]

>>> Please make libraries an explicit parameter of CHECK_LIB_AVAILABLE, so
>>> that it does not look like it is completely ignored.
>> 
>> Well, it is a matter of taste.  Hidding things is not good, but
>> redundant info is not good either, and in this case, there's no way
>> CHECK_LIB_AVAILABLE is ever gonna get passed anything but `libraries'. 

> Implicit parameters are bad when you want to find out what's going on and
> can hide hard to track bugs.

100% agreement.  Please don't introduce such situations unless it's really
painful to pass the parameters explicitly.


        Stefan

Re: [PATCH] Delayed loading of image libraries

[Miles Bader]

On Wed, Jun 30, 2004 at 01:34:25PM +0200, Juanma Barranquero wrote:
> > On a related note, what is Finit_image_library supposed to be passed in
> > lookup_image_type?
> 
> I'm not sure.  Miles added that code, so I suppose he found some problem
> that I did overlook.

The result of `create-image' can be saved in a lisp file, and later loaded,
in which case the image will be displayed without `init-image-library' being
called from `image-type-available-p'y I added a the call to make sure it
always gets called one way or another.

-Miles
-- 
o The existentialist, not having a pillow, goes everywhere with the book by
  Sullivan, _I am going to spit on your graves_.

Re: [PATCH] Delayed loading of image libraries

[Juanma Barranquero]

On Wed, 30 Jun 2004 11:42:34 -0400
Miles Bader <miles@gnu.org> wrote:

> The result of `create-image' can be saved in a lisp file, and later loaded,
> in which case the image will be displayed without `init-image-library' being
> called from `image-type-available-p'y

It can be loaded, sure, but can it be *displayed* without
`image-type-available-p' being called?  I'd say that's a bug.

> I added a the call to make sure it
> always gets called one way or another.

The current situation is Not Good.  Andreas has fixed your patch by
passing it Qnil, which "works" in the sense that init-image-library
doesn't fail.  But:

  - It is now effectively a noop, because init-image-library'ing any
    image type from inside lookup_image_type will not load it.

  - Emacs on Windows crashes on certain kinds of image files (notably
    TIFFs).  I think that's related to something I asked yesterday:
    lookup_image_type can return NULL, but at places its return value is
    just checked with xassert, which could be a noop, so it is posible
    to end creating image types with image->type == NULL.  Why it is
    allowed?

  - We could make image-library-alist available from C and pass it to
    the call to init-image-library on lookup_image_type, but then, why
    make LIBRARIES an optional argument *at all*?  At first,
    init-image-library didn't accept a second argument, it just used
    image-library-alist, but Kim suggested passing it as an argument so
    the user could manipulate it or supply an alternative loading list.

So, IMHO:

  - We should make sure that image-type-available-p is called at the
    right places, so lookup_image_type does not need to call
    init-image-library.

  - We should guarantee that lookup_image_type is not going to return
    NULL (which doesn't make a lot of sense, as it is an internal
    function and we control whether it gets a valid image type or not).

                                                                Juanma

Re: [PATCH] Delayed loading of image libraries

[Miles Bader]

Juanma Barranquero <jmbarranquero@wke.es> writes:
>> The result of `create-image' can be saved in a lisp file, and later loaded,
>> in which case the image will be displayed without `init-image-library' being
>> called from `image-type-available-p'y
>
> It can be loaded, sure, but can it be *displayed* without
> `image-type-available-p' being called?  I'd say that's a bug.

With my patch they can.

Images are (currently) perfectly valid lisp values; if we don't want
them to be readable and writable to files, we should make them an opaque
type.

But that seems like a stupid thing to do.  Just make the C code deal
with the situation.  If my patch was wrong (though I admit, I still have
no idea why -- your discussion so far doesn't make much sense to me),
fix it, but don't remove the functionality it adds.

[Note that reading and writing images to files (and displaying them in a
new emacs instance, by just reading the file) worked fine before the
changes that added Finit_image_library etc., so the bug I fixed is a
regression.]

-Miles
-- 
My spirit felt washed.  With blood.  [Eli Shin, on "The Passion of the Christ"]

Re: [PATCH] Delayed loading of image libraries

[Juanma Barranquero]

On Thu, 01 Jul 2004 18:48:46 +0900
Miles Bader <miles@lsi.nec.co.jp> wrote:

> Images are (currently) perfectly valid lisp values; if we don't want
> them to be readable and writable to files, we should make them an opaque
> type.

Reading and writing a lisp value (whether an image, or any other kind)
does not need image support.  *Processing* one of such values as an
image (to display it, ask questions about it, etc.) does need it.  At
that point, code should've asked whether the image type was available. 
I'm not sure why you do conflate reading/writing with using-as-an-image
(even if loading an image-representing lisp value ends up displaying it).

> But that seems like a stupid thing to do.  Just make the C code deal
> with the situation.  If my patch was wrong (though I admit, I still have
> no idea why

???

init-image-library has two mandatory arguments.  You didn't pass the
second one: the alist specifying where to find the dynamic libraries
supporting the desired image type.

>-- your discussion so far doesn't make much sense to me),

Why?  Lack of English skills, (absence of) clarity, vocabulary mismatch?

> fix it, but don't remove the functionality it adds.

I'm not trying to remove any functionality.  Honest.  I'm trying to know
where should it be implemented.

> [Note that reading and writing images to files (and displaying them in a
> new emacs instance, by just reading the file) worked fine before the
> changes that added Finit_image_library etc., so the bug I fixed is a
> regression.]

There's no doubt that it must be fixed.  Currently, it is not (my Emacs
crashes on displaying TIFFs).

I'll repeat one of my questions, not for you in particular, but for
anyone who knows: Why can lookup_image_type return NULL?

                                                                Juanma

Re: [PATCH] Delayed loading of image libraries

[Miles Bader]

Juanma Barranquero <jmbarranquero@wke.es> writes:
>> Images are (currently) perfectly valid lisp values; if we don't want
>> them to be readable and writable to files, we should make them an opaque
>> type.
>
> Reading and writing a lisp value (whether an image, or any other kind)
> does not need image support.  *Processing* one of such values as an
> image (to display it, ask questions about it, etc.) does need it.  At
> that point, code should've asked whether the image type was available. 
> I'm not sure why you do conflate reading/writing with using-as-an-image
> (even if loading an image-representing lisp value ends up displaying it).

You're misreading what I wrote; the point of mentioning reading/writing
is that an `image' in Emacs lisp is just an ordinary lisp value, which
can be stored to a file (using print or whatever), and read back into a
new session.

Previously, the value returned by `create-image' was a valid image,
which could be displayed, even if saved to a file and read back in a new
emacs session.  After your change for windows, this was not true any
longer.  This was a regression, and my change was intended to fix it.

-Miles
-- 
P.S.  All information contained in the above letter is false,
      for reasons of military security.

Re: [PATCH] Delayed loading of image libraries

[Juanma Barranquero]

On Fri, 02 Jul 2004 10:16:03 +0900
Miles Bader <miles@lsi.nec.co.jp> wrote:

> Previously, the value returned by `create-image' was a valid image,
> which could be displayed, even if saved to a file and read back in a new
> emacs session.  After your change for windows, this was not true any
> longer.

Why?  In which way would it be different from the previous situation,
where you could make a PNG image, say, save it, and load it on an Emacs
*not* supporting PNG? After loading, Emacs *must* ask at a moment or
other whether the image type is available, mustn't it?

So, at what moment is the loaded image used, and why
`image-type-available-p' has not been called at that point?

> This was a regression, and my change was intended to fix it.

Yes.  But it didn't fix it, and Andreas patch doesn't either.

Miles, please, stop assuming that I don't accept there's a problem, or
that I don't want it fixed.  I'm just dissenting about the precise place
and method to fix it.

                                                                Juanma

Re: [PATCH] Delayed loading of image libraries

[David Kastrup]

Juanma Barranquero <jmbarranquero@wke.es> writes:

> On Fri, 02 Jul 2004 10:16:03 +0900
> Miles Bader <miles@lsi.nec.co.jp> wrote:
> 
> > Previously, the value returned by `create-image' was a valid
> > image, which could be displayed, even if saved to a file and read
> > back in a new emacs session.  After your change for windows, this
> > was not true any longer.
> 
> Why?  In which way would it be different from the previous
> situation, where you could make a PNG image, say, save it, and load
> it on an Emacs *not* supporting PNG?

In that we are not talking about an Emacs not supporting PNG.  It is
fine if an Emacs not supporting PNG does not display a PNG.  It is
not fine if an Emacs supporting PNG does not display a PNG.

> After loading, Emacs *must* ask at a moment or other whether the
> image type is available, mustn't it?

How about when displaying it?

> So, at what moment is the loaded image used, and why
> `image-type-available-p' has not been called at that point?

Why should image-type-available-p have been called?

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Delayed loading of image libraries

[Juanma Barranquero]

On 02 Jul 2004 09:10:55 +0200
David Kastrup <dak@gnu.org> wrote:

> It is
> not fine if an Emacs supporting PNG does not display a PNG.

Sure.  But currently, an Emacs supporting PNG *will* display a PNG if
the library is made available (either statically or dynamically).  Miles
thinks (or so I understand) that the responsability of making the
library available rests on lookup_image_type.  I believe the proper
thing to do is, at some point, calling `image-type-available-p'.

> > After loading, Emacs *must* ask at a moment or other whether the
> > image type is available, mustn't it?
> 
> How about when displaying it?

And is not exactly that what I propose?

> Why should image-type-available-p have been called?

Because, AFAICS, is the only documented way to know whether a given
image type is supported.

                                                                Juanma

Re: [PATCH] Delayed loading of image libraries

[Miles Bader]

Juanma Barranquero <jmbarranquero@wke.es> writes:
> Sure.  But currently, an Emacs supporting PNG *will* display a PNG if
> the library is made available (either statically or dynamically).  Miles
> thinks (or so I understand) that the responsability of making the
> library available rests on lookup_image_type.

I have no particular opinion about which exact function does it, but it
_should_ be done `transparently' -- that is any place where a lisp value
is used to compute deeper information (for display or otherwise).

Two important functions for that are `valid_image_p' (called by all
sorts of image-manipulation functions), and `lookup_image' (called by
the display engine).  Both of those call `lookup_image_type', and thus
my placing a call to image-type-validation there.

If I omitted an argument, obviously my change was flawed, but the
location seems reasonable.

-Miles
-- 
We are all lying in the gutter, but some of us are looking at the stars.
-Oscar Wilde

Re: [PATCH] Delayed loading of image libraries

[Juanma Barranquero]

On Fri, 02 Jul 2004 16:56:51 +0900
Miles Bader <miles@lsi.nec.co.jp> wrote:

> I have no particular opinion about which exact function does it, but it
> _should_ be done `transparently' -- that is any place where a lisp value
> is used to compute deeper information (for display or otherwise).

If it is done transparently, what to do when the image type is not
available?  You're worried about the case where you write an image and
load it afterwards on the same Emacs, but in the general case, you can
write an (image) lisp value with one Emacs and load it with another
(which could not support the type).  What's the expected behaviour at
that point?

If my reading of the code is right, when lookup_image_type returns NULL
anything can happen.  The code has a few xasserts here and there, but
there's no real handling of NULL image types.  Vide the current
situation: when you try to load a TIFF, lookup_image_type returns NULL 
(because init-image-library is getting a null alist, but that's
irrelevant to the argument).  At some point, this return value is not
checked and bingo!  Emacs crashes (at least on Windows).

> Both of those call `lookup_image_type', and thus
> my placing a call to image-type-validation there.
> 
> If I omitted an argument, obviously my change was flawed, but the
> location seems reasonable.

Perhaps you're right, I don't know.  If you are right, then
lookup_image_type would have to make sure that init-image-library is
using image-library-alist, at which point the use of that variable is
fixed in C code, and in fact no longer makes sense to pass it as an
argument to init-image-library (it could as well be directly used from
inside the function, as it is no longer overridable by the user).

                                                                Juanma

Re: [PATCH] Delayed loading of image libraries

[David Kastrup]

Juanma Barranquero <jmbarranquero@wke.es> writes:

> On 02 Jul 2004 09:10:55 +0200
> David Kastrup <dak@gnu.org> wrote:
> 
> > It is
> > not fine if an Emacs supporting PNG does not display a PNG.
> 
> Sure.  But currently, an Emacs supporting PNG *will* display a PNG if
> the library is made available (either statically or dynamically).  Miles
> thinks (or so I understand) that the responsability of making the
> library available rests on lookup_image_type.  I believe the proper
> thing to do is, at some point, calling `image-type-available-p'.

The way I understand this, the responsibility of making the library
available rests on the display code.  If I put the stuff into a
display property I saved just verbatim (without even taking a look at
what image formats would be in it), then after restarting an Emacs
session and restoring the display property, the PNG should display.
No calls to lookup_image_type or image-type-available-p or whatever
else by the user should be required.

> > > After loading, Emacs *must* ask at a moment or other whether the
> > > image type is available, mustn't it?
> > 
> > How about when displaying it?
> 
> And is not exactly that what I propose?

If it is, I have failed to get it.  It sounded to me like you were
proposing that the user has to something before the image cn be
displayed.

> > Why should image-type-available-p have been called?
> 
> Because, AFAICS, is the only documented way to know whether a given
> image type is supported.

What if a program does not care because it simply assumes that the
configuration is correct?  If a particular format is not supported,
we currently just get boxes.  That's ok.  It is not ok if we just get
boxes even _when_ a particular format is supported.

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH] Delayed loading of image libraries

[Miles Bader]

Juanma Barranquero <jmbarranquero@wke.es> writes:

> On Fri, 02 Jul 2004 16:56:51 +0900
> Miles Bader <miles@lsi.nec.co.jp> wrote:
>
>> I have no particular opinion about which exact function does it, but it
>> _should_ be done `transparently' -- that is any place where a lisp value
>> is used to compute deeper information (for display or otherwise).
>
> If it is done transparently, what to do when the image type is not
> available?  You're worried about the case where you write an image and
> load it afterwards on the same Emacs, but in the general case, you can
> write an (image) lisp value with one Emacs and load it with another
> (which could not support the type).  What's the expected behaviour at
> that point?

That seems obvious -- just don't display it.

> If my reading of the code is right, when lookup_image_type returns NULL
> anything can happen.

If lookup_image_type returns null, at least valid_image_p should return
0 (after my change), and many places use that as a guard.  Perhaps there
are more places checks should be inserted -- but inserting such checks
seems like the right thing to do.

-Miles
-- 
Is it true that nothing can be known?  If so how do we know this?  -Woody Allen

Re: [PATCH] Delayed loading of image libraries

[Juanma Barranquero]

On 02 Jul 2004 10:11:52 +0200
David Kastrup <dak@gnu.org> wrote:

> The way I understand this, the responsibility of making the library
> available rests on the display code.

Yes.

> If it is, I have failed to get it.  It sounded to me like you were
> proposing that the user has to something before the image cn be
> displayed.

No!  What I say is that the display code should call
image-type-available-p before trying to lookup image type or determine
whether the image type is valid.

> What if a program does not care because it simply assumes that the
> configuration is correct?  If a particular format is not supported,
> we currently just get boxes.  That's ok.  It is not ok if we just get
> boxes even _when_ a particular format is supported.

That certainly does not happen if you open an image file and are using
auto-image-file-mode, for example.  From this discussion, I gather that
it does happen if you use create-image, save the result and then load it
and try to display it...

                                                                Juanma

Re: [PATCH] Delayed loading of image libraries

[Juanma Barranquero]

On Fri, 02 Jul 2004 17:20:13 +0900
Miles Bader <miles@lsi.nec.co.jp> wrote:

> That seems obvious -- just don't display it.



> If lookup_image_type returns null, at least valid_image_p should return
> 0 (after my change), and many places use that as a guard.  Perhaps there
> are more places checks should be inserted -- but inserting such checks
> seems like the right thing to do.

There are many places where valid_image_p is checked, like this one:

static struct image *
make_image (spec, hash)
     Lisp_Object spec;
     unsigned hash;
{
  struct image *img = (struct image *) xmalloc (sizeof *img);

  xassert (valid_image_p (spec));
  bzero (img, sizeof *img);
  img->type = lookup_image_type (image_spec_value (spec, QCtype, NULL));
  xassert (img->type != NULL);
  img->spec = spec;
  img->data.lisp_val = Qnil;
  img->ascent = DEFAULT_IMAGE_ASCENT;
  img->hash = hash;
  return img;
}

Now, xassert is (in src/dispextern.h):

#if GLYPH_DEBUG
#define IF_DEBUG(X)	X
#define xassert(X)	do {if (!(X)) abort ();} while (0)
#else
#define IF_DEBUG(X)	(void) 0
#define xassert(X)	(void) 0
#endif

I don't think GLYPH_DEBUG is always defined, so xassert will do nothing. 
But even if it did, asserting out of a function because you have a Lisp
value that cannot be *displayed* doesn't seem right.  The decision rests
at a higher level about what to do with the value.  A PNG on a non-PNG
Emacs shouldn't be non-valid; what it is, is not-available.

                                                                Juanma

Re: [PATCH] Delayed loading of image libraries

[Miles Bader]

Juanma Barranquero <jmbarranquero@wke.es> writes:
>> If lookup_image_type returns null, at least valid_image_p should return
>> 0 (after my change), and many places use that as a guard.  Perhaps there
>> are more places checks should be inserted -- but inserting such checks
>> seems like the right thing to do.
>
> There are many places where valid_image_p is checked, like this one:
>
> make_image (spec, hash)
> {
...
>   xassert (valid_image_p (spec));
...
> I don't think GLYPH_DEBUG is always defined, so xassert will do nothing. 
> But even if it did, asserting out of a function because you have a Lisp
> value that cannot be *displayed* doesn't seem right.  The decision rests
> at a higher level about what to do with the value.  A PNG on a non-PNG
> Emacs shouldn't be non-valid; what it is, is not-available.

Clearly if there are situations where a bogus image value placed by
lisp can cause the an assertion failure, they should be fixed -- that
should never happen.

The C code _must_ deal with lisp providing invalid images in a
non-crashing manner (by not displaying the image).

However, it's possible that uses of valid_image_p such as the one you
quote above rely on other C code to make sure an invalid image never
reaches them, and the additional call to `xassert (valid_image_p (...))'
is merely an additional safeguard to catch bugs earlier.  You cannot
know unless you trace through the situations where the function in
question (such as make_image above) are called.

A quick look shows that all the functions which use `xassert (valid_image_p (...))'
(lookup_image_type, make_image, and image_spec_value) are only called
in places where the image spec they use is guaranteed to be valid,
because they are guarded by a previous call to valid_image_p.

-Miles
-- 
Is it true that nothing can be known?  If so how do we know this?  -Woody Allen

Re: [PATCH] Delayed loading of image libraries

[Miles Bader]

I wrote:
> A quick look shows that all the functions which use `xassert (valid_image_p (...))'
> (lookup_image_type, make_image, and image_spec_value) are only called
> in places where the image spec they use is guaranteed to be valid,
> because they are guarded by a previous call to valid_image_p.

Sorry, that list is actually (lookup_image, make_image, and image_spec_value).

-Miles
-- 
Love is a snowmobile racing across the tundra.  Suddenly it flips over,
pinning you underneath.  At night the ice weasels come.  --Nietzsche

Re: [PATCH] Delayed loading of image libraries

[Richard Stallman]

    Why?  In which way would it be different from the previous situation,
    where you could make a PNG image, say, save it, and load it on an Emacs
    *not* supporting PNG?

That seems legitimate.  Emacs would be unable to display the image,
but loading the Lisp data should work regardless.