A variety of SkPngCodec clean-ups

src/codec/SkPngCodec.cpp

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
 #include "SkColorTable.h"
 #include "SkBitmap.h"
 #include "SkMath.h"
+#include "SkOpts.h"
 #include "SkPngCodec.h"
 #include "SkPngFilters.h"
 #include "SkSize.h"
 #include "SkStream.h"
 #include "SkSwizzler.h"
 #include "SkTemplates.h"
 
 #if defined(__SSE2__)
     #include "pngstruct.h"
 
     extern "C" void sk_png_init_filter_functions_sse2(png_structp png, unsigned int bpp) {
         if (bpp == 3) {
             png->read_filter[PNG_FILTER_VALUE_SUB  -1] =   sk_sub3_sse2;
             png->read_filter[PNG_FILTER_VALUE_AVG  -1] =   sk_avg3_sse2;
             png->read_filter[PNG_FILTER_VALUE_PAETH-1] = sk_paeth3_sse2;
         }
         if (bpp == 4) {
             png->read_filter[PNG_FILTER_VALUE_SUB  -1] =   sk_sub4_sse2;
             png->read_filter[PNG_FILTER_VALUE_AVG  -1] =   sk_avg4_sse2;
             png->read_filter[PNG_FILTER_VALUE_PAETH-1] = sk_paeth4_sse2;
         }
     }
 #endif
 
 ///////////////////////////////////////////////////////////////////////////////
-// Helper macros

[msarett, 2016/01/28 01:26:18]

IMO these don't help much.  We need to be compiled 1.6+ anyway to optimize the
filter functions.

[scroggo, 2016/01/28 21:07:32]

They were added to the original SkImageDecoder in
c1c5695f8f0cda7cbabb490d274d742ca0e26eed. I copied them over to SkCodec.

Does removing these give us problems when using the older versions of libpng? Do
we care?

[msarett, 2016/01/28 22:19:24]

Thanks for the reference.  Looking at those notes, I actually I think they
won't.  If they do, Google3 should break and tell us.
I don't :).

