Bug 1631615 - Don't color manage PNG/JPG/WebP if not tagged. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Mon, 04 May 2020 21:51:25 +0000
changeset 527996 bf2757bbcf71a91d5e220dbed89218a52740f80e
parent 527995 427d247d7c2f9b10c48b5f5714613b6489d0bbd4
child 527997 557f9a4844738cebb4559f9eae50317b49f6f8bd
push id37379
push userccoroiu@mozilla.com
push dateTue, 05 May 2020 03:49:49 +0000
treeherdermozilla-central@786823305560 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1631615
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1631615 - Don't color manage PNG/JPG/WebP if not tagged. r=tnikkel It appears some websites assume we will not color manage untagged images and encode metadata in the image's surface data. Chrome matches this behaviour. We should probably do the same for webcompat. Differential Revision: https://phabricator.services.mozilla.com/D73737
image/decoders/nsJPEGDecoder.cpp
image/decoders/nsPNGDecoder.cpp
image/decoders/nsWebPDecoder.cpp
image/test/gtest/Common.cpp
image/test/gtest/Common.h
image/test/gtest/TestDecodeToSurface.cpp
--- a/image/decoders/nsJPEGDecoder.cpp
+++ b/image/decoders/nsJPEGDecoder.cpp
@@ -321,18 +321,16 @@ LexerTransition<nsJPEGDecoder::State> ns
               intent = qcms_profile_get_rendering_intent(mInProfile);
             }
 
             // Create the color management transform.
             mTransform = qcms_transform_create(mInProfile, *inputType,
                                                GetCMSOutputProfile(),
                                                outputType, (qcms_intent)intent);
           }
-        } else if (mCMSMode == eCMSMode_All) {
-          mTransform = GetCMSsRGBTransform(SurfaceFormat::OS_RGBX);
         }
       }
 
       // We don't want to use the pipe buffers directly because we don't want
       // any reads on non-BGRA formatted data.
       if (mInfo.out_color_space == JCS_GRAYSCALE ||
           mInfo.out_color_space == JCS_CMYK) {
         mCMSLine = new (std::nothrow) uint32_t[mInfo.image_width];
--- a/image/decoders/nsPNGDecoder.cpp
+++ b/image/decoders/nsPNGDecoder.cpp
@@ -680,18 +680,18 @@ void nsPNGDecoder::info_callback(png_str
         inType = QCMS_DATA_GRAY_8;
         outType = gfxPlatform::GetCMSOSRGBAType();
       }
     }
 
     decoder->mTransform = qcms_transform_create(decoder->mInProfile, inType,
                                                 decoder->GetCMSOutputProfile(),
                                                 outType, (qcms_intent)intent);
