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

diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index caf6f52..fcc4a33 100644 (file)
--- 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 (file)
--- 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<SkColorSpace> 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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 (file)
--- 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<SkColorSpaceXform> 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 (file)
--- 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<SkStream> stream(resource(path));
+    SkAutoTDelete<SkCodec> 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);
+}