Bug #40969 for Data-FormValidator-Filters-Image: Resizes image, even if not required

Sun Nov 16 01:53:14 2008 GTERMARS [...] cpan.org - Ticket created

Subject:

Resizes image, even if not required

Saw this while trying to figure out why an animated GIF image that was being uploaded by one of my customers was rejected. The image being uploaded matched the "max width" and "max height" for the image filter _exactly_, but that DFV::F::Image was still resizing the image. By resizing the image, it got pumped through ImageMagick, which then caused the resulting file size to *grow*; the original uploaded image was ~24KBytes, but the one that ImageMagick spat out was ~44KBytes. It _looks like_ ImageMagick remapped the animation frames in the GIF and that caused the file size to increase. I'd like to submit a patch, but wasn't sure what the purpose of the undocumented "@the_rest" was in "__shrink_image()". Well, I see what its for, but can't see where its being used (in part because its undocumented). As the documentation reads, I get the impression that the image will be resized *down* to fit within the given size restrictions. If the image fits within the restrictions, though, I read the docs as implying that no resize is necessary. Can we just short-circuit this so that if the image height/width already fits within the given bounds that we just exit early without doing anything to the provided image?

Subject:

your-ad-will-help.gif

Download your-ad-will-help.gif
image/gif 22.7k

Sun Nov 16 02:07:01 2008 GTERMARS [...] cpan.org - Correspondence added 5 min

Here's a quick patch that fixes the issue in my test environment. Not sure how to integrate this into your test setup, though; been using Test::More for so long that _not_ using it seems alien... :o

--- Image.pm.bk 2008-11-14 19:58:45.000000000 -0800 +++ Image.pm 2008-11-14 20:00:02.000000000 -0800 @@ -120,6 +120,14 @@ return $fh; } + if ( $max_width && ($ow <= $max_width) + && $max_height && ($oh <= $max_height) + ) { + #warn "Image does not need to be resized"; + seek( $fh, 0, 0 ); + return $fh; + } + if ( $max_width && $nw > $max_width ) { $nw = $max_width; $nh = $oh * ( $max_width / $ow );

Sun Nov 16 02:07:04 2008 GTERMARS [...] cpan.org - Status changed from 'new' to 'open'

Sun Nov 16 02:07:15 2008 GTERMARS [...] cpan.org - Status changed from 'open' to 'new'

Sun Nov 16 02:10:35 2008 GTERMARS [...] cpan.org - Correspondence added

Updated version of previous patch, which takes into consideration that we may _not_ have been given *both* the height+width as restrictions. This patch checks against the "new height/width" and compares it to the "original height/width" in order to see if the image needs to be resized.

--- Image.pm.bk 2008-11-14 19:58:45.000000000 -0800 +++ Image.pm 2008-11-14 20:09:44.000000000 -0800 @@ -129,6 +129,12 @@ $nw = $ow * ( $max_height / $oh ); } + if (($nh <= $oh) && ($nw <= $ow)) { + #warn "Image does not need to be resized"; + seek( $fh, 0, 0 ); + return $fh; + } + $result = $image->Resize( width => $nw, height => $nh, @the_rest ); if ("$result") { # quotes are there as per the Image::Magick examples #warn "$result";

Sat Mar 14 02:39:28 2009 GTERMARS [...] cpan.org - Correspondence added

Cees, I've just made a commit on my Github fork which fixes this...

Sat Mar 14 02:39:29 2009 GTERMARS [...] cpan.org - Status changed from 'new' to 'open'

Wed Mar 25 00:48:53 2009 GTERMARS [...] cpan.org - Correspondence added

Marking ticket as "resolved"; Cees pulled my fork on Github and integrated the change into his master branch. Was released as part of v0.40, on March 23, 2009

Wed Mar 25 00:48:54 2009 GTERMARS [...] cpan.org - Status changed from 'open' to 'resolved'