Re: Problem with image libraries on Windows (reprise)

[Juanma Barranquero <lekktu@gmail.com>]

Well, I've finally re-implemented the JPEG stuff. I'll summarize the
issue for those unable or unwilling to access old messages from the
thread.

The problem: on environments where the image libraries are dynamically
loaded (which currently is only Windows, though the machinery to do it
on other platforms is in place), it is not advisable to pass a FILE *
from Emacs to functions in a DLL. The reason is that FILE *'s are not
shareable between different CRT instances, and there's *no guarantee*
that the DLL and Emacs will be using a single CRT instance; not even
when both have been compiled with GCC/MinGW, as there are several
models of compilation which can produce different CRT configurations,
and we have *no control* over which jpeg DLL will have the user.

The symptom is Emacs crashes when using functions that read JPEG
images from file, for example:

  (insert-image (create-image "/test/image.jpg"))

The problem can be fixed by implementing three small functions. The
resulting fix has about 100 lines of new code, plus a few renames of
some "memory source" functions (strictly not needed, but nice for
consistency).

My previous implementation used code from jdatasrc.c, which is part of
the jpeglib sources. As this could be problematic from a legal point
of view (the jpeglib license would force adding a copyright notice,
etc.), I've re-implemented the patch using the jpeglib documentation,
and also the ChangeLog entry I wrote for the previous implementation.
(Note, however, that as I don't come equipped with flash memory, I'm
unable to do a controlled self-erase, so some similarity is to be
expected; most of it can be traced back to the "memory source"
implementation, already in image.c, which has served as inspiration
for a sizable part of this new patch, as it was for the previous one.)

I've tested the patch in a few dozens of jpeg files and seems to be
working OK. However, more testing would be welcome.

If no one objects, I'll install the patch next week.

                    /L/e/k/t/u



Index: src/ChangeLog
===================================================================
RCS file: /cvsroot/emacs/emacs/src/ChangeLog,v
retrieving revision 1.4627
diff -u -2 -r1.4627 ChangeLog
--- src/ChangeLog	4 Oct 2005 14:13:50 -0000	1.4627
+++ src/ChangeLog	5 Oct 2005 13:52:51 -0000
@@ -1,2 +1,18 @@
+2005-10-05  Juanma Barranquero  <lekktu@gmail.com>
+
+	* image.c (fn_jpeg_stdio_src): Don't define it.
+	(init_jpeg_functions): Don't initialize `fn_jpeg_stdio_src'.
+	(our_common_init_source): Rename from `our_init_source'.
+	(our_common_term_source): Rename from `our_term_source'.
+	(our_memory_fill_input_buffer): Rename from
+	`our_fill_input_buffer'.
+	(our_memory_skip_input_data): Rename from `our_skip_input_data'.
+	(jpeg_memory_src): Use the new names.
+	(struct jpeg_stdio_mgr): New struct.
+	(JPEG_STDIO_BUFFER_SIZE): New constant.
+	(our_stdio_fill_input_buffer, our_stdio_skip_input_data)
+	(jpeg_file_src): New functions.
+	(jpeg_load): Use `jpeg_file_src' instead of `fn_jpeg_stdio_src'.
+
 2005-10-04  Kim F. Storm  <storm@cua.dk>

Index: src/image.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/image.c,v
retrieving revision 1.35
diff -u -2 -r1.35 image.c
--- src/image.c	30 Sep 2005 22:38:15 -0000	1.35
+++ src/image.c	5 Oct 2005 13:36:49 -0000
@@ -6359,5 +6359,4 @@
 DEF_IMGLIB_FN (jpeg_read_header);
 DEF_IMGLIB_FN (jpeg_read_scanlines);
-DEF_IMGLIB_FN (jpeg_stdio_src);
 DEF_IMGLIB_FN (jpeg_std_error);
 DEF_IMGLIB_FN (jpeg_resync_to_restart);
@@ -6375,5 +6374,4 @@
   LOAD_IMGLIB_FN (library, jpeg_start_decompress);
   LOAD_IMGLIB_FN (library, jpeg_read_header);
-  LOAD_IMGLIB_FN (library, jpeg_stdio_src);
   LOAD_IMGLIB_FN (library, jpeg_CreateDecompress);
   LOAD_IMGLIB_FN (library, jpeg_destroy_decompress);
@@ -6401,5 +6399,4 @@
 #define fn_jpeg_read_header		jpeg_read_header
 #define fn_jpeg_read_scanlines		jpeg_read_scanlines
-#define fn_jpeg_stdio_src		jpeg_stdio_src
 #define fn_jpeg_std_error		jpeg_std_error
 #define jpeg_resync_to_restart_wrapper	jpeg_resync_to_restart
@@ -6428,5 +6425,15 @@

 static void
