Revert of Checking for valid colorType, alphaType, colorSpace in SkCodec (patchset #2 id:100001 of https://codereview.chromium.org/2319293003/ )
Reason for revert:
Broken perf bots
Original issue's description:
> 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
>
> Committed: https://skia.googlesource.com/skia/+/7a9900d6d34e437bb24beb5524a1f6488ae138c9
TBR=scroggo@google.com,brianosman@google.com
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=skia:
Review-Url: https://codereview.chromium.org/2328663002
diff --git a/src/android/SkBitmapRegionCodec.cpp b/src/android/SkBitmapRegionCodec.cpp
index df0a32c..9c21484 100644
--- a/src/android/SkBitmapRegionCodec.cpp
+++ b/src/android/SkBitmapRegionCodec.cpp
@@ -135,8 +135,5 @@
}
bool SkBitmapRegionCodec::conversionSupported(SkColorType colorType) {
- // 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());
+ return conversion_possible(fCodec->getInfo().makeColorType(colorType), fCodec->getInfo());
}
diff --git a/src/codec/SkBmpCodec.cpp b/src/codec/SkBmpCodec.cpp
index 2f796ad..1c999eb 100644
--- a/src/codec/SkBmpCodec.cpp
+++ b/src/codec/SkBmpCodec.cpp
@@ -603,7 +603,7 @@
SkCodec::Result SkBmpCodec::onStartScanlineDecode(const SkImageInfo& dstInfo,
const SkCodec::Options& options, SkPMColor inputColorPtr[], int* inputColorCount) {
- if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) {
+ if (!conversion_possible(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 5b28252..7d2d398 100644
--- a/src/codec/SkBmpMaskCodec.cpp
+++ b/src/codec/SkBmpMaskCodec.cpp
@@ -39,7 +39,7 @@
return kInvalidScale;
}
- if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) {
+ if (!conversion_possible(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 252d63e..02b42f6 100644
--- a/src/codec/SkBmpRLECodec.cpp
+++ b/src/codec/SkBmpRLECodec.cpp
@@ -44,7 +44,7 @@
// Subsets are not supported.
return kUnimplemented;
}
- if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) {
+ if (!conversion_possible(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 089e310..651ff83 100644
--- a/src/codec/SkBmpStandardCodec.cpp
+++ b/src/codec/SkBmpStandardCodec.cpp
@@ -48,7 +48,7 @@
SkCodecPrintf("Error: scaling not supported.\n");
return kInvalidScale;
}
- if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) {
+ if (!conversion_possible(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 4285786..9a8a43e 100644
--- a/src/codec/SkCodecPriv.h
+++ b/src/codec/SkCodecPriv.h
@@ -107,18 +107,14 @@
}
/*
- * 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 RGBA, BGRA
+ * - always support N32
* - otherwise match the src color type
*/
-static inline bool conversion_possible_ignore_color_space(const SkImageInfo& dst,
- const SkImageInfo& src) {
+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;
@@ -339,38 +335,4 @@
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 1e6e300..dcc25b8 100644
--- a/src/codec/SkGifCodec.cpp
+++ b/src/codec/SkGifCodec.cpp
@@ -450,8 +450,9 @@
SkCodec::Result SkGifCodec::prepareToDecode(const SkImageInfo& dstInfo, SkPMColor* inputColorPtr,
int* inputColorCount, const Options& opts) {
// Check for valid input parameters
- if (!conversion_possible_ignore_color_space(dstInfo, this->getInfo())) {
- return gif_error("Cannot convert input type to output type.\n", kInvalidConversion);
+ if (!conversion_possible(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 d2f7cdb..fcf89d0 100644
--- a/src/codec/SkJpegCodec.cpp
+++ b/src/codec/SkJpegCodec.cpp
@@ -357,7 +357,7 @@
* image has been implemented
* Sets the output color space
*/
-bool SkJpegCodec::setOutputColorSpace(const SkImageInfo& dstInfo) {
+bool SkJpegCodec::setOutputColorSpace(const SkImageInfo& dstInfo, bool needsColorXform) {
if (kUnknown_SkAlphaType == dstInfo.alphaType()) {
return false;
}
@@ -384,7 +384,7 @@
case kBGRA_8888_SkColorType:
if (isCMYK) {
fDecoderMgr->dinfo()->out_color_space = JCS_CMYK;
- } else if (fColorXform) {
+ } else if (needsColorXform) {
// 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 @@
}
return true;
case kRGB_565_SkColorType:
- if (fColorXform) {
+ if (needsColorXform) {
return false;
}
@@ -405,17 +405,14 @@
}
return true;
case kGray_8_SkColorType:
- if (fColorXform || JCS_GRAYSCALE != encodedColorType) {
+ if (needsColorXform || JCS_GRAYSCALE != encodedColorType) {
return false;
}
fDecoderMgr->dinfo()->out_color_space = JCS_GRAYSCALE;
return true;
case kRGBA_F16_SkColorType:
- SkASSERT(fColorXform);
- if (!dstInfo.colorSpace()->gammaIsLinear()) {
- return false;
- }
+ SkASSERT(needsColorXform);
if (isCMYK) {
fDecoderMgr->dinfo()->out_color_space = JCS_CMYK;
@@ -548,13 +545,16 @@
return fDecoderMgr->returnFailure("setjmp", kInvalidInput);
}
- this->initializeColorXform(dstInfo);
-
// Check if we can decode to the requested destination and set the output color space
- if (!this->setOutputColorSpace(dstInfo)) {
+ bool needsColorXform = needs_color_xform(dstInfo, this->getInfo());
+ if (!this->setOutputColorSpace(dstInfo, needsColorXform)) {
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,12 +630,16 @@
SkASSERT(fSwizzler);
}
-void SkJpegCodec::initializeColorXform(const SkImageInfo& dstInfo) {
- if (needs_color_xform(dstInfo, this->getInfo())) {
+bool SkJpegCodec::initializeColorXform(const SkImageInfo& dstInfo, bool needsColorXform) {
+ if (needsColorXform) {
fColorXform = SkColorSpaceXform::New(sk_ref_sp(this->getInfo().colorSpace()),
sk_ref_sp(dstInfo.colorSpace()));
- SkASSERT(fColorXform);
+ if (!fColorXform && kRGBA_F16_SkColorType == dstInfo.colorType()) {
+ return false;
+ }
}
+
+ return true;
}
SkSampler* SkJpegCodec::getSampler(bool createIfNecessary) {
@@ -657,11 +661,14 @@
return kInvalidInput;
}
- this->initializeColorXform(dstInfo);
-
// Check if we can decode to the requested destination and set the output color space
- if (!this->setOutputColorSpace(dstInfo)) {
- return fDecoderMgr->returnFailure("setOutputColorSpace", kInvalidConversion);
+ bool needsColorXform = needs_color_xform(dstInfo, this->getInfo());
+ if (!this->setOutputColorSpace(dstInfo, needsColorXform)) {
+ return kInvalidConversion;
+ }
+
+ if (!this->initializeColorXform(dstInfo, needsColorXform)) {
+ return fDecoderMgr->returnFailure("initializeColorXform", kInvalidParameters);
}
if (!jpeg_start_decompress(fDecoderMgr->dinfo())) {
diff --git a/src/codec/SkJpegCodec.h b/src/codec/SkJpegCodec.h
index 30425ee..192e4be 100644
--- a/src/codec/SkJpegCodec.h
+++ b/src/codec/SkJpegCodec.h
@@ -104,10 +104,10 @@
*
* Sets the output color space.
*/
- bool setOutputColorSpace(const SkImageInfo& dst);
+ bool setOutputColorSpace(const SkImageInfo& dst, bool needsColorXform);
void initializeSwizzler(const SkImageInfo& dstInfo, const Options& options);
- void initializeColorXform(const SkImageInfo& dstInfo);
+ bool initializeColorXform(const SkImageInfo& dstInfo, bool needsColorXform);
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 36ec21f..34e6f91 100644
--- a/src/codec/SkPngCodec.cpp
+++ b/src/codec/SkPngCodec.cpp
@@ -357,6 +357,25 @@
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:
@@ -403,7 +422,7 @@
Result onStartScanlineDecode(const SkImageInfo& dstInfo, const Options& options,
SkPMColor ctable[], int* ctableCount) override {
- if (!conversion_possible(dstInfo, this->getInfo()) ||
+ if (!png_conversion_possible(dstInfo, this->getInfo()) ||
!this->initializeXforms(dstInfo, options, ctable, ctableCount))
{
return kInvalidConversion;
@@ -470,7 +489,7 @@
Result onStartScanlineDecode(const SkImageInfo& dstInfo, const Options& options,
SkPMColor ctable[], int* ctableCount) override {
- if (!conversion_possible(dstInfo, this->getInfo()) ||
+ if (!png_conversion_possible(dstInfo, this->getInfo()) ||
!this->initializeXforms(dstInfo, options, ctable, ctableCount))
{
return kInvalidConversion;
@@ -788,10 +807,20 @@
fSwizzler.reset(nullptr);
fColorXform = nullptr;
- if (needs_color_xform(dstInfo, this->getInfo())) {
+ bool needsColorXform = needs_color_xform(dstInfo, this->getInfo());
+ if (needsColorXform) {
+ if (kGray_8_SkColorType == dstInfo.colorType() ||
+ kRGB_565_SkColorType == dstInfo.colorType())
+ {
+ return false;
+ }
+
fColorXform = SkColorSpaceXform::New(sk_ref_sp(this->getInfo().colorSpace()),
sk_ref_sp(dstInfo.colorSpace()));
- SkASSERT(fColorXform);
+
+ if (!fColorXform && kRGBA_F16_SkColorType == dstInfo.colorType()) {
+ return false;
+ }
}
// If the image is RGBA and we have a color xform, we can skip the swizzler.
@@ -878,7 +907,7 @@
size_t rowBytes, const Options& options,
SkPMColor ctable[], int* ctableCount,
int* rowsDecoded) {
- if (!conversion_possible(dstInfo, this->getInfo()) ||
+ if (!png_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 2a6a48f..37d2d35 100644
--- a/src/codec/SkRawCodec.cpp
+++ b/src/codec/SkRawCodec.cpp
@@ -683,7 +683,7 @@
size_t dstRowBytes, const Options& options,
SkPMColor ctable[], int* ctableCount,
int* rowsDecoded) {
- if (!conversion_possible_ignore_color_space(requestedInfo, this->getInfo())) {
+ if (!conversion_possible(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 e8b27b2..0c3aa40 100644
--- a/src/codec/SkWebpCodec.cpp
+++ b/src/codec/SkWebpCodec.cpp
@@ -144,6 +144,25 @@
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
@@ -193,15 +212,15 @@
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()));
- SkASSERT(colorXform);
+ }
+
+ if (!webp_conversion_possible(dstInfo, this->getInfo(), colorXform.get())) {
+ return kInvalidConversion;
}
WebPDecoderConfig config;