[PNGImageDecoder] Clean up readColorSpace, TODOs, cruft

third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp

 /*
  * Copyright (C) 2006 Apple Computer, Inc.
  * Copyright (C) Research In Motion Limited 2009-2010. All rights reserved.
  *
  * Portions are Copyright (C) 2001 mozilla.org
  *
  * Other contributors:
  *   Stuart Parmenter <stuart@mozilla.com>
  *
  * This library is free software; you can redistribute it and/or
@@ skipping 36 matching lines @@
 
 PNGImageDecoder::PNGImageDecoder(AlphaOption alphaOption,
                                  const ColorBehavior& colorBehavior,
                                  size_t maxDecodedBytes,
                                  size_t offset)
     : ImageDecoder(alphaOption, colorBehavior, maxDecodedBytes),
       m_offset(offset) {}
 
 PNGImageDecoder::~PNGImageDecoder() {}
 
-inline float pngFixedToFloat(png_fixed_point x) {
-  return ((float)x) * 0.00001f;
-}
+inline sk_sp<SkColorSpace> readColorSpace(png_structp png, png_infop info) {
+  if (png_get_valid(png, info, PNG_INFO_sRGB))
+    return SkColorSpace::MakeSRGB();
 
-inline sk_sp<SkColorSpace> readColorSpace(png_structp png, png_infop info) {
-  if (png_get_valid(png, info, PNG_INFO_sRGB)) {
-    return SkColorSpace::MakeSRGB();
-  }
-
-  png_charp name = nullptr;
-  int compression = 0;
-  png_bytep profile = nullptr;
-  png_uint_32 length = 0;
-  if (png_get_iCCP(png, info, &name, &compression, &profile, &length)) {
+  png_charp name;
+  int compression;
+  png_bytep profile;
+  png_uint_32 length;
+  if (png_get_iCCP(png, info, &name, &compression, &profile, &length))
     return SkColorSpace::MakeICC(profile, length);
-  }
 
   png_fixed_point chrm[8];
-  if (png_get_cHRM_fixed(png, info, &chrm[0], &chrm[1], &chrm[2], &chrm[3],
-                         &chrm[4], &chrm[5], &chrm[6], &chrm[7])) {
-    SkColorSpacePrimaries primaries;
-    primaries.fRX = pngFixedToFloat(chrm[2]);
-    primaries.fRY = pngFixedToFloat(chrm[3]);
-    primaries.fGX = pngFixedToFloat(chrm[4]);
-    primaries.fGY = pngFixedToFloat(chrm[5]);
-    primaries.fBX = pngFixedToFloat(chrm[6]);
-    primaries.fBY = pngFixedToFloat(chrm[7]);
-    primaries.fWX = pngFixedToFloat(chrm[0]);
-    primaries.fWY = pngFixedToFloat(chrm[1]);
+  if (!png_get_cHRM_fixed(png, info, &chrm[0], &chrm[1], &chrm[2], &chrm[3],
+                          &chrm[4], &chrm[5], &chrm[6], &chrm[7]))
+    return nullptr;
 
-    SkMatrix44 toXYZD50(SkMatrix44::kUninitialized_Constructor);
-    if (primaries.toXYZD50(&toXYZD50)) {
-      png_fixed_point gammaFixed;
-      if (PNG_INFO_gAMA == png_get_gAMA_fixed(png, info, &gammaFixed)) {
-        SkColorSpaceTransferFn fn;
-        fn.fA = 1.0f;
-        fn.fB = fn.fC = fn.fD = fn.fE = fn.fF = 0.0f;
-        // This is necessary because the gAMA chunk actually stores 1/gamma.
-        fn.fG = 1.0f / pngFixedToFloat(gammaFixed);
-        return SkColorSpace::MakeRGB(fn, toXYZD50);
-      }
+  png_fixed_point inverseGamma;
+  if (!png_get_gAMA_fixed(png, info, &inverseGamma))
+    return nullptr;
 
-      // Note that we only use the cHRM tag when gAMA is present.  The
-      // specification states that the cHRM is valid even without a gAMA
-      // tag, but we cannot apply the cHRM without guessing a transfer
-      // function.  It's possible that we should guess sRGB transfer
-      // function, given that unmarked PNGs should be treated as sRGB.
-      // However, the current behavior matches Safari and Firefox.

[Noel Gordon, 2017/02/23 10:53:41]

"... that unmarked PNGs should be treated as sRGB" is a matter for some
higher-layer to tell us, via a blink config flag or similar.

We should not try to guess about profiles, nor try to patch over their
bogus-ness, we should just deem them invalid.

There is only one exception to this that know of: when a color profile has
degenerate gamma curves. Refer to https://crbg.com/492379

[msarett1, 2017/02/23 14:07:03]

I don't completely agree.  It is widely accepted that untagged images should be
treated as sRGB - this is how Safari behaves and how Chrome intends to behave.

Images with only cHRM are a strange case - I would argue that they are not
"invalid".  Right now, we ignore the tags, which seems fine.  When we start
treating untagged images as sRGB, these would naturally be treated as sRGB as
well.  This, to me, seems worse than at least paying attention to the gamut
specified by the cHRM.

Of course, if our goal is to match Safari, I understand that as well.

[Noel Gordon, 2017/02/23 18:06:47]

Not sure you understood this point, but the crux is about following the specs.
Sure, in a browser, per the CSS specs.  The comment as written seemed to suggest
to me that we just do it, but talking about CSS needs in an image decoder is
probably not the right place to mention it.

This image decoder does what is told by the system we are in (the browser),
perhaps by a run-time flag, or perhaps via an decoder argument.  We do have an
argument that enables/disables color correction, and WebGL needs that (a
disable), but we don't talk/comment about WebGL in this code, right?  So no need
to add comments about CSS spec issues like sRGB, I feel.
Yes strange case cHRM: and no matter what we do, we'll not have good compat
here.  The lone cHRM is not well-specified by the spec, and that results in
different interpretations in different implementations.  Treating it as sRGB
might gain some compat, but I agree, the render might be worse as you suggest,
which creates yet another compat issue :)
 
Our goal is compat in browsers, but lack of decent spec text (lone cHRM handling
here) leads to compat problems.

-    }
-  }
+  // cHRM and gAMA tags are both present. The PNG spec states that cHRM is
+  // valid even without gAMA but we cannot apply the cHRM without guessing
+  // a gAMA. Color correction is not a guessing game: match the behavior
+  // of Safari and Firefox instead (compat).
 
-  return nullptr;
+  struct pngFixedToFloat {
+    explicit pngFixedToFloat(png_fixed_point value)
+        : floatValue(.00001f * value) {}
+    operator float() { return floatValue; }
+    float floatValue;
+  };
+
+  SkColorSpacePrimaries primaries;
+  primaries.fRX = pngFixedToFloat(chrm[2]);
+  primaries.fRY = pngFixedToFloat(chrm[3]);
+  primaries.fGX = pngFixedToFloat(chrm[4]);
+  primaries.fGY = pngFixedToFloat(chrm[5]);
+  primaries.fBX = pngFixedToFloat(chrm[6]);
+  primaries.fBY = pngFixedToFloat(chrm[7]);
+  primaries.fWX = pngFixedToFloat(chrm[0]);
+  primaries.fWY = pngFixedToFloat(chrm[1]);
+
+  SkMatrix44 toXYZD50(SkMatrix44::kUninitialized_Constructor);
+  if (!primaries.toXYZD50(&toXYZD50))
+    return nullptr;
+
+  SkColorSpaceTransferFn fn;
+  fn.fG = 1.0f / pngFixedToFloat(inverseGamma);
+  fn.fA = 1.0f;
+  fn.fB = fn.fC = fn.fD = fn.fE = fn.fF = 0.0f;
+
+  return SkColorSpace::MakeRGB(fn, toXYZD50);
 }
 
 void PNGImageDecoder::headerAvailable() {
   png_structp png = m_reader->pngPtr();
   png_infop info = m_reader->infoPtr();
   png_uint_32 width = png_get_image_width(png, info);
   png_uint_32 height = png_get_image_height(png, info);
 
   // Protect against large PNGs. See http://bugzil.la/251381 for more details.
   const unsigned long maxPNGSize = 1000000UL;
   if (width > maxPNGSize || height > maxPNGSize) {
     longjmp(JMPBUF(png), 1);
     return;
   }
 
   // Set the image size now that the image header is available.
   if (!setSize(width, height)) {
     longjmp(JMPBUF(png), 1);
     return;
   }
 
-  int bitDepth, colorType, interlaceType, compressionType, filterType, channels;
+  int bitDepth, colorType, interlaceType, compressionType;
   png_get_IHDR(png, info, &width, &height, &bitDepth, &colorType,
-               &interlaceType, &compressionType, &filterType);
+               &interlaceType, &compressionType, nullptr);
 
   // The options we set here match what Mozilla does.
 
   // Expand to ensure we use 24-bit for RGB and 32-bit for RGBA.
   if (colorType == PNG_COLOR_TYPE_PALETTE ||
       (colorType == PNG_COLOR_TYPE_GRAY && bitDepth < 8))
     png_set_expand(png);
 
   png_bytep trns = 0;
   int trnsCount = 0;
   if (png_get_valid(png, info, PNG_INFO_tRNS)) {
     png_get_tRNS(png, info, &trns, &trnsCount, 0);

[scroggo_chromium, 2017/02/23 15:11:51]

Another issue you brought up in the APNG patch - trns and trnsCount are never
used.

trnsCount was used before [1], but trns was not used when it was introduced [2].
And if we remove those, there is no reason to call png_get_tRNS, which looks to
just return values that we ignore [3].

Or would you prefer to land that as a separate CL?

[1] crrev.com/2426723005
[2]
https://chromium.googlesource.com/chromium/src/+/1861a9cf28b51732a362539a36c3...
[3]
https://cs.chromium.org/chromium/src/third_party/libpng/pngget.c?q=png_get_tr...

[Noel Gordon, 2017/02/23 16:49:05]

Ah yes, that's orthogonal to your APNG patch too.
 
Yes, IIRC I added trnsCount because QCMS needed it.
Right, we can remove that part.  I think have to retain the 

  png_set_expand(png);

if there is a PNG_INFO_tRNS.
Nah, let's do it here and let the png-suite tests tell us if we are wrong.

     png_set_expand(png);
   }
 
   if (bitDepth == 16)
     png_set_strip_16(png);
 
   if (colorType == PNG_COLOR_TYPE_GRAY ||
       colorType == PNG_COLOR_TYPE_GRAY_ALPHA)
     png_set_gray_to_rgb(png);
 
   if ((colorType & PNG_COLOR_MASK_COLOR) && !ignoresColorSpace()) {
     // We only support color profiles for color PALETTE and RGB[A] PNG.
-    // Supporting color profiles for gray-scale images is slightly tricky, at
-    // least using the CoreGraphics ICC library, because we expand gray-scale
-    // images to RGB but we do not similarly transform the color profile. We'd
-    // either need to transform the color profile or we'd need to decode into a
-    // gray-scale image buffer and hand that to CoreGraphics.
-    sk_sp<SkColorSpace> colorSpace = readColorSpace(png, info);
-    if (colorSpace) {
-      setEmbeddedColorSpace(colorSpace);
-    }
+    // TODO(msarret): Add GRAY, CMYK profile support?

[msarett1, 2017/02/23 14:07:03]

nit: msarett
nit: CMYK doesn't seem relevant here since we can't get it from a PNG.

FWIW, Gray support is ready.  Thanks for the reminder.

[Noel Gordon, 2017/02/23 16:49:05]

Yes but reading SkColorSpace::MakeICC, it seems to accept a CMYK profile if it
was present, eg., someone added a CMYK profile to the PNG :)

Silly thing to do sure, and probably rare, but we just need some way to check
for CMYK profile, and so ignore CMYK profiles here. 

I think the same issue can happen in the JPEG decoder now (I've seen JPEG with
CMYK profiles), and maybe in the other decoders too.

Basic issue is SkColorSpace::MakeICC accepts CMYK profiles, and the image
decoders haven't been changed to handle that :(

Changed the comment to say, ... block CMYK?
Ah cool.  It would be nice have a layout tests for gray profiled images since
they have uses in medical applications, for example.

+    if (sk_sp<SkColorSpace> colorSpace = readColorSpace(png, info))
+      setEmbeddedColorSpace(std::move(colorSpace));
   }
 
   if (!hasEmbeddedColorSpace()) {
-    // TODO (msarett):
-    // Applying the transfer function (gamma) should be handled by
-    // SkColorSpaceXform.  Here we always convert to a transfer function that
-    // is a 2.2 exponential.  This is a little strange given that the dst
-    // transfer function is not necessarily a 2.2 exponential.

[Noel Gordon, 2017/02/23 10:43:56]

Here be dragons for reference. The sad story of PNG and gamma [1] and the story
of the Apple and the Pear [2]. Perhaps let sleeping dogs lie.

[1] https://hsivonen.fi/png-gamma/
[2] https://bugs.chromium.org/p/chromium/issues/detail?id=517481

[msarett1, 2017/02/23 14:07:03]

This comment seems valid to me?

[1] seems to point out that browsers handle gamma wrong - which is a problem
that we can/should fix.
[2] the apple/pear image "works" regardless of whether we correct to 2.2 or the
monitor gamma.

[Noel Gordon, 2017/02/23 18:06:47]

I don't think the lone gAMA tag is a problem worth fixing: you'd need to change
Windows (IE draws a Pear, the Windows Image Viewer draws a Pear, or vice-sersa,
fix OSX, and so on).
If you remove the block of code, you get an Apple, if you retain it you get a
Pear.  Poor spec text, and we get compat problems in implementations.  I would
not describe it as "works".

Moral of this story: good specs can be followed and we should do so to gain
compat.  Are we following the ICC spec?  I don't see anywhere in it that
suggests we patch over problems in color profiles or describe exactly how.

But reading the Skia impl code, I see cases were do patch over, rather than
declaring a profile bogus as conforming implementations should. Taking the PNG
spec as an example, we know that road can lead to compat problems, I'm sad to
say.

-    // TODO (msarett):
-    // Often, PNGs that specify their transfer function with the gAMA tag will
-    // also specify their gamut with the cHRM tag.  We should read this tag
-    // and do a full color space transformation if it is present.

[Noel Gordon, 2017/02/23 10:43:56]

Fixed a while back. Though the Android pixels results seem to be back-the front
(R swapped with B), started when color correction was enabled for Android.  I
don't know what's going one there.

[msarett1, 2017/02/23 14:07:03]

Yes this comment is out of date, thanks for removing.

Not sure what you're referring to with Android, but that seems like a serious
issue.

[msarett1, 2017/02/23 14:09:34]

Oops, I see the bug now.  Thanks.

[Noel Gordon, 2017/02/23 16:49:05]

K.
Yeap it looked somewhat serious to me.
Issue 676561

Yeah, the test is not flaky. It has an <img src="...">, no timers or nothing.

https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/images/pa...

The actual layout image is drawn, but output looks to me like the R and G
channels have been swapped.  Weird.

Try loading the test URL in your phone and see if looks color correct?

     const double inverseGamma = 0.45455;
     const double defaultGamma = 2.2;
     double gamma;
     if (!ignoresColorSpace() && png_get_gAMA(png, info, &gamma)) {
       const double maxGamma = 21474.83;
       if ((gamma <= 0.0) || (gamma > maxGamma)) {
         gamma = inverseGamma;
         png_set_gAMA(png, info, gamma);
       }
       png_set_gamma(png, defaultGamma, gamma);
     } else {
       png_set_gamma(png, defaultGamma, inverseGamma);
     }
   }
 
   // Tell libpng to send us rows for interlaced pngs.
   if (interlaceType == PNG_INTERLACE_ADAM7)
     png_set_interlace_handling(png);
 
-  // Update our info now.
+  // Update our info now (so we can read color channel info).
   png_read_update_info(png, info);
-  channels = png_get_channels(png, info);
-  ASSERT(channels == 3 || channels == 4);
 
+  int channels = png_get_channels(png, info);
+  DCHECK(channels == 3 || channels == 4);
   m_reader->setHasAlpha(channels == 4);
 
   if (m_reader->decodingSizeOnly()) {
 // If we only needed the size, halt the reader.
 #if PNG_LIBPNG_VER_MAJOR > 1 || \
     (PNG_LIBPNG_VER_MAJOR == 1 && PNG_LIBPNG_VER_MINOR >= 5)
     // Passing '0' tells png_process_data_pause() not to cache unprocessed data.
     m_reader->setReadOffset(m_reader->currentBufferSize() -
                             png_process_data_pause(png, 0));
 #else
@@ skipping 31 matching lines @@
     }
 
     buffer.setStatus(ImageFrame::FramePartial);
     buffer.setHasAlpha(false);
 
     // For PNGs, the frame always fills the entire image.
     buffer.setOriginalFrameRect(IntRect(IntPoint(), size()));
   }
 
   /* libpng comments (here to explain what follows).
-     *
-     * this function is called for every row in the image. If the
-     * image is interlacing, and you turned on the interlace handler,
-     * this function will be called for every row in every pass.
-     * Some of these rows will not be changed from the previous pass.
-     * When the row is not changed, the new_row variable will be NULL.
-     * The rows and passes are called in order, so you don't really
-     * need the row_num and pass, but I'm supplying them because it
-     * may make your life easier.
-     */
+   *
+   * this function is called for every row in the image. If the
+   * image is interlacing, and you turned on the interlace handler,
+   * this function will be called for every row in every pass.
+   * Some of these rows will not be changed from the previous pass.
+   * When the row is not changed, the new_row variable will be NULL.
+   * The rows and passes are called in order, so you don't really
+   * need the row_num and pass, but I'm supplying them because it
+   * may make your life easier.
+   */
 
   // Nothing to do if the row is unchanged, or the row is outside
   // the image bounds: libpng may send extra rows, ignore them to
   // make our lives easier.
   if (!rowBuffer)
     return;
   int y = rowIndex;
   if (y < 0 || y >= size().height())
     return;
 
   /* libpng comments (continued).
-     *
-     * For the non-NULL rows of interlaced images, you must call
-     * png_progressive_combine_row() passing in the row and the
-     * old row.  You can call this function for NULL rows (it will
-     * just return) and for non-interlaced images (it just does the
-     * memcpy for you) if it will make the code easier. Thus, you
-     * can just do this for all cases:
-     *
-     *    png_progressive_combine_row(png_ptr, old_row, new_row);
-     *
-     * where old_row is what was displayed for previous rows. Note
-     * that the first pass (pass == 0 really) will completely cover
-     * the old row, so the rows do not have to be initialized. After
-     * the first pass (and only for interlaced images), you will have
-     * to pass the current row, and the function will combine the
-     * old row and the new row.
-     */
+   *
+   * For the non-NULL rows of interlaced images, you must call
+   * png_progressive_combine_row() passing in the row and the
+   * old row.  You can call this function for NULL rows (it will
+   * just return) and for non-interlaced images (it just does the
+   * memcpy for you) if it will make the code easier. Thus, you
+   * can just do this for all cases:
+   *
+   *    png_progressive_combine_row(png_ptr, old_row, new_row);
+   *
+   * where old_row is what was displayed for previous rows. Note
+   * that the first pass (pass == 0 really) will completely cover
+   * the old row, so the rows do not have to be initialized. After
+   * the first pass (and only for interlaced images), you will have
+   * to pass the current row, and the function will combine the
+   * old row and the new row.
+   */
 
   bool hasAlpha = m_reader->hasAlpha();
   png_bytep row = rowBuffer;
 
   if (png_bytep interlaceBuffer = m_reader->interlaceBuffer()) {
     unsigned colorChannels = hasAlpha ? 4 : 3;
     row = interlaceBuffer + (rowIndex * colorChannels * size().width());
     png_progressive_combine_row(m_reader->pngPtr(), row, rowBuffer);
   }
 
