Checking for valid colorType, alphaType, colorSpace in SkCodec

src/codec/SkJpegCodec.cpp

Index: src/codec/SkJpegCodec.cpp
diff --git a/src/codec/SkJpegCodec.cpp b/src/codec/SkJpegCodec.cpp
index fcf89d0c1d23b7c3ca6d55ec2674754602778cbb..da608ab5d133bff3f050c763b7e2696f40fceb5e 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,14 +548,16 @@ SkCodec::Result SkJpegCodec::onGetPixels(const SkImageInfo& dstInfo,
         return fDecoderMgr->returnFailure("setjmp", 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 fDecoderMgr->returnFailure("setOutputColorSpace", kInvalidConversion);
+    if (needsColorXform) {
+        if (!this->initializeColorXform(dstInfo)) {
+            return fDecoderMgr->returnFailure("initializeColorXform", kInvalidParameters);
+        }
     }
 
-    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(dinfo)) {
@@ -630,13 +635,12 @@ void SkJpegCodec::initializeSwizzler(const SkImageInfo& dstInfo, const Options&
     SkASSERT(fSwizzler);
 }
 
-bool SkJpegCodec::initializeColorXform(const SkImageInfo& dstInfo, bool needsColorXform) {
-    if (needsColorXform) {
-        fColorXform = SkColorSpaceXform::New(sk_ref_sp(this->getInfo().colorSpace()),
-                                             sk_ref_sp(dstInfo.colorSpace()));
-        if (!fColorXform && kRGBA_F16_SkColorType == dstInfo.colorType()) {
-            return false;
-        }
+bool SkJpegCodec::initializeColorXform(const SkImageInfo& dstInfo) {
+    fColorXform = SkColorSpaceXform::New(sk_ref_sp(this->getInfo().colorSpace()),
+                                         sk_ref_sp(dstInfo.colorSpace()));
+    // TODO: Can we assert this instead?

[msarett, 2016/09/08 16:01:58]

This can be an assert when Brian's CL lands:
https://codereview.chromium.org/2323003002/

+    if (!fColorXform) {
+        return false;
     }
 
     return true;
@@ -661,14 +665,16 @@ 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());

[scroggo, 2016/09/08 16:33:11]

Both callers check this first and otherwise don't use the boolean. Why not fold
it into the method?

[msarett, 2016/09/08 16:49:45]

Yes, sgtm.

-    if (!this->setOutputColorSpace(dstInfo, needsColorXform)) {
-        return kInvalidConversion;
+    if (needsColorXform) {
+        if (!this->initializeColorXform(dstInfo)) {
+            return fDecoderMgr->returnFailure("initializeColorXform", kInvalidParameters);
+        }
     }
 
-    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())) {