- the conv filter now enforces that the sum of the coefficients is
authorTony Cook <tony@develop=help.com>
Mon, 28 Sep 2009 09:33:53 +0000 (09:33 +0000)
committerTony Cook <tony@develop=help.com>
Mon, 28 Sep 2009 09:33:53 +0000 (09:33 +0000)
   non-zero.  Also, rather than skipping pixels off the edge off the
   edge of the image, the closest edge pixel is used.  Previously
   dividing by the zero sum of coefficients could cause invalid
   results or runtime exceptions.
   Thanks to David Cantrell's Alpha-NetBSD CPAN test box for revealing
   this bug.

Changes
Imager.pm
Imager.xs
conv.im
imager.h
lib/Imager/Filters.pod
t/t61filters.t

diff --git a/Changes b/Changes
index 5bf1302..6f32073 100644 (file)
--- a/Changes
+++ b/Changes
@@ -1,5 +1,18 @@
 Imager release history.  Older releases can be found in Changes.old
 
+Imager 0.71 - unreleased
+===========
+
+Bug fixes:
+
+ - the conv filter now enforces that the sum of the coefficients is
+   non-zero.  Also, rather than skipping pixels off the edge off the
+   edge of the image, the closest edge pixel is used.  Previously
+   dividing by the zero sum of coefficients could cause invalid
+   results or runtime exceptions.
+   Thanks to David Cantrell's Alpha-NetBSD CPAN test box for revealing
+   this bug.
+
 Imager 0.70 - 21 Sep 2009
 ===========
 
index bde3713..ec5f08e 100644 (file)
--- a/Imager.pm
+++ b/Imager.pm
@@ -243,11 +243,17 @@ BEGIN {
                       callsub => sub { my %hsh=@_; i_radnoise($hsh{image},$hsh{xo},$hsh{yo},$hsh{rscale},$hsh{ascale}); }
                      };
 