-our_init_source (cinfo)
+our_common_init_source (cinfo)
+     j_decompress_ptr cinfo;
+{
+}
+
+
+/* Method to terminate data source.  Called by
+   jpeg_finish_decompress() after all data has been processed.  */
+
+static void
+our_common_term_source (cinfo)
      j_decompress_ptr cinfo;
 {
@@ -6439,5 +6446,5 @@

  static boolean
-our_fill_input_buffer (cinfo)
+our_memory_fill_input_buffer (cinfo)
      j_decompress_ptr cinfo;
 {
@@ -6459,5 +6466,5 @@

 static void
-our_skip_input_data (cinfo, num_bytes)
+our_memory_skip_input_data (cinfo, num_bytes)
      j_decompress_ptr cinfo;
      long num_bytes;
@@ -6476,14 +6483,4 @@


-/* Method to terminate data source.  Called by
-   jpeg_finish_decompress() after all data has been processed.  */
-
-static void
-our_term_source (cinfo)
-     j_decompress_ptr cinfo;
-{
-}
-
-
  /* Set up the JPEG lib for reading an image from DATA which contains
    LEN bytes.  CINFO is the decompression info structure created for
@@ -6509,9 +6506,9 @@

   src = (struct jpeg_source_mgr *) cinfo->src;
-  src->init_source = our_init_source;
-  src->fill_input_buffer = our_fill_input_buffer;
-  src->skip_input_data = our_skip_input_data;
+  src->init_source = our_common_init_source;
+  src->fill_input_buffer = our_memory_fill_input_buffer;
+  src->skip_input_data = our_memory_skip_input_data;
   src->resync_to_restart = jpeg_resync_to_restart_wrapper; /* Use
default method.  */
-  src->term_source = our_term_source;
+  src->term_source = our_common_term_source;
   src->bytes_in_buffer = len;
   src->next_input_byte = data;
@@ -6519,4 +6516,118 @@


+struct jpeg_stdio_mgr
+{
+  struct jpeg_source_mgr mgr;
+  boolean finished;
+  FILE *file;
+  JOCTET *buffer;
+};
+
+
+/* Size of buffer to read JPEG from file.
+   Not too big, as we want to use alloc_small.  */
+#define JPEG_STDIO_BUFFER_SIZE 8192
+
+
+/* Fill input buffer method for JPEG data source manager.  Called
+   whenever more data is needed.  The data is read from a FILE *.  */
+
+static boolean
+our_stdio_fill_input_buffer (cinfo)
+     j_decompress_ptr cinfo;
+{
+  struct jpeg_stdio_mgr *src;
+
+  src = (struct jpeg_stdio_mgr *) cinfo->src;
+  if (!src->finished)
+    {
+      size_t bytes;
+
+      bytes = fread (src->buffer, 1, JPEG_STDIO_BUFFER_SIZE, src->file);
+      if (bytes > 0)
+        src->mgr.bytes_in_buffer = bytes;
+      else
+        {
+          WARNMS (cinfo, JWRN_JPEG_EOF);
+          src->finished = 1;
+          src->buffer[0] = (JOCTET) 0xFF;
+          src->buffer[1] = (JOCTET) JPEG_EOI;
+          src->mgr.bytes_in_buffer = 2;
+        }
+      src->mgr.next_input_byte = src->buffer;
+    }
+
+  return 1;
+}
+
+
+/* Method to skip over NUM_BYTES bytes in the image data.  CINFO->src
+   is the JPEG data source manager.  */
+
+static void
+our_stdio_skip_input_data (cinfo, num_bytes)
+     j_decompress_ptr cinfo;
+     long num_bytes;
+{
+  struct jpeg_stdio_mgr *src;
+  src = (struct jpeg_stdio_mgr *) cinfo->src;
+
+  while (num_bytes > 0 && !src->finished)
+    {
+      if (num_bytes <= src->mgr.bytes_in_buffer)
+        {
+          src->mgr.bytes_in_buffer -= num_bytes;
+          src->mgr.next_input_byte += num_bytes;
+          break;
+        }
+      else
+        {
+          num_bytes -= src->mgr.bytes_in_buffer;
+          src->mgr.bytes_in_buffer = 0;
+          src->mgr.next_input_byte = NULL;
+
+          our_stdio_fill_input_buffer (cinfo);
+        }
+    }
+}
+
+
+/* Set up the JPEG lib for reading an image from a FILE *.
+   CINFO is the decompression info structure created for
+   reading the image.  */
+
+static void
+jpeg_file_src (cinfo, fp)
+     j_decompress_ptr cinfo;
+     FILE *fp;
+{
+  struct jpeg_stdio_mgr *src;
+
+  if (cinfo->src != NULL)
+      src = (struct jpeg_stdio_mgr *) cinfo->src;
+  else
+    {
+      /* First time for this JPEG object?  */
+      cinfo->src = (struct jpeg_source_mgr *)
+        (*cinfo->mem->alloc_small) ((j_common_ptr) cinfo, JPOOL_PERMANENT,
+                                    sizeof (struct jpeg_stdio_mgr));
+      src = (struct jpeg_stdio_mgr *) cinfo->src;
+      src->buffer = (JOCTET *)
+          (*cinfo->mem->alloc_small) ((j_common_ptr) cinfo, JPOOL_PERMANENT,
+                                      JPEG_STDIO_BUFFER_SIZE);
+    }
+
+  src->file = fp;
+  src->finished = 0;
+  src->mgr.init_source = our_common_init_source;
+  src->mgr.fill_input_buffer = our_stdio_fill_input_buffer;
+  src->mgr.skip_input_data = our_stdio_skip_input_data;
+  src->mgr.resync_to_restart = jpeg_resync_to_restart_wrapper; /* Use
default method.  */
+  src->mgr.term_source = our_common_term_source;
+  src->mgr.bytes_in_buffer = 0;
+  src->mgr.next_input_byte = NULL;
+}
+
+
  /* Load image IMG for use on frame F.  Patterned after example.c
    from the JPEG lib.  */
@@ -6602,5 +6713,5 @@

   if (NILP (specified_data))
-    fn_jpeg_stdio_src (&cinfo, (FILE *) fp);
+    jpeg_file_src (&cinfo, (FILE *) fp);
   else
     jpeg_memory_src (&cinfo, SDATA (specified_data),