(bug 28762) Resizing to specified height broken for very thin images

Restructure and comment of ImageHandler::normaliseParams(). ImageHandler::normaliseParams() will now output both width/height as physicalWidth/Height, where width/height are the dimensions used by the browser, and physicalWidth/Height are the dimensions of the thumbnail that will be created. This allows keeping the use of unique dimensions for a specified thumbnail width, by forcing client side resizing of 1px-wide images with non corresponding height.
Also fixed a bug where the check for mustRender() was in an if statement where it should not have been.

diff --git a/includes/media/Bitmap.php b/includes/media/Bitmap.php
index 665aca5..a1cbbc5 100644 (file)
--- a/includes/media/Bitmap.php
+++ b/includes/media/Bitmap.php
@@ -35,16 +35,14 @@ class BitmapHandler extends ImageHandler {
                                wfDebug( __METHOD__ . ": Swapping width and height because the file will be rotated $rotation degrees\n" );
 
                                $swapDimensions = true;
-                               $width = $params['width'];
-                               $params['width'] = $params['height'];
-                               $params['height'] = $width;
+                               list( $params['width'], $params['height'] ) = 
+                                       array(  $params['width'], $params['height'] );
+                               list( $params['physicalWidth'], $params['physicalHeight'] ) = 
+                                       array( $params['physicalWidth'], $params['physicalHeight'] );
                        }
                }
 
                # Don't make an image bigger than the source
