From 57228556e66e3e0925412f7b22a1b5e13640cf71 Mon Sep 17 00:00:00 2001 From: Brian Wolff Date: Fri, 9 Sep 2011 20:13:09 +0000 Subject: [PATCH] (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. --- includes/media/Bitmap.php | 38 ++++++++-------- .../includes/media/BitmapScalingTest.php | 44 ++++++++++++++++++- .../includes/media/ExifRotationTest.php | 28 +++++++++--- 3 files changed, 83 insertions(+), 27 deletions(-) diff --git a/includes/media/Bitmap.php b/includes/media/Bitmap.php index 17593788b0..c90e6eea20 100644 --- 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 bca4b55a5f..5bcd32321e 100644 --- a/tests/phpunit/includes/media/BitmapScalingTest.php +++ b/tests/phpunit/includes/media/BitmapScalingTest.php @@ -1,13 +1,24 @@ 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 9d7f6b9ea9..d7e781f29e 100644 --- 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 ); + } } -- 2.20.1