Do a better job of enforcing the semantics of the setRequireUnpremultipliedColors flag

src/images/SkImageDecoder_ktx.cpp

Index: src/images/SkImageDecoder_ktx.cpp
diff --git a/src/images/SkImageDecoder_ktx.cpp b/src/images/SkImageDecoder_ktx.cpp
index 6ff245954008fed7845d651436157240ddc2870d..c0751d5d36e7068f9191f28aad58fb545e8b12e7 100644
--- a/src/images/SkImageDecoder_ktx.cpp
+++ b/src/images/SkImageDecoder_ktx.cpp
@@ -67,14 +67,32 @@ bool SkKTXImageDecoder::onDecode(SkStream* stream, SkBitmap* bm, Mode mode) {
         return false;
     }
 
+    // Set a flag if our source is premultiplied alpha
+    const SkString premulKey("KTXPremultipliedAlpha");
+    bool bSrcIsPremul = ktxFile.getValueForKey(premulKey) == SkString("True");

[scroggo, 2014/06/10 13:50:23]

const

[krajcevski, 2014/06/10 14:47:29]

Done.

+
     // Setup the sampler...
     SkScaledBitmapSampler sampler(width, height, this->getSampleSize());
 
+    // Determine the alpha of the bitmap...
+    SkAlphaType alphaType = kOpaque_SkAlphaType;
+    if (ktxFile.isRGBA8()) {
+        if (this->getRequireUnpremultipliedColors()) {
+            alphaType = kUnpremul_SkAlphaType;
+            // If the client wants unpremul colors and we only have
+            // premul, then we cannot honor their wish.

[scroggo, 2014/06/10 13:50:23]

FWIW, we *could* do a second pass and unpremultiply the data
(src/ports/SkImageDecoder_CG.cpp does this). I think it's okay to return false
though.

[krajcevski, 2014/06/10 14:47:29]

I agree, although I believe the use case for that is pretty limited so it's not
worth the time right now.

+            if (bSrcIsPremul) {
+                return false;
+            }
+        } else {
+            alphaType = kPremul_SkAlphaType;
+        }
+    }
+
     // Set the config...
     bm->setConfig(SkBitmap::kARGB_8888_Config,
                   sampler.scaledWidth(), sampler.scaledHeight(),
-                  0,
-                  ktxFile.isRGBA8()? kPremul_SkAlphaType : kOpaque_SkAlphaType);
+                  0, alphaType);
     if (SkImageDecoder::kDecodeBounds_Mode == mode) {
         return true;
     }
@@ -136,18 +154,23 @@ bool SkKTXImageDecoder::onDecode(SkStream* stream, SkBitmap* bm, Mode mode) {
 
     } else if (ktxFile.isRGBA8()) {
 
-        // If we know that the image contains premultiplied alpha, then
-        // don't premultiply it upon decoding.
-        bool setRequireUnpremul = false;
-        const SkString premulKey("KTXPremultipliedAlpha");
-        if (ktxFile.getValueForKey(premulKey) == SkString("True")) {
-            this->setRequireUnpremultipliedColors(true);
-            setRequireUnpremul = true;
-        }
-
         // Uncompressed RGBA data
-        if (!sampler.begin(bm, SkScaledBitmapSampler::kRGBA, *this)) {
-            return false;
+
+        // If we know that the image contains premultiplied alpha, then
+        // we need to turn off the premultiplier
+        if (bSrcIsPremul) {
+            SkASSERT(bm->alphaType() == kPremul_SkAlphaType);
+            SkASSERT(!this->getRequireUnpremultipliedColors());
+
+            SkScaledBitmapSampler::Options opts (*this);

[scroggo, 2014/06/10 13:50:23]

It might be simpler to move this outside of the if statement:

SkScaledBitmapSampler::Options opts (*this);
if (bSrcIsPremul) {
  // asserts
  opts.fPremultiplyAlpha = false;
}
if (!sampler.begin(bm, SkScaledBitmapSampler::kRGBA, opts)) {
  return false;
}

[krajcevski, 2014/06/10 14:47:29]

Done.

+            opts.fPremultiplyAlpha = false;
+            if (!sampler.begin(bm, SkScaledBitmapSampler::kRGBA, opts)) {
+                return false;
+            }
+        } else {
+            if (!sampler.begin(bm, SkScaledBitmapSampler::kRGBA, *this)) {
+                return false;
+            }
         }
 
         // Just need to read RGBA pixels
@@ -160,11 +183,6 @@ bool SkKTXImageDecoder::onDecode(SkStream* stream, SkBitmap* bm, Mode mode) {
             srcRow += sampler.srcDY() * srcRowBytes;
         }
 
-        // Reset this in case the decoder needs to be used again.
-        if (setRequireUnpremul) {
-            this->setRequireUnpremultipliedColors(false);
-        }
-
         return true;
     }