bug#17790: GIFlib-5.1.0 or former conditional

[Eli Zaretskii]

> From: Andreas Schwab <schwab@suse.de>
> Date: Tue, 17 Jun 2014 09:05:00 +0200
> Cc: 17790@debbugs.gnu.org
> 
> Makoto Fujiwara <makoto@ki.nu> writes:
> 
> > +#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)  
> 
> What about libgif6?  Please remove the extra parens.
> 
> > +  int * return_value_p;
> 
> This is not a predicate, please remove _p.
> 
> > +#endif
> >    /* Before reading entire contents, check the declared image size. */
> >    if (!check_image_size (f, gif->SWidth, gif->SHeight))
> >      {
> >        image_error ("Invalid image size (see `max-image-size')", Qnil, 
> > Qnil);
> > +#if (GIFLIB_MAJOR == 5) && (GIFLIB_MINOR > 0)
> > +      fn_DGifCloseFile (gif, return_value_p);
> > +      return *return_value_p;
> 
> What is the meaning of the return value?  Is it compatible with the
> intented bool value?

No, it isn't.  Also, no other image loading method returns values
specific to some image type to its caller, so I don't think it's a
good idea.

Moreover, in most of the places where we call DGifCloseFile, we don't
really care about any errors it encounters, because we are already in
the context of a more significant error.  So it is better to simply
disregard the errors there.

I propose a slightly different changeset below.  Unless someone sees
some problem with it, I will commit soon.  (I guess this should go to
the release branch, right? Stefan? Glenn?)

Finally, a question to Eric: can we rely on DGifCloseFile to accept
NULL as the second argument, like the other APIs do?  This is not
documented, but the source clearly says that the behavior is the same
as with the other APIs.

Here's the suggested patch:

=== modified file 'src/image.c'
--- src/image.c 2014-05-04 18:51:32 +0000
+++ src/image.c 2014-06-17 14:50:54 +0000
@@ -7255,7 +7255,11 @@ gif_image_p (Lisp_Object object)
 #ifdef WINDOWSNT
 
 /* GIF library details.  */
+#if 5 < GIFLIB_MAJOR + (1 <= GIFLIB_MINOR)
+DEF_IMGLIB_FN (int, DGifCloseFile, (GifFileType *, int *));
+#else
 DEF_IMGLIB_FN (int, DGifCloseFile, (GifFileType *));
+#endif
 DEF_IMGLIB_FN (int, DGifSlurp, (GifFileType *));
 #if GIFLIB_MAJOR < 5
 DEF_IMGLIB_FN (GifFileType *, DGifOpen, (void *, InputFunc));
@@ -7325,6 +7329,19 @@ gif_read_from_memory (GifFileType *file,
   return len;
 }
 
+static int
+gif_close (GifFileType *gif, int *err)
+{
+  int retval;
+
+#if 5 < GIFLIB_MAJOR + (1 <= GIFLIB_MINOR)
+  retval = fn_DGifCloseFile (gif, err);
+#else
+  retval = fn_DGifCloseFile (gif);
+  if (err)
+    *err = gif->Error;
+#endif
+}
 
 /* Load GIF image IMG for use on frame F.  Value is true if
    successful.  */
@@ -7419,7 +7436,7 @@ gif_load (struct frame *f, struct image 
   if (!check_image_size (f, gif->SWidth, gif->SHeight))
     {
       image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
-      fn_DGifCloseFile (gif);
+      gif_close (gif, NULL);
       return 0;
     }
 
@@ -7428,7 +7445,7 @@ gif_load (struct frame *f, struct image 
   if (rc == GIF_ERROR || gif->ImageCount <= 0)
     {
       image_error ("Error reading `%s'", img->spec, Qnil);
-      fn_DGifCloseFile (gif);
+      gif_close (gif, NULL);
       return 0;
     }
 
@@ -7440,7 +7457,7 @@ gif_load (struct frame *f, struct image 
       {
        image_error ("Invalid image number `%s' in image `%s'",
                     image_number, img->spec);
-       fn_DGifCloseFile (gif);
+       gif_close (gif, NULL);
        return 0;
       }
   }
@@ -7458,7 +7475,7 @@ gif_load (struct frame *f, struct image 
   if (!check_image_size (f, width, height))
     {
       image_error ("Invalid image size (see `max-image-size')", Qnil, Qnil);
-      fn_DGifCloseFile (gif);
+      gif_close (gif, NULL);
       return 0;
     }
 
@@ -7476,7 +7493,7 @@ gif_load (struct frame *f, struct image 
             && 0 <= subimg_left && subimg_left <= width - subimg_width))
        {
          image_error ("Subimage does not fit in image", Qnil, Qnil);
-         fn_DGifCloseFile (gif);
+         gif_close (gif, NULL);
          return 0;
        }
     }
@@ -7484,7 +7501,7 @@ gif_load (struct frame *f, struct image 
   /* Create the X image and pixmap.  */
   if (!image_create_x_image_and_pixmap (f, img, width, height, 0, &ximg, 0))
     {
-      fn_DGifCloseFile (gif);
+      gif_close (gif, NULL);
       return 0;
     }
 
@@ -7655,7 +7672,18 @@ gif_load (struct frame *f, struct image 
                            Fcons (make_number (gif->ImageCount),
                                   img->lisp_data));
 
-  fn_DGifCloseFile (gif);
+  if (gif_close (gif, &gif_err) == GIF_ERROR)
+    {
+#if 5 <= GIFLIB_MAJOR
+      char *error_text = fn_GifErrorString (gif_err);
+
+      if (error_text)
+       image_error ("Error closing `%s': %s",
+                    img->spec, build_string (error_text));
+#else
+      image_error ("Error closing `%s'", img->spec);
+#endif
+    }
 
   /* Maybe fill in the background field while we have ximg handy. */
   if (NILP (image_spec_value (img->spec, QCbackground, NULL)))

=== modified file 'lisp/term/w32-win.el'
--- lisp/term/w32-win.el        2014-04-11 07:02:28 +0000
+++ lisp/term/w32-win.el        2014-06-17 14:51:18 +0000
@@ -251,13 +251,16 @@ This returns an error if any Emacs frame
        ;; libraries according to the version of giflib we were
        ;; compiled against.  (If we were compiled without GIF support,
        ;; libgif-version's value is -1.)
-       (if (>= libgif-version 50000)
-          ;; Yes, giflib 5.x uses 6 as the major version of the API,
-          ;; thus "libgif-6.dll" below (giflib 4.x used 5 as the
-          ;; major API version).
-          ;; giflib5.dll is from the lua-files project.
-          '(gif "libgif-6.dll" "giflib5.dll")
-        '(gif "libgif-5.dll" "giflib4.dll" "libungif4.dll" "libungif.dll"))
+       (if (>= libgif-version 50100)
+          ;; Yes, giflib 5.0 uses 6 as the major version of the API,
+          ;; and giflib 5.1 uses 7, thus "libgif-7.dll" and
+          ;; "libgif-6.dll" below (giflib 4.x used 5 as the major API
+          ;; version).  giflib5.dll is from the lua-files project,
+          ;; and gif.dll is from luapower.
+          '(gif "libgif-7.dll")
+        (if (>= libgif-version 50000)
+            '(gif "libgif-6.dll" "giflib5.dll" "gif.dll")
+        '(gif "libgif-5.dll" "giflib4.dll" "libungif4.dll" "libungif.dll")))
        '(svg "librsvg-2-2.dll")
        '(gdk-pixbuf "libgdk_pixbuf-2.0-0.dll")
        '(glib "libglib-2.0-0.dll")