[PNGImageDecoder] Clean up readColorSpace, TODOs, cruft

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

Index: third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
diff --git a/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp b/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
index a2ebf6cb967b33593313ca267d227800dbbf3fbe..6deba9ce22adfd250319cac12dada224f706df3d 100644
--- a/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
+++ b/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp
@@ -54,58 +54,58 @@ PNGImageDecoder::PNGImageDecoder(AlphaOption alphaOption,
 
 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)) {
+  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]);
-
-    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);
-      }
-
-      // 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.
-    }
-  }
-
-  return nullptr;
+  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;
+
+  png_fixed_point inverseGamma;
+  if (!png_get_gAMA_fixed(png, info, &inverseGamma))
+    return nullptr;
+
+  // 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).
+
+  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() {
@@ -127,9 +127,9 @@ void PNGImageDecoder::headerAvailable() {
     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.
 
@@ -138,12 +138,8 @@ void PNGImageDecoder::headerAvailable() {
       (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);
+  if (png_get_valid(png, info, PNG_INFO_tRNS))
     png_set_expand(png);
-  }
 
   if (bitDepth == 16)
     png_set_strip_16(png);
@@ -154,27 +150,12 @@ void PNGImageDecoder::headerAvailable() {
 
   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 profile support, block CYMK?
+    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.
-    // 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.
     const double inverseGamma = 0.45455;
     const double defaultGamma = 2.2;
     double gamma;
@@ -194,11 +175,11 @@ void PNGImageDecoder::headerAvailable() {
   if (interlaceType == PNG_INTERLACE_ADAM7)
     png_set_interlace_handling(png);
 
-  // Update our info now.
+  // Update our info now (so we can get 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()) {
@@ -250,16 +231,16 @@ void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer,
   }
 
   /* 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
@@ -271,23 +252,23 @@ void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer,
     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;
@@ -301,7 +282,6 @@ void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer,
   // Write the decoded row pixels to the frame buffer. The repetitive
   // form of the row write loops is for speed.
   ImageFrame::PixelData* const dstRow = buffer.getAddr(0, y);
-  unsigned alphaMask = 255;
   int width = size().width();
 
   png_bytep srcPtr = row;
@@ -321,26 +301,31 @@ void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer,
           SkColorSpaceXform::kRGBA_8888_ColorFormat;
       xform->apply(colorFormat, dstRow, colorFormat, srcPtr, size().width(),
                    kUnpremul_SkAlphaType);
-      srcPtr = (png_bytep)dstRow;
+      srcPtr = png_bytep(dstRow);
     }
 
+    unsigned alphaMask = 255;
     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);
     }
 
@@ -353,9 +338,6 @@ void PNGImageDecoder::rowAvailable(unsigned char* rowBuffer,
     }
   }
 
-  if (alphaMask != 255 && !buffer.hasAlpha())
-    buffer.setHasAlpha(true);
-
   buffer.setPixelsChanged(true);
 }