unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Problem with image libraries on Windows (reprise)
@ 2005-06-15 10:34 Juanma Barranquero
  2005-06-15 10:34 ` Juanma Barranquero
                   ` (3 more replies)
  0 siblings, 4 replies; 7+ messages in thread
From: Juanma Barranquero @ 2005-06-15 10:34 UTC (permalink / raw)


I've already discussed this on the "Problem with library images on
Windows (again)" [yeah, there's a word reversal on the subject that I
didn't notice till now], but I'm doing a reprise there so I can get
feedback and fix the problem ASAP.

Problem:
  Image functions that load JPEG and PNG images from a FILE * can
crash Emacs on Windows.  In particular:
    - MSVC builds of Emacs crash when using either MinGW or MSVC
builds of the libraries
    - MinGW builds of Emacs crash when using MSVC builds of the libraries

Cause:
  Documentation for the JPEG library states: " Microsoft C cannot pass
file pointers between applications and DLLs. (See Microsoft Knowledge
Base, PSS ID Number Q50336.) " I've been unable to find article
Q50336, probably because it is very old, but the description of the
problem seems to fit what I find. Moreover, tracing calls all the way
through NTDLL shows that the Windows library is receiving an non-valid
FILE * (but the image DLLs are receiving a valid one, so it is not a
problem in Emacs).

Fix:
  Both libraries (jpeglib and libpng) allow setting up custom reading
functions. This allows the libraries to call back into program space,
where the FILE * can be safely used. The fix for libpng is almost
trivial, just adding one tiny function; jpeglib has a slightly more
complex protocol and requires defining a bunch of functions for
initializing the data source, filling the temporary buffer and
skipping unwanted input data.

Problems:
  - With the PNG fix, there should be none. My patch changes the
method for reading from FILE * for all platforms, because it seems
better to have one single code and the callback method should work
fine everywhere. However, I can make this change conditional to
HAVE_NTGUI if deemed necessary.
  - With the JPEG fix: none technical. However, the code I've used is
lifted from jdatasrc.c on the jpeglib distribution, so there could be
legal problems. Datapoints:
   - The code is pretty much jdatasrc.c, with only a few names changed
and little more.
   - It is similar to the already installed code for jpeg_memory_src,
installed by Gerd on 1999-12-31.
   - Documentation for the problem I'm trying to fix explicitly
states: "So jdatasrc.c and
  jdatadst.c don't work if you open a file in your application and then pass
  the pointer to the DLL.  One workaround is to make jdatasrc.c/jdatadst.c
  part of your main application rather than part of the DLL."
   - Jpeglib docs have this bit of legalese: "(1) If any part of the
source code for this software is distributed, then this README file
must be included, with this copyright and no-warranty notice
unaltered; and any additions, deletions, or changes to the original
files must be clearly indicated in accompanying documentation."

Actions:
  I'm sending the two fixes as separate patches, so people can give
feedback on them separately.
  - With regard to the PNG patch, I'd like an answer about making it
the only way to read PNG image from files, or using it only for
Windows.
  - On the JPEG front: Richard should make a comment about the legal
issues. If the code cannot be included as is, I'd like for someone
who's not been "tainted" by looking at jdatasrc.c to reimplement it.

The patches follow in the next messages on this thread.

Thanks,

                    /L/e/k/t/u

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Problem with image libraries on Windows (reprise)
  2005-06-15 10:34 Problem with image libraries on Windows (reprise) Juanma Barranquero
@ 2005-06-15 10:34 ` Juanma Barranquero
  2005-06-15 10:35 ` Juanma Barranquero
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 7+ messages in thread
From: Juanma Barranquero @ 2005-06-15 10:34 UTC (permalink / raw)


The PNG patch:

