[RT #67912] writing GIFs now always uses the generated (or supplied) colors
authorTony Cook <tony@develop-help.com>
Fri, 27 May 2011 12:45:52 +0000 (22:45 +1000)
committerTony Cook <tony@develop-help.com>
Mon, 30 May 2011 11:07:15 +0000 (21:07 +1000)
Changes
GIF/Changes
GIF/Makefile.PL
GIF/imgif.c
GIF/t/t30fixed.t [new file with mode: 0644]
MANIFEST
quant.c

diff --git a/Changes b/Changes
index d8fa09e..4f54950 100644 (file)
--- a/Changes
+++ b/Changes
@@ -1,6 +1,6 @@
 Imager release history.  Older releases can be found in Changes.old
 
-Imager 0.84
+Imager 0.84 (** update GIF version!)
 ===========
 
  - Imager no longer inherits from Exporter (unless you're running an
@@ -27,6 +27,11 @@ Bug fixes:
    translation.
    https://rt.cpan.org/Ticket/Display.html?id=68508
 
+ - writing a paletted image to GIF wouldn't always use the colors
+   supplied (or generated, eg. via make_colors => "mono"), which was
+   confusing at best.
+   https://rt.cpan.org/Ticket/Display.html?id=67912
+
 Imager 0.83 - 21 May 2011
 ===========
 
index 549cb8d..e23c60e 100644 (file)
@@ -1,3 +1,11 @@
+Imager-File-GIF 0.79
+====================
+
+ - writing a paletted image to GIF wouldn't always use the colors
+   supplied (or generated, eg. via make_colors => "mono"), which was
+   confusing at best.  Requires changes from Imager 0.84.
+   https://rt.cpan.org/Ticket/Display.html?id=67912
+
 Imager-File-GIF 0.78
 ====================
 
index 6b25f2f..884c27c 100644 (file)
@@ -38,7 +38,7 @@ else {
   $opts{TYPEMAPS} = [ Imager::ExtUtils->typemap ];
 
   # Imager required configure through use
-  my @Imager_req = ( Imager => "0.78" );
+  my @Imager_req = ( Imager => "0.84" );
   if ($MM_ver >= 6.46) {
     $opts{META_MERGE} =
       {
index 20b4acd..623a567 100644 (file)
@@ -1229,23 +1229,26 @@ in_palette(i_color *c, i_quantize *quant, int size) {
 }
 
 /*
-=item has_common_palette(imgs, count, quant, want_trans)
+=item has_common_palette(imgs, count, quant)
 
-Tests if all the given images are paletted and have a common palette,
-if they do it builds that palette.
+Tests if all the given images are paletted and their colors are in the
+palette produced.
 
-A possible improvement might be to eliminate unused colors in the
-images palettes.
+Previously this would build a consolidated palette from the source,
+but that meant that if the caller supplied a static palette (or
+specified a fixed palette like "webmap") then we wouldn't be
+quantizing to the caller specified palette.
 
 =cut
 */
+
 static int
-has_common_palette(i_img **imgs, int count, i_quantize *quant, 
-                   int want_trans) {
+has_common_palette(i_img **imgs, int count, i_quantize *quant) {
   int size = quant->mc_count;
   int i;
   int imgn;
   char used[256];
+  int col_count;
 
   /* we try to build a common palette here, if we can manage that, then
      that's the palette we use */
@@ -1277,25 +1280,22 @@ has_common_palette(i_img **imgs, int count, i_quantize *quant,
       memset(used, 1, sizeof(used));
     }
 
-    for (i = 0; i < i_colorcount(imgs[imgn]); ++i) {
+    col_count = i_colorcount(imgs[imgn]);
+    for (i = 0; i < col_count; ++i) {
       i_color c;
       
       i_getcolors(imgs[imgn], i, &c, 1);
       if (used[i]) {
-        if (in_palette(&c, quant, size) < 0) {
-          if (size < quant->mc_size) {
-            quant->mc_colors[size++] = c;
-          }
-          else {
-            /* oops, too many colors */
-            return 0;
-          }
+        if (in_palette(&c, quant, quant->mc_count) < 0) {
+         mm_log((1, "  color not found in palette, no palette shortcut\n"));
+  
+         return 0;
         }
       }
     }
   }
 
-  quant->mc_count = size;
+  mm_log((1, "  all colors found in palette, palette shortcut\n"));
 
   return 1;
 }
@@ -1424,13 +1424,9 @@ i_writegif_low(i_quantize *quant, GifFileType *gf, i_img **imgs, int count) {
       mm_log((2, "  reserving color for transparency\n"));
       --quant->mc_size;
     }
-    if (has_common_palette(glob_imgs, glob_img_count, quant, want_trans)) {
-      glob_paletted = 1;
-    }
-    else {
-      glob_paletted = 0;
-      i_quant_makemap(quant, glob_imgs, glob_img_count);
-    }
+
+    i_quant_makemap(quant, glob_imgs, glob_img_count);
+    glob_paletted = has_common_palette(glob_imgs, glob_img_count, quant);
     glob_color_count = quant->mc_count;
     quant->mc_colors = orig_colors;
   }
@@ -1451,14 +1447,10 @@ i_writegif_low(i_quantize *quant, GifFileType *gf, i_img **imgs, int count) {
   }
   else {
     want_trans = quant->transp != tr_none && imgs[0]->channels == 4;
-    if (has_common_palette(imgs, 1, quant, want_trans)) {
-      colors_paletted = 1;
-    }
-    else {
-      colors_paletted = 0;
-      i_quant_makemap(quant, imgs, 1);
-    }
+    i_quant_makemap(quant, imgs, 1);
+    colors_paletted = has_common_palette(imgs, 1, quant);
   }
+
   if ((map = make_gif_map(quant, imgs[0], want_trans)) == NULL) {
     myfree(glob_colors);
     myfree(localmaps);
@@ -1525,13 +1517,8 @@ i_writegif_low(i_quantize *quant, GifFileType *gf, i_img **imgs, int count) {
          to give room for a transparency colour */
       if (want_trans && quant->mc_size == 256)
         --quant->mc_size;
-      if (has_common_palette(imgs, 1, quant, want_trans)) {
-        colors_paletted = 1;
-      }
-      else {
-        colors_paletted = 0;
-        i_quant_makemap(quant, imgs, 1);
-      }
+      i_quant_makemap(quant, imgs, 1);
+      colors_paletted = has_common_palette(imgs, 1, quant);
       if ((map = make_gif_map(quant, imgs[0], want_trans)) == NULL) {
        myfree(glob_colors);
        myfree(localmaps);
@@ -1639,7 +1626,7 @@ i_writegif_low(i_quantize *quant, GifFileType *gf, i_img **imgs, int count) {
       if (want_trans && quant->mc_size == 256)
        --quant->mc_size;
 
-      if (has_common_palette(imgs+imgn, 1, quant, want_trans)) {
+      if (has_common_palette(imgs+imgn, 1, quant)) {
         result = quant_paletted(quant, imgs[imgn]);
       }
       else {
diff --git a/GIF/t/t30fixed.t b/GIF/t/t30fixed.t
new file mode 100644 (file)
index 0000000..30b6965
--- /dev/null
@@ -0,0 +1,47 @@
+#!perl -w
+use strict;
+use Test::More tests => 9;
+use Imager;
+use Imager::Test qw(test_image);
+
+-d "testout" or mkdir "testout";
+
+Imager->open_log(log => "testout/t30fixed.log");
+
+{
+  # RT 67912
+  # previously, if you tried to write a paletted image to GIF:
+  #  - specified a fixed palette with make_colors => "mono", "web" or "none"
+  #  - there was room for the colors in the image in the rest of the
+  #  palette (or they were found in the generated palette)
+  # the GIF would be written with essentially it's original palette
+  # instead of the specified palette
+  #
+  # This was confusing, especially if you specified a restricted
+  # palette such as mono or a small greyscale ramp
+
+  my $src = test_image();
+  ok($src, "make source image");
+  my $pal = $src->to_paletted(max_colors => 250);
+  ok($pal, "make paletted version");
+  cmp_ok($pal->colorcount, "<=", 250, "make sure not too many colors");
+
+  my $mono = $src->to_paletted(make_colors => "mono", translate => "errdiff");
+  ok($mono, "make mono image directly");
+  ok($mono->write(file => "testout/t30monodirect.gif", type => "gif"),
+     "save mono direct image");
+
+  Imager->log("Save manually paletted version\n");
+  ok($pal->write(file => "testout/t30color.gif"),
+     "save generated palette version");
+  Imager->log("Save mono version\n");
+  ok($pal->write(file => "testout/t30monoind.gif", type => "gif",
+                make_colors => "mono", translate => "errdiff"),
+     "write paletted with mono colormap");
+
+  my $rd = Imager->new(file => "testout/t30monoind.gif", type => "gif");
+  ok($rd, "read it back in");
+  is($rd->colorcount, 2, "should only have 2 colors");
+}
+
+Imager->close_log;
index ab4cd02..29adf64 100644 (file)
--- a/MANIFEST
+++ b/MANIFEST
@@ -76,6 +76,7 @@ GIF/imgif.h
 GIF/Makefile.PL
 GIF/t/t10gif.t
 GIF/t/t20new.t
+GIF/t/t30fixed.t
 GIF/testimg/badindex.gif       GIF with a bad color index
 GIF/testimg/bandw.gif
 GIF/testimg/expected.gif
diff --git a/quant.c b/quant.c
index de14b29..32938ac 100644 (file)
--- a/quant.c
+++ b/quant.c
@@ -5,10 +5,13 @@
 #include "imager.h"
 #include "imageri.h"
 
+static void makemap_webmap(i_quantize *);
 static void makemap_addi(i_quantize *, i_img **imgs, int count);
 static void makemap_mediancut(i_quantize *, i_img **imgs, int count);
 static void makemap_mono(i_quantize *);
 
+static int makemap_palette(i_quantize *, i_img **imgs, int count);
+
 static
 void
 setcol(i_color *cl,unsigned char r,unsigned char g,unsigned char b,unsigned char a) {
@@ -58,15 +61,7 @@ i_quant_makemap(i_quantize *quant, i_img **imgs, int count) {
     /* use user's specified map */
     break;
   case mc_web_map:
-    {
-      int r, g, b;
-      int i = 0;
-      for (r = 0; r < 256; r+=0x33)
-       for (g = 0; g < 256; g+=0x33)
-         for (b = 0; b < 256; b += 0x33)
-           setcol(quant->mc_colors+i++, r, g, b, 255);
-      quant->mc_count = i;
-    }
+    makemap_webmap(quant);
     break;
 
   case mc_median_cut:
@@ -299,6 +294,9 @@ makemap_addi(i_quantize *quant, i_img **imgs, int count) {
 
   mm_log((1, "makemap_addi(quant %p { mc_count=%d, mc_colors=%p }, imgs %p, count %d)\n", 
           quant, quant->mc_count, quant->mc_colors, imgs, count));
+
+  if (makemap_palette(quant, imgs, count))
+    return;
          
   i_mempool_init(&mp);
 
@@ -446,6 +444,8 @@ makemap_addi(i_quantize *quant, i_img **imgs, int count) {
 #endif
 
   i_mempool_destroy(&mp);
+
+  mm_log((1, "makemap_addi() - %d colors\n", quant->mc_count));
 }
 
 typedef struct {
@@ -549,7 +549,11 @@ makemap_mediancut(i_quantize *quant, i_img **imgs, int count) {
      this isn't terribly efficient, but it should work */
   int chan_count; 
 
-  /*printf("images %d  pal size %d\n", count, quant->mc_size);*/
+  mm_log((1, "makemap_mediancut(quant %p { mc_count=%d, mc_colors=%p }, imgs %p, count %d)\n", 
+          quant, quant->mc_count, quant->mc_colors, imgs, count));
+
+  if (makemap_palette(quant, imgs, count))
+    return;
 
   i_mempool_init(&mp);
 
@@ -704,6 +708,8 @@ makemap_mediancut(i_quantize *quant, i_img **imgs, int count) {
   }
   /*printf("out %d colors\n", quant->mc_count);*/
   i_mempool_destroy(&mp);
+
+  mm_log((1, "makemap_mediancut() - %d colors\n", quant->mc_count));
 }
 
 static void
@@ -719,6 +725,112 @@ makemap_mono(i_quantize *quant) {
   quant->mc_count = 2;
 }
 
+static void
+makemap_webmap(i_quantize *quant) {
+  int r, g, b;
+
+  int i = 0;
+  for (r = 0; r < 256; r+=0x33)
+    for (g = 0; g < 256; g+=0x33)
+      for (b = 0; b < 256; b += 0x33)
+       setcol(quant->mc_colors+i++, r, g, b, 255);
+  quant->mc_count = i;
+}
+
+static int 
+in_palette(i_color *c, i_quantize *quant, int size) {
+  int i;
+
+  for (i = 0; i < size; ++i) {
+    if (c->channel[0] == quant->mc_colors[i].channel[0]
+        && c->channel[1] == quant->mc_colors[i].channel[1]
+        && c->channel[2] == quant->mc_colors[i].channel[2]) {
+      return i;
+    }
+  }
+
+  return -1;
+}
+
+/*
+=item makemap_palette(quant, imgs, count)
+
+Tests if all the given images are paletted and have a common palette,
+if they do it builds that palette.
+
+A possible improvement might be to eliminate unused colors in the
+images palettes.
+
+=cut
+*/
+
+static int
+makemap_palette(i_quantize *quant, i_img **imgs, int count) {
+  int size = quant->mc_count;
+  int i;
+  int imgn;
+  char used[256];
+  int col_count;
+
+  mm_log((1, "makemap_palette(quant %p { mc_count=%d, mc_colors=%p }, imgs %p, count %d)\n", 
+          quant, quant->mc_count, quant->mc_colors, imgs, count));
+  /* we try to build a common palette here, if we can manage that, then
+     that's the palette we use */
+  for (imgn = 0; imgn < count; ++imgn) {
+    int eliminate_unused;
+    if (imgs[imgn]->type != i_palette_type) {
+      mm_log((1, "makemap_palette() -> 0 (non-palette image)\n"));
+      return 0;
+    }
+
+    if (!i_tags_get_int(&imgs[imgn]->tags, "gif_eliminate_unused", 0, 
+                        &eliminate_unused)) {
+      eliminate_unused = 1;
+    }
+
+    if (eliminate_unused) {
+      i_palidx *line = mymalloc(sizeof(i_palidx) * imgs[imgn]->xsize);
+      int x, y;
+      memset(used, 0, sizeof(used));
+
+      for (y = 0; y < imgs[imgn]->ysize; ++y) {
+        i_gpal(imgs[imgn], 0, imgs[imgn]->xsize, y, line);
+        for (x = 0; x < imgs[imgn]->xsize; ++x)
+          used[line[x]] = 1;
+      }
+
+      myfree(line);
+    }
+    else {
+      /* assume all are in use */
+      memset(used, 1, sizeof(used));
+    }
+
+    col_count = i_colorcount(imgs[imgn]);
+    for (i = 0; i < col_count; ++i) {
+      i_color c;
+      
+      i_getcolors(imgs[imgn], i, &c, 1);
+      if (used[i]) {
+        if (in_palette(&c, quant, size) < 0) {
+          if (size < quant->mc_size) {
+            quant->mc_colors[size++] = c;
+          }
+          else {
+           mm_log((1, "makemap_palette() -> 0 (too many colors)\n"));
+            return 0;
+          }
+        }
+      }
+    }
+  }
+
+  mm_log((1, "makemap_palette() -> 1 (%d total colors)\n", size));
+  quant->mc_count = size;
+
+  return 1;
+}
+
 #define pboxjump 32
 
 /* Define one of the following 4 symbols to choose a colour search method