Changes to GIF support:

- the Nescape loop extension block is written if gif_loop is set.
This requires libungif/libgif 4.1.3 or later.

Resolves: http://rt.cpan.org/Ticket/Display.html?id=21185

- we now set the GIF header depending on the features enabled in the
GIF.  Since we don't do configure type probes for bugs we have to
enable this based on the gif_lib.h version number, it's enabled for
4.1.0 and later, but requires 4.1.3 and later (maybe 4.1.3) to run
without crashing.

You can avoid this crash by passing --nogifsetversion to Makefile.PL,
or by installing a non-buggy libungif/libgif, if there is such a
thing.

diff --git a/Makefile.PL b/Makefile.PL
index b1338860044a0ecd24d4e56954610d2eb6a01a15..28e2fb7fa0967ee5d053c0d5612af122ec8d7230 100644 (file)
--- a/Makefile.PL
+++ b/Makefile.PL
@@ -38,6 +38,7 @@ my @incpaths; # places to look for headers
 my @libpaths; # places to look for libraries
 my $noprobe; # non-zero to disable newer probes
 my $noexif; # non-zero to disable EXIF parsing of JPEGs
+my $no_gif_set_version; # disable calling EGifSetGifVersion
 GetOptions("help" => \$help,
            "enable=s" => \@enable,
            "disable=s" => \@disable,
@@ -46,7 +47,8 @@ GetOptions("help" => \$help,
            "noprobe" => \$noprobe,
            "verbose|v" => \$VERBOSE,
            "nolog" => \$NOLOG,
-          "noexif" => \$noexif);
+          "noexif" => \$noexif,
+           "nogifsetversion" => \$no_gif_set_version);
 
 if ($VERBOSE) { 
   print "Verbose mode\n"; 
@@ -304,6 +306,8 @@ sub gifcheck {
 
   push @defines, [ IM_GIFMAJOR => $major, "Parsed giflib version" ];
   push @defines, [ IM_GIFMINOR => $minor ];
+  push @defines, [ IM_NO_SET_GIF_VERSION => 1, "Disable EGifSetGifVersion" ]
+    if $no_gif_set_version;
 }
 
 

diff --git a/gif.c b/gif.c
index 84b51bf17bff752cf1cc83317faccc1a44d75233..792b840759a45b9c7fa35e25410f7109be89f7d8 100644 (file)
--- a/gif.c
+++ b/gif.c
@@ -1369,9 +1369,11 @@ static int do_comments(GifFileType *gf, i_img *img) {
 
 Internal.  Add the Netscape2.0 loop extension block, if requested.
 
-The code for this function is currently "#if 0"ed out since the giflib
-extension writing code currently doesn't seem to support writing
-application extension blocks.
+Giflib/libungif prior to 4.1.1 didn't support writing application
+extension blocks, so we don't attempt to write them for older versions.
+
+Giflib/libungif prior to 4.1.3 used the wrong write mechanism when
+writing extension blocks so that they could only be written to files.
 
 =cut
 */
@@ -1387,13 +1389,13 @@ static int do_ns_loop(GifFileType *gf, i_img *img)
      If giflib's callback interface wasn't broken by default, I'd 
      force file writes to use callbacks, but it is broken by default.
   */
-#if 0
   /* yes this was another attempt at supporting the loop extension */
+#if IM_GIFMAJOR == 4 && IM_GIFMINOR >= 1
   int loop_count;
   if (i_tags_get_int(&img->tags, "gif_loop", 0, &loop_count)) {
     unsigned char nsle[12] = "NETSCAPE2.0";
     unsigned char subblock[3];
-    if (EGifPutExtension(gf, 0xFF, 11, nsle) == GIF_ERROR) {
+    if (EGifPutExtensionFirst(gf, APPLICATION_EXT_FUNC_CODE, 11, nsle) == GIF_ERROR) {
       gif_push_error();
       i_push_error(0, "writing loop extension");
       return 0;
@@ -1401,18 +1403,14 @@ static int do_ns_loop(GifFileType *gf, i_img *img)
     subblock[0] = 1;
     subblock[1] = loop_count % 256;
     subblock[2] = loop_count / 256;
-    if (EGifPutExtension(gf, 0, 3, subblock) == GIF_ERROR) {
+    if (EGifPutExtensionLast(gf, APPLICATION_EXT_FUNC_CODE, 3, subblock) == GIF_ERROR) {
       gif_push_error();
       i_push_error(0, "writing loop extention sub-block");
       return 0;
     }
-    if (EGifPutExtension(gf, 0, 0, subblock) == GIF_ERROR) {
-      gif_push_error();
-      i_push_error(0, "writing loop extension terminator");
-      return 0;
-    }
   }
 #endif
+
   return 1;
 }
 
@@ -1487,24 +1485,53 @@ which is very broken.
 Failing to set the correct GIF version doesn't seem to cause a problem
 with readers.
 
+Modern versions (4.1.4 anyway) of giflib/libungif handle
+EGifSetGifVersion correctly.
+
+If t/t105gif.t crashes here then run Makefile.PL with
+--nogifsetversion, eg.:
+
+  perl Makefile.PL --nogifsetversion
+
+or install a less buggy giflib.
+
 =cut
 */
 
 static void gif_set_version(i_quantize *quant, i_img **imgs, int count) {
-  /* the following crashed giflib
-     the EGifSetGifVersion() is seriously borked in giflib
-     it's less borked in the ungiflib beta, but we don't have a mechanism
-     to distinguish them
-     Needs to be updated to support tags.
-     if (opts->delay_count
-     || opts->user_input_count
-     || opts->disposal_count
-     || opts->loop_count
-     || quant->transp != tr_none)
+#if (IM_GIFMAJOR >= 4 || IM_GIFMAJOR == 4 && IM_GIFMINOR >= 1) \
+       && !defined(IM_NO_SET_GIF_VERSION)
+  int need_89a = 0;
+  int temp;
+  int i;
+
+  if (quant->transp != tr_none)
+    need_89a = 1;
+  else {
+    for (i = 0; i < count; ++i) {
+      if (i_tags_get_int(&imgs[i]->tags, "gif_delay", 0, &temp)) { 
+        need_89a = 1; 
+        break;
+      }
+      if (i_tags_get_int(&imgs[i]->tags, "gif_user_input", 0, &temp) && temp) {
+        need_89a = 1; 
+        break;
+      }
+      if (i_tags_get_int(&imgs[i]->tags, "gif_disposal", 0, &temp)) {
+        need_89a = 1;
+        break;
+      }
+      if (i_tags_get_int(&imgs[i]->tags, "gif_loop", 0, &temp)) {
+        need_89a = 1;
+        break;
+      }
+    }
+  }
+  if (need_89a)
      EGifSetGifVersion("89a");
-     else
+  else
      EGifSetGifVersion("87a");
-  */
+#endif
 }
 
 static int 

diff --git a/t/t105gif.t b/t/t105gif.t
index 2c8175feeb628d77cad692b7260c44b99db30a94..b6f7366f2177c7926ddced02330e572eed2bc9bd 100644 (file)
--- a/t/t105gif.t
+++ b/t/t105gif.t
@@ -1,8 +1,18 @@
 #!perl -w
+=pod
+
+IF THIS TEST CRASHES
+
+Giflib/libungif have a long history of bugs, so if this script crashes
+and you aren't running version 4.1.4 of giflib or libungif then
+UPGRADE.
+
+=cut
+
 use strict;
 $|=1;
 use lib 't';
-use Test::More tests => 107;
+use Test::More tests => 113;
 use Imager qw(:all);
 BEGIN { require "t/testtools.pl"; }
 use Carp 'confess';
@@ -636,6 +646,26 @@ EOS
     cmp_ok($res->errstr, "=~", 'page 3 not found',
           "check error message");
   }
+  {
+    # testing writing the loop extension
+    my $im1 = Imager->new(xsize => 100, ysize => 100);
+    $im1->box(filled => 1, color => '#FF0000');
+    my $im2 = Imager->new(xsize => 100, ysize => 100);
+    $im2->box(filled => 1, color => '#00FF00');
+    ok(Imager->write_multi({
+                            gif_loop => 5, 
+                            gif_delay => 50, 
+                            file => 'testout/t105loop.gif'
+                           }, $im1, $im2),
+       "write with loop extension");
+
+    my @im = Imager->read_multi(file => 'testout/t105loop.gif');
+    is(@im, 2, "read loop images back");
+    is($im[0]->tags(name => 'gif_loop'), 5, "first loop read back");
+    is($im[1]->tags(name => 'gif_loop'), 5, "second loop read back");
+    is($im[0]->tags(name => 'gif_delay'), 50, "first delay read back");
+    is($im[1]->tags(name => 'gif_delay'), 50, "second delay read back");
+  }
 }
 
 sub test_readgif_cb {