treat the ico mask as an alpha channel, since this is less confusing

in most use-cases.

diff --git a/Changes b/Changes
index 4ebb29ee06b74769d0a42b5646f255cd2f2d925a..5e7e94c6debc58297167c495cdbb022436b7f015 100644 (file)
--- a/Changes
+++ b/Changes
@@ -15,6 +15,12 @@ Imager 0.60
    Obviously problems are my fault :)
    https://rt.cpan.org/Ticket/Display.html?id=28142
 
+ - the mask for ICO/CUR images is now applied as an alpha channel to
+   the returned image.  For the old behaviour, supply ico_masked => 0
+   to read() or read_multi().  This should be less confusing when
+   using Imager as a general image processor.
+   https://rt.cpan.org/Ticket/Display.html?id=29001
+
 Bug fixes:
 
  - in some cases it's possible for giflib/libungif to return color 

diff --git a/ICO/ICO.pm b/ICO/ICO.pm
index 1b187a9eeaa4bda9afe6caad5b3e26cddff046b3..56ee231a0161591a6a10cf4b325d9dd3d551c8f3 100644 (file)
--- a/ICO/ICO.pm
+++ b/ICO/ICO.pm
@@ -23,7 +23,9 @@ Imager->register_reader
    single => 
    sub { 
      my ($im, $io, %hsh) = @_;
-     $im->{IMG} = i_readico_single($io, $hsh{page} || 0);
+     my $masked = 
+       exists $hsh{ico_masked} ? $hsh{ico_masked} : 1;
+     $im->{IMG} = i_readico_single($io, $hsh{page} || 0, $masked);
 
      unless ($im->{IMG}) {
        $im->_set_error(Imager->_error_as_msg);
@@ -35,7 +37,9 @@ Imager->register_reader
    sub {
      my ($io, %hsh) = @_;
      
-     my @imgs = i_readico_multi($io);
+     my $masked = 
+       exists $hsh{ico_masked} ? $hsh{ico_masked} : 1;
+     my @imgs = i_readico_multi($io, $masked);
      unless (@imgs) {
        Imager->_set_error(Imager->_error_as_msg);
        return;
@@ -53,7 +57,9 @@ Imager->register_reader
    single => 
    sub { 
      my ($im, $io, %hsh) = @_;
-     $im->{IMG} = i_readico_single($io, $hsh{page} || 0);
+     my $masked = 
+       exists $hsh{ico_masked} ? $hsh{ico_masked} : 1;
+     $im->{IMG} = i_readico_single($io, $hsh{page} || 0, $masked);
 
      unless ($im->{IMG}) {
        $im->_set_error(Imager->_error_as_msg);
@@ -65,7 +71,9 @@ Imager->register_reader
    sub {
      my ($io, %hsh) = @_;
      
-     my @imgs = i_readico_multi($io);
+     my $masked = 
+       exists $hsh{ico_masked} ? $hsh{ico_masked} : 1;
+     my @imgs = i_readico_multi($io, $masked);
      unless (@imgs) {
        Imager->_set_error(Imager->_error_as_msg);
        return;

diff --git a/ICO/ICO.xs b/ICO/ICO.xs
index 0baa43a2f07bc9fba7ad0dcede737c92716e0de9..abdad0e85783edb35c7375c2f0804dd113e8da37 100644 (file)
--- a/ICO/ICO.xs
+++ b/ICO/ICO.xs
@@ -13,19 +13,21 @@ MODULE = Imager::File::ICO  PACKAGE = Imager::File::ICO
 PROTOTYPES: DISABLE
 
 Imager::ImgRaw
-i_readico_single(ig, index)
+i_readico_single(ig, index, masked = 0)
        Imager::IO ig
        int index
+       bool masked
 
 void
-i_readico_multi(ig)
+i_readico_multi(ig, masked = 0)
        Imager::IO ig
+       bool masked
       PREINIT:
         i_img **imgs;
         int count;
         int i;
       PPCODE:
-        imgs = i_readico_multi(ig, &count);
+        imgs = i_readico_multi(ig, &count, masked);
         if (imgs) {
           EXTEND(SP, count);
           for (i = 0; i < count; ++i) {

diff --git a/ICO/imicon.c b/ICO/imicon.c
index 00b516a242e66a16f42815c2dfee07c8ef750b69..2fc62deb7fe5bc7f76b84b91eb889387c291cf6e 100644 (file)
--- a/ICO/imicon.c
+++ b/ICO/imicon.c
@@ -13,7 +13,7 @@ ico_push_error(int error) {
 
 static
 i_img *
-read_one_icon(ico_reader_t *file, int index) {
+read_one_icon(ico_reader_t *file, int index, int masked) {
   ico_image_t *image;
   int error;
   i_img *result;
@@ -25,6 +25,22 @@ read_one_icon(ico_reader_t *file, int index) {
     return NULL;
   }
 
+  if (masked) {
+    /* check to make sure we should do the masking, if the mask has
+       nothing set we don't mask */
+    int pos;
+    int total = image->width * image->height;
+    unsigned char *inp = image->mask_data;
+
+    masked = 0;
+    for (pos = 0; pos < total; ++pos) {
+      if (*inp++) {
+       masked = 1;
+       break;
+      }
+    }
+  }
+
   if (image->direct) {
     int x, y;
     i_color *line_buf;
@@ -63,13 +79,14 @@ read_one_icon(ico_reader_t *file, int index) {
     int pal_index;
     int y;
     unsigned char *image_data;
+    int channels = masked ? 4 : 3;
 
-    if (!i_int_check_image_file_limits(image->width, image->height, 3, 1)) {
+    if (!i_int_check_image_file_limits(image->width, image->height, channels, 1)) {
       ico_image_release(image);
       return NULL;
     }
 
-    result = i_img_pal_new(image->width, image->height, 3, 256);
+    result = i_img_pal_new(image->width, image->height, channels, 256);
     if (!result) {
       ico_image_release(image);
       return NULL;
@@ -81,7 +98,7 @@ read_one_icon(ico_reader_t *file, int index) {
       c.rgba.r = image->palette[pal_index].r;
       c.rgba.g = image->palette[pal_index].g;
       c.rgba.b = image->palette[pal_index].b;
-      c.rgba.a = 255; /* so as to not confuse some code */
+      c.rgba.a = 255;
 
       if (i_addcolors(result, &c, 1) < 0) {
        i_push_error(0, "could not add color to palette");
@@ -128,6 +145,36 @@ read_one_icon(ico_reader_t *file, int index) {
     
     myfree(mask);
   }
+
+  /* if the user requests, treat the mask as an alpha channel.
+     Note: this converts the image into a direct image if it was paletted
+  */
+  if (masked) {
+    unsigned char *inp = image->mask_data;
+    int x, y;
+    i_color *line_buf = mymalloc(sizeof(i_color) * image->width);
+
+    for (y = 0; y < image->height; ++y) {
+      int changed = 0;
+      int first;
+      int last;
+      for (x = 0; x < image->width; ++x) {
+       if (*inp++) {
+         if (!changed) {
+           first = x;
+           i_glin(result, first, image->width, y, line_buf);
+           changed = 1;
+         }
+         last = x;
+         line_buf[x-first].rgba.a = 0;
+       }
+      }
+      if (changed) {
+       i_plin(result, first, last + 1, y, line_buf);
+      }
+    }
+    myfree(line_buf);
+  }
   if (ico_type(file) == ICON_ICON) {
     i_tags_setn(&result->tags, "ico_bits", image->bit_count);
     i_tags_set(&result->tags, "i_format", "ico", 3);
@@ -145,7 +192,7 @@ read_one_icon(ico_reader_t *file, int index) {
 }
 
 i_img *
-i_readico_single(io_glue *ig, int index) {
+i_readico_single(io_glue *ig, int index, int masked) {
   ico_reader_t *file;
   i_img *result;
   int error;
@@ -161,14 +208,14 @@ i_readico_single(io_glue *ig, int index) {
 
   /* the index is range checked by msicon.c - don't duplicate it here */
 
-  result = read_one_icon(file, index);
+  result = read_one_icon(file, index, masked);
   ico_reader_close(file);
 
   return result;
 }
 
 i_img **
-i_readico_multi(io_glue *ig, int *count) {
+i_readico_multi(io_glue *ig, int *count, int masked) {
   ico_reader_t *file;
   int index;
   int error;
@@ -187,7 +234,7 @@ i_readico_multi(io_glue *ig, int *count) {
 
   *count = 0;
   for (index = 0; index < ico_image_count(file); ++index) {
-    i_img *im = read_one_icon(file, index);
+    i_img *im = read_one_icon(file, index, masked);
     if (!im)
       break;
 

diff --git a/ICO/imicon.h b/ICO/imicon.h
index bde48212c96d48b61f4c689b7d7f6b639cdb870d..7e26f4a8928ddab6796321ccd57ca62aa68f7501 100644 (file)
--- a/ICO/imicon.h
+++ b/ICO/imicon.h
@@ -4,9 +4,9 @@
 #include "imext.h"
 
 extern i_img *
-i_readico_single(io_glue *ig, int index);
+i_readico_single(io_glue *ig, int index, int masked);
 extern i_img **
-i_readico_multi(io_glue *ig, int *count);
+i_readico_multi(io_glue *ig, int *count, int masked);
 
 extern int
 i_writeico_wiol(i_io_glue_t *ig, i_img *im);

diff --git a/ICO/t/t10icon.t b/ICO/t/t10icon.t
index 61e27e41fad8e3972a4e87cddd72c727c3e9493a..bd2d4fe5995576048a9b8d5f77fdd0c5cf952c71 100644 (file)
--- a/ICO/t/t10icon.t
+++ b/ICO/t/t10icon.t
@@ -1,6 +1,7 @@
 #!perl -w
 use strict;
-use Test::More tests => 94;
+use Test::More tests => 98;
+use Imager::Test qw(is_image);
 
 BEGIN { use_ok('Imager::File::ICO'); }
 
@@ -9,7 +10,7 @@ BEGIN { use_ok('Imager::File::ICO'); }
 my $im = Imager->new;
 # type=>'ico' or 'cur' and read ico and cur since they're pretty much
 # the same
-ok($im->read(file => "testimg/rgba3232.ico", type=>"ico"),
+ok($im->read(file => "testimg/rgba3232.ico", type=>"ico", ico_masked => 0),
    "read 32 bit")
   or print "# ", $im->errstr, "\n";
 is($im->getwidth, 32, "check width");
@@ -64,7 +65,7 @@ SKIP:
      "compare image data");
 }
 
-ok($im->read(file => 'testimg/pal83232.ico', type=>'ico'),
+ok($im->read(file => 'testimg/pal83232.ico', type=>'ico', ico_masked => 0),
    "read 8 bit")
   or print "# ", $im->errstr, "\n";
 is($im->getwidth, 32, "check width");
@@ -83,7 +84,7 @@ SKIP:
 }
 $im->write(file=>'testout/pal83232.ppm');
 
-ok($im->read(file => 'testimg/pal43232.ico', type=>'ico'),
+ok($im->read(file => 'testimg/pal43232.ico', type=>'ico', ico_masked => 0),
    "read 4 bit")
   or print "# ", $im->errstr, "\n";
 is($im->getwidth, 32, "check width");
@@ -102,7 +103,7 @@ SKIP:
 }
 
 $im->write(file=>'testout/pal43232.ppm');
-ok($im->read(file => 'testimg/pal13232.ico', type=>'ico'),
+ok($im->read(file => 'testimg/pal13232.ico', type=>'ico', ico_masked => 0),
    "read 1 bit")
   or print "# ", $im->errstr, "\n";
 is($im->getwidth, 32, "check width");
@@ -116,7 +117,8 @@ $im->write(file=>'testout/pal13232.ppm');
 # combo was created with the GIMP, which has a decent mechanism for selecting
 # the output format
 # you get different size icon images by making different size layers.
-my @imgs = Imager->read_multi(file => 'testimg/combo.ico', type=>'ico');
+my @imgs = Imager->read_multi(file => 'testimg/combo.ico', type=>'ico', 
+                             ico_masked => 0);
 is(scalar(@imgs), 3, "read multiple");
 is($imgs[0]->getwidth, 48, "image 0 width");
 is($imgs[0]->getheight, 48, "image 0 height");
@@ -167,7 +169,7 @@ ok($im->write(file=>'testout/t10_32.ico', type=>'ico'),
    "write 32-bit icon");
 
 my $im2 = Imager->new;
-ok($im2->read(file=>'testout/t10_32.ico', type=>'ico'),
+ok($im2->read(file=>'testout/t10_32.ico', type=>'ico', ico_masked => 0),
    "read it back in");
 
 is(Imager::i_img_diff($im->{IMG}, $im2->{IMG}), 0,
@@ -181,7 +183,7 @@ is($im->bits, $im2->bits, "check same bits");
   ok(Imager->write_multi({ data => \$data, type=>'ico' }, $im, $im),
      "write multi icons");
   ok(length $data, "and it wrote data");
-  my @im = Imager->read_multi(data => $data);
+  my @im = Imager->read_multi(data => $data, ico_masked => 0);
   is(@im, 2, "got all the images back");
   is(Imager::i_img_diff($im->{IMG}, $im[0]{IMG}), 0, "check first image");
   is(Imager::i_img_diff($im->{IMG}, $im[1]{IMG}), 0, "check second image");
@@ -193,7 +195,7 @@ is($im->bits, $im2->bits, "check same bits");
   my $data;
   ok($im->write(data => \$data, type=>'ico'), "write 1 channel image");
   my $im2 = Imager->new;
-  ok($im2->read(data => $data), "read it back");
+  ok($im2->read(data => $data, ico_masked => 0), "read it back");
   is($im2->getchannels, 4, "check channels");
   my $imrgb = $im->convert(preset => 'rgb')
     ->convert(preset => 'addalpha');
@@ -207,7 +209,7 @@ is($im->bits, $im2->bits, "check same bits");
   my $data;
   ok($base->write(data => \$data, type=>'ico'), "write 2 channel image");
   my $read = Imager->new;
-  ok($read->read(data => $data), "read it back");
+  ok($read->read(data => $data, ico_masked => 0), "read it back");
   is($read->getchannels, 4, "check channels");
   my $imrgb = $base->convert(preset => 'rgb');
   is(Imager::i_img_diff($imrgb->{IMG}, $read->{IMG}), 0,
@@ -221,7 +223,7 @@ is($im->bits, $im2->bits, "check same bits");
   my $data;
   ok($base->write(data => \$data, type=>'ico'), "write 4 channel image");
   my $read = Imager->new;
-  ok($read->read(data => $data, type=>'ico'), "read it back")
+  ok($read->read(data => $data, type=>'ico', ico_masked => 0), "read it back")
     or print "# ", $read->errstr, "\n";
   is(Imager::i_img_diff($base->{IMG}, $read->{IMG}), 0,
      "check image matches expected");
@@ -257,7 +259,7 @@ EOS
   ok($base->write(data => \$data, type => 'ico'),
      "write with mask tag set");
   my $read = Imager->new;
-  ok($read->read(data => $data), "read it back");
+  ok($read->read(data => $data, ico_masked => 0), "read it back");
   my $mask2 = $mask;
   $mask2 =~ tr/01/.*/;
   $mask2 =~ s/\n$//;
@@ -277,7 +279,7 @@ EOS
   ok($base->write(data => \$data, type=>'ico'),
      "save icon with short mask tag");
   my $read = Imager->new;
-  ok($read->read(data => $data), "read it back");
+  ok($read->read(data => $data, ico_masked => 0), "read it back");
   my $read_mask = $read->tags(name => 'ico_mask');
   my $expected_mask = ".*" . ( "\n" . "." x 16 ) x 16;
   is($read_mask, $expected_mask, "check the mask");
@@ -286,7 +288,7 @@ EOS
   $base->settag(name => 'ico_mask', value => 'abcd');
   ok($base->write(data => \$data, type => 'ico'), 
      "write with bad format mask tag");
-  ok($read->read(data => $data), "read it back");
+  ok($read->read(data => $data, ico_masked => 0), "read it back");
   $read_mask = $read->tags(name => 'ico_mask');
   is($read_mask, $expected_mask, "check the mask");
 
@@ -294,7 +296,7 @@ EOS
   $base->settag(name => 'ico_mask', value => ".*\n....xxx..");
   ok($base->write(data => \$data, type => 'ico'), 
      "write with unexpected chars in mask");
-  ok($read->read(data => $data), "read it back");
+  ok($read->read(data => $data, ico_masked => 0), "read it back");
   $read_mask = $read->tags(name => 'ico_mask');
   is($read_mask, $expected_mask, "check the mask");
 }
@@ -313,7 +315,7 @@ EOS
   ok($base->write(data => \$data, type => 'ico'),
      "write grayscale paletted");
   my $read = Imager->new;
-  ok($read->read(data => $data), "read it back")
+  ok($read->read(data => $data, ico_masked => 0), "read it back")
     or print "# ", $read->errstr, "\n";
   is($read->type, 'paletted', "check type");
   is($read->getchannels, 3, "check channels");
@@ -321,3 +323,41 @@ EOS
   is(Imager::i_img_diff($base->{IMG}, $read->{IMG}), 0,
      "check the image");
 }
+
+{
+  # check default mask processing
+  #
+  # the query at http://www.cpanforum.com/threads/5958 made it fairly
+  # obvious that the way Imager handled the mask in 0.59 was confusing
+  # when compared with loading other images with some sort of
+  # secondary alpha channel (eg. gif)
+  # So from 0.60 the mask is applied as an alpha channel by default.
+  # make sure that application works.
+
+  # the strange mask checks the optimization paths of the mask application
+  my $mask = <<EOS;
+01
+1001
+1100
+0011
+0000
+0010
+EOS
+  chomp $mask;
+  my $im = Imager->new(xsize => 4, ysize => 5, type => 'paletted');
+  $im->addcolors(colors => [ '#FF0000' ]);
+  $im->box(filled => 1, color => '#FF0000');
+  $im->settag(name => 'ico_mask', value => $mask);
+  my $imcopy = $im->convert(preset=>'addalpha');
+  my $red_alpha = Imager::Color->new(255, 0, 0, 0);
+  $imcopy->setpixel( 'x' => [ qw/0 3 0 1 2 3 2/ ],
+                    'y' => [ qw/0 0 1 1 2 2 4/ ],
+                    color => $red_alpha);
+  my $data;
+  ok($im->write(data => \$data, type => 'ico'),
+     "save icon + mask");
+  my $im2 = Imager->new;
+  ok($im2->read(data => $data), "read ico with defaults");
+  is($im2->type, 'direct', 'expect a direct image');
+  is_image($im2, $imcopy, 'check against expected');
+}

diff --git a/lib/Imager/Files.pod b/lib/Imager/Files.pod
index 412041806f377a7437f8fc3d199b96e8f83f35a8..8d401fa8455f42e02cbbb2965cf63ef5df47de13 100644 (file)
--- a/lib/Imager/Files.pod
+++ b/lib/Imager/Files.pod
@@ -1035,6 +1035,27 @@ clipped to the size of the image when written to the file.
 
 =back
 
+The following parameters can be supplied to read() or read_multi() to
+control reading of ICO/CUR files:
+
+=over
+
+=item *
+
+ico_masked - if true, the default, then the icon/cursors mask is
+applied as an alpha channel to the image.  This may result in a
+paletted image being returned as a direct color image.  Default: 1
+
+  # retrieve the image as stored, without using the mask as an alpha
+  # channel
+  $img->read(file => 'foo.ico', ico_masked => 0)
+    or die $img->errstr;
+
+This was introduced in Imager 0.60.  Previously reading ICO images
+acted as if C<<ico_masked => 0>>.
+
+=back
+
 C<cur_bits> is set when reading a cursor.
 
 Examples: