Bug 1631615 - Don't color manage PNG/JPG/WebP if not tagged. r=tnikkel a=pascalc
authorAndrew Osmond <aosmond@mozilla.com>
Mon, 04 May 2020 21:51:25 +0000
changeset 591357 54ca458874c29dc4d7bec908d511929a6c9dfe53
parent 591356 9fcdff1da854a46f48646f2f87d38eea74714847
child 591358 b6e80ba0f44ea07d61fb3e2730a1ddee31ad229f
push id13087
push userarchaeopteryx@coole-files.de
push dateTue, 05 May 2020 17:41:19 +0000
treeherdermozilla-beta@c9a2a8be0647 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel, pascalc
bugs1631615
milestone77.0
Bug 1631615 - Don't color manage PNG/JPG/WebP if not tagged. r=tnikkel a=pascalc 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) {