-               $params['physicalWidth'] = $params['width'];
-               $params['physicalHeight'] = $params['height'];
-
                if ( $params['physicalWidth'] >= $srcWidth ) {
                        if ( $swapDimensions ) {
                                $params['physicalWidth'] = $srcHeight;
@@ -53,10 +51,11 @@ class BitmapHandler extends ImageHandler {
                                $params['physicalWidth'] = $srcWidth;
                                $params['physicalHeight'] = $srcHeight;
                        }
-                       # Skip scaling limit checks if no scaling is required
-                       if ( !$image->mustRender() )
-                               return true;
                }
+               
+               # Skip scaling limit checks if no scaling is required
+               if ( !$image->mustRender() )
+                       return true;            
 
                # Don't thumbnail an image so big that it will fill hard drives and send servers into swap
                # JPEG has the handy property of allowing thumbnailing without full decompression, so we make

diff --git a/includes/media/DjVu.php b/includes/media/DjVu.php
index 43d72aa..2833f68 100644 (file)
--- a/includes/media/DjVu.php
+++ b/includes/media/DjVu.php
@@ -147,7 +147,8 @@ class DjVuHandler extends ImageHandler {
 
                # Use a subshell (brackets) to aggregate stderr from both pipeline commands
                # before redirecting it to the overall stdout. This works in both Linux and Windows XP.
-               $cmd = '(' . wfEscapeShellArg( $wgDjvuRenderer ) . " -format=ppm -page={$page} -size={$width}x{$height} " .
+               $cmd = '(' . wfEscapeShellArg( $wgDjvuRenderer ) . " -format=ppm -page={$page}" . 
+                       " -size={$params['physicalWidth']}x{$params['physicalHeight']} " .
                        wfEscapeShellArg( $srcPath );
                if ( $wgDjvuPostProcessor ) {
                        $cmd .= " | {$wgDjvuPostProcessor}";

diff --git a/includes/media/Generic.php b/includes/media/Generic.php
index 2a52683..48735eb 100644 (file)
--- a/includes/media/Generic.php
+++ b/includes/media/Generic.php
@@ -570,13 +570,44 @@ abstract class ImageHandler extends MediaHandler {
 
                $srcWidth = $image->getWidth( $params['page'] );
                $srcHeight = $image->getHeight( $params['page'] );
+               
                if ( isset( $params['height'] ) && $params['height'] != -1 ) {
+                       # Height & width were both set
                        if ( $params['width'] * $srcHeight > $params['height'] * $srcWidth ) {
+                               # Height is the relative smaller dimension, so scale width accordingly
                                $params['width'] = wfFitBoxWidth( $srcWidth, $srcHeight, $params['height'] );
+                               
+                               if ( $params['width'] == 0 ) {
+                                       # Very small image, so we need to rely on client side scaling :(
+                                       $params['width'] = 1;
+                               }
+                               
+                               $params['physicalWidth'] = $params['width'];
+                       } else {
+                               # Height was crap, unset it so that it will be calculated later
+                               unset( $params['height'] );
                        }
                }
-               $params['height'] = File::scaleHeight( $srcWidth, $srcHeight, $params['width'] );
-               if ( !$this->validateThumbParams( $params['width'], $params['height'], $srcWidth, $srcHeight, $mimeType ) ) {
+               
+               if ( !isset( $params['physicalWidth'] ) ) {
+                       # Passed all validations, so set the physicalWidth
+                       $params['physicalWidth'] = $params['width'];
+               }
+               
+               # Because thumbs are only referred to by width, the height always needs
+               # to be scaled by the width to keep the thumbnail sizes consistent,
+               # even if it was set inside the if block above
+               $params['physicalHeight'] = File::scaleHeight( $srcWidth, $srcHeight, 
+                       $params['physicalWidth'] );
+
+               # Set the height if it was not validated in the if block higher up 
+               if ( !isset( $params['height'] ) || $params['height'] == -1 ) {
+                       $params['height'] = $params['physicalHeight'];
+               }
+
+               
+               if ( !$this->validateThumbParams( $params['physicalWidth'], 
+                               $params['physicalHeight'], $srcWidth, $srcHeight, $mimeType ) ) {
                        return false;
                }
                return true;
@@ -613,6 +644,10 @@ abstract class ImageHandler extends MediaHandler {
                }
 
                $height = File::scaleHeight( $srcWidth, $srcHeight, $width );
+               if ( $height == 0 ) {
+                       # Force height to be at least 1 pixel
+                       $height = 1;
+               }
                return true;
        }
 

diff --git a/includes/media/SVG.php b/includes/media/SVG.php
index a7565be..17fb946 100644 (file)
--- a/includes/media/SVG.php
+++ b/includes/media/SVG.php
@@ -59,8 +59,6 @@ class SvgHandler extends ImageHandler {
                        return false;
                }
                # Don't make an image bigger than wgMaxSVGSize
-               $params['physicalWidth'] = $params['width'];
-               $params['physicalHeight'] = $params['height'];
                if ( $params['physicalWidth'] > $wgSVGMaxSize ) {
                        $srcWidth = $image->getWidth( $params['page'] );
                        $srcHeight = $image->getHeight( $params['page'] );

diff --git a/tests/phpunit/includes/media/BitmapScalingTest.php b/tests/phpunit/includes/media/BitmapScalingTest.php
new file mode 100644 (file)
index 0000000..bca4b55
--- /dev/null
+++ b/tests/phpunit/includes/media/BitmapScalingTest.php
@@ -0,0 +1,96 @@
+<?php
+
+class BitmapScalingTest extends MediaWikiTestCase {
+       /**
+        * @dataProvider provideNormaliseParams
+        */
+       function testNormaliseParams( $fileDimensions, $expectedParams, $params, $msg ) {
+               $file = new FakeDimensionFile( $fileDimensions );
+               $handler = new BitmapHandler;
+               $handler->normaliseParams( $file, $params );
+               $this->assertEquals( $expectedParams, $params, $msg );
+       }
+       
+       function provideNormaliseParams() {
+               return array(
+                       /* Regular resize operations */ 
+                       array(
+                               array( 1024, 768 ),
+                               array( 
+                                       'width' => 512, 'height' => 384, 
+                                       'physicalWidth' => 512, 'physicalHeight' => 384,
+                                       'page' => 1,
+                               ),
+                               array( 'width' => 512 ),
+                               'Resizing with width set',
+                       ),
+                       array(
+                               array( 1024, 768 ),
+                               array( 
+                                       'width' => 512, 'height' => 384, 
+                                       'physicalWidth' => 512, 'physicalHeight' => 384,
+                                       'page' => 1, 
+                               ),
+                               array( 'width' => 512, 'height' => 768 ),
+                               'Resizing with height set too high',
+                       ),
+                       array(
+                               array( 1024, 768 ),
+                               array( 
+                                       'width' => 512, 'height' => 384, 
+                                       'physicalWidth' => 512, 'physicalHeight' => 384,
+                                       'page' => 1, 
+                               ),
+                               array( 'width' => 1024, 'height' => 384 ),
+                               'Resizing with height set',
+                       ),
+                       
+                       /* Very tall images */
+                       array(
+                               array( 1000, 100 ),
+                               array( 
+                                       'width' => 5, 'height' => 1,
+                                       'physicalWidth' => 5, 'physicalHeight' => 1,
+                                       'page' => 1, 
+                               ),
+                               array( 'width' => 5 ),
+                               'Very wide image',
+                       ),
+                       
+                       array(
+                               array( 100, 1000 ),
+                               array( 
+                                       'width' => 1, 'height' => 10,
+                                       'physicalWidth' => 1, 'physicalHeight' => 10,
+                                       'page' => 1, 
+                               ),
+                               array( 'width' => 1 ),
+                               'Very high image',
+                       ),
+                       array(
+                               array( 100, 1000 ),
+                               array( 
+                                       'width' => 1, 'height' => 5,
+                                       'physicalWidth' => 1, 'physicalHeight' => 10,
+                                       'page' => 1, 
+                               ),
+                               array( 'width' => 10, 'height' => 5 ),
+                               'Very high image with height set',
+                       ),
+               );
+       } 
+}
+
+class FakeDimensionFile extends File {
+       public function __construct( $dimensions ) {
+               parent::__construct( Title::makeTitle( NS_FILE, 'Test' ), null );
+               
+               $this->dimensions = $dimensions;
+       }
+       public function getWidth( $page = 1 ) {
+               return $this->dimensions[0];
+       }
+       public function getHeight( $page = 1 ) {
+               return $this->dimensions[1];
+       }
+}
\ No newline at end of file