Index: src/ChangeLog
===================================================================
RCS file: /cvsroot/emacs/emacs/src/ChangeLog,v
retrieving revision 1.4462
diff -u -2 -r1.4462 ChangeLog
--- src/ChangeLog	15 Jun 2005 02:30:03 -0000	1.4462
+++ src/ChangeLog	15 Jun 2005 09:16:33 -0000
@@ -1,2 +1,9 @@
+2005-06-15  Juanma Barranquero  <lekktu@gmail.com>
+
+	* image.c (fn_png_init_io): Don't define it.
+	(init_png_functions) [HAVE_NTGUI]: Don't initialize fn_png_init_io.
+	(png_read_from_file): New function, based on png_read_from_memory.
+	(png_load): Use it, instead of fn_png_init_io.
+
 2005-06-15  YAMAMOTO Mitsuharu  <mituharu@math.s.chiba-u.ac.jp>
 
Index: src/image.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/image.c,v
retrieving revision 1.27
diff -u -2 -r1.27 image.c
--- src/image.c	11 Jun 2005 16:24:36 -0000	1.27
+++ src/image.c	15 Jun 2005 09:16:25 -0000
@@ -5632,5 +5632,4 @@
 DEF_IMGLIB_FN (png_destroy_read_struct);
 DEF_IMGLIB_FN (png_set_read_fn);
-DEF_IMGLIB_FN (png_init_io);
 DEF_IMGLIB_FN (png_set_sig_bytes);
 DEF_IMGLIB_FN (png_read_info);
@@ -5664,5 +5663,4 @@
   LOAD_IMGLIB_FN (library, png_destroy_read_struct);
   LOAD_IMGLIB_FN (library, png_set_read_fn);
-  LOAD_IMGLIB_FN (library, png_init_io);
   LOAD_IMGLIB_FN (library, png_set_sig_bytes);
   LOAD_IMGLIB_FN (library, png_read_info);
@@ -5690,5 +5688,4 @@
 #define fn_png_destroy_read_struct	png_destroy_read_struct
 #define fn_png_set_read_fn		png_set_read_fn
-#define fn_png_init_io			png_init_io
 #define fn_png_set_sig_bytes		png_set_sig_bytes
 #define fn_png_read_info		png_read_info
@@ -5763,4 +5760,21 @@
 
 
+/* Function set as reader function when reading PNG image from a file.
+   PNG_PTR is a pointer to the PNG control structure.  Copy LENGTH
+   bytes from the input to DATA.  */
+
+static void
+png_read_from_file (png_ptr, data, length)
+     png_structp png_ptr;
+     png_bytep data;
+     png_size_t length;
+{
+  FILE *fp = (FILE *) fn_png_get_io_ptr (png_ptr);
+
+  if (fread (data, 1, length, fp) < length)
+    fn_png_error (png_ptr, "Read error");
+}
+
+
 /* Load PNG image IMG for use on frame F.  Value is non-zero if
    successful.  */
@@ -5896,5 +5910,5 @@
     fn_png_set_read_fn (png_ptr, (void *) &tbr, png_read_from_memory);
   else
-    fn_png_init_io (png_ptr, fp);
+    fn_png_set_read_fn (png_ptr, (void *) fp, png_read_from_file);
 
   fn_png_set_sig_bytes (png_ptr, sizeof sig);

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Problem with image libraries on Windows (reprise)
  2005-06-15 10:34 Problem with image libraries on Windows (reprise) Juanma Barranquero
  2005-06-15 10:34 ` Juanma Barranquero
@ 2005-06-15 10:35 ` Juanma Barranquero
  2005-06-16  3:46 ` David Hunter
  2005-06-22 13:18 ` Juanma Barranquero
  3 siblings, 0 replies; 7+ messages in thread
From: Juanma Barranquero @ 2005-06-15 10:35 UTC (permalink / raw)


The JPEG patch:

