Support color xforms for kIndex8 pngs

src/codec/SkPngCodec.cpp

Index: src/codec/SkPngCodec.cpp
diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp
index cee20607cf5df72afe4650da2511e21fee7542e0..e69c3efb44f96b9c2f6d0430f53fae98e90dff74 100644
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp
@@ -92,8 +92,12 @@ private:
 };
 #define AutoCleanPng(...) SK_REQUIRE_LOCAL_VAR(AutoCleanPng)
 
+static inline SkAlphaType xform_alpha_type(SkAlphaType dstAlphaType, SkAlphaType srcAlphaType) {
+    return (kOpaque_SkAlphaType == srcAlphaType) ? kOpaque_SkAlphaType : dstAlphaType;
+}
+
 // Note: SkColorTable claims to store SkPMColors, which is not necessarily the case here.
-bool SkPngCodec::createColorTable(SkColorType dstColorType, bool premultiply, int* ctableCount) {
+bool SkPngCodec::createColorTable(const SkImageInfo& dstInfo, int* ctableCount) {
 
     int numColors;
     png_color* palette;
@@ -103,13 +107,18 @@ bool SkPngCodec::createColorTable(SkColorType dstColorType, bool premultiply, in
 
     // Note: These are not necessarily SkPMColors.
     SkPMColor colorPtr[256];

[mtklein, 2016/08/22 14:22:51]

Just noticed it's a little odd to have an array named ...Ptr.

Given how you always call it the color table, maybe, colorTable.

Instead of noting what these are not, you might want to explain what these can
be:

// Contents depend on tableColorType: {premul,unpremul} x {bgra,rgba} x ...
uint32_t colorTable[256];

[msarett, 2016/08/22 15:01:35]

Done.

+    SkColorType tableColorType = fColorXform ? kRGBA_8888_SkColorType : dstInfo.colorType();
 
     png_bytep alphas;
     int numColorsWithAlpha = 0;
     if (png_get_tRNS(fPng_ptr, fInfo_ptr, &alphas, &numColorsWithAlpha, nullptr)) {
+        // If we are performing a color xform, it will handle the premultiply.

[mtklein, 2016/08/22 14:22:51]

... otherwise we'll do it here.

[msarett, 2016/08/22 15:01:35]

Done.

+        bool needsPremul = needs_premul(dstInfo, this->getInfo());
+        bool doPremul =  needsPremul && !fColorXform;

[mtklein, 2016/08/22 14:22:51]

Might be just as clear as

   bool premultiply = !fColorXform && needs_premul(dstInfo, this->getInfo());

[msarett, 2016/08/22 15:01:35]

Done.

+
         // Choose which function to use to create the color table. If the final destination's
         // colortype is unpremultiplied, the color table will store unpremultiplied colors.
-        PackColorProc proc = choose_pack_color_proc(premultiply, dstColorType);
+        PackColorProc proc = choose_pack_color_proc(doPremul, tableColorType);
 
         for (int i = 0; i < numColorsWithAlpha; i++) {
             // We don't have a function in SkOpts that combines a set of alphas with a set
@@ -129,7 +138,7 @@ bool SkPngCodec::createColorTable(SkColorType dstColorType, bool premultiply, in
         SkASSERT(&palette->green < &palette->blue);
 #endif
 
-        if (is_rgba(dstColorType)) {
+        if (is_rgba(tableColorType)) {
             SkOpts::RGB_to_RGB1(colorPtr + numColorsWithAlpha, palette,
                     numColors - numColorsWithAlpha);
         } else {
@@ -138,6 +147,20 @@ bool SkPngCodec::createColorTable(SkColorType dstColorType, bool premultiply, in
         }
     }
 
+    // If we are not decoding to F16, we can color xform now and store the results
+    // in the color table.
+    if (fColorXform && kRGBA_F16_SkColorType != dstInfo.colorType()) {
+        SkColorType xformColorType = is_rgba(dstInfo.colorType()) ?
+                kRGBA_8888_SkColorType : kBGRA_8888_SkColorType;
+        SkAlphaType xformAlphaType = xform_alpha_type(dstInfo.alphaType(),
+                                                      this->getInfo().alphaType());
+        fColorXform->apply(colorPtr, colorPtr, numColors, xformColorType, xformAlphaType);
+
+        // We have already applied the color xform.  Set it to nullptr so we don't try to
+        // use it again.
+        fColorXform = nullptr;

[mtklein, 2016/08/22 14:22:51]

Is there not some sort of early return we can make instead?  Code can get
confusing when variables are conditionally reassigned.

[msarett, 2016/08/22 15:01:35]

Yeah this is confusing, I'll try this another way.

+    }
+
     // Pad the color table with the last color in the table (or black) in the case that
     // invalid pixel indices exceed the number of colors in the table.
     const int maxColors = 1 << fBitDepth;
@@ -406,8 +429,8 @@ public:
             swizzlerDstRowBytes = 0;
         }
 
-        SkAlphaType xformAlphaType = (kOpaque_SkAlphaType == this->getInfo().alphaType()) ?
-                kOpaque_SkAlphaType : dstInfo.alphaType();
+        SkAlphaType xformAlphaType = xform_alpha_type(dstInfo.alphaType(),
+                                                      this->getInfo().alphaType());
         for (; y < count; y++) {
             png_read_row(fPng_ptr, fSwizzlerSrcRow, nullptr);
             fSwizzler->swizzle(swizzlerDstRow, fSwizzlerSrcRow);
@@ -506,8 +529,8 @@ public:
             swizzlerDstRowBytes = 0;
         }
 
-        SkAlphaType xformAlphaType = (kOpaque_SkAlphaType == this->getInfo().alphaType()) ?
-                kOpaque_SkAlphaType : dstInfo.alphaType();
+        SkAlphaType xformAlphaType = xform_alpha_type(dstInfo.alphaType(),
+                                                      this->getInfo().alphaType());
         srcRow = storage.get();
         for (int y = 0; y < count; y++) {
             fSwizzler->swizzle(swizzlerDstRow, srcRow);
@@ -792,6 +815,8 @@ bool SkPngCodec::initializeXforms(const SkImageInfo& dstInfo, const Options& opt
                     swizzlerInfo = swizzlerInfo.makeAlphaType(kUnpremul_SkAlphaType);
                 }
                 break;
+            case kIndex_8_SkColorType:
+                break;
             default:
                 return false;
         }
@@ -805,8 +830,7 @@ bool SkPngCodec::initializeXforms(const SkImageInfo& dstInfo, const Options& opt
     }
 
     if (SkEncodedInfo::kPalette_Color == this->getEncodedInfo().color()) {
-        if (!this->createColorTable(swizzlerInfo.colorType(),
-                                    kPremul_SkAlphaType == swizzlerInfo.alphaType(), ctableCount)) {
+        if (!this->createColorTable(dstInfo, ctableCount)) {
             return false;
         }
     }