-///////////////////////////////////////////////////////////////////////////////
-
-#ifndef png_jmpbuf
-#  define png_jmpbuf(png_ptr) ((png_ptr)->jmpbuf)
-#endif
-
-/* These were dropped in libpng >= 1.4 */
-#ifndef png_infopp_NULL
-    #define png_infopp_NULL nullptr
-#endif
-
-#ifndef png_bytepp_NULL
-    #define png_bytepp_NULL nullptr
-#endif
-
-#ifndef int_p_NULL
-    #define int_p_NULL nullptr
-#endif
-
-#ifndef png_flush_ptr_NULL
-    #define png_flush_ptr_NULL nullptr
-#endif
-
-///////////////////////////////////////////////////////////////////////////////
 // Callback functions
 ///////////////////////////////////////////////////////////////////////////////
 
 static void sk_error_fn(png_structp png_ptr, png_const_charp msg) {
     SkCodecPrintf("------ png error %s\n", msg);
     longjmp(png_jmpbuf(png_ptr), 1);
 }
 
 void sk_warning_fn(png_structp, png_const_charp msg) {
     SkCodecPrintf("----- png warning %s\n", msg);
@@ skipping 26 matching lines @@
 class AutoCleanPng : public SkNoncopyable {
 public:
     AutoCleanPng(png_structp png_ptr)
         : fPng_ptr(png_ptr)
         , fInfo_ptr(nullptr) {}
 
     ~AutoCleanPng() {
         // fInfo_ptr will never be non-nullptr unless fPng_ptr is.
         if (fPng_ptr) {
             png_infopp info_pp = fInfo_ptr ? &fInfo_ptr : nullptr;
-            png_destroy_read_struct(&fPng_ptr, info_pp, png_infopp_NULL);
+            png_destroy_read_struct(&fPng_ptr, info_pp, nullptr);
         }
     }
 
     void setInfoPtr(png_infop info_ptr) {
         SkASSERT(nullptr == fInfo_ptr);
         fInfo_ptr = info_ptr;
     }
 
     void detach() {
         fPng_ptr = nullptr;
         fInfo_ptr = nullptr;
     }
 
 private:
     png_structp     fPng_ptr;
     png_infop       fInfo_ptr;
 };
 #define AutoCleanPng(...) SK_REQUIRE_LOCAL_VAR(AutoCleanPng)
 
-//checks if there is transparency info in the tRNS chunk
-//image types which could have data in the tRNS chunk include: Index8, Gray8, RGB
-static bool has_transparency_in_tRNS(png_structp png_ptr,
-                                        png_infop info_ptr) {
-    if (!png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) {
-        return false;
-    }
-
-    png_bytep trans;
-    int num_trans;
-    png_get_tRNS(png_ptr, info_ptr, &trans, &num_trans, nullptr);
-    return num_trans > 0;
-}
-
-// Method for coverting to either an SkPMColor or a similarly packed
-// unpremultiplied color.
-typedef uint32_t (*PackColorProc)(U8CPU a, U8CPU r, U8CPU g, U8CPU b);
-
 // Note: SkColorTable claims to store SkPMColors, which is not necessarily
 // the case here.
 bool SkPngCodec::decodePalette(bool premultiply, int* ctableCount) {
-    int numPalette;
+
+    int numColors;
     png_colorp palette;
-    png_bytep trans;
-
-    if (!png_get_PLTE(fPng_ptr, fInfo_ptr, &palette, &numPalette)) {
+    if (!png_get_PLTE(fPng_ptr, fInfo_ptr, &palette, &numColors)) {
         return false;
     }
 
-    // Note: These are not necessarily SkPMColors
-    SkPMColor colorStorage[256];    // worst-case storage
-    SkPMColor* colorPtr = colorStorage;
+    // Note: These are not necessarily SkPMColors.
+    SkPMColor colorPtr[256];
+#ifdef SK_PMCOLOR_IS_RGBA
+    SkOpts::RGB_to_RGB1(colorPtr, palette, numColors);

[msarett, 2016/01/28 01:26:18]

It's a little frustrating to optimize this knowing that we are going to memcpy()
it (possibly twice I think), later on.

[msarett, 2016/01/28 02:06:45]

Here we treat a pointer to the png_color struct as uint8_t*.  I think this is
safe unless png changes the implementation of the png_color struct.  Currently
it is:

struct {
uint8 red;
uint8 green;
uint8 blue;
}

[scroggo, 2016/01/28 21:07:33]

I can think of two places we memcpy:
- Into the SkColorTable
Maybe we should fix SkColorTable's API?
- Into the client's memory in getPixels
This is a problem with the SkCodec/SkImageGenerator API, which we should
reconsider

There may be others. I think it makes sense to optimize them separately.

[scroggo, 2016/01/28 21:07:33]

Can you add these comments plus some asserts here? Something like

#ifdef SK_DEBUG
for (int i = 0; i < numColors; i++) {
  png_colorp color = &png_colorp[i];
  SkASSERT(color->red == color[0]);
  ...
}
#endif

Or do you think that's overkill? Or maybe just a static_assert that checks the
struct itself? (Not sure if that's possible.)

[msarett, 2016/01/28 22:19:25]

Yes not going to worry about the memcpys() in this CL.

I've added a static assert and a check for RGB order.

+#else
+    SkOpts::RGB_to_BGR1(colorPtr, palette, numColors);

[scroggo, 2016/01/28 21:07:33]

Should we also consider the case where the client wanted the non-native swizzle?
I suppose we read the colors before that (and even could, if they call a second
time).

[msarett, 2016/01/28 22:19:25]

I'm not sure I completely understand.

I think SkCodec as a whole ought to support the non-native swizzle.  I don't
think this code makes that issue any better or worse.

Same as before, we just always provide the native swizzle.

[scroggo, 2016/01/29 15:40:27]

I think this is a good change, I just wanted to think ahead to when we do
support the non-native swizzle. Ideally, we would decode the palette differently
if they want non-native swizzle, which would imply changing this code some. (So
maybe there should be a TODO here.)

My caveat ("I suppose we read...", above) was that I was thinking we called this
method inside the constructor, meaning we don't yet know which swizzle they
want. But it looks like I was mistaken. We call it in
onGetPixels/onStartScanlineDecode. So we *do* know the order they want. (On
another note, we always redecode the palette on a second call to getPixels. It
would probably be more efficient to skip this if we already have a color table.
But that will require some extra work to make sure that we correct on a second
call if the client swapped their preference.)

[msarett, 2016/01/29 18:56:08]

Thanks, adding a TODO.

+#endif
 
-    int numTrans;
-    if (png_get_valid(fPng_ptr, fInfo_ptr, PNG_INFO_tRNS)) {
-        png_get_tRNS(fPng_ptr, fInfo_ptr, &trans, &numTrans, nullptr);
-    } else {
-        numTrans = 0;
+
+    png_bytep alphas;
+    int numColorsWithAlpha;
+    if (png_get_tRNS(fPng_ptr, fInfo_ptr, &alphas, &numColorsWithAlpha, nullptr)) {
+        for (int i = 0; i < numColorsWithAlpha; i++) {

[msarett, 2016/01/28 01:26:18]

I don't SIMD premuls will do much here for two reasons.

(1) We need to touch each pixel anyway to set a new alpha value.
(2) I would guess that numColorsWithAlpha is often "1" or small.

[msarett, 2016/01/28 02:06:45]

Maybe we should check if alpha is zero (which it will often be) before
multiplying?

Maybe we ought to call SkMulDiv255?

[scroggo, 2016/01/28 21:07:32]

SkPremultiplyARGBInline does. It also calls SkMulDiv255Round. I'm in favor of
being consistent.

[msarett, 2016/01/28 22:19:25]

Changed to use SkMulDiv255Round.

+            // Note that we inline this because we need to make sure that we don't

[scroggo, 2016/01/28 21:07:32]

I didn't understand this comment at first. I think you're saying that you
*already* did the swap, if it was necessary?

I find this a little less clear than the old code, which handles the transparent
colors first, then does the rest. You could use the same logic here, so you
wouldn't have this weird (to me, at least) logic of "we don't need to swap this
time".

[msarett, 2016/01/28 22:19:25]

Yes this is unclear.  I'll fix the comment for now, and I'll come back to the
other stuff.  I changed the order because I think it *should* be faster this
way.

But now I'm realizing that Android does weird things when they encode their
PNGs, and I'd like to fix that first.

[msarett, 2016/01/29 18:56:08]

I think you're right.  My version was less clear and less optimal.  I've changed
this again to be more similar to how it was before.

+            // swap color channels.
+            uint8_t a = alphas[i];
+            uint8_t b = colorPtr[i] >> 16;
+            uint8_t g = colorPtr[i] >>  8;
+            uint8_t r = colorPtr[i] >>  0;
+            b = (b*a+127)/255;
+            g = (g*a+127)/255;
+            r = (r*a+127)/255;
+            colorPtr[i] = (uint32_t)a << 24
+                        | (uint32_t)b << 16
+                        | (uint32_t)g <<  8
+                        | (uint32_t)r <<  0;
+        }
     }
 
-    // check for bad images that might make us crash
-    if (numTrans > numPalette) {
-        numTrans = numPalette;
+    // Pad the color table with the last color in the table (or black) in the case that

[scroggo, 2016/01/28 21:07:32]

But you forgot to include "BUGGY IMAGE WORKAROUND"! ;)

I do wonder why we chose black. I guess it is arbitrary for a bad image, so it
probably doesn't matter.

[msarett, 2016/01/28 22:19:25]

Agreed.

I almost removed the black part completely, but it wasn't clear to me that
libpng checked for zero sized palettes, so I figured we should keep being safe.

+    // invalid pixel indices exceed the number of colors in the table.
+    const int maxColors = 1 << fBitDepth;
+    if (numColors < maxColors) {
+        SkPMColor lastColor = numColors > 0 ? colorPtr[numColors - 1] : SK_ColorBLACK;
+        sk_memset32(colorPtr + numColors, lastColor, maxColors - numColors);
     }
 
-    int index = 0;
-
-    // Choose which function to use to create the color table. If the final destination's
-    // colortype is unpremultiplied, the color table will store unpremultiplied colors.
-    PackColorProc proc;
-    if (premultiply) {
-        proc = &SkPreMultiplyARGB;
-    } else {
-        proc = &SkPackARGB32NoCheck;
-    }
-    for (; index < numTrans; index++) {
-        *colorPtr++ = proc(*trans++, palette->red, palette->green, palette->blue);
-        palette++;
+    // Set the new color count.
+    if (ctableCount != nullptr) {
+        *ctableCount = maxColors;
     }
 
-    for (; index < numPalette; index++) {
-        *colorPtr++ = SkPackARGB32(0xFF, palette->red, palette->green, palette->blue);
-        palette++;
-    }
-
-    /*  BUGGY IMAGE WORKAROUND
-        Invalid images could contain pixel values that are greater than the number of palette
-        entries. Since we use pixel values as indices into the palette this could result in reading
-        beyond the end of the palette which could leak the contents of uninitialized memory. To
-        ensure this doesn't happen, we grow the colortable to the maximum size that can be
-        addressed by the bitdepth of the image and fill it with the last palette color or black if
-        the palette is empty (really broken image).
-    */
-    int colorCount = SkTMax(numPalette, 1 << SkTMin(fBitDepth, 8));
-    SkPMColor lastColor = index > 0 ? colorPtr[-1] : SkPackARGB32(0xFF, 0, 0, 0);
-    for (; index < colorCount; index++) {
-        *colorPtr++ = lastColor;
-    }
-
-    // Set the new color count
-    if (ctableCount != nullptr) {
-        *ctableCount = colorCount;
-    }
-
-    fColorTable.reset(new SkColorTable(colorStorage, colorCount));
+    fColorTable.reset(new SkColorTable(colorPtr, maxColors));
     return true;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Creation
 ///////////////////////////////////////////////////////////////////////////////
 
 bool SkPngCodec::IsPng(const char* buf, size_t bytesRead) {
     return !png_sig_cmp((png_bytep) buf, (png_size_t)0, bytesRead);
 }
@@ skipping 52 matching lines @@
     if (chunkReader) {
         png_set_keep_unknown_chunks(png_ptr, PNG_HANDLE_CHUNK_ALWAYS, (png_byte*)"", 0);
         png_set_read_user_chunk_fn(png_ptr, (png_voidp) chunkReader, sk_read_user_chunk);
     }
 #endif
 
     // The call to png_read_info() gives us all of the information from the
     // PNG file before the first IDAT (image data chunk).
     png_read_info(png_ptr, info_ptr);
     png_uint_32 origWidth, origHeight;
-    int bitDepth, colorType;
+    int bitDepth, encodedColorType;
     png_get_IHDR(png_ptr, info_ptr, &origWidth, &origHeight, &bitDepth,
-                 &colorType, int_p_NULL, int_p_NULL, int_p_NULL);
+                 &encodedColorType, nullptr, nullptr, nullptr);
 
     if (bitDepthPtr) {
         *bitDepthPtr = bitDepth;
     }
 
-    // sanity check for size
-    {
-        int64_t size = sk_64_mul(origWidth, origHeight);

[scroggo, 2016/01/28 21:07:32]

Looks like this got removed. Can you add that to the commit message?

[msarett, 2016/01/28 22:19:25]

Done.

-        // now check that if we are 4-bytes per pixel, we also don't overflow
-        if (size < 0 || size > (0x7FFFFFFF >> 2)) {
-            return false;
-        }
+    // Tell libpng to strip 16 bit/color files down to 8 bits/color.

[scroggo, 2016/01/28 21:07:33]

I mentioned this in the RAW code review - would it make sense to add a swizzler
option which takes 16 bit/color as an input? Perhaps this would speed up
libpng's work?

[msarett, 2016/01/28 22:19:25]

We could certainly handle this case faster in the swizzler.  It's low priority
since I think these images are rare.  So for now, we let libpng handle it.

However, if this would also help RAW, we should consider it more seriously.

+    if (bitDepth == 16) {
+        SkASSERT(PNG_COLOR_TYPE_PALETTE != encodedColorType);
+        png_set_strip_16(png_ptr);
     }
 
-    // Tell libpng to strip 16 bit/color files down to 8 bits/color
-    if (bitDepth == 16) {
-        png_set_strip_16(png_ptr);
-    }
-#ifdef PNG_READ_PACK_SUPPORTED

[scroggo, 2016/01/28 21:07:32]

Why did you remove this?

[msarett, 2016/01/28 22:19:25]

AFAICT if READ_PACK isn't supported we'll decode the PNGs wrong (and leave
memory uninitialized).

I think it's best to fail to compile in this case.

-    // Extract multiple pixels with bit depths of 1, 2, and 4 from a single
-    // byte into separate bytes (useful for paletted and grayscale images).
-    if (bitDepth < 8) {
-        png_set_packing(png_ptr);
-    }
-#endif
-    // Expand grayscale images to the full 8 bits from 1, 2, or 4 bits/pixel.
-    if (colorType == PNG_COLOR_TYPE_GRAY && bitDepth < 8) {
-        png_set_expand_gray_1_2_4_to_8(png_ptr);
-    }
+    // Now determine the default colorType and alphaType and set the required transforms.
+    // Often, we depend on SkSwizzler to perform any transforms that we need.  However, we
+    // still depend on libpng for many of the rare and PNG-specific cases.
+    SkColorType colorType = kUnknown_SkColorType;
+    SkAlphaType alphaType = kUnknown_SkAlphaType;
+    switch (encodedColorType) {
+        case PNG_COLOR_TYPE_PALETTE:
+            // Extract multiple pixels with bit depths of 1, 2, and 4 from a single
+            // byte into separate bytes (useful for paletted and grayscale images).
+            if (bitDepth < 8) {
+                png_set_packing(png_ptr);

[scroggo, 2016/01/28 21:07:32]

Could we instead use SkSwizzler::kIndex1 etc?

[msarett, 2016/01/28 22:19:25]

Yes.  Adding a TODO.

+            }
 
-    // Now determine the default SkColorType and SkAlphaType and set required transforms
-    SkColorType skColorType = kUnknown_SkColorType;
-    SkAlphaType skAlphaType = kUnknown_SkAlphaType;
-    switch (colorType) {
-        case PNG_COLOR_TYPE_PALETTE:
-            skColorType = kIndex_8_SkColorType;
-            skAlphaType = has_transparency_in_tRNS(png_ptr, info_ptr) ?
+            colorType = kIndex_8_SkColorType;
+            // Set the alpha type depending on if a transparency chunk exists.
+            alphaType = png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS) ?

[scroggo, 2016/01/28 21:07:32]

So we no longer check to see how many transparent colors there are. Is that
okay? Are there ever images which have a tRNS chunk but it doesn't have any
transparent colors in it? Or is that a bogus/rare case that we don't care about?
The end result would that such an image can no longer be decoded to 565, and
we'll draw it slightly slower in 8888.

If we do make this change, it would be good to state in the commit message that
it's changed (and why).

[msarett, 2016/01/28 22:19:25]

In libpng, a "valid" tRNS chunk has at least one color.  So there should be no
behavior change.
Yep sorry about the commit message.  I think it's better now.

                     kUnpremul_SkAlphaType : kOpaque_SkAlphaType;
             break;
         case PNG_COLOR_TYPE_RGB:
-            if (has_transparency_in_tRNS(png_ptr, info_ptr)) {
-                //convert to RGBA with tranparency information in tRNS chunk if it exists
+            if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) {
+                // Convert to RGBA if transparency chunk exists.
                 png_set_tRNS_to_alpha(png_ptr);
-                skAlphaType = kUnpremul_SkAlphaType;
+                alphaType = kUnpremul_SkAlphaType;
             } else {
-                skAlphaType = kOpaque_SkAlphaType;
+                alphaType = kOpaque_SkAlphaType;
             }
-            skColorType = kN32_SkColorType;
+            colorType = kN32_SkColorType;
             break;
         case PNG_COLOR_TYPE_GRAY:
-            if (has_transparency_in_tRNS(png_ptr, info_ptr)) {
-                //FIXME: support gray with alpha as a color type
-                //convert to RGBA if there is transparentcy info in the tRNS chunk
+            // Expand grayscale images to the full 8 bits from 1, 2, or 4 bits/pixel.
+            if (bitDepth < 8) {
+                png_set_expand_gray_1_2_4_to_8(png_ptr);

[scroggo, 2016/01/28 21:07:32]

Swizzler instead? (I'd be fine with a TODO for these, if you think it makes
sense, but don't want it to hold this CL up).

[msarett, 2016/01/28 22:19:24]

Yes I'll add TODOs.  I haven't gotten to these rarer cases yet and am unsure
that we will be faster.

+            }
+
+            if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) {
+                // Convert to RGBA if there is a transparency chunk.
                 png_set_tRNS_to_alpha(png_ptr);
                 png_set_gray_to_rgb(png_ptr);
-                skColorType = kN32_SkColorType;
-                skAlphaType = kUnpremul_SkAlphaType;
+                colorType = kN32_SkColorType;
+                alphaType = kUnpremul_SkAlphaType;
             } else {
-                skColorType = kGray_8_SkColorType;
-                skAlphaType = kOpaque_SkAlphaType;
+                colorType = kGray_8_SkColorType;
+                alphaType = kOpaque_SkAlphaType;
             }
             break;
         case PNG_COLOR_TYPE_GRAY_ALPHA:
-            //FIXME: support gray with alpha as a color type
-            //convert to RGBA
+            // Convert to RGBA if the image has alpha.
             png_set_gray_to_rgb(png_ptr);
-            skColorType = kN32_SkColorType;
-            skAlphaType = kUnpremul_SkAlphaType;
+            colorType = kN32_SkColorType;
+            alphaType = kUnpremul_SkAlphaType;
             break;
         case PNG_COLOR_TYPE_RGBA:
-            skColorType = kN32_SkColorType;
-            skAlphaType = kUnpremul_SkAlphaType;
+            colorType = kN32_SkColorType;
+            alphaType = kUnpremul_SkAlphaType;
             break;
         default:
-            //all the color types have been covered above
+            // All the color types have been covered above.
             SkASSERT(false);
     }
 
     int numberPasses = png_set_interlace_handling(png_ptr);
     if (numberPassesPtr) {
         *numberPassesPtr = numberPasses;
     }
 
     // FIXME: Also need to check for sRGB ( https://bug.skia.org/3471 ).
 
     if (imageInfo) {
-        *imageInfo = SkImageInfo::Make(origWidth, origHeight, skColorType, skAlphaType);
+        *imageInfo = SkImageInfo::Make(origWidth, origHeight, colorType, alphaType);
     }
     autoClean.detach();
     if (png_ptrp) {
         *png_ptrp = png_ptr;
     }
     if (info_ptrp) {
         *info_ptrp = info_ptr;
     }
 
     return true;
@@ skipping 11 matching lines @@
 {}
 
 SkPngCodec::~SkPngCodec() {
     this->destroyReadStruct();
 }
 
 void SkPngCodec::destroyReadStruct() {
     if (fPng_ptr) {
         // We will never have a nullptr fInfo_ptr with a non-nullptr fPng_ptr
         SkASSERT(fInfo_ptr);
-        png_destroy_read_struct(&fPng_ptr, &fInfo_ptr, png_infopp_NULL);
+        png_destroy_read_struct(&fPng_ptr, &fInfo_ptr, nullptr);
         fPng_ptr = nullptr;
         fInfo_ptr = nullptr;
     }
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Getting the pixels
 ///////////////////////////////////////////////////////////////////////////////
 
 SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
                                                const Options& options,
                                                SkPMColor ctable[],
                                                int* ctableCount) {
     // FIXME: Could we use the return value of setjmp to specify the type of
     // error?
     if (setjmp(png_jmpbuf(fPng_ptr))) {
         SkCodecPrintf("setjmp long jump!\n");
         return kInvalidInput;
     }
     png_read_update_info(fPng_ptr, fInfo_ptr);
 
-    //srcColorType was determined in read_header() which determined png color type
+    // srcColorType was determined in read_header() which determined png color type
     const SkColorType srcColorType = this->getInfo().colorType();
 
     switch (srcColorType) {
         case kIndex_8_SkColorType:
             //decode palette to Skia format
             fSrcConfig = SkSwizzler::kIndex;
             if (!this->decodePalette(kPremul_SkAlphaType == requestedInfo.alphaType(),
                     ctableCount)) {
                 return kInvalidInput;
             }
             break;
         case kGray_8_SkColorType:
             fSrcConfig = SkSwizzler::kGray;
             break;
         case kN32_SkColorType:
             if (this->getInfo().alphaType() == kOpaque_SkAlphaType) {
                     fSrcConfig = SkSwizzler::kRGB;
                 } else {
                     fSrcConfig = SkSwizzler::kRGBA;
             }
             break;
         default:
-            //would have exited before now if the colorType was supported by png
+            // We will always recommend one the above colorTypes.

[scroggo, 2016/01/28 21:07:32]

one of*

[msarett, 2016/01/28 22:19:25]

Done.

             SkASSERT(false);
     }
 
     // Copy the color table to the client if they request kIndex8 mode
     copy_color_table(requestedInfo, fColorTable, ctable, ctableCount);
 
     // Create the swizzler.  SkPngCodec retains ownership of the color table.
     const SkPMColor* colors = get_color_ptr(fColorTable.get());
     fSwizzler.reset(SkSwizzler::CreateSwizzler(fSrcConfig, colors, requestedInfo, options));
-    if (!fSwizzler) {
-        // FIXME: CreateSwizzler could fail for another reason.
-        return kUnimplemented;
-    }
+    SkASSERT(fSwizzler);
+
     return kSuccess;
 }
 
 
 bool SkPngCodec::onRewind() {
     // This sets fPng_ptr and fInfo_ptr to nullptr. If read_header
     // succeeds, they will be repopulated, and if it fails, they will
     // remain nullptr. Any future accesses to fPng_ptr and fInfo_ptr will
     // come through this function which will rewind and again attempt
     // to reinitialize them.
@@ skipping 62 matching lines @@
         const int bpp = SkSwizzler::BytesPerPixel(fSrcConfig);
         const size_t srcRowBytes = width * bpp;
 
         storage.reset(width * height * bpp);
         uint8_t* const base = storage.get();
 
         for (int i = 0; i < fNumberPasses; i++) {
             uint8_t* srcRow = base;
             for (int y = 0; y < height; y++) {
                 uint8_t* bmRow = srcRow;
-                png_read_rows(fPng_ptr, &bmRow, png_bytepp_NULL, 1);
+                png_read_rows(fPng_ptr, &bmRow, nullptr, 1);
                 srcRow += srcRowBytes;
             }
         }
 
         // Now swizzle it.
         uint8_t* srcRow = base;
         for (int y = 0; y < height; y++) {
             fSwizzler->swizzle(dstRow, srcRow);
             dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
             srcRow += srcRowBytes;
         }
     } else {
         storage.reset(requestedInfo.width() * SkSwizzler::BytesPerPixel(fSrcConfig));
         uint8_t* srcRow = storage.get();
         for (; row < requestedInfo.height(); row++) {
-            png_read_rows(fPng_ptr, &srcRow, png_bytepp_NULL, 1);
+            png_read_rows(fPng_ptr, &srcRow, nullptr, 1);
             // FIXME: Only call IsOpaque once, outside the loop. Same for onGetScanlines.
             fSwizzler->swizzle(dstRow, srcRow);
             dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
         }
     }
 
-    // FIXME: do we need substituteTranspColor? Note that we cannot do it for

[msarett, 2016/01/28 02:06:46]

Substituting in a "transparent color" is necessary when a transparent color is
specified in a tRNS chunk.

However, we always tell libpng to handle this by calling
png_set_tRNS_to_alpha(png_ptr) and getting the output as RGBA.

[scroggo, 2016/01/28 21:07:32]

Good to know. This would be a good thing to specify in the commit message.

[msarett, 2016/01/28 22:19:25]

Done.

-    // scanline decoding, but we could do it here. Alternatively, we could do
-    // it as we go, instead of in post-processing like SkPNGImageDecoder.
-
     if (setjmp(png_jmpbuf(fPng_ptr))) {
         // We've already read all the scanlines. This is a success.
         return kSuccess;
     }
 
     // read rest of file, and get additional comment and time chunks in info_ptr
     png_read_end(fPng_ptr, fInfo_ptr);
 
     return kSuccess;
 }
@@ skipping 36 matching lines @@
     int onGetScanlines(void* dst, int count, size_t rowBytes) override {
         // Assume that an error in libpng indicates an incomplete input.
         int row = 0;
         if (setjmp(png_jmpbuf(this->png_ptr()))) {
             SkCodecPrintf("setjmp long jump!\n");
             return row;
         }
 
         void* dstRow = dst;
         for (; row < count; row++) {
-            png_read_rows(this->png_ptr(), &fSrcRow, png_bytepp_NULL, 1);
+            png_read_rows(this->png_ptr(), &fSrcRow, nullptr, 1);
             this->swizzler()->swizzle(dstRow, fSrcRow);
             dstRow = SkTAddOffset<void>(dstRow, rowBytes);
         }
 
         return row;
     }
 
     bool onSkipScanlines(int count) override {
         // Assume that an error in libpng indicates an incomplete input.
         if (setjmp(png_jmpbuf(this->png_ptr()))) {
             SkCodecPrintf("setjmp long jump!\n");
             return false;
         }
         //there is a potential tradeoff of memory vs speed created by putting this in a loop.
         //calling png_read_rows in a loop is insignificantly slower than calling it once with count
         //as png_read_rows has it's own loop which calls png_read_row count times.
         for (int row = 0; row < count; row++) {
-            png_read_rows(this->png_ptr(), &fSrcRow, png_bytepp_NULL, 1);
+            png_read_rows(this->png_ptr(), &fSrcRow, nullptr, 1);
         }
         return true;
     }
 
 private:
     SkAutoTMalloc<uint8_t>      fStorage;
     uint8_t*                    fSrcRow;
 
     typedef SkPngCodec INHERITED;
 };
@@ skipping 63 matching lines @@
             // fail on the first pass, we can still report than some scanlines are initialized.
             return 0;
         }
         SkAutoTMalloc<uint8_t> storage(count * fSrcRowBytes);
         uint8_t* storagePtr = storage.get();
         uint8_t* srcRow;
         const int startRow = this->nextScanline();
         for (int i = 0; i < this->numberPasses(); i++) {
             // read rows we planned to skip into garbage row
             for (int y = 0; y < startRow; y++){
-                png_read_rows(this->png_ptr(), &fGarbageRowPtr, png_bytepp_NULL, 1);
+                png_read_rows(this->png_ptr(), &fGarbageRowPtr, nullptr, 1);
             }
             // read rows we care about into buffer
             srcRow = storagePtr;
             for (int y = 0; y < count; y++) {
-                png_read_rows(this->png_ptr(), &srcRow, png_bytepp_NULL, 1);
+                png_read_rows(this->png_ptr(), &srcRow, nullptr, 1);
                 srcRow += fSrcRowBytes;
             }
             // read rows we don't want into garbage buffer
             for (int y = 0; y < fHeight - startRow - count; y++) {
-                png_read_rows(this->png_ptr(), &fGarbageRowPtr, png_bytepp_NULL, 1);
+                png_read_rows(this->png_ptr(), &fGarbageRowPtr, nullptr, 1);
             }
         }
         //swizzle the rows we care about
         srcRow = storagePtr;
         void* dstRow = dst;
         for (int y = 0; y < count; y++) {
             this->swizzler()->swizzle(dstRow, srcRow);
             dstRow = SkTAddOffset<void>(dstRow, dstRowBytes);
             srcRow += fSrcRowBytes;
         }
@@ skipping 42 matching lines @@
     }
 
     if (1 == numberPasses) {
         return new SkPngScanlineDecoder(imageInfo, streamDeleter.detach(), chunkReader,
                                         png_ptr, info_ptr, bitDepth);
     }
 
     return new SkPngInterlacedScanlineDecoder(imageInfo, streamDeleter.detach(), chunkReader,
                                               png_ptr, info_ptr, bitDepth, numberPasses);
 }