Changes to GIF support:
authorTony Cook <tony@develop=help.com>
Thu, 31 Aug 2006 13:16:45 +0000 (13:16 +0000)
committerTony Cook <tony@develop=help.com>
Thu, 31 Aug 2006 13:16:45 +0000 (13:16 +0000)
- 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.

Makefile.PL
gif.c
t/t105gif.t

index b133886..28e2fb7 100644 (file)
@@ -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 84b51bf..792b840 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 
index 2c8175f..b6f7366 100644 (file)
@@ -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 {