Index: src/ChangeLog
===================================================================
RCS file: /cvsroot/emacs/emacs/src/ChangeLog,v
retrieving revision 1.4462
diff -u -2 -r1.4462 ChangeLog
--- src/ChangeLog	15 Jun 2005 02:30:03 -0000	1.4462
+++ src/ChangeLog	15 Jun 2005 09:32:35 -0000
@@ -1,2 +1,19 @@
+2005-06-15  Juanma Barranquero  <lekktu@gmail.com>
+
+	* image.c (fn_jpeg_stdio_src): Don't define it.
+	(init_jpeg_functions) [HAVE_NTGUI]: Don't initialize
+	fn_jpeg_stdio_src.
+	(our_common_term_source): Rename from our_term_source.
+	(our_memory_init_source): Rename from our_init_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 renamed functions.
+	(my_jpeg_file_src): New struct.
+	(my_jpeg_file_ptr): New typedef.
+	(JPEG_INPUT_BUF_SIZE): New constant.
+	(our_file_init_source, our_file_fill_input_buffer)
+	(our_file_skip_input_data, jpeg_file_src): New functions.
+	(jpeg_load): Use jpeg_file_src instead of fn_jpeg_stdio_src.
+
 2005-06-15  YAMAMOTO Mitsuharu  <mituharu@math.s.chiba-u.ac.jp>
 
