Fixes for decoding to 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 417 matching lines @@
     if (0 != theTranspColor) {
         reallyHasAlpha |= substituteTranspColor(decodedBitmap, theTranspColor);
     }
     if (reallyHasAlpha && this->getRequireUnpremultipliedColors() &&
         SkBitmap::kARGB_8888_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.
         return false;
     }
+    if (SkBitmap::kA8_Config == decodedBitmap->config()) {

[reed1, 2013/10/08 16:25:57]

Shouldn't this block be done before line 431?

+        reallyHasAlpha = true;

[reed1, 2013/10/04 08:56:27]

I thought this variable was set when we actually saw a non-opaque value. If that
is true, why do we need to force it to true here? What if the alpha-only PNG was
in fact 100% opaque?

[scroggo, 2013/10/04 19:45:53]

Setting the image to be opaque is an optimization for drawing fast. An alpha
only PNG that is 100% opaque is not very interesting, and I suspect not used
much. Right now we don't know whether the image has alpha for certain, and we'd
have to look at each pixel to determine the answer (we could update the sampler
to check, but in the fast case of skipping the sampler, we don't look at the
pixels directly). I don't think it makes sense to add the opaque optimization
for the rare case while penalizing the common case.

[reed1, 2013/10/07 13:31:39]

Ah, I think I see. You're saying we don't always perform the check for
reallyHasAlpha (I thought we did). But now I see around line 350 we seem to not
inspect the values. If that is the case, I suggest we a the line

reallyHasAlpha = true;

... in that spot, with a comment to the effect of // likely we do have non-0xFF,
but faster not to check...
rather than down here.

[scroggo, 2013/10/07 22:10:29]

Sounds good to me. Uploaded https://codereview.chromium.org/26210007/, which
includes fixes for other bugs I found while making these changes.

+    }
     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 433 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;
+    }
     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());
 }
 #endif
@@ skipping 295 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);