Image decoder fixes (mostly) around A8.

src/images/SkImageDecoder_libpng.cpp

 
 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 
 #include "SkImageDecoder.h"
@@ skipping 356 matching lines @@
                               png_set_interlace_handling(png_ptr) : 1;
 
     /* Optional call to gamma correct and add the background to the palette
     *  and update info structure.  REQUIRED if you are expecting libpng to
     *  update the palette for you (ie you selected such a transform above).
     */
     png_read_update_info(png_ptr, info_ptr);
 
     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);
                 png_read_rows(png_ptr, &bmRow, png_bytepp_NULL, 1);
             }
         }
     } else {
         SkScaledBitmapSampler::SrcConfig sc;
         int srcBytesPerPixel = 4;
 
@@ skipping 62 matching lines @@
         }
     }
 
     /* read rest of file, and get additional chunks in info_ptr - REQUIRED */
     png_read_end(png_ptr, info_ptr);
 
     if (0 != theTranspColor) {
         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;
 }
 
 
 
 bool SkPNGImageDecoder::getBitmapConfig(png_structp png_ptr, png_infop info_ptr,
                                         SkBitmap::Config* SK_RESTRICT configp,
                                         bool* SK_RESTRICT hasAlphap,
                                         SkPMColor* SK_RESTRICT theTranspColorp) {
@@ skipping 350 matching lines @@
 #else
     // FIXME: This sets pass as desired, but also sets iwidth. Is that ok?
     png_set_interlaced_pass(png_ptr, 0);
 #endif
     png_read_update_info(png_ptr, info_ptr);
 
     int actualTop = rect.fTop;
 
     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);
             for (int j = 0; j < rect.fTop - actualTop; j++) {
                 uint8_t* bmRow = decodedBitmap.getAddr8(0, 0);
                 png_read_rows(png_ptr, &bmRow, png_bytepp_NULL, 1);
             }
             png_uint_32 bitmapHeight = (png_uint_32) decodedBitmap.height();
             for (png_uint_32 y = 0; y < bitmapHeight; y++) {
                 uint8_t* bmRow = decodedBitmap.getAddr8(0, y);
@@ skipping 70 matching lines @@
                 if (y < height - 1) {
                     skip_src_rows(png_ptr, srcRow, sampler.srcDY() - 1);
                 }
             }
         }
     }
 
     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);
 
     if (swapOnly) {
         bm->swap(decodedBitmap);
         return true;
     }
     return this->cropBitmap(bm, &decodedBitmap, sampleSize, region.x(), region.y(),
                             region.width(), region.height(), 0, rect.y());
 }
@@ skipping 296 matching lines @@
     return SkImageDecoder::kUnknown_Format;
 }
 
 SkImageEncoder* sk_libpng_efactory(SkImageEncoder::Type t) {
     return (SkImageEncoder::kPNG_Type == t) ? SkNEW(SkPNGImageEncoder) : NULL;
 }
 
 static SkImageDecoder_DecodeReg gDReg(sk_libpng_dfactory);
 static SkImageDecoder_FormatReg gFormatReg(get_format_png);
 static SkImageEncoder_EncodeReg gEReg(sk_libpng_efactory);