Index: src/image.c
===================================================================
RCS file: /cvsroot/emacs/emacs/src/image.c,v
retrieving revision 1.27
diff -u -2 -r1.27 image.c
--- src/image.c	11 Jun 2005 16:24:36 -0000	1.27
+++ src/image.c	15 Jun 2005 09:25:19 -0000
@@ -6295,5 +6295,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);
@@ -6311,5 +6310,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);
@@ -6337,5 +6335,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
@@ -6359,4 +6356,15 @@
 
 
+/* Method to terminate data source.  Called by
+   jpeg_finish_decompress() after all data has been processed.
+   Common to memory and file source managers.  */
+
+static void
+our_common_term_source (cinfo)
+     j_decompress_ptr cinfo;
+{
+}
+
+
 /* Init source method for JPEG data source manager.  Called by
    jpeg_read_header() before any data is actually read.  See
@@ -6364,5 +6372,5 @@
 
 static void
-our_init_source (cinfo)
+our_memory_init_source (cinfo)
      j_decompress_ptr cinfo;
 {
@@ -6375,5 +6383,5 @@
 
 static boolean
-our_fill_input_buffer (cinfo)
+our_memory_fill_input_buffer (cinfo)
      j_decompress_ptr cinfo;
 {
@@ -6395,5 +6403,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;
@@ -6412,14 +6420,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
@@ -6445,9 +6443,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_memory_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;
@@ -6455,4 +6453,136 @@
 
 
+/* JPEG data source manager setup for JPEG images read from a file.
+   Heavily based on jdatasrc.c from the JPEG library.  */
+
+struct my_jpeg_file_src {
+  struct jpeg_source_mgr pub;   /* public fields */
+
+  FILE *infile;                 /* source stream */
+  JOCTET *buffer;               /* start of buffer */
+  boolean start_of_file;        /* have we gotten any data yet? */
+};
+
+typedef struct my_jpeg_file_src *my_jpeg_file_ptr;
+
+#define JPEG_INPUT_BUF_SIZE 4096    /* choose an efficiently fread'able size */
+
+
+/* Init source method for JPEG data source manager.
+   Called by jpeg_read_header() before any data is actually read.  */
+
+static void
+our_file_init_source (j_decompress_ptr cinfo)
+{
+  my_jpeg_file_ptr src = (my_jpeg_file_ptr) cinfo->src;
+
+  /* We reset the empty-input-file flag for each image,
+   * but we don't clear the input buffer.
+   * This is correct behavior for reading a series of images from one source.
+   */
+  src->start_of_file = 1;
+}
+
+
+/* Fill input buffer method for JPEG data source manager.
+   Called whenever more data is needed.  */
+
+static boolean
+our_file_fill_input_buffer (j_decompress_ptr cinfo)
+{
+  my_jpeg_file_ptr src = (my_jpeg_file_ptr) cinfo->src;
+  size_t nbytes;
+
+  nbytes = fread (src->buffer, 1, JPEG_INPUT_BUF_SIZE, src->infile);
+
+  if (nbytes <= 0) {
+    if (src->start_of_file)	/* Treat empty input file as fatal error */
+      ERREXIT (cinfo, JERR_INPUT_EMPTY);
+    WARNMS (cinfo, JWRN_JPEG_EOF);
+    /* Insert a fake EOI marker */
+    src->buffer[0] = (JOCTET) 0xFF;
+    src->buffer[1] = (JOCTET) JPEG_EOI;
+    nbytes = 2;
+  }
+
+  src->pub.next_input_byte = src->buffer;
+  src->pub.bytes_in_buffer = nbytes;
+  src->start_of_file = 0;
+
+  return 1;
+}
+
+
+/* Method to skip over NUM_BYTES bytes in the image data.
+   CINFO->src is the JPEG data source manager.  */
+
+static void
+our_file_skip_input_data (j_decompress_ptr cinfo, long num_bytes)
+{
+  my_jpeg_file_ptr src = (my_jpeg_file_ptr) cinfo->src;
+
+  /* Just a dumb implementation for now.  Could use fseek() except
+   * it doesn't work on pipes.	Not clear that being smart is worth
+   * any trouble anyway --- large skips are infrequent.
+   */
+  if (num_bytes > 0)
+    {
+      while (num_bytes > (long) src->pub.bytes_in_buffer)
+        {
+          num_bytes -= (long) src->pub.bytes_in_buffer;
+          (void) our_file_fill_input_buffer (cinfo);
+          /* note we assume that fill_file_input_buffer will never
return FALSE,
+           * so suspension need not be handled.
+           */
+        }
+      src->pub.next_input_byte += (size_t) num_bytes;
+      src->pub.bytes_in_buffer -= (size_t) num_bytes;
+    }
+}
+
+
+/* Set up the JPEG lib for reading an image from a FILE * via
+   a customized function, thus avoiding FILE * conflicts between
+   different builds of the image library.  CINFO is the
+   decompression info structure created for reading the image.  */
+
+static void
+jpeg_file_src (cinfo, infile)
+     j_decompress_ptr cinfo;
+     FILE *infile;
+{
+    my_jpeg_file_ptr src;
+
+  /* The source object and input buffer are made permanent so that a series
+   * of JPEG images can be read from the same file by calling jpeg_stdio_src
+   * only before the first one.	 (If we discarded the buffer at the end of
+   * one image, we'd likely lose the start of the next one.)
+   * This makes it unsafe to use this manager and a different source
+   * manager serially with the same JPEG object.  Caveat programmer.
+   */
+    if (cinfo->src == NULL)
+      {
+        /* first time for this JPEG object? */
+        cinfo->src = (struct jpeg_source_mgr *)
+          (*cinfo->mem->alloc_small) ((j_common_ptr) cinfo, JPOOL_PERMANENT,
+                                      sizeof (struct my_jpeg_file_src));
+        src = (my_jpeg_file_ptr) cinfo->src;
+        src->buffer = (JOCTET *)
+          (*cinfo->mem->alloc_small) ((j_common_ptr) cinfo, JPOOL_PERMANENT,
+                                      JPEG_INPUT_BUF_SIZE * sizeof (JOCTET));
+      }
+
+    src = (my_jpeg_file_ptr) cinfo->src;
+    src->pub.init_source = our_file_init_source;
+    src->pub.fill_input_buffer = our_file_fill_input_buffer;
+    src->pub.skip_input_data = our_file_skip_input_data;
+    src->pub.resync_to_restart = jpeg_resync_to_restart_wrapper; /*
use default method */
+    src->pub.term_source = our_common_term_source;
+    src->infile = infile;
+    src->pub.bytes_in_buffer = 0; /* forces fill_input_buffer on first read */
+    src->pub.next_input_byte = NULL; /* until buffer loaded */
+}
+
+
 /* Load image IMG for use on frame F.  Patterned after example.c
    from the JPEG lib.  */
