Image decoder fixes (mostly) around A8.

src/images/SkImageDecoder_libpng.cpp

Index: src/images/SkImageDecoder_libpng.cpp
diff --git a/src/images/SkImageDecoder_libpng.cpp b/src/images/SkImageDecoder_libpng.cpp
index 4e4106aede85f8266c3b9ffd234e025f9f6a0e02..ed9b59e2d0fce2c996f2e50fba933346a9807df9 100644
--- a/src/images/SkImageDecoder_libpng.cpp
+++ b/src/images/SkImageDecoder_libpng.cpp
@@ -374,9 +374,14 @@ bool SkPNGImageDecoder::onDecode(SkStream* sk_stream, SkBitmap* decodedBitmap,
 
     if ((SkBitmap::kA8_Config == config || SkBitmap::kIndex8_Config == config)
         && 1 == sampleSize) {
-        // A8 is only allowed if the original was GRAY.
-        SkASSERT(config != SkBitmap::kA8_Config
-                 || PNG_COLOR_TYPE_GRAY == colorType);
+        if (SkBitmap::kA8_Config == config) {
+            // For an A8 bitmap, we assume there is an alpha for speed. It is
+            // possible the bitmap is opaque, but that is an unlikely use case
+            // since it would not be very interesting.
+            reallyHasAlpha = true;
+            // A8 is only allowed if the original was GRAY.
+            SkASSERT(PNG_COLOR_TYPE_GRAY == colorType);
+        }
         for (int i = 0; i < number_passes; i++) {
             for (png_uint_32 y = 0; y < origHeight; y++) {
                 uint8_t* bmRow = decodedBitmap->getAddr8(0, y);
@@ -459,15 +464,14 @@ bool SkPNGImageDecoder::onDecode(SkStream* sk_stream, SkBitmap* decodedBitmap,
         reallyHasAlpha |= substituteTranspColor(decodedBitmap, theTranspColor);
     }
     if (reallyHasAlpha && this->getRequireUnpremultipliedColors() &&
-        SkBitmap::kARGB_8888_Config != decodedBitmap->config()) {
+        SkBitmap::kARGB_8888_Config != decodedBitmap->config() &&
+        SkBitmap::kA8_Config != decodedBitmap->config()) {
         // If the caller wants an unpremultiplied bitmap, and we let them get
         // away with a config other than 8888, and it has alpha after all,
         // return false, since the result will have premultiplied colors.
+        // The exception is A8, where there is nothing to premultiply.
         return false;
     }
-    if (SkBitmap::kA8_Config == decodedBitmap->config()) {
-        reallyHasAlpha = true;
-    }
     decodedBitmap->setIsOpaque(!reallyHasAlpha);
     return true;
 }
@@ -838,9 +842,14 @@ bool SkPNGImageDecoder::onDecodeSubset(SkBitmap* bm, const SkIRect& region) {
 
     if ((SkBitmap::kA8_Config == config || SkBitmap::kIndex8_Config == config)
         && 1 == sampleSize) {
-        // A8 is only allowed if the original was GRAY.
-        SkASSERT(config != SkBitmap::kA8_Config
-                 || PNG_COLOR_TYPE_GRAY == colorType);
+        if (SkBitmap::kA8_Config == config) {
+            // For an A8 bitmap, we assume there is an alpha for speed. It is
+            // possible the bitmap is opaque, but that is an unlikely use case
+            // since it would not be very interesting.
+            reallyHasAlpha = true;
+            // A8 is only allowed if the original was GRAY.
+            SkASSERT(PNG_COLOR_TYPE_GRAY == colorType);
+        }
 
         for (int i = 0; i < number_passes; i++) {
             png_configure_decoder(png_ptr, &actualTop, i);
@@ -931,8 +940,14 @@ bool SkPNGImageDecoder::onDecodeSubset(SkBitmap* bm, const SkIRect& region) {
     if (0 != theTranspColor) {
         reallyHasAlpha |= substituteTranspColor(&decodedBitmap, theTranspColor);
     }
-    if (SkBitmap::kA8_Config == decodedBitmap.config()) {
-        reallyHasAlpha = true;
+    if (reallyHasAlpha && this->getRequireUnpremultipliedColors() &&
+        SkBitmap::kARGB_8888_Config != decodedBitmap.config() &&
+        SkBitmap::kA8_Config != decodedBitmap.config()) {
+        // If the caller wants an unpremultiplied bitmap, and we let them get
+        // away with a config other than 8888, and it has alpha after all,
+        // return false, since the result will have premultiplied colors.
+        // The exception is A8, where there is nothing to premultiply.

[reed1, 2013/10/08 16:24:17]

What is the config that 
- has alpha
- has premul color components
- is not 8888
- cannot support unpremul
?

[scroggo, 2013/10/10 21:46:59]

4444 and Index8. Both of them COULD support unpremul (and in fact for Index8 we
have actually skipped premultiplication), but we had decided to return false and
limit unpremul to 8888.

[reed1, 2013/10/14 13:26:05]

Thanks for the dox... I think the code is not as clear. Is it correct to write
the code as:

if (hasAlpha && this->requireUnpremul()) {
    switch (config) {
        ....
        return false;   // we have chosen to not support premul for these.
    }
}

[scroggo, 2013/10/14 17:15:19]

Yes. Latest code follows this pattern.

+        return false;
     }
     decodedBitmap.setIsOpaque(!reallyHasAlpha);