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

src/images/SkImageDecoder_ktx.cpp

 /*
  * Copyright 2014 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkColorPriv.h"
 #include "SkImageDecoder.h"
 #include "SkPixelRef.h"
@@ skipping 49 matching lines @@
     }
 
     const unsigned short width = ktxFile.width();
     const unsigned short height = ktxFile.height();
 
     // should we allow the Chooser (if present) to pick a config for us???
     if (!this->chooseFromOneChoice(SkBitmap::kARGB_8888_Config, width, height)) {
         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;
     }
     
     // If we've made it this far, then we know how to grok the data.
     if (!this->allocPixelRef(bm, NULL)) {
         return false;
     }
 
     // Lock the pixels, since we're about to write to them...
@@ skipping 41 matching lines @@
         srcRow += sampler.srcY0() * srcRowBytes;
         for (int y = 0; y < dstHeight; ++y) {
             sampler.next(srcRow);
             srcRow += sampler.srcDY() * srcRowBytes;
         }
 
         return true;
 
     } else if (ktxFile.isRGBA8()) {
 
+        // Uncompressed RGBA data
+
         // 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;
-        }
+        // we need to turn off the premultiplier
+        if (bSrcIsPremul) {
+            SkASSERT(bm->alphaType() == kPremul_SkAlphaType);
+            SkASSERT(!this->getRequireUnpremultipliedColors());
 
-        // Uncompressed RGBA data
-        if (!sampler.begin(bm, SkScaledBitmapSampler::kRGBA, *this)) {
-            return false;
+            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
         const int srcRowBytes = width * 4;
         const int dstHeight = sampler.scaledHeight();
         const uint8_t *srcRow = reinterpret_cast<const uint8_t *>(ktxFile.pixelData());
         srcRow += sampler.srcY0() * srcRowBytes;
         for (int y = 0; y < dstHeight; ++y) {
             sampler.next(srcRow);
             srcRow += sampler.srcDY() * srcRowBytes;
         }
 
-        // Reset this in case the decoder needs to be used again.
-        if (setRequireUnpremul) {
-            this->setRequireUnpremultipliedColors(false);
-        }
-
         return true;
     }
 
     return false;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 
 // KTX Image Encoder
 //
@@ skipping 75 matching lines @@
     return SkImageDecoder::kUnknown_Format;
 }
 
 SkImageEncoder* sk_libktx_efactory(SkImageEncoder::Type t) {
     return (SkImageEncoder::kKTX_Type == t) ? SkNEW(SkKTXImageEncoder) : NULL;
 }
 
 static SkImageDecoder_DecodeReg gReg(sk_libktx_dfactory);
 static SkImageDecoder_FormatReg gFormatReg(get_format_ktx);
 static SkImageEncoder_EncodeReg gEReg(sk_libktx_efactory);