From: Matthias Mullie <git@mullie.eu>
Date: Tue, 30 Aug 2016 14:36:09 +0000 (+0200)
Subject: Embed TinyRGB color profile when JPG EXIF Color Space = sRGB but no profile embedded
X-Git-Tag: 1.31.0-rc.0~3345^2
X-Git-Url: http://git.cyclocoop.org/url?a=commitdiff_plain;h=a428e3f067;p=lhc%2Fweb%2Fwiklou.git

Embed TinyRGB color profile when JPG EXIF Color Space = sRGB but no profile embedded

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
---

diff --git a/includes/media/ExifBitmap.php b/includes/media/ExifBitmap.php
index 7aeefa02e4..0e10abb9f9 100644
--- a/includes/media/ExifBitmap.php
+++ b/includes/media/ExifBitmap.php
@@ -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;
-	}
 }
diff --git a/includes/media/Jpeg.php b/includes/media/Jpeg.php
index b8b6f6c987..8631553346 100644
--- a/includes/media/Jpeg.php
+++ b/includes/media/Jpeg.php
@@ -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
index 0000000000..470c2d638f
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
index 0000000000..4085f0aa0e
Binary files /dev/null and b/tests/phpunit/data/media/missingprofile.jpg differ
diff --git a/tests/phpunit/data/media/srgb.jpg b/tests/phpunit/data/media/srgb.jpg
index b965dc4f6d..f10ced0d6a 100644
Binary files a/tests/phpunit/data/media/srgb.jpg and b/tests/phpunit/data/media/srgb.jpg differ
diff --git a/tests/phpunit/data/media/tinyrgb.jpg b/tests/phpunit/data/media/tinyrgb.jpg
index 12a8e09f90..63b687e38c 100644
Binary files a/tests/phpunit/data/media/tinyrgb.jpg and b/tests/phpunit/data/media/tinyrgb.jpg differ
diff --git a/tests/phpunit/includes/media/ExifBitmapTest.php b/tests/phpunit/includes/media/ExifBitmapTest.php
index 47ed67bdb1..3dd7e4c6bd 100644
--- a/tests/phpunit/includes/media/ExifBitmapTest.php
+++ b/tests/phpunit/includes/media/ExifBitmapTest.php
@@ -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'
-			]
-		];
-	}
 }
diff --git a/tests/phpunit/includes/media/JpegTest.php b/tests/phpunit/includes/media/JpegTest.php
index 05aed4ab21..b0f40ef006 100644
--- a/tests/phpunit/includes/media/JpegTest.php
+++ b/tests/phpunit/includes/media/JpegTest.php
@@ -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'
+			]
+		];
+	}
 }