(bug 30640; follow-up r92279) Rotating images was making skewed images

This is Bryan's patch from bug 30640 with a couple minor related changes, plus some unit tests.
This also addresses an issue with preventing too-big images from being scaled from r92279.

I also noticed that image magick's rotation support is broken in 6.3.7 (the version I had installed locally. I've since upgraded) I'm not sure if we should be doing something about that...

I did test this without both image magick, and gd (although only very briefly with gd) both seemed to work well. I didn't test any other image scalars.

diff --git a/includes/media/Bitmap.php b/includes/media/Bitmap.php
index 1759378..c90e6ee 100644 (file)
--- a/includes/media/Bitmap.php
+++ b/includes/media/Bitmap.php
@@ -15,7 +15,9 @@ class BitmapHandler extends ImageHandler {
 
        /**
         * @param $image File
-        * @param  $params
+        * @param $params array Transform parameters. Entries with the keys 'width'
+        * and 'height' are the respective screen width and height, while the keys
+        * 'physicalWidth' and 'physicalHeight' indicate the thumbnail dimensions.
         * @return bool
         */
        function normaliseParams( $image, &$params ) {
@@ -25,37 +27,35 @@ class BitmapHandler extends ImageHandler {
                }
 
                $mimeType = $image->getMimeType();
+               # Obtain the source, pre-rotation dimensions
                $srcWidth = $image->getWidth( $params['page'] );
                $srcHeight = $image->getHeight( $params['page'] );
 
-               $swapDimensions = false;
+               # Don't make an image bigger than the source
+               if ( $params['physicalWidth'] >= $srcWidth ) {
+                       $params['physicalWidth'] = $srcWidth;
+                       $params['physicalHeight'] = $srcHeight;
+
+                       # Skip scaling limit checks if no scaling is required
+                       # due to requested size being bigger than source.
+                       if ( !$image->mustRender() ) {
+                               return true;
+                       }
+               }
+
+
                if ( self::canRotate() ) {
                        $rotation = $this->getRotation( $image );
                        if ( $rotation == 90 || $rotation == 270 ) {
                                wfDebug( __METHOD__ . ": Swapping width and height because the file will be rotated $rotation degrees\n" );
 
-                               $swapDimensions = true;
                                list( $params['width'], $params['height'] ) =
-                                       array(  $params['width'], $params['height'] );
+                                       array(  $params['height'], $params['width'] );
                                list( $params['physicalWidth'], $params['physicalHeight'] ) =
-                                       array( $params['physicalWidth'], $params['physicalHeight'] );
-                       }
-               }
-
-               # Don't make an image bigger than the source
-               if ( $params['physicalWidth'] >= $srcWidth ) {
-                       if ( $swapDimensions ) {
-                               $params['physicalWidth'] = $srcHeight;
-                               $params['physicalHeight'] = $srcWidth;
-                       } else {
-                               $params['physicalWidth'] = $srcWidth;
-                               $params['physicalHeight'] = $srcHeight;
+                                       array( $params['physicalHeight'], $params['physicalWidth'] );
                        }
                }
 
-               # 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/tests/phpunit/includes/media/BitmapScalingTest.php b/tests/phpunit/includes/media/BitmapScalingTest.php
index bca4b55..5bcd323 100644 (file)
--- a/tests/phpunit/includes/media/BitmapScalingTest.php
+++ b/tests/phpunit/includes/media/BitmapScalingTest.php
@@ -1,13 +1,24 @@
 <?php
 
 class BitmapScalingTest extends MediaWikiTestCase {
+
+       function setUp() {
+               global $wgMaxImageArea;
+               $this->oldMaxImageArea = $wgMaxImageArea;
+               $wgMaxImageArea = 1.25e7; // 3500x3500 
+       }
+       function tearDown() {
+               global $wgMaxImageArea;
+               $wgMaxImageArea = $this->oldMaxImageArea;
+       }
        /**
         * @dataProvider provideNormaliseParams
         */
        function testNormaliseParams( $fileDimensions, $expectedParams, $params, $msg ) {
                $file = new FakeDimensionFile( $fileDimensions );
                $handler = new BitmapHandler;
-               $handler->normaliseParams( $file, $params );
+               $valid = $handler->normaliseParams( $file, $params );
+               $this->assertTrue( $valid );
                $this->assertEquals( $expectedParams, $params, $msg );
        }
        
@@ -77,11 +88,37 @@ class BitmapScalingTest extends MediaWikiTestCase {
                                array( 'width' => 10, 'height' => 5 ),
                                'Very high image with height set',
                        ),
+                       /* Max image area */
+                       array(
+                               array( 4000, 4000 ),
+                               array( 
+                                       'width' => 5000, 'height' => 5000,
+                                       'physicalWidth' => 4000, 'physicalHeight' => 4000,
+                                       'page' => 1, 
+                               ),
+                               array( 'width' => 5000 ),
+                               'Bigger than max image size but doesn\'t need scaling',
+                       ),
                );
        } 
+       function testTooBigImage() {
+               $file = new FakeDimensionFile( array( 4000, 4000 ) );
+               $handler = new BitmapHandler;
+               $params = array( 'width' => '3700' ); // Still bigger than max size.
+               $this->assertFalse( $handler->normaliseParams( $file, $params ) );
+       }
+       function testTooBigMustRenderImage() {
+               $file = new FakeDimensionFile( array( 4000, 4000 ) );
+               $file->mustRender = true;
+               $handler = new BitmapHandler;
+               $params = array( 'width' => '5000' ); // Still bigger than max size.
+               $this->assertFalse( $handler->normaliseParams( $file, $params ) );
+       }
 }
 
 class FakeDimensionFile extends File {
+       public $mustRender = false;
+
        public function __construct( $dimensions ) {
                parent::__construct( Title::makeTitle( NS_FILE, 'Test' ), null );
                
@@ -93,4 +130,7 @@ class FakeDimensionFile extends File {
        public function getHeight( $page = 1 ) {
                return $this->dimensions[1];
        }
-}
\ No newline at end of file
+       public function mustRender() {
+               return $this->mustRender;
+       }
+}

diff --git a/tests/phpunit/includes/media/ExifRotationTest.php b/tests/phpunit/includes/media/ExifRotationTest.php
index 9d7f6b9..d7e781f 100644 (file)
--- a/tests/phpunit/includes/media/ExifRotationTest.php
+++ b/tests/phpunit/includes/media/ExifRotationTest.php
@@ -8,6 +8,7 @@ class ExifRotationTest extends MediaWikiTestCase {
        function setUp() {
                parent::setUp();
                $this->filePath = dirname( __FILE__ ) . '/../../data/media/';
+               $this->handler = new BitmapHandler();
        }
 
        /**
@@ -15,17 +16,16 @@ class ExifRotationTest extends MediaWikiTestCase {
         * @dataProvider providerFiles
         */
        function testMetadata( $name, $type, $info ) {
-               $handler = new BitmapHandler();
                # Force client side resizing
                $params = array( 'width' => 10000, 'height' => 10000 );
                $file = UnregisteredLocalFile::newFromPath( $this->filePath . $name, $type );
                
                # Normalize parameters
-               $this->assertTrue( $handler->normaliseParams( $file, $params ) );
-               $rotation = $handler->getRotation( $file );
+               $this->assertTrue( $this->handler->normaliseParams( $file, $params ) );
+               $rotation = $this->handler->getRotation( $file );
                
                # Check if pre-rotation dimensions are still good
-               list( $width, $height ) = $handler->extractPreRotationDimensions( $params, $rotation );
+               list( $width, $height ) = $this->handler->extractPreRotationDimensions( $params, $rotation );
                $this->assertEquals( $file->getWidth(), $width, 
                        "$name: pre-rotation width check, $rotation:$width" );
                $this->assertEquals( $file->getHeight(), $height, 
@@ -73,8 +73,7 @@ class ExifRotationTest extends MediaWikiTestCase {
         * @dataProvider provideBitmapExtractPreRotationDimensions
         */
        function testBitmapExtractPreRotationDimensions( $rotation, $expected ) {
-               $handler = new BitmapHandler;
-               $result = $handler->extractPreRotationDimensions( array(
+               $result = $this->handler->extractPreRotationDimensions( array(
                                'physicalWidth' => self::TEST_WIDTH, 
                                'physicalHeight' => self::TEST_HEIGHT,
                        ), $rotation );
@@ -101,5 +100,22 @@ class ExifRotationTest extends MediaWikiTestCase {
                        ),                      
                );
        }
+
+       function testWidthFlipping() {
+               $file = UnregisteredLocalFile::newFromPath( $this->filePath . 'portrait-rotated.jpg', 'image/jpeg' );
+               $params = array( 'width' => '50' );
+               $this->assertTrue( $this->handler->normaliseParams( $file, $params ) );
+
+               $this->assertEquals( 50, $params['height'] );
+               $this->assertEquals( round( (768/1024)*50 ), $params['width'], '', 0.1 );
+       }
+       function testWidthNotFlipping() {
+               $file = UnregisteredLocalFile::newFromPath( $this->filePath . 'landscape-plain.jpg', 'image/jpeg' );
+               $params = array( 'width' => '50' );
+               $this->assertTrue( $this->handler->normaliseParams( $file, $params ) );
+
+               $this->assertEquals( 50, $params['width'] );
+               $this->assertEquals( round( (768/1024)*50 ), $params['height'], '', 0.1 );
+       }
 }