Merge "Embed TinyRGB color profile when JPG EXIF Color Space = sRGB but no profile...
authorjenkins-bot <jenkins-bot@gerrit.wikimedia.org>
Thu, 4 May 2017 07:59:08 +0000 (07:59 +0000)
committerGerrit Code Review <gerrit@wikimedia.org>
Thu, 4 May 2017 07:59:08 +0000 (07:59 +0000)
includes/media/ExifBitmap.php
includes/media/Jpeg.php
tests/phpunit/data/media/adobergb.jpg [new file with mode: 0644]
tests/phpunit/data/media/missingprofile.jpg [new file with mode: 0644]
tests/phpunit/data/media/srgb.jpg
tests/phpunit/data/media/tinyrgb.jpg
tests/phpunit/includes/media/ExifBitmapTest.php
tests/phpunit/includes/media/JpegTest.php

index 7aeefa0..0e10abb 100644 (file)
@@ -30,7 +30,6 @@
 class ExifBitmapHandler extends BitmapHandler {
        const BROKEN_FILE = '-1'; // error extracting metadata
        const OLD_BROKEN_FILE = '0'; // outdated error extracting metadata.
-       const SRGB_ICC_PROFILE_NAME = 'IEC 61966-2.1 Default RGB colour space - sRGB';
 
        function convertMetadataVersion( $metadata, $version = 1 ) {
                // basically flattens arrays.
@@ -243,75 +242,4 @@ class ExifBitmapHandler extends BitmapHandler {
 
                return 0;
        }
-
-       protected function transformImageMagick( $image, $params ) {
-               global $wgUseTinyRGBForJPGThumbnails;
-
-               $ret = parent::transformImageMagick( $image, $params );
-
-               if ( $ret ) {
-                       return $ret;
-               }
-
-               if ( $params['mimeType'] === 'image/jpeg' && $wgUseTinyRGBForJPGThumbnails ) {
-                       // T100976 If the profile embedded in the JPG is sRGB, swap it for the smaller
-                       // (and free) TinyRGB
-
-                       $this->swapICCProfile(
-                               $params['dstPath'],
-                               self::SRGB_ICC_PROFILE_NAME,
-                               realpath( __DIR__ ) . '/tinyrgb.icc'
-                       );
-               }
-
-               return false;
-       }
-
-       /**
-        * Swaps an embedded ICC profile for another, if found.
-        * Depends on exiftool, no-op if not installed.
-        * @param string $filepath File to be manipulated (will be overwritten)
-        * @param string $oldProfileString Exact name of color profile to look for
-        *  (the one that will be replaced)
-        * @param string $profileFilepath ICC profile file to apply to the file
-        * @since 1.26
-        * @return bool
-        */
-       public function swapICCProfile( $filepath, $oldProfileString, $profileFilepath ) {
-               global $wgExiftool;
-
-               if ( !$wgExiftool || !is_executable( $wgExiftool ) ) {
-                       return false;
-               }
-
-               $cmd = wfEscapeShellArg( $wgExiftool,
-                       '-DeviceModelDesc',
-                       '-S',
-                       '-T',
-                       $filepath
-               );
-
-               $output = wfShellExecWithStderr( $cmd, $retval );
-
-               if ( $retval !== 0 || strcasecmp( trim( $output ), $oldProfileString ) !== 0 ) {
-                       // We can't establish that this file has the expected ICC profile, don't process it
-                       return false;
-               }
-
-               $cmd = wfEscapeShellArg( $wgExiftool,
-                       '-overwrite_original',
-                       '-icc_profile<=' . $profileFilepath,
-                       $filepath
-               );
-
-               $output = wfShellExecWithStderr( $cmd, $retval );
-
-               if ( $retval !== 0 ) {
-                       $this->logErrorForExternalProcess( $retval, $output, $cmd );
-
-                       return false;
-               }
-
-               return true;
-       }
 }
index c9f0dfa..5822699 100644 (file)
@@ -31,6 +31,8 @@
  * @ingroup Media
  */
 class JpegHandler extends ExifBitmapHandler {
+       const SRGB_EXIF_COLOR_SPACE = 'sRGB';
+       const SRGB_ICC_PROFILE_DESCRIPTION = 'sRGB IEC61966-2.1';
 
        function normaliseParams( $image, &$params ) {
                if ( !parent::normaliseParams( $image, $params ) ) {
@@ -171,4 +173,118 @@ class JpegHandler extends ExifBitmapHandler {
 
                return $params;
        }
+
+       /**
+        * {@inheritdoc}
+        */
+       protected function transformImageMagick( $image, $params ) {
+               global $wgUseTinyRGBForJPGThumbnails;
+
+               $ret = parent::transformImageMagick( $image, $params );
+
+               if ( $ret ) {
+                       return $ret;
+               }
+
+               if ( $wgUseTinyRGBForJPGThumbnails ) {
+                       // T100976 If the profile embedded in the JPG is sRGB, swap it for the smaller
+                       // (and free) TinyRGB
+
+                       /**
+                        * We'll want to replace the color profile for JPGs:
+                        * * in the sRGB color space, or with the sRGB profile
+                        *   (other profiles will be left untouched)
+                        * * without color space or profile, in which case browsers
+                        *   should assume sRGB, but don't always do (e.g. on wide-gamut
+                        *   monitors (unless it's meant for low bandwith)
+                        * @see https://phabricator.wikimedia.org/T134498
+                        */
+                       $colorSpaces = [ self::SRGB_EXIF_COLOR_SPACE, '-' ];
+                       $profiles = [ self::SRGB_ICC_PROFILE_DESCRIPTION ];
+
+                       // we'll also add TinyRGB profile to images lacking a profile, but
+                       // only if they're not low quality (which are meant to save bandwith
+                       // and we don't want to increase the filesize by adding a profile)
+                       if ( $params['quality'] > 30 ) {
+                               $profiles[] = '-';
+                       }
+
+                       $this->swapICCProfile(
+                               $params['dstPath'],
+                               $colorSpaces,
+                               $profiles,
+                               realpath( __DIR__ ) . '/tinyrgb.icc'
+                       );
+               }
+
+               return false;
+       }
+
+       /**
+        * Swaps an embedded ICC profile for another, if found.
+        * Depends on exiftool, no-op if not installed.
+        * @param string $filepath File to be manipulated (will be overwritten)
+        * @param array $colorSpaces Only process files with this/these Color Space(s)
+        * @param array $oldProfileStrings Exact name(s) of color profile to look for
+        *  (the one that will be replaced)
+        * @param string $profileFilepath ICC profile file to apply to the file
+        * @since 1.26
+        * @return bool
+        */
+       public function swapICCProfile( $filepath, array $colorSpaces,
+                                                                       array $oldProfileStrings, $profileFilepath
+       ) {
+               global $wgExiftool;
+
+               if ( !$wgExiftool || !is_executable( $wgExiftool ) ) {
+                       return false;
+               }
+
+               $cmd = wfEscapeShellArg( $wgExiftool,
+                       '-EXIF:ColorSpace',
+                       '-ICC_Profile:ProfileDescription',
+                       '-S',
+                       '-T',
+                       $filepath
+               );
+
+               $output = wfShellExecWithStderr( $cmd, $retval );
+
+               // Explode EXIF data into an array with [0 => Color Space, 1 => Device Model Desc]
+               $data = explode( "\t", trim( $output ) );
+
+               if ( $retval !== 0 ) {
+                       return false;
+               }
+
+               // Make a regex out of the source data to match it to an array of color
+               // spaces in a case-insensitive way
+               $colorSpaceRegex = '/'.preg_quote( $data[0], '/' ).'/i';
+               if ( empty( preg_grep( $colorSpaceRegex, $colorSpaces ) ) ) {
+                       // We can't establish that this file matches the color space, don't process it
+                       return false;
+               }
+
+               $profileRegex = '/'.preg_quote( $data[1], '/' ).'/i';
+               if ( empty( preg_grep( $profileRegex, $oldProfileStrings ) ) ) {
+                       // We can't establish that this file has the expected ICC profile, don't process it
+                       return false;
+               }
+
+               $cmd = wfEscapeShellArg( $wgExiftool,
+                       '-overwrite_original',
+                       '-icc_profile<=' . $profileFilepath,
+                       $filepath
+               );
+
+               $output = wfShellExecWithStderr( $cmd, $retval );
+
+               if ( $retval !== 0 ) {
+                       $this->logErrorForExternalProcess( $retval, $output, $cmd );
+
+                       return false;
+               }
+
+               return true;
+       }
 }
diff --git a/tests/phpunit/data/media/adobergb.jpg b/tests/phpunit/data/media/adobergb.jpg
new file mode 100644 (file)
index 0000000..470c2d6
Binary files /dev/null and b/tests/phpunit/data/media/adobergb.jpg differ
diff --git a/tests/phpunit/data/media/missingprofile.jpg b/tests/phpunit/data/media/missingprofile.jpg
new file mode 100644 (file)
index 0000000..4085f0a
Binary files /dev/null and b/tests/phpunit/data/media/missingprofile.jpg differ
index b965dc4..f10ced0 100644 (file)
Binary files a/tests/phpunit/data/media/srgb.jpg and b/tests/phpunit/data/media/srgb.jpg differ
index 12a8e09..63b687e 100644 (file)
Binary files a/tests/phpunit/data/media/tinyrgb.jpg and b/tests/phpunit/data/media/tinyrgb.jpg differ
index 47ed67b..3dd7e4c 100644 (file)
@@ -142,61 +142,4 @@ class ExifBitmapTest extends MediaWikiMediaTestCase {
                $res = $this->handler->convertMetadataVersion( $metadata, 1 );
                $this->assertEquals( $expected, $res );
        }
-
-       /**
-        * @dataProvider provideSwappingICCProfile
-        * @covers ExifBitmapHandler::swapICCProfile
-        */
-       public function testSwappingICCProfile(
-               $sourceFilename, $controlFilename, $newProfileFilename, $oldProfileName
-       ) {
-               global $wgExiftool;
-
-               if ( !$wgExiftool || !is_file( $wgExiftool ) ) {
-                       $this->markTestSkipped( "Exiftool not installed, cannot test ICC profile swapping" );
-               }
-
-               $this->setMwGlobals( 'wgUseTinyRGBForJPGThumbnails', true );
-
-               $sourceFilepath = $this->filePath . $sourceFilename;
-               $controlFilepath = $this->filePath . $controlFilename;
-               $profileFilepath = $this->filePath . $newProfileFilename;
-               $filepath = $this->getNewTempFile();
-
-               copy( $sourceFilepath, $filepath );
-
-               $file = $this->dataFile( $sourceFilename, 'image/jpeg' );
-               $this->handler->swapICCProfile( $filepath, $oldProfileName, $profileFilepath );
-
-               $this->assertEquals(
-                       sha1( file_get_contents( $filepath ) ),
-                       sha1( file_get_contents( $controlFilepath ) )
-               );
-       }
-
-       public function provideSwappingICCProfile() {
-               return [
-                       // File with sRGB should end up with TinyRGB
-                       [
-                               'srgb.jpg',
-                               'tinyrgb.jpg',
-                               'tinyrgb.icc',
-                               'IEC 61966-2.1 Default RGB colour space - sRGB'
-                       ],
-                       // File with TinyRGB should be left unchanged
-                       [
-                               'tinyrgb.jpg',
-                               'tinyrgb.jpg',
-                               'tinyrgb.icc',
-                               'IEC 61966-2.1 Default RGB colour space - sRGB'
-                       ],
-                       // File with no profile should be left unchanged
-                       [
-                               'test.jpg',
-                               'test.jpg',
-                               'tinyrgb.icc',
-                               'IEC 61966-2.1 Default RGB colour space - sRGB'
-                       ]
-               ];
-       }
 }
index 05aed4a..b0f40ef 100644 (file)
@@ -51,4 +51,73 @@ class JpegTest extends MediaWikiMediaTestCase {
 
                $this->assertEquals( $res, $expected );
        }
+
+       /**
+        * @dataProvider provideSwappingICCProfile
+        * @covers ExifBitmapHandler::swapICCProfile
+        */
+       public function testSwappingICCProfile(
+               $sourceFilename, $controlFilename, $newProfileFilename, $oldProfileName
+       ) {
+               global $wgExiftool;
+
+               if ( !$wgExiftool || !is_file( $wgExiftool ) ) {
+                       $this->markTestSkipped( "Exiftool not installed, cannot test ICC profile swapping" );
+               }
+
+               $this->setMwGlobals( 'wgUseTinyRGBForJPGThumbnails', true );
+
+               $sourceFilepath = $this->filePath . $sourceFilename;
+               $controlFilepath = $this->filePath . $controlFilename;
+               $profileFilepath = $this->filePath . $newProfileFilename;
+               $filepath = $this->getNewTempFile();
+
+               copy( $sourceFilepath, $filepath );
+
+               $file = $this->dataFile( $sourceFilename, 'image/jpeg' );
+               $this->handler->swapICCProfile(
+                       $filepath,
+                       [ 'sRGB', '-' ],
+                       [ $oldProfileName ],
+                       $profileFilepath
+               );
+
+               $this->assertEquals(
+                       sha1( file_get_contents( $filepath ) ),
+                       sha1( file_get_contents( $controlFilepath ) )
+               );
+       }
+
+       public function provideSwappingICCProfile() {
+               return [
+                       // File with sRGB should end up with TinyRGB
+                       [
+                               'srgb.jpg',
+                               'tinyrgb.jpg',
+                               'tinyrgb.icc',
+                               'sRGB IEC61966-2.1'
+                       ],
+                       // File with TinyRGB should be left unchanged
+                       [
+                               'tinyrgb.jpg',
+                               'tinyrgb.jpg',
+                               'tinyrgb.icc',
+                               'sRGB IEC61966-2.1'
+                       ],
+                       // File without profile should end up with TinyRGB
+                       [
+                               'missingprofile.jpg',
+                               'tinyrgb.jpg',
+                               'tinyrgb.icc',
+                               'sRGB IEC61966-2.1'
+                       ],
+                       // Non-sRGB file should be left untouched
+                       [
+                               'adobergb.jpg',
+                               'adobergb.jpg',
+                               'tinyrgb.icc',
+                               'sRGB IEC61966-2.1'
+                       ]
+               ];
+       }
 }