-  $filters{conv} ={
-                      callseq => ['image', 'coef'],
-                      defaults => { },
-                      callsub => sub { my %hsh=@_; i_conv($hsh{image},$hsh{coef}); }
-                     };
+  $filters{conv} =
+    {
+     callseq => ['image', 'coef'],
+     defaults => { },
+     callsub => 
+     sub { 
+       my %hsh=@_;
+       i_conv($hsh{image},$hsh{coef})
+        or die Imager->_error_as_msg() . "\n";
+     }
+    };
 
   $filters{gradgen} =
     {
index 2de3747..5968c30 100644 (file)
--- a/Imager.xs
+++ b/Imager.xs
@@ -1756,27 +1756,26 @@ i_unsharp_mask(im,stdev,scale)
             float     stdev
              double    scale
 
-void
-i_conv(im,pcoef)
-    Imager::ImgRaw     im
-            PREINIT:
-            float*    coeff;
-            int     len;
-            AV* av;
-            SV* sv1;
-            int i;
-            PPCODE:
-            if (!SvROK(ST(1))) croak("Imager: Parameter 1 must be a reference to an array\n");
-            if (SvTYPE(SvRV(ST(1))) != SVt_PVAV) croak("Imager: Parameter 1 must be a reference to an array\n");
-            av=(AV*)SvRV(ST(1));
-            len=av_len(av)+1;
-            coeff=mymalloc( len*sizeof(float) );
-            for(i=0;i<len;i++) {
-              sv1=(*(av_fetch(av,i,0)));
-              coeff[i]=(float)SvNV(sv1);
-            }
-            i_conv(im,coeff,len);
-            myfree(coeff);
+int
+i_conv(im,coef)
+       Imager::ImgRaw     im
+       AV *coef
+     PREINIT:
+       double*    c_coef;
+       int     len;
+       SV* sv1;
+       int i;
+    CODE:
+       len = av_len(coef) + 1;
+       c_coef=mymalloc( len * sizeof(double) );
+       for(i = 0; i  < len; i++) {
+         sv1 = (*(av_fetch(coef, i, 0)));
+         c_coef[i] = (double)SvNV(sv1);
+       }
+       RETVAL = i_conv(im, c_coef, len);
+       myfree(c_coef);
+    OUTPUT:
+       RETVAL
 
 Imager::ImgRaw
 i_convert(src, avmain)
diff --git a/conv.im b/conv.im
index 986bbae..b7afa64 100644 (file)
--- a/conv.im
+++ b/conv.im
@@ -1,4 +1,5 @@
 #include "imager.h"
+#include "imageri.h"
 
 /*
   General convolution for 2d decoupled filters
            (since the filter is even);
 */
 
-void
-i_conv(i_img *im,const float *coeff,int len) {
-  int i,l,c,ch,center;
+int
+i_conv(i_img *im, const double *coeff,int len) {
+  int xo, yo; /* output pixel co-ordinate */
+  int c, ch, center;
   double pc;
-  double res[11];
+  double res[MAXCHANNELS];
   i_img *timg;
 
   mm_log((1,"i_conv(im %p, coeff %p, len %d)\n",im,coeff,len));
+  i_clear_error();
  
-  timg = i_sametype(im, im->xsize, im->ysize);
-
   center=(len-1)/2;
 
+  pc = 0;
+  for (c = 0; c < len; ++c)
+    pc += coeff[c];
+
+  if (pc == 0) {
+    i_push_error(0, "sum of coefficients is zero");
+    return 0;
+  }
+
+  timg = i_sametype(im, im->xsize, im->ysize);
+
 #code im->bits <= 8
   IM_COLOR rcolor;
   /* don't move the calculation of pc up here, it depends on which pixels
      are readable */
-  for(l=0;l<im->ysize;l++) {
-    for(i=0;i<im->xsize;i++) {
-      pc=0.0;
-      for(ch=0;ch<im->channels;ch++) 
-       res[ch]=0;
-      for(c=0;c<len;c++)
-       if (IM_GPIX(im,i+c-center,l,&rcolor)!=-1) {
-         for(ch=0;ch<im->channels;ch++) 
-            res[ch] += (rcolor.channel[ch])*coeff[c];
-         pc+=coeff[c];
+  for(yo = 0; yo < im->ysize; yo++) {
+    for(xo = 0; xo < im->xsize; xo++) {
+      for(ch = 0;ch < im->channels; ch++) 
+       res[ch] = 0;
+      for(c = 0;c < len; c++) {
+       int xi = xo + c - center;
+       if (xi < 0)
+         xi = 0;
+       else if (xi >= im->xsize)
+         xi = im->xsize - 1;
+
+       if (IM_GPIX(im, xi, yo, &rcolor)!=-1) {
+         for(ch = 0; ch < im->channels; ch++) 
+            res[ch] += (rcolor.channel[ch])  *coeff[c];
        }
-      for(ch=0;ch<im->channels;ch++) {
-        double temp = res[ch]/pc;
+      }
+      im_assert(pc != 0);
+      for(ch = 0; ch < im->channels; ch++) {
+        double temp = res[ch] / pc;
         rcolor.channel[ch] = 
           temp < 0 ? 0 : temp > IM_SAMPLE_MAX ? IM_SAMPLE_MAX : (IM_SAMPLE_T)temp;
       }
-      IM_PPIX(timg,i,l,&rcolor);
+      IM_PPIX(timg, xo, yo, &rcolor);
     }
   }
 
-  for(l=0;l<im->xsize;l++) {
-    for(i=0;i<im->ysize;i++) {
-      pc=0.0;  
-      for(ch=0;ch<im->channels;ch++) res[ch]=0;
-      for(c=0;c<len;c++) {
-       if (IM_GPIX(timg,l,i+c-center,&rcolor)!=-1) {
-         for(ch=0;ch<im->channels;ch++) 
-           res[ch] += (rcolor.channel[ch])*coeff[c];
-         pc+=coeff[c];
+  for(xo = 0; xo < im->xsize; xo++) {
+    for(yo = 0;yo < im->ysize; yo++) {
+      for(ch =  0; ch < im->channels; ch++)
+       res[ch] = 0;
+      for(c = 0; c < len; c++) {
+       int yi = yo + c - center;
+       if (yi < 0)
+         yi = 0;
+       else if (yi >= im->ysize)
+         yi = im->ysize - 1;
+       if (IM_GPIX(timg, xo, yi, &rcolor) != -1) {
+         for(ch = 0;ch < im->channels; ch++) 
+           res[ch] += (rcolor.channel[ch]) * coeff[c];
        }
       }
-      for(ch=0;ch<im->channels;ch++) {
-       double temp = res[ch]/pc;
-       rcolor.channel[ch]= 
+      im_assert(pc != 0);
+      for(ch = 0;ch < im->channels; ch++) {
+       double temp = res[ch] / pc;
+       rcolor.channel[ch] = 
          temp < 0 ? 0 : temp > IM_SAMPLE_MAX ? IM_SAMPLE_MAX : (IM_SAMPLE_T)temp;
       }
-      IM_PPIX(im,l,i,&rcolor);
+      IM_PPIX(im, xo, yo,&rcolor);
     }
   }
 #/code
 
   i_img_destroy(timg);
-}
-
-
-
-
-
-
-
-
 
+  return 1;
+}
index de45b78..2508b86 100644 (file)
--- a/imager.h
+++ b/imager.h
@@ -205,7 +205,7 @@ undef_int i_flood_cfill_border(i_img *im, int seedx, int seedy, i_fill_t *fill,
 /* image processing functions */
 
 int i_gaussian    (i_img *im, double stdev);
-void i_conv        (i_img *im,const float *coeff,int len);
+int i_conv        (i_img *im,const double *coeff,int len);
 void i_unsharp_mask(i_img *im, double stddev, double scale);
 
 /* colour manipulation */
index 6939501..8981076 100644 (file)
@@ -198,6 +198,10 @@ coefficients must be non-zero.
   $img->filter(type=>"conv", coef=>[ 1, 2, 1 ])
     or die $img->errstr;
 
+  # error
+  $img->filter(type=>"conv", coef=>[ -0.5, 1, -0.5 ])
+    or die $img->errstr;
+
 =item fountain
 
 renders a fountain fill, similar to the gradient tool in most paint
index 061eee3..fc76b04 100644 (file)
@@ -1,7 +1,7 @@
 #!perl -w
 use strict;
 use Imager qw(:handy);
-use Test::More tests => 73;
+use Test::More tests => 79;
 Imager::init_log("testout/t61filters.log", 1);
 use Imager::Test qw(is_image_similar test_image is_image);
 # meant for testing the filters themselves
@@ -17,8 +17,31 @@ test($imbase, {type=>'contrast', intensity=>0.5},
      'testout/t61_contrast.ppm');
 
 # this one's kind of cool
-test($imbase, {type=>'conv', coef=>[ -0.5, 1, -0.5, ], },
-     'testout/t61_conv.ppm');
+test($imbase, {type=>'conv', coef=>[ 0.3, 1, 0.3, ], },
+     'testout/t61_conv_blur.ppm');
+
+{
+  my $work8 = $imbase->copy;
+  ok(!$work8->filter(type => "conv", coef => "ABC"),
+     "coef not an array");
+}
+{
+  my $work8 = $imbase->copy;
+  ok(!$work8->filter(type => "conv", coef => [ -1, 2, -1 ]),
+     "should fail if sum of coef is 0");
+  is($work8->errstr, "sum of coefficients is zero", "check message");
+}
+
+{
+  my $work8 = $imbase->copy;
+  my $work16 = $imbase->to_rgb16;
+  my $coef = [ -0.2, 1, -0.2 ];
+  ok($work8->filter(type => "conv", coef => $coef),
+     "filter 8 bit image");
+  ok($work16->filter(type => "conv", , coef => $coef),
+     "filter 16 bit image");
+  is_image_similar($work8, $work16, 80000, "8 and 16 bit conv match");
+}
 
 {
   my $gauss = test($imbase, {type=>'gaussian', stddev=>5 },
@@ -292,6 +315,7 @@ sub test {
       or print "# ",$copy->errstr,"\n";
   }
   else {
+    diag($copy->errstr);
   SKIP: 
     {
       skip("couldn't filter", 1);