-  } else if ((sRGBTag && decoder->mCMSMode == eCMSMode_TaggedOnly) ||
-             decoder->mCMSMode == eCMSMode_All) {
+  } else if (sRGBTag && (decoder->mCMSMode == eCMSMode_TaggedOnly ||
+                         decoder->mCMSMode == eCMSMode_All)) {
     // If the transform happens with SurfacePipe, it will be in RGBA if we
     // have an alpha channel, because the swizzle and premultiplication
     // happens after color management. Otherwise it will be in OS_RGBA because
     // the swizzle happens at the start.
     if (transparency == TransparencyType::eAlpha) {
       decoder->mTransform =
           decoder->GetCMSsRGBTransform(SurfaceFormat::R8G8B8A8);
     } else {
--- a/image/decoders/nsWebPDecoder.cpp
+++ b/image/decoders/nsWebPDecoder.cpp
@@ -284,18 +284,17 @@ void nsWebPDecoder::EndFrame() {
   mLastRow = 0;
   ++mCurrentFrame;
 }
 
 void nsWebPDecoder::ApplyColorProfile(const char* aProfile, size_t aLength) {
   MOZ_ASSERT(!mGotColorProfile);
   mGotColorProfile = true;
 
-  if (mCMSMode == eCMSMode_Off || !GetCMSOutputProfile() ||
-      (mCMSMode == eCMSMode_TaggedOnly && !aProfile)) {
+  if (mCMSMode == eCMSMode_Off || !GetCMSOutputProfile() || !aProfile) {
     return;
   }
 
   if (!aProfile) {
     MOZ_LOG(sWebPLog, LogLevel::Debug,
             ("[this=%p] nsWebPDecoder::ApplyColorProfile -- not tagged, use "
              "sRGB transform\n",
              this));
--- a/image/test/gtest/Common.cpp
+++ b/image/test/gtest/Common.cpp
@@ -388,26 +388,27 @@ void CheckTransformedWritePixels(
                         BGRAColor::Transparent(), aFuzz);
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Test Data
 ///////////////////////////////////////////////////////////////////////////////
 
 ImageTestCase GreenPNGTestCase() {
-  return ImageTestCase("green.png", "image/png", IntSize(100, 100));
+  return ImageTestCase("green.png", "image/png", IntSize(100, 100),
+                       TEST_CASE_ASSUME_SRGB_OUTPUT);
 }
 
 ImageTestCase GreenGIFTestCase() {
   return ImageTestCase("green.gif", "image/gif", IntSize(100, 100));
 }
 
 ImageTestCase GreenJPGTestCase() {
   return ImageTestCase("green.jpg", "image/jpeg", IntSize(100, 100),
-                       TEST_CASE_IS_FUZZY);
+                       TEST_CASE_IS_FUZZY | TEST_CASE_ASSUME_SRGB_OUTPUT);
 }
 
 ImageTestCase GreenBMPTestCase() {
   return ImageTestCase("green.bmp", "image/bmp", IntSize(100, 100));
 }
 
 ImageTestCase GreenICOTestCase() {
   // This ICO contains a 32-bit BMP, and we use a BMP's alpha data by default
@@ -417,17 +418,18 @@ ImageTestCase GreenICOTestCase() {
 }
 
 ImageTestCase GreenIconTestCase() {
   return ImageTestCase("green.icon", "image/icon", IntSize(100, 100),
                        TEST_CASE_IS_TRANSPARENT);
 }
 
 ImageTestCase GreenWebPTestCase() {
-  return ImageTestCase("green.webp", "image/webp", IntSize(100, 100));
+  return ImageTestCase("green.webp", "image/webp", IntSize(100, 100),
+                       TEST_CASE_ASSUME_SRGB_OUTPUT);
 }
 
 // Forcing sRGB is required until nsAVIFDecoder supports ICC profiles
 // See bug 1634741
 ImageTestCase GreenAVIFTestCase() {
   return ImageTestCase("green.avif", "image/avif", IntSize(100, 100))
       .WithSurfaceFlags(SurfaceFlags::TO_SRGB_COLORSPACE);
 }
@@ -448,37 +450,41 @@ ImageTestCase GreenWebPIccSrgbTestCase()
 
 ImageTestCase GreenFirstFrameAnimatedGIFTestCase() {
   return ImageTestCase("first-frame-green.gif", "image/gif", IntSize(100, 100),
                        TEST_CASE_IS_ANIMATED);
 }
 
 ImageTestCase GreenFirstFrameAnimatedPNGTestCase() {
   return ImageTestCase("first-frame-green.png", "image/png", IntSize(100, 100),
-                       TEST_CASE_IS_TRANSPARENT | TEST_CASE_IS_ANIMATED);
+                       TEST_CASE_IS_TRANSPARENT | TEST_CASE_IS_ANIMATED |
+                           TEST_CASE_ASSUME_SRGB_OUTPUT);
 }
 
 ImageTestCase GreenFirstFrameAnimatedWebPTestCase() {
   return ImageTestCase("first-frame-green.webp", "image/webp",
-                       IntSize(100, 100), TEST_CASE_IS_ANIMATED);
+                       IntSize(100, 100),
+                       TEST_CASE_IS_ANIMATED | TEST_CASE_ASSUME_SRGB_OUTPUT);
 }
 
 ImageTestCase BlendAnimatedGIFTestCase() {
   return ImageTestCase("blend.gif", "image/gif", IntSize(100, 100),
                        TEST_CASE_IS_ANIMATED);
 }
 
 ImageTestCase BlendAnimatedPNGTestCase() {
   return ImageTestCase("blend.png", "image/png", IntSize(100, 100),
-                       TEST_CASE_IS_TRANSPARENT | TEST_CASE_IS_ANIMATED);
+                       TEST_CASE_IS_TRANSPARENT | TEST_CASE_IS_ANIMATED |
+                           TEST_CASE_ASSUME_SRGB_OUTPUT);
 }
 
 ImageTestCase BlendAnimatedWebPTestCase() {
   return ImageTestCase("blend.webp", "image/webp", IntSize(100, 100),
-                       TEST_CASE_IS_TRANSPARENT | TEST_CASE_IS_ANIMATED);
+                       TEST_CASE_IS_TRANSPARENT | TEST_CASE_IS_ANIMATED |
+                           TEST_CASE_ASSUME_SRGB_OUTPUT);
 }
 
 ImageTestCase CorruptTestCase() {
   return ImageTestCase("corrupt.jpg", "image/jpeg", IntSize(100, 100),
                        TEST_CASE_HAS_ERROR);
 }
 
 ImageTestCase CorruptBMPWithTruncatedHeader() {
@@ -637,17 +643,18 @@ ImageTestCase LargeICOWithBMPTestCase() 
 
 ImageTestCase LargeICOWithPNGTestCase() {
   return ImageTestCase("green-large-png.ico", "image/x-icon", IntSize(512, 512),
                        TEST_CASE_IS_TRANSPARENT);
 }
 
 ImageTestCase GreenMultipleSizesICOTestCase() {
   return ImageTestCase("green-multiple-sizes.ico", "image/x-icon",
-                       IntSize(256, 256));
+                       IntSize(256, 256))
+      .WithSurfaceFlags(SurfaceFlags::TO_SRGB_COLORSPACE);
 }
 
 ImageTestCase PerfGrayJPGTestCase() {
   return ImageTestCase("perf_gray.jpg", "image/jpeg", IntSize(1000, 1000));
 }
 
 ImageTestCase PerfCmykJPGTestCase() {
   return ImageTestCase("perf_cmyk.jpg", "image/jpeg", IntSize(1000, 1000));
--- a/image/test/gtest/Common.h
+++ b/image/test/gtest/Common.h
@@ -102,16 +102,17 @@ struct BGRAColor {
 
 enum TestCaseFlags {
   TEST_CASE_DEFAULT_FLAGS = 0,
   TEST_CASE_IS_FUZZY = 1 << 0,
   TEST_CASE_HAS_ERROR = 1 << 1,
   TEST_CASE_IS_TRANSPARENT = 1 << 2,
   TEST_CASE_IS_ANIMATED = 1 << 3,
   TEST_CASE_IGNORE_OUTPUT = 1 << 4,
+  TEST_CASE_ASSUME_SRGB_OUTPUT = 1 << 5,
 };
 
 struct ImageTestCase {
   ImageTestCase(const char* aPath, const char* aMimeType, gfx::IntSize aSize,
                 uint32_t aFlags = TEST_CASE_DEFAULT_FLAGS)
       : mPath(aPath),
         mMimeType(aMimeType),
         mSize(aSize),
@@ -133,17 +134,22 @@ struct ImageTestCase {
 
   ImageTestCase WithSurfaceFlags(SurfaceFlags aSurfaceFlags) const {
     ImageTestCase self = *this;
     self.mSurfaceFlags = aSurfaceFlags;
     return self;
   }
 
   BGRAColor ChooseColor(const BGRAColor& aColor) const {
-    if (mSurfaceFlags & SurfaceFlags::TO_SRGB_COLORSPACE) {
+    // If we are forcing the output to be sRGB via the surface flag, or the
+    // test case is marked as assuming sRGB (used when the image itself is not
+    // explicitly tagged, and as a result, imagelib won't perform any color
+    // conversion), we should use the sRGB presentation of the color.
+    if ((mSurfaceFlags & SurfaceFlags::TO_SRGB_COLORSPACE) ||
+        (mFlags & TEST_CASE_ASSUME_SRGB_OUTPUT)) {
       return aColor.sRGBColor();
     }
     return aColor.DeviceColor();
   }
 
   BGRAColor Color() const { return ChooseColor(mColor); }
 
   uint8_t Fuzz() const {
--- a/image/test/gtest/TestDecodeToSurface.cpp
+++ b/image/test/gtest/TestDecodeToSurface.cpp
@@ -39,24 +39,26 @@ class DecodeToSurfaceRunnable : public R
   }
 
   void Go() {
     Maybe<IntSize> outputSize;
     if (mTestCase.mOutputSize != mTestCase.mSize) {
       outputSize.emplace(mTestCase.mOutputSize);
     }
 
+    uint32_t flags = FromSurfaceFlags(mTestCase.mSurfaceFlags);
+
     if (mImageBuffer) {
       mSurface = ImageOps::DecodeToSurface(
-          mImageBuffer, nsDependentCString(mTestCase.mMimeType),
-          imgIContainer::DECODE_FLAGS_DEFAULT, outputSize);
+          mImageBuffer, nsDependentCString(mTestCase.mMimeType), flags,
+          outputSize);
     } else {
       mSurface = ImageOps::DecodeToSurface(
-          mInputStream.forget(), nsDependentCString(mTestCase.mMimeType),
-          imgIContainer::DECODE_FLAGS_DEFAULT, outputSize);
+          mInputStream.forget(), nsDependentCString(mTestCase.mMimeType), flags,
+          outputSize);
     }
     ASSERT_TRUE(mSurface != nullptr);
 
     EXPECT_TRUE(mSurface->IsDataSourceSurface());
     EXPECT_TRUE(mSurface->GetFormat() == SurfaceFormat::OS_RGBX ||
                 mSurface->GetFormat() == SurfaceFormat::OS_RGBA);
 
     if (outputSize) {