Use SkColorSpaceXform to handle color conversions in decoders

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 27 matching lines @@
 
 #include "platform/image-decoders/png/PNGImageDecoder.h"
 
 #include "png.h"
 #include "wtf/PtrUtil.h"
 #include <memory>
 
 #if !defined(PNG_LIBPNG_VER_MAJOR) || !defined(PNG_LIBPNG_VER_MINOR)
 #error version error: compile against a versioned libpng.
 #endif
-#if USE(QCMSLIB)
-#include "qcms.h"
-#endif
 
 #if PNG_LIBPNG_VER_MAJOR > 1 || \
     (PNG_LIBPNG_VER_MAJOR == 1 && PNG_LIBPNG_VER_MINOR >= 4)
 #define JMPBUF(png_ptr) png_jmpbuf(png_ptr)
 #else
 #define JMPBUF(png_ptr) png_ptr->jmpbuf
 #endif
 
 namespace {
 
@@ skipping 28 matching lines @@
   USING_FAST_MALLOC(PNGImageReader);
   WTF_MAKE_NONCOPYABLE(PNGImageReader);
 
  public:
   PNGImageReader(PNGImageDecoder* decoder, size_t readOffset)
       : m_decoder(decoder),
         m_readOffset(readOffset),
         m_currentBufferSize(0),
         m_decodingSizeOnly(false),
         m_hasAlpha(false)
-#if USE(QCMSLIB)
-        ,
-        m_rowBuffer()
-#endif
   {
     m_png = png_create_read_struct(PNG_LIBPNG_VER_STRING, 0, pngFailed, 0);
     m_info = png_create_info_struct(m_png);
     png_set_progressive_read_fn(m_png, m_decoder, pngHeaderAvailable,
                                 pngRowAvailable, pngComplete);
   }
 
   ~PNGImageReader() {
     png_destroy_read_struct(m_png ? &m_png : 0, m_info ? &m_info : 0, 0);
     ASSERT(!m_png && !m_info);
@@ skipping 30 matching lines @@
   void setReadOffset(size_t offset) { m_readOffset = offset; }
   size_t currentBufferSize() const { return m_currentBufferSize; }
   bool decodingSizeOnly() const { return m_decodingSizeOnly; }
   void setHasAlpha(bool hasAlpha) { m_hasAlpha = hasAlpha; }
   bool hasAlpha() const { return m_hasAlpha; }
 
   png_bytep interlaceBuffer() const { return m_interlaceBuffer.get(); }
   void createInterlaceBuffer(int size) {
     m_interlaceBuffer = wrapArrayUnique(new png_byte[size]);
   }
-#if USE(QCMSLIB)
-  png_bytep rowBuffer() const { return m_rowBuffer.get(); }
-  void createRowBuffer(int size) {
-    m_rowBuffer = wrapArrayUnique(new png_byte[size]);
-  }
-#endif
 
  private:
   png_structp m_png;
   png_infop m_info;
   PNGImageDecoder* m_decoder;
   size_t m_readOffset;
   size_t m_currentBufferSize;
   bool m_decodingSizeOnly;
   bool m_hasAlpha;
   std::unique_ptr<png_byte[]> m_interlaceBuffer;
-#if USE(QCMSLIB)
-  std::unique_ptr<png_byte[]> m_rowBuffer;
-#endif
 };
 
 PNGImageDecoder::PNGImageDecoder(AlphaOption alphaOption,
                                  GammaAndColorProfileOption colorOptions,
                                  size_t maxDecodedBytes,
                                  size_t offset)
     : ImageDecoder(alphaOption, colorOptions, maxDecodedBytes),
       m_offset(offset) {}
 
 PNGImageDecoder::~PNGImageDecoder() {}
@@ skipping 42 matching lines @@
       colorType == PNG_COLOR_TYPE_GRAY_ALPHA)
     png_set_gray_to_rgb(png);
 
   if ((colorType & PNG_COLOR_MASK_COLOR) && !m_ignoreGammaAndColorProfile) {
     // 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.
-    bool imageHasAlpha = (colorType & PNG_COLOR_MASK_ALPHA) || trnsCount;
 #ifdef PNG_iCCP_SUPPORTED
     if (png_get_valid(png, info, PNG_INFO_sRGB)) {
-      setColorProfileAndComputeTransform(nullptr, 0, imageHasAlpha,
-                                         true /* useSRGB */);
+      setColorSpaceAndComputeTransform(nullptr, 0, true /* useSRGB */);
     } else {
       char* profileName = nullptr;
       int compressionType = 0;
 #if (PNG_LIBPNG_VER < 10500)
       png_charp profile = nullptr;
 #else
       png_bytep profile = nullptr;
 #endif
       png_uint_32 profileLength = 0;
       if (png_get_iCCP(png, info, &profileName, &compressionType, &profile,
                        &profileLength)) {
-        setColorProfileAndComputeTransform(reinterpret_cast<char*>(profile),
-                                           profileLength, imageHasAlpha,
-                                           false /* useSRGB */);
+        setColorSpaceAndComputeTransform(reinterpret_cast<char*>(profile),
+                                         profileLength, false /* useSRGB */);
       }
     }
 #endif  // PNG_iCCP_SUPPORTED
   }
 
   if (!hasColorProfile()) {
-    // Deal with gamma and keep it under our control.
+    // 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;
     if (!m_ignoreGammaAndColorProfile && 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);
@@ skipping 47 matching lines @@
     if (PNG_INTERLACE_ADAM7 ==
         png_get_interlace_type(png, m_reader->infoPtr())) {
       m_reader->createInterlaceBuffer(colorChannels * size().width() *
                                       size().height());
       if (!m_reader->interlaceBuffer()) {
         longjmp(JMPBUF(png), 1);
         return;
       }
     }
 
-#if USE(QCMSLIB)
-    if (colorTransform()) {
-      m_reader->createRowBuffer(colorChannels * size().width());
-      if (!m_reader->rowBuffer()) {
-        longjmp(JMPBUF(png), 1);
-        return;
-      }
-    }
-#endif
     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
@@ skipping 36 matching lines @@
 
   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);
   }
 