@@ -6538,5 +6668,5 @@
 
   if (NILP (specified_data))
-    fn_jpeg_stdio_src (&cinfo, (FILE *) fp);
+    jpeg_file_src (&cinfo, fp);
   else
     jpeg_memory_src (&cinfo, SDATA (specified_data),

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Problem with image libraries on Windows (reprise)
  2005-06-15 10:34 Problem with image libraries on Windows (reprise) Juanma Barranquero
  2005-06-15 10:34 ` Juanma Barranquero
  2005-06-15 10:35 ` Juanma Barranquero
@ 2005-06-16  3:46 ` David Hunter
  2005-06-16  8:39   ` Juanma Barranquero
  2005-06-22 13:18 ` Juanma Barranquero
  3 siblings, 1 reply; 7+ messages in thread
From: David Hunter @ 2005-06-16  3:46 UTC (permalink / raw)
  Cc: emacs-devel

Juanma Barranquero wrote:
> Cause:
>   Documentation for the JPEG library states: " Microsoft C cannot pass
> file pointers between applications and DLLs. (See Microsoft Knowledge
> Base, PSS ID Number Q50336.) " I've been unable to find article
> Q50336, probably because it is very old, but the description of the
> problem seems to fit what I find. Moreover, tracing calls all the way
> through NTDLL shows that the Windows library is receiving an non-valid
> FILE * (but the image DLLs are receiving a valid one, so it is not a
> problem in Emacs).

I don't know about that particular KB article, but the issue still applies to recent MS Visual C runtimes (CRT).  In short, FILE pointers are valid only within the instance of the CRT library that created them.  As long as your application and all dependent libraries dynamically link to the CRT (MSVCRT.DLL), then all application and library code can share FILE pointers.  However, if an application or library (module) statically links to a CRT (LIBC[MT].LIB), then that module must keep its FILE pointers to itself, as no other CRT library (static or dynamic) will be able to use them.

This issue is not limited to FILE pointers, but also extends to include most CRT resources, including memory (malloc/free).  You must not call one CRT's malloc and pass the block to another CRT's free; it won't work.

I believe this sharing issue is also encountered if you attempt to share FILE pointers between the retail (MSVCRT.DLL) and debug (MSVCRTD.DLL) libraries.

Additionally, due to the release of Visual C++ .NET, there is a whole new set of .NET CRT libraries (e.g., MSVCR71.DLL).  The dynamic CRT library used by a given module depends on the version of C++ used to build it.  It is possible to have linked all modules to a dynamic CRT, and still the process may end up loading both old and .NET CRTs to satisfy modules built on different C++ versions.  I have no firsthand knowledge with sharing FILE pointers between the old CRTs and these new .NET CRTs, but I think Microsoft does say this is bad:

http://msdn.microsoft.com/library/default.asp?url=/library/en-us/vclib/html/_crt_c_run.2d.time_libraries.asp

The compiler flags that control CRT linking are given in the above article.

The moral of this story:  Encapsulate, obfuscate, and hide CRT resources.

-Dave

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Problem with image libraries on Windows (reprise)
  2005-06-16  3:46 ` David Hunter
@ 2005-06-16  8:39   ` Juanma Barranquero
  0 siblings, 0 replies; 7+ messages in thread
From: Juanma Barranquero @ 2005-06-16  8:39 UTC (permalink / raw)
  Cc: emacs-devel

On 6/16/05, David Hunter <hunterd42@comcast.net> wrote:

> As long as your application and all dependent libraries dynamically
> link to the CRT (MSVCRT.DLL), then all application and library code
> can share FILE pointers.  However, if an application or library (module)
> statically links to a CRT (LIBC[MT].LIB), then that module must keep
> its FILE pointers to itself, as no other CRT library (static or dynamic)
> will be able to use them.

Very, very interesting:

  1) MinGW-built Emacs: depends on MSVCRT.DLL
  2) MSVC-built Emacs: doesn't depend on MSVCR*.DLL (it's linked with LIBC.LIB)
  3) Binary tarballs of GnuWin32 image libraries (MinGW): depend on MSVCRT.DLL
  4) My MSVC-built image libraries (.NET 2003): depend on MSVCR71.DLL

So 1) is compatible with 3) and would break with 4). OTOH, 2) should
break with 3) and 4). And that's *exactly* what I'm seeing. Thanks!

> This issue is not limited to FILE pointers, but also extends to include most
> CRT resources, including memory (malloc/free).  You must not call one
> CRT's malloc and pass the block to another CRT's free; it won't work.

Fortunately this is not happening. The image libraries do their own
memory handling.

> The moral of this story:  Encapsulate, obfuscate, and hide CRT resources.

Yeah.

In this case, the moral is: on Windows builds, we *cannot* rely on
passing FILE * to any image library (or any other library whatsoever)
because we cannot guarantee the libraries will use the same CRT. So we
have to go the alternate datasource route (that's what my patches
implement).

Thanks for a illuminating and very informative posting.

-- 
                    /L/e/k/t/u

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Problem with image libraries on Windows (reprise)
  2005-06-15 10:34 Problem with image libraries on Windows (reprise) Juanma Barranquero
                   ` (2 preceding siblings ...)
  2005-06-16  3:46 ` David Hunter
@ 2005-06-22 13:18 ` Juanma Barranquero
  2005-10-05 14:46   ` Juanma Barranquero
  3 siblings, 1 reply; 7+ messages in thread
From: Juanma Barranquero @ 2005-06-22 13:18 UTC (permalink / raw)


I wrote about the image libraries' problems a week ago and I didn't
receive any answers (not a complain, just explaining why am I bringing
it back again).

Please note that this is *not* a new feature, and it is *not* related
to the dynamic loading feature devised for image libraries (though it
depends on Emacs on Windows using DLLs and not statically linked
libraries). These are patches for a bug that can crash any Emacs on
Windows if the user accidentally uses image libraries linked with
different linking options than the Emacs executable.

>   - With regard to the PNG patch, I'd like an answer about making it
> the only way to read PNG image from files, or using it only for
> Windows.

If I heard no objection, I'll install the PNG part of the patch
tomorrow. It'll use the custom read functions in all platforms, not
just Windows.

>   - On the JPEG front: Richard should make a comment about the legal
> issues. If the code cannot be included as is, I'd like for someone
> who's not been "tainted" by looking at jdatasrc.c to reimplement it.

I'm not going to commit to the repository code which could be "legally
unsound". I would really appreciate an answer on this one.

Thanks,

                 /L/e/k/t/u

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: Problem with image libraries on Windows (reprise)
  2005-06-22 13:18 ` Juanma Barranquero
@ 2005-10-05 14:46   ` Juanma Barranquero
  0 siblings, 0 replies; 7+ messages in thread
From: Juanma Barranquero @ 2005-10-05 14:46 UTC (permalink / raw)


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2005-10-05 14:46 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-06-15 10:34 Problem with image libraries on Windows (reprise) Juanma Barranquero
2005-06-15 10:34 ` Juanma Barranquero
2005-06-15 10:35 ` Juanma Barranquero
2005-06-16  3:46 ` David Hunter
2005-06-16  8:39   ` Juanma Barranquero
2005-06-22 13:18 ` Juanma Barranquero
2005-10-05 14:46   ` Juanma Barranquero

Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).