@@ skipping 13 matching lines @@
     // while the pixels are a linear space.
     // We cannot do this because when we apply the gamma encoding after
     // the premultiply, we will very likely end up with valid pixels
     // where R, G, and/or B are greater than A.  The legacy drawing
     // pipeline does not know how to handle this.
     if (SkColorSpaceXform* xform = colorTransform()) {
       SkColorSpaceXform::ColorFormat colorFormat =
           SkColorSpaceXform::kRGBA_8888_ColorFormat;
       xform->apply(colorFormat, dstRow, colorFormat, srcPtr, size().width(),
                    kUnpremul_SkAlphaType);
-      srcPtr = (png_bytep)dstRow;
+      srcPtr = png_bytep(dstRow);
     }
 

[scroggo_chromium, 2017/02/23 15:11:51]

As long as you're moving where alphaMask is referenced, its declaration can be
moved here.

[Noel Gordon, 2017/02/23 16:49:05]

Done.

     if (buffer.premultiplyAlpha()) {
       for (auto *dstPixel = dstRow; dstPixel < dstRow + width;
-           dstPixel++, srcPtr += 4) {
+           srcPtr += 4, ++dstPixel) {
         buffer.setRGBAPremultiply(dstPixel, srcPtr[0], srcPtr[1], srcPtr[2],
                                   srcPtr[3]);
         alphaMask &= srcPtr[3];
       }
     } else {
       for (auto *dstPixel = dstRow; dstPixel < dstRow + width;
-           dstPixel++, srcPtr += 4) {
+           srcPtr += 4, ++dstPixel) {
         buffer.setRGBARaw(dstPixel, srcPtr[0], srcPtr[1], srcPtr[2], srcPtr[3]);
         alphaMask &= srcPtr[3];
       }
     }
