jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/307511 )
Change subject: Embed TinyRGB color profile when JPG EXIF Color Space = sRGB
but no profile embedded
......................................................................
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
---
M includes/media/ExifBitmap.php
M includes/media/Jpeg.php
A tests/phpunit/data/media/adobergb.jpg
A tests/phpunit/data/media/missingprofile.jpg
M tests/phpunit/data/media/srgb.jpg
M tests/phpunit/data/media/tinyrgb.jpg
M tests/phpunit/includes/media/ExifBitmapTest.php
M tests/phpunit/includes/media/JpegTest.php
8 files changed, 185 insertions(+), 129 deletions(-)
Approvals:
jenkins-bot: Verified
Gilles: Looks good to me, approved
diff --git a/includes/media/ExifBitmap.php b/includes/media/ExifBitmap.php
index 7aeefa0..0e10abb 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.
@@ -242,76 +241,5 @@
}
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 b8b6f6c..8631553 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 @@
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 0000000..470c2d6
--- /dev/null
+++ b/tests/phpunit/data/media/adobergb.jpg
Binary files differ
diff --git a/tests/phpunit/data/media/missingprofile.jpg
b/tests/phpunit/data/media/missingprofile.jpg
new file mode 100644
index 0000000..4085f0a
--- /dev/null
+++ b/tests/phpunit/data/media/missingprofile.jpg
Binary files differ
diff --git a/tests/phpunit/data/media/srgb.jpg
b/tests/phpunit/data/media/srgb.jpg
index b965dc4..f10ced0 100644
--- a/tests/phpunit/data/media/srgb.jpg
+++ b/tests/phpunit/data/media/srgb.jpg
Binary files differ
diff --git a/tests/phpunit/data/media/tinyrgb.jpg
b/tests/phpunit/data/media/tinyrgb.jpg
index 12a8e09..63b687e 100644
--- a/tests/phpunit/data/media/tinyrgb.jpg
+++ b/tests/phpunit/data/media/tinyrgb.jpg
Binary files differ
diff --git a/tests/phpunit/includes/media/ExifBitmapTest.php
b/tests/phpunit/includes/media/ExifBitmapTest.php
index 47ed67b..3dd7e4c 100644
--- a/tests/phpunit/includes/media/ExifBitmapTest.php
+++ b/tests/phpunit/includes/media/ExifBitmapTest.php
@@ -142,61 +142,4 @@
$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 05aed4a..b0f40ef 100644
--- a/tests/phpunit/includes/media/JpegTest.php
+++ b/tests/phpunit/includes/media/JpegTest.php
@@ -51,4 +51,73 @@
$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'
+ ]
+ ];
+ }
}
--
To view, visit https://gerrit.wikimedia.org/r/307511
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I722dd6f66f6007182ad9a215e5eb382776983c05
Gerrit-PatchSet: 7
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Matthias Mullie <[email protected]>
Gerrit-Reviewer: Brian Wolff <[email protected]>
Gerrit-Reviewer: Gilles <[email protected]>
Gerrit-Reviewer: Matthias Mullie <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
