Support color xforms for kIndex8 pngs This change started as: "Always use color xforms to premultiply". We need to be in a linear space to premultiply correctly. It became clear that we also need to support kIndex8 color xforms in order to make this change. BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2246143002 Review-Url: https://codereview.chromium.org/2246143002
diff --git a/src/codec/SkCodecImageGenerator.cpp b/src/codec/SkCodecImageGenerator.cpp index e579da9..1bae1df 100644 --- a/src/codec/SkCodecImageGenerator.cpp +++ b/src/codec/SkCodecImageGenerator.cpp
@@ -37,7 +37,12 @@ bool SkCodecImageGenerator::onGetPixels(const SkImageInfo& info, void* pixels, size_t rowBytes, SkPMColor ctable[], int* ctableCount) { - SkCodec::Result result = fCodec->getPixels(info, pixels, rowBytes, nullptr, ctable, + // FIXME (msarett): + // We don't give the client the chance to request an SkColorSpace. Until we improve + // the API, let's assume that they want legacy mode. + SkImageInfo decodeInfo = info.makeColorSpace(nullptr); + + SkCodec::Result result = fCodec->getPixels(decodeInfo, pixels, rowBytes, nullptr, ctable, ctableCount); switch (result) { case SkCodec::kSuccess:
diff --git a/src/codec/SkCodecPriv.h b/src/codec/SkCodecPriv.h index b768b23..1749407 100644 --- a/src/codec/SkCodecPriv.h +++ b/src/codec/SkCodecPriv.h
@@ -310,10 +310,25 @@ } } +static inline bool needs_premul(const SkImageInfo& dstInfo, const SkImageInfo& srcInfo) { + return kPremul_SkAlphaType == dstInfo.alphaType() && + kUnpremul_SkAlphaType == srcInfo.alphaType(); +} + static inline bool needs_color_xform(const SkImageInfo& dstInfo, const SkImageInfo& srcInfo) { - return (kRGBA_F16_SkColorType == dstInfo.colorType()) || - (dstInfo.colorSpace() && !SkColorSpace::Equals(srcInfo.colorSpace(), - dstInfo.colorSpace())); + // Color xform is necessary in order to correctly perform premultiply in linear space. + bool needsPremul = needs_premul(dstInfo, srcInfo); + + // F16 is by definition a linear space, so we always must perform a color xform. + bool isF16 = kRGBA_F16_SkColorType == dstInfo.colorType(); + + // Need a color xform when dst space does not match the src. + bool srcDstNotEqual = !SkColorSpace::Equals(srcInfo.colorSpace(), dstInfo.colorSpace()); + + // We never perform a color xform in legacy mode. + bool isLegacy = nullptr == dstInfo.colorSpace(); + + return !isLegacy && (needsPremul || isF16 || srcDstNotEqual); } #endif // SkCodecPriv_DEFINED
diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp index 2b52ab7..8f7ab79 100644 --- a/src/codec/SkPngCodec.cpp +++ b/src/codec/SkPngCodec.cpp
@@ -92,8 +92,12 @@ }; #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; @@ -101,21 +105,27 @@ return false; } - // Note: These are not necessarily SkPMColors. - SkPMColor colorPtr[256]; + // Contents depend on tableColorType and our choice of if/when to premultiply: + // { kPremul, kUnpremul, kOpaque } x { RGBA, BGRA } + SkPMColor colorTable[256]; + 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. Otherwise, + // we'll do it here. + bool premultiply = !fColorXform && needs_premul(dstInfo, this->getInfo()); + // 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(premultiply, 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 // of RGBs. We could write one, but it's hardly worth it, given that this // is such a small fraction of the total decode time. - colorPtr[i] = proc(alphas[i], palette->red, palette->green, palette->blue); + colorTable[i] = proc(alphas[i], palette->red, palette->green, palette->blue); palette++; } } @@ -129,21 +139,31 @@ SkASSERT(&palette->green < &palette->blue); #endif - if (is_rgba(dstColorType)) { - SkOpts::RGB_to_RGB1(colorPtr + numColorsWithAlpha, palette, + if (is_rgba(tableColorType)) { + SkOpts::RGB_to_RGB1(colorTable + numColorsWithAlpha, palette, numColors - numColorsWithAlpha); } else { - SkOpts::RGB_to_BGR1(colorPtr + numColorsWithAlpha, palette, + SkOpts::RGB_to_BGR1(colorTable + numColorsWithAlpha, palette, numColors - numColorsWithAlpha); } } + // 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(colorTable, colorTable, numColors, xformColorType, xformAlphaType); + } + // 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; if (numColors < maxColors) { - SkPMColor lastColor = numColors > 0 ? colorPtr[numColors - 1] : SK_ColorBLACK; - sk_memset32(colorPtr + numColors, lastColor, maxColors - numColors); + SkPMColor lastColor = numColors > 0 ? colorTable[numColors - 1] : SK_ColorBLACK; + sk_memset32(colorTable + numColors, lastColor, maxColors - numColors); } // Set the new color count. @@ -151,7 +171,7 @@ *ctableCount = maxColors; } - fColorTable.reset(new SkColorTable(colorPtr, maxColors)); + fColorTable.reset(new SkColorTable(colorTable, maxColors)); return true; } @@ -367,6 +387,11 @@ fColorXform ? SkTAddOffset<uint32_t>(fSwizzlerSrcRow, SkAlign4(fSrcRowBytes)) : 0; } +static inline bool apply_xform_on_decode(SkColorType dstColorType, SkEncodedInfo::Color srcColor) { + // We will apply the color xform when reading the color table, unless F16 is requested. + return SkEncodedInfo::kPalette_Color != srcColor || kRGBA_F16_SkColorType == dstColorType; +} + class SkPngNormalCodec : public SkPngCodec { public: SkPngNormalCodec(const SkEncodedInfo& encodedInfo, const SkImageInfo& imageInfo, @@ -400,18 +425,21 @@ void* swizzlerDstRow = dst; size_t swizzlerDstRowBytes = rowBytes; - if (fColorXform) { + + bool colorXform = fColorXform && + apply_xform_on_decode(dstInfo.colorType(), this->getEncodedInfo().color()); + if (colorXform) { swizzlerDstRow = fColorXformSrcRow; 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); - if (fColorXform) { + if (colorXform) { fColorXform->apply(dst, (const uint32_t*) swizzlerDstRow, fSwizzler->swizzleWidth(), dstInfo.colorType(), xformAlphaType); dst = SkTAddOffset<void>(dst, rowBytes); @@ -442,7 +470,6 @@ typedef SkPngCodec INHERITED; }; - class SkPngInterlacedCodec : public SkPngCodec { public: SkPngInterlacedCodec(const SkEncodedInfo& encodedInfo, const SkImageInfo& imageInfo, @@ -500,25 +527,25 @@ // Swizzle and xform the rows we care about void* swizzlerDstRow = dst; size_t swizzlerDstRowBytes = rowBytes; - if (fColorXform) { + + bool colorXform = fColorXform && + apply_xform_on_decode(dstInfo.colorType(), this->getEncodedInfo().color()); + if (colorXform) { swizzlerDstRow = fColorXformSrcRow; 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); srcRow = SkTAddOffset<uint8_t>(srcRow, fSrcRowBytes); - if (fColorXform) { - if (fColorXform) { - fColorXform->apply(dst, (const uint32_t*) swizzlerDstRow, - fSwizzler->swizzleWidth(), dstInfo.colorType(), - xformAlphaType); - dst = SkTAddOffset<void>(dst, rowBytes); - } + if (colorXform) { + fColorXform->apply(dst, (const uint32_t*) swizzlerDstRow, fSwizzler->swizzleWidth(), + dstInfo.colorType(), xformAlphaType); + dst = SkTAddOffset<void>(dst, rowBytes); } swizzlerDstRow = SkTAddOffset<void>(swizzlerDstRow, swizzlerDstRowBytes); @@ -802,6 +829,8 @@ swizzlerInfo = swizzlerInfo.makeAlphaType(kUnpremul_SkAlphaType); } break; + case kIndex_8_SkColorType: + break; default: return false; } @@ -815,8 +844,7 @@ } if (SkEncodedInfo::kPalette_Color == this->getEncodedInfo().color()) { - if (!this->createColorTable(swizzlerInfo.colorType(), - kPremul_SkAlphaType == swizzlerInfo.alphaType(), ctableCount)) { + if (!this->createColorTable(dstInfo, ctableCount)) { return false; } }
diff --git a/src/codec/SkPngCodec.h b/src/codec/SkPngCodec.h index 9d97c71..aace7b1 100644 --- a/src/codec/SkPngCodec.h +++ b/src/codec/SkPngCodec.h
@@ -66,7 +66,7 @@ int fBitDepth; private: - bool createColorTable(SkColorType dstColorType, bool premultiply, int* ctableCount); + bool createColorTable(const SkImageInfo& dstInfo, int* ctableCount); void destroyReadStruct(); typedef SkCodec INHERITED;