+
+    if (alphaMask != 255 && !buffer.hasAlpha())
+      buffer.setHasAlpha(true);
+
   } else {
     for (auto *dstPixel = dstRow; dstPixel < dstRow + width;
-         dstPixel++, srcPtr += 3) {
+         srcPtr += 3, ++dstPixel) {
       buffer.setRGBARaw(dstPixel, srcPtr[0], srcPtr[1], srcPtr[2], 255);
     }
 
     // We'll apply the color space xform to opaque pixels after they have been
     // written to the ImageFrame, purely because SkColorSpaceXform supports
     // RGBA (and not RGB).
     if (SkColorSpaceXform* xform = colorTransform()) {
       xform->apply(xformColorFormat(), dstRow, xformColorFormat(), dstRow,
                    size().width(), kOpaque_SkAlphaType);
     }
   }
 
-  if (alphaMask != 255 && !buffer.hasAlpha())
-    buffer.setHasAlpha(true);
-
   buffer.setPixelsChanged(true);
 }
 
 void PNGImageDecoder::complete() {
   if (m_frameBufferCache.isEmpty())
     return;
 
   m_frameBufferCache[0].setStatus(ImageFrame::FrameComplete);
 }
 
@@ skipping 12 matching lines @@
   // has failed.
   if (!m_reader->decode(*m_data, onlySize) && isAllDataReceived())
     setFailed();
 
   // If decoding is done or failed, we don't need the PNGImageReader anymore.
   if (isComplete(this) || failed())
     m_reader.reset();
 }
 
 }  // namespace blink