From 6a3cbaef62aadb0bd0aa0174e99cc129d9f032f6 Mon Sep 17 00:00:00 2001 From: Tony Cook Date: Mon, 28 Sep 2009 09:33:53 +0000 Subject: [PATCH] - 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. --- Changes | 13 ++++++ Imager.pm | 16 ++++--- Imager.xs | 41 +++++++++--------- conv.im | 98 ++++++++++++++++++++++++------------------ imager.h | 2 +- lib/Imager/Filters.pod | 4 ++ t/t61filters.t | 30 +++++++++++-- 7 files changed, 133 insertions(+), 71 deletions(-) diff --git a/Changes b/Changes index 5bf13029..6f320737 100644 --- 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 =========== diff --git a/Imager.pm b/Imager.pm index bde3713c..ec5f08e9 100644 --- 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} = { diff --git a/Imager.xs b/Imager.xs index 2de3747f..5968c300 100644 --- 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;ixsize, 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;lysize;l++) { - for(i=0;ixsize;i++) { - pc=0.0; - for(ch=0;chchannels;ch++) - res[ch]=0; - for(c=0;cchannels;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;chchannels;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;lxsize;l++) { - for(i=0;iysize;i++) { - pc=0.0; - for(ch=0;chchannels;ch++) res[ch]=0; - for(c=0;cchannels;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;chchannels;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; +} diff --git a/imager.h b/imager.h index de45b786..2508b86f 100644 --- 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 */ diff --git a/lib/Imager/Filters.pod b/lib/Imager/Filters.pod index 6939501a..89810766 100644 --- a/lib/Imager/Filters.pod +++ b/lib/Imager/Filters.pod @@ -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 diff --git a/t/t61filters.t b/t/t61filters.t index 061eee39..fc76b047 100644 --- a/t/t61filters.t +++ b/t/t61filters.t @@ -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); -- 2.39.5