Embed TinyRGB color profile when JPG EXIF Color Space = sRGB but no profile embedded
authorMatthias Mullie <git@mullie.eu>
Tue, 30 Aug 2016 14:36:09 +0000 (16:36 +0200)
committerMatthias Mullie <mmullie@wikimedia.org>
Tue, 15 Nov 2016 12:17:31 +0000 (12:17 +0000)
Existing srgb.jpg & tinyrgb.jpg have been replaced to be able to
easily compare a "fixed" missingprofile.jpg to tinyrgb.jpg.
With the existing files, when the tinyrgb profile was added to
missingprofile.jpg, it would end up basically the same as tinyrgb.jpg,
except that not all the exif data would be in the exact same order.
I've rebuilt srgb.jpg & tinyrgb.jpg by first removing their profile
(which is what missingprofile.jpg is), and then copying it over again:
    exiftool -tagsfromfile srgb.jpg -ICC_Profile new_srgb.jpg

Meanwhile also moved the profile-swapping code to JpegHandler, as it
was jpeg-specific.

Bug: T134498
Change-Id: I722dd6f66f6007182ad9a215e5eb382776983c05

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 b8b6f6c..8631553 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'
+                       ]
+               ];
+       }
 }