Follow-up r79845: Fix rotation. Turns out that we need to pass the pre-rotation dimen...

diff --git a/includes/media/Bitmap.php b/includes/media/Bitmap.php
index be4fb2d..665aca5 100644 (file)
--- a/includes/media/Bitmap.php
+++ b/includes/media/Bitmap.php
@@ -28,11 +28,13 @@ class BitmapHandler extends ImageHandler {
                $srcWidth = $image->getWidth( $params['page'] );
                $srcHeight = $image->getHeight( $params['page'] );
 
+               $swapDimensions = false;
                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;
                                $width = $params['width'];
                                $params['width'] = $params['height'];
                                $params['height'] = $width;
@@ -44,8 +46,13 @@ class BitmapHandler extends ImageHandler {
                $params['physicalHeight'] = $params['height'];
 
                if ( $params['physicalWidth'] >= $srcWidth ) {
-                       $params['physicalWidth'] = $srcWidth;
-                       $params['physicalHeight'] = $srcHeight;
+                       if ( $swapDimensions ) {
+                               $params['physicalWidth'] = $srcHeight;
+                               $params['physicalHeight'] = $srcWidth;                          
+                       } else {
+                               $params['physicalWidth'] = $srcWidth;
+                               $params['physicalHeight'] = $srcHeight;
+                       }
                        # Skip scaling limit checks if no scaling is required
                        if ( !$image->mustRender() )
                                return true;
@@ -63,6 +70,25 @@ class BitmapHandler extends ImageHandler {
 
                return true;
        }
+       
+       /**
+        * Extracts the width/height if the image will be scaled before rotating
+        * 
+        * @param $params array Parameters as returned by normaliseParams
+        * @param $rotation int The rotation angle that will be applied
+        * @return array ($width, $height) array
+        */
+       public function extractPreRotationDimensions( $params, $rotation ) {
+               if ( $rotation == 90 || $rotation == 270 ) {
+                       # We'll resize before rotation, so swap the dimensions again
+                       $width = $params['physicalHeight'];
+                       $height = $params['physicalWidth'];
+               } else {
+                       $width = $params['physicalWidth'];
+                       $height = $params['physicalHeight'];
+               }
+               return array( $width, $height );
+       }
 
 
        // Function that returns the number of pixels to be thumbnailed.
@@ -287,6 +313,9 @@ class BitmapHandler extends ImageHandler {
                if ( strval( $wgImageMagickTempDir ) !== '' ) {
                        $env['MAGICK_TMPDIR'] = $wgImageMagickTempDir;
                }
+               
+               $rotation = $this->getRotation( $image );
+               list( $width, $height ) = $this->extractPreRotationDimensions( $params, $rotation );
 
                $cmd  =
                        wfEscapeShellArg( $wgImageMagickConvertCommand ) .
@@ -299,7 +328,7 @@ class BitmapHandler extends ImageHandler {
                        // For the -thumbnail option a "!" is needed to force exact size,
                        // or ImageMagick may decide your ratio is wrong and slice off
                        // a pixel.
-                       " -thumbnail " . wfEscapeShellArg( "{$params['physicalDimensions']}!" ) .
+                       " -thumbnail " . wfEscapeShellArg( "{$width}x{$height}!" ) .
                        // Add the source url as a comment to the thumb, but don't add the flag if there's no comment
                        ( $params['comment'] !== ''
                                ? " -set comment " . wfEscapeShellArg( $this->escapeMagickProperty( $params['comment'] ) )
@@ -362,14 +391,7 @@ class BitmapHandler extends ImageHandler {
                        }
 
                        $rotation = $this->getRotation( $image );
-                       if ( $rotation == 90 || $rotation == 270 ) {
-                               // We'll resize before rotation, so swap the dimensions again
-                               $width = $params['physicalHeight'];
-                               $height = $params['physicalWidth'];
-                       } else {
-                               $width = $params['physicalWidth'];
-                               $height = $params['physicalHeight'];
-                       }
+                       list( $width, $height ) = $this->extractPreRotationDimensions( $params, $rotation );
 
                        $im->setImageBackgroundColor( new ImagickPixel( 'white' ) );
 
@@ -509,14 +531,7 @@ class BitmapHandler extends ImageHandler {
                $src_image = call_user_func( $loader, $params['srcPath'] );
 
                $rotation = function_exists( 'imagerotate' ) ? $this->getRotation( $image ) : 0;
-               if ( $rotation == 90 || $rotation == 270 ) {
-                       # We'll resize before rotation, so swap the dimensions again
-                       $width = $params['physicalHeight'];
-                       $height = $params['physicalWidth'];
-               } else {
-                       $width = $params['physicalWidth'];
-                       $height = $params['physicalHeight'];
-               }
+               list( $width, $height ) = $this->extractPreRotationDimensions( $params, $rotation );
                $dst_image = imagecreatetruecolor( $width, $height );
 
                // Initialise the destination image to transparent instead of

diff --git a/tests/phpunit/includes/media/ExifRotationTest.php b/tests/phpunit/includes/media/ExifRotationTest.php
index b3e2aae..1d4b7c5 100644 (file)
--- a/tests/phpunit/includes/media/ExifRotationTest.php
+++ b/tests/phpunit/includes/media/ExifRotationTest.php
@@ -1,11 +1,12 @@
 <?php
 
 /**
- * @group Broken
+ * Tests related to auto rotation
  */
 class ExifRotationTest extends MediaWikiTestCase {
 
        function setUp() {
+               parent::setUp();
        }
 
        /**
@@ -14,10 +15,32 @@ class ExifRotationTest extends MediaWikiTestCase {
         */
        function testMetadata( $name, $type, $info ) {
                $handler = new BitmapHandler();
-
+               # Force client side resizing
+               $params = array( 'width' => 10000, 'height' => 10000 );
                $file = UnregisteredLocalFile::newFromPath( dirname( __FILE__ ) . '/' . $name, $type );
-               $this->assertEquals( $file->getWidth(), $info['width'], "$name: width check" );
-               $this->assertEquals( $file->getHeight(), $info['height'], "$name: height check" );
+               
+               # Normalize parameters
+               $this->assertTrue( $handler->normaliseParams( $file, $params ) );
+               $rotation = $handler->getRotation( $file );
+               
+               # Check if pre-rotation dimensions are still good
+               list( $width, $height ) = $handler->extractPreRotationDimensions( $params, $rotation );
+               $this->assertEquals( $file->getWidth(), $width, 
+                       "$name: pre-rotation width check, $rotation:$width" );
+               $this->assertEquals( $file->getHeight(), $height, 
+                       "$name: pre-rotation height check, $rotation" );
+               
+               # Any further test require a scaler that can rotate
+               if ( !BitmapHandler::canRotate() ) {
+                       $this->markTestIncomplete( 'Scaler does not support rotation' );
+                       return;
+               }
+               
+               # Check post-rotation width
+               $this->assertEquals( $params['physicalWidth'], $info['width'], 
+                       "$name: post-rotation width check" );
+               $this->assertEquals( $params['physicalHeight'], $info['height'], 
+                       "$name: post-rotation height check" );
        }
 
        function providerFiles() {
@@ -40,5 +63,42 @@ class ExifRotationTest extends MediaWikiTestCase {
                        )
                );
        }
+       
+       
+       const TEST_WIDTH = 100;
+       const TEST_HEIGHT = 200;
+       
+       /**
+        * @dataProvider provideBitmapExtractPreRotationDimensions
+        */
+       function testBitmapExtractPreRotationDimensions( $rotation, $expected ) {
+               $handler = new BitmapHandler;
+               $result = $handler->extractPreRotationDimensions( array(
+                               'physicalWidth' => self::TEST_WIDTH, 
+                               'physicalHeight' => self::TEST_HEIGHT,
+                       ), $rotation );
+               $this->assertEquals( $expected, $result );
+       }
+       
+       function provideBitmapExtractPreRotationDimensions() {
+               return array(
+                       array(
+                               0,
+                               array( self::TEST_WIDTH, self::TEST_HEIGHT ) 
+                       ),
+                       array(
+                               90,
+                               array( self::TEST_HEIGHT, self::TEST_WIDTH ) 
+                       ),
+                       array(
+                               180,
+                               array( self::TEST_WIDTH, self::TEST_HEIGHT ) 
+                       ),
+                       array(
+                               270,
+                               array( self::TEST_HEIGHT, self::TEST_WIDTH ) 
+                       ),                      
+               );
+       }
 }