From 7a9900d6d34e437bb24beb5524a1f6488ae138c9 Mon Sep 17 00:00:00 2001 From: msarett Date: Thu, 8 Sep 2016 10:14:04 -0700 Subject: [PATCH] Checking for valid colorType, alphaType, colorSpace in SkCodec * Refactor to share code between SkPngCodec and SkWebpCodec * Didn't end up sharing with SkJpegCodec but did refactor that code a bit * Disallow conversions to F16 with non-linear color spaces * Fail to decode if we fail to create a SkColorSpaceXform (should be an assert soon). We used to fallback on a legacy decode if we failed to create the transform. * A bunch of name changes BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2319293003 Review-Url: https://codereview.chromium.org/2319293003 --- dm/DMSrcSink.cpp | 3 +++ src/android/SkBitmapRegionCodec.cpp | 5 ++++- src/codec/SkBmpCodec.cpp | 2 +- src/codec/SkBmpMaskCodec.cpp | 2 +- src/codec/SkBmpRLECodec.cpp | 2 +- src/codec/SkBmpStandardCodec.cpp | 2 +- src/codec/SkCodecPriv.h | 42 ++++++++++++++++++++++++++++++++++-- src/codec/SkGifCodec.cpp | 5 ++--- src/codec/SkJpegCodec.cpp | 43 ++++++++++++++++--------------------- src/codec/SkJpegCodec.h | 4 ++-- src/codec/SkPngCodec.cpp | 39 +++++---------------------------- src/codec/SkRawCodec.cpp | 2 +- src/codec/SkWebpCodec.cpp | 27 ++++------------------- tests/CodecTest.cpp | 30 ++++++++++++++++++++++++++ 14 files changed, 113 insertions(+), 95 deletions(-) diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp index caf6f52..fcc4a33 100644 --- a/dm/DMSrcSink.cpp +++ b/dm/DMSrcSink.cpp @@ -896,6 +896,9 @@ Error ColorCodecSrc::draw(SkCanvas* canvas) const { if (kUnpremul_SkAlphaType == decodeInfo.alphaType()) { decodeInfo = decodeInfo.makeAlphaType(kPremul_SkAlphaType); } + if (kRGBA_F16_SkColorType == fColorType) { + decodeInfo = decodeInfo.makeColorSpace(decodeInfo.colorSpace()->makeLinearGamma()); + } SkImageInfo bitmapInfo = decodeInfo; if (kRGBA_8888_SkColorType == decodeInfo.colorType() || diff --git a/src/android/SkBitmapRegionCodec.cpp b/src/android/SkBitmapRegionCodec.cpp index 9c21484..df0a32c 100644 --- a/src/android/SkBitmapRegionCodec.cpp +++ b/src/android/SkBitmapRegionCodec.cpp @@ -135,5 +135,8 @@ bool SkBitmapRegionCodec::decodeRegion(SkBitmap* bitmap, SkBRDAllocator* allocat } bool SkBitmapRegionCodec::conversionSupported(SkColorType colorType) { - return conversion_possible(fCodec->getInfo().makeColorType(colorType), fCodec->getInfo()); + // Enable legacy behavior. + sk_sp colorSpace = nullptr; + SkImageInfo dstInfo = fCodec->getInfo().makeColorType(colorType).makeColorSpace(colorSpace); + return conversion_possible_ignore_color_space(dstInfo, fCodec->getInfo()); } diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp index 1c999eb..2f796ad 100644 --- a/src/codec/SkBmpCodec.cpp +++ b/src/codec/SkBmpCodec.cpp @@ -603,7 +603,7 @@ int32_t SkBmpCodec::getDstRow(int32_t y, int32_t height) const { SkCodec::Result SkBmpCodec::onStartScanlineDecode(const SkImageInfo& dstInfo, const SkCodec::Options& options, SkPMColor inputColorPtr[], int* inputColorCount) { - if (!conversion_possible(dstInfo, this->getInfo())) { + if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) { SkCodecPrintf("Error: cannot convert input type to output type.\n"); return kInvalidConversion; } diff --git a/src/codec/SkBmpMaskCodec.cpp b/src/codec/SkBmpMaskCodec.cpp index 7d2d398..5b28252 100644 --- a/src/codec/SkBmpMaskCodec.cpp +++ b/src/codec/SkBmpMaskCodec.cpp @@ -39,7 +39,7 @@ SkCodec::Result SkBmpMaskCodec::onGetPixels(const SkImageInfo& dstInfo, return kInvalidScale; } - if (!conversion_possible(dstInfo, this->getInfo())) { + if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) { SkCodecPrintf("Error: cannot convert input type to output type.\n"); return kInvalidConversion; } diff --git a/src/codec/SkBmpRLECodec.cpp b/src/codec/SkBmpRLECodec.cpp index 02b42f6..252d63e 100644 --- a/src/codec/SkBmpRLECodec.cpp +++ b/src/codec/SkBmpRLECodec.cpp @@ -44,7 +44,7 @@ SkCodec::Result SkBmpRLECodec::onGetPixels(const SkImageInfo& dstInfo, // Subsets are not supported. return kUnimplemented; } - if (!conversion_possible(dstInfo, this->getInfo())) { + if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) { SkCodecPrintf("Error: cannot convert input type to output type.\n"); return kInvalidConversion; } diff --git a/src/codec/SkBmpStandardCodec.cpp b/src/codec/SkBmpStandardCodec.cpp index 651ff83..089e310 100644 --- a/src/codec/SkBmpStandardCodec.cpp +++ b/src/codec/SkBmpStandardCodec.cpp @@ -48,7 +48,7 @@ SkCodec::Result SkBmpStandardCodec::onGetPixels(const SkImageInfo& dstInfo, SkCodecPrintf("Error: scaling not supported.\n"); return kInvalidScale; } - if (!conversion_possible(dstInfo, this->getInfo())) { + if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) { SkCodecPrintf("Error: cannot convert input type to output type.\n"); return kInvalidConversion; } diff --git a/src/codec/SkCodecPriv.h b/src/codec/SkCodecPriv.h index 9a8a43e..4285786 100644 --- a/src/codec/SkCodecPriv.h +++ b/src/codec/SkCodecPriv.h @@ -107,14 +107,18 @@ static inline bool valid_alpha(SkAlphaType dstAlpha, SkAlphaType srcAlpha) { } /* + * Original version of conversion_possible that does not account for color spaces. + * Used by codecs that have not been updated to support color spaces. + * * Most of our codecs support the same conversions: * - opaque to any alpha type * - 565 only if opaque * - premul to unpremul and vice versa - * - always support N32 + * - always support RGBA, BGRA * - otherwise match the src color type */ -static inline bool conversion_possible(const SkImageInfo& dst, const SkImageInfo& src) { +static inline bool conversion_possible_ignore_color_space(const SkImageInfo& dst, + const SkImageInfo& src) { // Ensure the alpha type is valid if (!valid_alpha(dst.alphaType(), src.alphaType())) { return false; @@ -335,4 +339,38 @@ static inline SkAlphaType select_alpha_xform(SkAlphaType dstAlphaType, SkAlphaTy return (kOpaque_SkAlphaType == srcAlphaType) ? kOpaque_SkAlphaType : dstAlphaType; } +/* + * Alpha Type Conversions + * - kOpaque to kOpaque, kUnpremul, kPremul is valid + * - kUnpremul to kUnpremul, kPremul is valid + * + * Color Type Conversions + * - Always support kRGBA_8888, kBGRA_8888 + * - Support kRGBA_F16 when there is a linear dst color space + * - Support kIndex8 if it matches the src + * - Support k565, kGray8 if kOpaque and color correction is not required + */ +static inline bool conversion_possible(const SkImageInfo& dst, const SkImageInfo& src) { + // Ensure the alpha type is valid. + if (!valid_alpha(dst.alphaType(), src.alphaType())) { + return false; + } + + // Check for supported color types. + switch (dst.colorType()) { + case kRGBA_8888_SkColorType: + case kBGRA_8888_SkColorType: + return true; + case kRGBA_F16_SkColorType: + return dst.colorSpace() && dst.colorSpace()->gammaIsLinear(); + case kIndex_8_SkColorType: + return kIndex_8_SkColorType == src.colorType(); + case kRGB_565_SkColorType: + case kGray_8_SkColorType: + return kOpaque_SkAlphaType == src.alphaType() && !needs_color_xform(dst, src); + default: + return false; + } +} + #endif // SkCodecPriv_DEFINED diff --git a/src/codec/SkGifCodec.cpp b/src/codec/SkGifCodec.cpp index dcc25b8..1e6e300 100644 --- a/src/codec/SkGifCodec.cpp +++ b/src/codec/SkGifCodec.cpp @@ -450,9 +450,8 @@ void SkGifCodec::initializeColorTable(const SkImageInfo& dstInfo, SkPMColor* inp SkCodec::Result SkGifCodec::prepareToDecode(const SkImageInfo& dstInfo, SkPMColor* inputColorPtr, int* inputColorCount, const Options& opts) { // Check for valid input parameters - if (!conversion_possible(dstInfo, this->getInfo())) { - return gif_error("Cannot convert input type to output type.\n", - kInvalidConversion); + if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) { + return gif_error("Cannot convert input type to output type.\n", kInvalidConversion); } // Initialize color table and copy to the client if necessary diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp index fcf89d0..d2f7cdb 100644 --- a/src/codec/SkJpegCodec.cpp +++ b/src/codec/SkJpegCodec.cpp @@ -357,7 +357,7 @@ bool SkJpegCodec::onRewind() { * image has been implemented * Sets the output color space */ -bool SkJpegCodec::setOutputColorSpace(const SkImageInfo& dstInfo, bool needsColorXform) { +bool SkJpegCodec::setOutputColorSpace(const SkImageInfo& dstInfo) { if (kUnknown_SkAlphaType == dstInfo.alphaType()) { return false; } @@ -384,7 +384,7 @@ bool SkJpegCodec::setOutputColorSpace(const SkImageInfo& dstInfo, bool needsColo case kBGRA_8888_SkColorType: if (isCMYK) { fDecoderMgr->dinfo()->out_color_space = JCS_CMYK; - } else if (needsColorXform) { + } else if (fColorXform) { // Our color transformation code requires RGBA order inputs, but it'll swizzle // to BGRA for us. fDecoderMgr->dinfo()->out_color_space = JCS_EXT_RGBA; @@ -393,7 +393,7 @@ bool SkJpegCodec::setOutputColorSpace(const SkImageInfo& dstInfo, bool needsColo } return true; case kRGB_565_SkColorType: - if (needsColorXform) { + if (fColorXform) { return false; } @@ -405,14 +405,17 @@ bool SkJpegCodec::setOutputColorSpace(const SkImageInfo& dstInfo, bool needsColo } return true; case kGray_8_SkColorType: - if (needsColorXform || JCS_GRAYSCALE != encodedColorType) { + if (fColorXform || JCS_GRAYSCALE != encodedColorType) { return false; } fDecoderMgr->dinfo()->out_color_space = JCS_GRAYSCALE; return true; case kRGBA_F16_SkColorType: - SkASSERT(needsColorXform); + SkASSERT(fColorXform); + if (!dstInfo.colorSpace()->gammaIsLinear()) { + return false; + } if (isCMYK) { fDecoderMgr->dinfo()->out_color_space = JCS_CMYK; @@ -545,16 +548,13 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo, return fDecoderMgr->returnFailure("setjmp", kInvalidInput); } + this->initializeColorXform(dstInfo); + // Check if we can decode to the requested destination and set the output color space - bool needsColorXform = needs_color_xform(dstInfo, this->getInfo()); - if (!this->setOutputColorSpace(dstInfo, needsColorXform)) { + if (!this->setOutputColorSpace(dstInfo)) { return fDecoderMgr->returnFailure("setOutputColorSpace", kInvalidConversion); } - if (!this->initializeColorXform(dstInfo, needsColorXform)) { - return fDecoderMgr->returnFailure("initializeColorXform", kInvalidParameters); - } - if (!jpeg_start_decompress(dinfo)) { return fDecoderMgr->returnFailure("startDecompress", kInvalidInput); } @@ -630,16 +630,12 @@ void SkJpegCodec::initializeSwizzler(const SkImageInfo& dstInfo, const Options& SkASSERT(fSwizzler); } -bool SkJpegCodec::initializeColorXform(const SkImageInfo& dstInfo, bool needsColorXform) { - if (needsColorXform) { +void SkJpegCodec::initializeColorXform(const SkImageInfo& dstInfo) { + if (needs_color_xform(dstInfo, this->getInfo())) { fColorXform = SkColorSpaceXform::New(sk_ref_sp(this->getInfo().colorSpace()), sk_ref_sp(dstInfo.colorSpace())); - if (!fColorXform && kRGBA_F16_SkColorType == dstInfo.colorType()) { - return false; - } + SkASSERT(fColorXform); } - - return true; } SkSampler* SkJpegCodec::getSampler(bool createIfNecessary) { @@ -661,14 +657,11 @@ SkCodec::Result SkJpegCodec::onStartScanlineDecode(const SkImageInfo& dstInfo, return kInvalidInput; } - // Check if we can decode to the requested destination and set the output color space - bool needsColorXform = needs_color_xform(dstInfo, this->getInfo()); - if (!this->setOutputColorSpace(dstInfo, needsColorXform)) { - return kInvalidConversion; - } + this->initializeColorXform(dstInfo); - if (!this->initializeColorXform(dstInfo, needsColorXform)) { - return fDecoderMgr->returnFailure("initializeColorXform", kInvalidParameters); + // Check if we can decode to the requested destination and set the output color space + if (!this->setOutputColorSpace(dstInfo)) { + return fDecoderMgr->returnFailure("setOutputColorSpace", kInvalidConversion); } if (!jpeg_start_decompress(fDecoderMgr->dinfo())) { diff --git a/src/codec/SkJpegCodec.h b/src/codec/SkJpegCodec.h index 192e4be..30425ee 100644 --- a/src/codec/SkJpegCodec.h +++ b/src/codec/SkJpegCodec.h @@ -104,10 +104,10 @@ private: * * Sets the output color space. */ - bool setOutputColorSpace(const SkImageInfo& dst, bool needsColorXform); + bool setOutputColorSpace(const SkImageInfo& dst); void initializeSwizzler(const SkImageInfo& dstInfo, const Options& options); - bool initializeColorXform(const SkImageInfo& dstInfo, bool needsColorXform); + void initializeColorXform(const SkImageInfo& dstInfo); void allocateStorage(const SkImageInfo& dstInfo); int readRows(const SkImageInfo& dstInfo, void* dst, size_t rowBytes, int count); diff --git a/src/codec/SkPngCodec.cpp b/src/codec/SkPngCodec.cpp index 34e6f91..36ec21f 100644 --- a/src/codec/SkPngCodec.cpp +++ b/src/codec/SkPngCodec.cpp @@ -357,25 +357,6 @@ static int bytes_per_pixel(int bitsPerPixel) { return bitsPerPixel / 8; } -static bool png_conversion_possible(const SkImageInfo& dst, const SkImageInfo& src) { - // Ensure the alpha type is valid - if (!valid_alpha(dst.alphaType(), src.alphaType())) { - return false; - } - - // Check for supported color types - switch (dst.colorType()) { - case kRGBA_8888_SkColorType: - case kBGRA_8888_SkColorType: - case kRGBA_F16_SkColorType: - return true; - case kRGB_565_SkColorType: - return kOpaque_SkAlphaType == src.alphaType(); - default: - return dst.colorType() == src.colorType(); - } -} - void SkPngCodec::allocateStorage(const SkImageInfo& dstInfo) { switch (fXformMode) { case kSwizzleOnly_XformMode: @@ -422,7 +403,7 @@ public: Result onStartScanlineDecode(const SkImageInfo& dstInfo, const Options& options, SkPMColor ctable[], int* ctableCount) override { - if (!png_conversion_possible(dstInfo, this->getInfo()) || + if (!conversion_possible(dstInfo, this->getInfo()) || !this->initializeXforms(dstInfo, options, ctable, ctableCount)) { return kInvalidConversion; @@ -489,7 +470,7 @@ public: Result onStartScanlineDecode(const SkImageInfo& dstInfo, const Options& options, SkPMColor ctable[], int* ctableCount) override { - if (!png_conversion_possible(dstInfo, this->getInfo()) || + if (!conversion_possible(dstInfo, this->getInfo()) || !this->initializeXforms(dstInfo, options, ctable, ctableCount)) { return kInvalidConversion; @@ -807,20 +788,10 @@ bool SkPngCodec::initializeXforms(const SkImageInfo& dstInfo, const Options& opt fSwizzler.reset(nullptr); fColorXform = nullptr; - bool needsColorXform = needs_color_xform(dstInfo, this->getInfo()); - if (needsColorXform) { - if (kGray_8_SkColorType == dstInfo.colorType() || - kRGB_565_SkColorType == dstInfo.colorType()) - { - return false; - } - + if (needs_color_xform(dstInfo, this->getInfo())) { fColorXform = SkColorSpaceXform::New(sk_ref_sp(this->getInfo().colorSpace()), sk_ref_sp(dstInfo.colorSpace())); - - if (!fColorXform && kRGBA_F16_SkColorType == dstInfo.colorType()) { - return false; - } + SkASSERT(fColorXform); } // If the image is RGBA and we have a color xform, we can skip the swizzler. @@ -907,7 +878,7 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, size_t rowBytes, const Options& options, SkPMColor ctable[], int* ctableCount, int* rowsDecoded) { - if (!png_conversion_possible(dstInfo, this->getInfo()) || + if (!conversion_possible(dstInfo, this->getInfo()) || !this->initializeXforms(dstInfo, options, ctable, ctableCount)) { return kInvalidConversion; diff --git a/src/codec/SkRawCodec.cpp b/src/codec/SkRawCodec.cpp index 37d2d35..2a6a48f 100644 --- a/src/codec/SkRawCodec.cpp +++ b/src/codec/SkRawCodec.cpp @@ -683,7 +683,7 @@ SkCodec::Result SkRawCodec::onGetPixels(const SkImageInfo& requestedInfo, void* size_t dstRowBytes, const Options& options, SkPMColor ctable[], int* ctableCount, int* rowsDecoded) { - if (!conversion_possible(requestedInfo, this->getInfo())) { + if (!conversion_possible_ignore_color_space(requestedInfo, this->getInfo())) { SkCodecPrintf("Error: cannot convert input type to output type.\n"); return kInvalidConversion; } diff --git a/src/codec/SkWebpCodec.cpp b/src/codec/SkWebpCodec.cpp index 0c3aa40..e8b27b2 100644 --- a/src/codec/SkWebpCodec.cpp +++ b/src/codec/SkWebpCodec.cpp @@ -144,25 +144,6 @@ SkCodec* SkWebpCodec::NewFromStream(SkStream* stream) { streamDeleter.release(), demux.release(), std::move(data)); } -static bool webp_conversion_possible(const SkImageInfo& dst, const SkImageInfo& src, - SkColorSpaceXform* colorXform) { - if (!valid_alpha(dst.alphaType(), src.alphaType())) { - return false; - } - - switch (dst.colorType()) { - case kRGBA_F16_SkColorType: - return nullptr != colorXform; - case kBGRA_8888_SkColorType: - case kRGBA_8888_SkColorType: - return true; - case kRGB_565_SkColorType: - return nullptr == colorXform && src.alphaType() == kOpaque_SkAlphaType; - default: - return false; - } -} - SkISize SkWebpCodec::onGetScaledDimensions(float desiredScale) const { SkISize dim = this->getInfo().dimensions(); // SkCodec treats zero dimensional images as errors, so the minimum size @@ -212,15 +193,15 @@ bool SkWebpCodec::onGetValidSubset(SkIRect* desiredSubset) const { SkCodec::Result SkWebpCodec::onGetPixels(const SkImageInfo& dstInfo, void* dst, size_t rowBytes, const Options& options, SkPMColor*, int*, int* rowsDecodedPtr) { + if (!conversion_possible(dstInfo, this->getInfo())) { + return kInvalidConversion; + } std::unique_ptr colorXform = nullptr; if (needs_color_xform(dstInfo, this->getInfo())) { colorXform = SkColorSpaceXform::New(sk_ref_sp(this->getInfo().colorSpace()), sk_ref_sp(dstInfo.colorSpace())); - } - - if (!webp_conversion_possible(dstInfo, this->getInfo(), colorXform.get())) { - return kInvalidConversion; + SkASSERT(colorXform); } WebPDecoderConfig config; diff --git a/tests/CodecTest.cpp b/tests/CodecTest.cpp index 9c60be4..b01750d 100644 --- a/tests/CodecTest.cpp +++ b/tests/CodecTest.cpp @@ -1120,3 +1120,33 @@ DEF_TEST(Codec_PngRoundTrip, r) { REPORTER_ASSERT(r, SkCodec::kSuccess == result); check_round_trip(r, bm2); } + +static void test_conversion_possible(skiatest::Reporter* r, const char* path, + bool testScanlineDecoder) { + SkAutoTDelete stream(resource(path)); + SkAutoTDelete codec(SkCodec::NewFromStream(stream.release())); + SkImageInfo infoF16 = codec->getInfo().makeColorType(kRGBA_F16_SkColorType); + + SkBitmap bm; + bm.allocPixels(infoF16); + SkCodec::Result result = codec->getPixels(infoF16, bm.getPixels(), bm.rowBytes()); + REPORTER_ASSERT(r, SkCodec::kInvalidConversion == result); + if (testScanlineDecoder) { + result = codec->startScanlineDecode(infoF16); + REPORTER_ASSERT(r, SkCodec::kInvalidConversion == result); + } + + infoF16 = infoF16.makeColorSpace(infoF16.colorSpace()->makeLinearGamma()); + result = codec->getPixels(infoF16, bm.getPixels(), bm.rowBytes()); + REPORTER_ASSERT(r, SkCodec::kSuccess == result); + if (testScanlineDecoder) { + result = codec->startScanlineDecode(infoF16); + REPORTER_ASSERT(r, SkCodec::kSuccess == result); + } +} + +DEF_TEST(Codec_F16ConversionPossible, r) { + test_conversion_possible(r, "color_wheel.webp", false); + test_conversion_possible(r, "mandrill_512_q075.jpg", true); + test_conversion_possible(r, "yellow_rose.png", true); +} -- 2.7.4