-#if USE(QCMSLIB)
-  if (qcms_transform* transform = colorTransform()) {
-    qcms_transform_data(transform, row, m_reader->rowBuffer(), size().width());
-    row = m_reader->rowBuffer();
-  }
-#endif
-
   // Write the decoded row pixels to the frame buffer. The repetitive
   // form of the row write loops is for speed.
   ImageFrame::PixelData* address = buffer.getAddr(0, y);

[scroggo_chromium, 2016/10/20 12:45:04]

The variables "address" and "addr" could be easy to mix up. Maybe make this one
an

  ImageFrame::PixelData* const addr

?

[msarett, 2016/10/20 13:20:52]

Done.

   unsigned alphaMask = 255;
   int width = size().width();
 
   png_bytep pixel = row;
   if (hasAlpha) {
+#if USE(SKCOLORXFORM)
+    // TODO (msarett):
+    // Here we apply the color space transformation to the dst space.
+    // It does not really make sense to transform to a gamma-encoded
+    // space and then immediately after, perform a linear premultiply.
+    // Ideally we would pass kPremul_SkAlphaType to xform->apply(),
+    // instructing SkColorSpaceXform to perform the linear premultiply
+    // while the pixels are a linear space.
+    // We cannot do this because when we apply the gamma encoding after

[scroggo_chromium, 2016/10/20 12:45:04]

Is there an actual TODO here? Or should is this comment just an acknowledgement
that this is not "correct", but we don't see a way to do it better?

[msarett, 2016/10/20 13:20:52]

It's the latter.  I'll drop the TODO.

+    // 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, address, colorFormat, row, size().width(),
+                   kUnpremul_SkAlphaType);
+      pixel = (png_bytep)address;
+    }
+#endif
+
     if (buffer.premultiplyAlpha()) {
       for (int x = 0; x < width; ++x, pixel += 4) {
         buffer.setRGBAPremultiply(address++, pixel[0], pixel[1], pixel[2],
                                   pixel[3]);
         alphaMask &= pixel[3];
       }
     } else {
       for (int x = 0; x < width; ++x, pixel += 4) {
         buffer.setRGBARaw(address++, pixel[0], pixel[1], pixel[2], pixel[3]);
         alphaMask &= pixel[3];
       }
     }
   } else {
+    ImageFrame::PixelData* addr = address;

[scroggo_chromium, 2016/10/20 12:45:04]

Another way to distinguish addr from address - you could put this declaration
and the for loop inside its own scope. Or you could put addr inside the for
loop, e.g.

  for (auto* addr = address; addr < address + width; addr++, pixel += 3) {

(This also lets you drop the variable "x".)
(A counter-argument to this suggestion - I initially wrote "addr < addr +
width", which would loop infinitely :( But that should be caught pretty quickly,
and with distinguished names maybe it would be harder to make that mistake...)

In addition/instead, you could name them something like "rowAddress" and
"pixelAddress".

[msarett, 2016/10/20 13:20:52]

Changed the style of the loop.  Using dstRow and dstPixel.

     for (int x = 0; x < width; ++x, pixel += 3) {
-      buffer.setRGBARaw(address++, pixel[0], pixel[1], pixel[2], 255);
+      buffer.setRGBARaw(addr++, pixel[0], pixel[1], pixel[2], 255);
     }
+
+#if USE(SKCOLORXFORM)
+    // 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(), address, xformColorFormat(), address,
+                   size().width(), kOpaque_SkAlphaType);
+    }
+#endif
   }
 
   if (alphaMask != 255 && !buffer.hasAlpha())
     buffer.setHasAlpha(true);
 
   buffer.setPixelsChanged(true);
 }
 
 void PNGImageDecoder::complete() {
   if (m_frameBufferCache.isEmpty())
@@ skipping 17 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