Swizzling, except for premultiplying, done using libpng transforms

src/codec/SkCodec_libpng.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 "SkCodec_libpng.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
@@ skipping 196 matching lines @@
     }
     return true;
 }
 
 // Reads the header, and initializes the passed in fields, if not NULL (except
 // stream, which is passed to the read function).
 // Returns true on success, in which case the caller is responsible for calling
 // png_destroy_read_struct. If it returns false, the passed in fields (except
 // stream) are unchanged.
 static bool read_header(SkStream* stream, png_structp* png_ptrp,
-                        png_infop* info_ptrp, SkImageInfo* imageInfo) {
+                        png_infop* info_ptrp, SkImageInfo* imageInfo, int bitDepth) {

[scroggo, 2015/06/30 15:38:37]

I believe you want this to be an int* rather than an int. (You could make it an
int&, but by convention we use a pointer if we are going to modify an input
parameter.)

[emmaleer, 2015/06/30 20:13:41]

Done.

     // The image is known to be a PNG. Decode enough to know the SkImageInfo.
     png_structp png_ptr = png_create_read_struct(PNG_LIBPNG_VER_STRING, NULL,
                                                  sk_error_fn, sk_warning_fn);
     if (!png_ptr) {
         return false;
     }
 
     AutoCleanPng autoClean(png_ptr);
 
     png_infop info_ptr = png_create_info_struct(png_ptr);
@@ skipping 13 matching lines @@
 
     // FIXME: This is where the old code hooks up the Peeker. Does it need to
     // be set this early? (i.e. where are the user chunks? early in the stream,
     // potentially?)
     // If it does, we need to figure out a way to set it here.
 
     // 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 colorType;
     png_get_IHDR(png_ptr, info_ptr, &origWidth, &origHeight, &bitDepth,
                  &colorType, int_p_NULL, int_p_NULL, int_p_NULL);
 
     // sanity check for size
     {
         int64_t size = sk_64_mul(origWidth, origHeight);
         // 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
     if (bitDepth == 16) {
         png_set_strip_16(png_ptr);
     }
 #ifdef PNG_READ_PACK_SUPPORTED
     // 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 SkColorType and SkAlphaType.
+    // Now determine the default SkColorType and SkAlphaType and set required transforms
     SkColorType skColorType;
     SkAlphaType skAlphaType;
     switch (colorType) {
         case PNG_COLOR_TYPE_PALETTE:
             skColorType = kIndex_8_SkColorType;
             skAlphaType = has_transparency_in_palette(png_ptr, info_ptr) ?
                     kUnpremul_SkAlphaType : kOpaque_SkAlphaType;
             break;
+        case PNG_COLOR_TYPE_RGB:
+            if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) {

[scroggo, 2015/06/30 15:38:38]

In other places, we also check to see whether the number of transparent colors
in the tRNS chunk is greater than zero. Should we make that check here? (Or is
it unnecessary where we already do the check?)

[emmaleer, 2015/06/30 20:13:40]

Yes, I think it's important to make that check. I've added it in.

+                //convert to RGBA with tranparency information in tRNS chunk if it exists
+                png_set_tRNS_to_alpha(png_ptr);
+                skAlphaType = kUnpremul_SkAlphaType;
+            } else {
+                //convert to RGBA with Opaque Alpha 
+                png_set_filler(png_ptr, 0xff, PNG_FILLER_AFTER);
+                skAlphaType = kOpaque_SkAlphaType;
+            }
+            skColorType = kRGBA_8888_SkColorType;

[scroggo, 2015/06/30 15:38:38]

Interesting. I assume you chose RGBA because that is the order PNG has it
stored. I *think* we settled on reporting the info that best represents the data
(which is what you've done), but it leaves an open question in my mind - if a
client uses the info reported by the SkCodec, it may not be a good choice. For
example, we report that PNGs are unpremul, but we currently do not support
drawing unpremul sources. This means that in DM, we need to check for a reported
unpremul and switch to premul. (This also means various clients of
SkImageGenerator that use the info blindly do not work properly.)

Mike, do we support drawing the version of 8888 that is not N32? If not, we will
need to convert in this case as well.

[emmaleer, 2015/06/30 20:13:40]

Changed to N32

+            break;
         case PNG_COLOR_TYPE_GRAY:
-            if (false) {
-                // FIXME: Is this the wrong default behavior? This means if the
-                // caller supplies the info we gave them, they'll get Alpha 8.
-                skColorType = kAlpha_8_SkColorType;
-                // FIXME: Strangely, the canonical type for Alpha 8 is Premul.
-                skAlphaType = kPremul_SkAlphaType;
+            if (png_get_valid(png_ptr, info_ptr, PNG_INFO_tRNS)) {
+                //convert to RGBA if there is transparency information in the tRNS chunk

[scroggo, 2015/06/30 15:38:37]

Maybe this should be a FIXME; if we supported gray with alpha, we wouldn't need
to convert to RGBA.

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

+                png_set_tRNS_to_alpha(png_ptr);
+                png_set_gray_to_rgb(png_ptr);
+                skColorType = kRGBA_8888_SkColorType;
+                skAlphaType = kUnpremul_SkAlphaType;
             } else {
-                skColorType = kN32_SkColorType;
+                skColorType = kGray_8_SkColorType;
                 skAlphaType = kOpaque_SkAlphaType;
             }
             break;
+        case PNG_COLOR_TYPE_GRAY_ALPHA:
+            //convert to RGBA as GreyAlpha is not a Skia color type

[scroggo, 2015/06/30 15:38:37]

Maybe add a similar FIXME here.

(Also nit: I don't feel strongly about "Gray" vs "Grey", but libpng and Skia
seem to have chosen "Gray", so this should say "Gray", too.)

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

+            png_set_gray_to_rgb(png_ptr);

[scroggo, 2015/06/30 15:38:38]

Do we need to also call png_set_tRNS_to_alpha here?

[emmaleer, 2015/06/30 20:13:41]

No, as PNG_COLOR_TYPE_GRAY_ALPHA has an alpha channel
the tRNS chunk is used for images without an alpha channel, and gives an alpha
value for all the pixels in the image
So, the tRNS chunk is only converted to the alpha channel for images without an
alpha channel (gray, index, RGB)

+            skColorType = kRGBA_8888_SkColorType;
+            skAlphaType = kUnpremul_SkAlphaType;
+            break;
         default:
-            // Note: This *almost* mimics the code in SkImageDecoder_libpng.
-            // has_transparency_in_palette makes an additional check - whether
-            // numTrans is greater than 0. Why does the other code not make that
-            // check?
-            if (has_transparency_in_palette(png_ptr, info_ptr)
-                || PNG_COLOR_TYPE_RGB_ALPHA == colorType
-                || PNG_COLOR_TYPE_GRAY_ALPHA == colorType)
-            {
-                skAlphaType = kUnpremul_SkAlphaType;
-            } else {
-                skAlphaType = kOpaque_SkAlphaType;
-            }
-            skColorType = kN32_SkColorType;
+            //colorType = PNG_COLOR_TYPE_RGBA 

[scroggo, 2015/06/30 15:38:37]

Is this the only colorType left? (If so, I think it would be better to add
another case, and then assert false for the default.)

[emmaleer, 2015/06/30 20:13:41]

Yes, this is the last color type. Okay, I have done that.

+            skColorType = kRGBA_8888_SkColorType;
+            skAlphaType = kUnpremul_SkAlphaType;
             break;
     }
 
-    {
-        // FIXME: Again, this block needs to go into onGetPixels.
-        bool convertGrayToRGB = PNG_COLOR_TYPE_GRAY == colorType && skColorType != kAlpha_8_SkColorType;
-
-        // Unless the user is requesting A8, convert a grayscale image into RGB.
-        // GRAY_ALPHA will always be converted to RGB
-        if (convertGrayToRGB || colorType == PNG_COLOR_TYPE_GRAY_ALPHA) {
-            png_set_gray_to_rgb(png_ptr);
-        }
-
-        // Add filler (or alpha) byte (after each RGB triplet) if necessary.
-        // FIXME: It seems like we could just use RGB as the SrcConfig here.
-        if (colorType == PNG_COLOR_TYPE_RGB || convertGrayToRGB) {
-            png_set_filler(png_ptr, 0xff, PNG_FILLER_AFTER);
-        }
-    }
-
     // FIXME: Also need to check for sRGB (skbug.com/3471).
 
     if (imageInfo) {
-        *imageInfo = SkImageInfo::Make(origWidth, origHeight, skColorType,
-                                       skAlphaType);
+        *imageInfo = SkImageInfo::Make(origWidth, origHeight, skColorType, skAlphaType);
     }
     autoClean.detach();
     if (png_ptrp) {
         *png_ptrp = png_ptr;
     }
     if (info_ptrp) {
         *info_ptrp = info_ptr;
     }
     return true;
 }
 
 SkCodec* SkPngCodec::NewFromStream(SkStream* stream) {
     SkAutoTDelete<SkStream> streamDeleter(stream);
     png_structp png_ptr;
     png_infop info_ptr;
     SkImageInfo imageInfo;
-    if (read_header(stream, &png_ptr, &info_ptr, &imageInfo)) {
-        return SkNEW_ARGS(SkPngCodec, (imageInfo, streamDeleter.detach(), png_ptr, info_ptr));
+    int bitDepth;
+    if (read_header(stream, &png_ptr, &info_ptr, &imageInfo, bitDepth)) {

[scroggo, 2015/06/30 15:38:38]

Since the function takes an int, this does not modify the local variable. You
will be passing an uninitialized value to the SkPngCodec constructor.

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

+        return SkNEW_ARGS(SkPngCodec, (imageInfo, streamDeleter.detach(), 
+                png_ptr, info_ptr, bitDepth));
     }
     return NULL;
 }
 
 #define INVALID_NUMBER_PASSES -1
 SkPngCodec::SkPngCodec(const SkImageInfo& info, SkStream* stream,
-                       png_structp png_ptr, png_infop info_ptr)
+                       png_structp png_ptr, png_infop info_ptr, int bitDepth)
     : INHERITED(info, stream)
     , fPng_ptr(png_ptr)
     , fInfo_ptr(info_ptr)
     , fSrcConfig(SkSwizzler::kUnknown)
     , fNumberPasses(INVALID_NUMBER_PASSES)
     , fReallyHasAlpha(false)
+    , fBitDepth(bitDepth)
 {}
 
 SkPngCodec::~SkPngCodec() {
     this->destroyReadStruct();
 }
 
 void SkPngCodec::destroyReadStruct() {
     if (fPng_ptr) {
         // We will never have a NULL fInfo_ptr with a non-NULL fPng_ptr
         SkASSERT(fInfo_ptr);
@@ skipping 24 matching lines @@
         switch (dst.alphaType()) {
             case kPremul_SkAlphaType:
             case kUnpremul_SkAlphaType:
                 // The source is not opaque, so either of these is okay
                 break;
             default:
                 // We cannot decode a non-opaque image to opaque (or unknown)
                 return false;
         }
     }
-
     // Check for supported color types
     switch (dst.colorType()) {
-        // Allow output to kN32 from any type of input
-        case kN32_SkColorType:
+        // Allow output to RGBA from any type of input

[reed1, 2015/06/30 13:51:01]

not sure either of these comments are needed (also confusing since the component
order is reversed from the enum)

[emmaleer, 2015/06/30 20:13:40]

I removed the unneeded comments
In the enum in SkImageInfo.h the order is:

kUnknown_SkColorType,
kAlpha_8_SkColorType,
kRGB_565_SkColorType,
kARGB_4444_SkColorType,
kRGBA_8888_SkColorType,
kBGRA_8888_SkColorType,
kIndex_8_SkColorType,
kGray_8_SkColorType,

So, I think RGBA and then BGRA is okay.
Let me know if this is still the wrong ordering. 

[scroggo, 2015/06/30 20:50:03]

I think Mike's comment about ordering is actually for the next comment - you
mention "GBRA", when you mean "BGRA".

+        case kRGBA_8888_SkColorType:
+            return true;
+        // Allow output to GBRA from any type of input
+        case kBGRA_8888_SkColorType:
             return true;
         default:
             return dst.colorType() == src.colorType();
     }
 }
 
 SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
                                                void* dst, size_t rowBytes,
                                                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;
     }
 
-    // FIXME: We already retrieved this information. Store it in SkPngCodec?
-    png_uint_32 origWidth, origHeight;
-    int bitDepth, pngColorType, interlaceType;
-    png_get_IHDR(fPng_ptr, fInfo_ptr, &origWidth, &origHeight, &bitDepth,
-                 &pngColorType, &interlaceType, int_p_NULL, int_p_NULL);
-
-    fNumberPasses = (interlaceType != PNG_INTERLACE_NONE) ?
-            png_set_interlace_handling(fPng_ptr) : 1;
-
     // Set to the default before calling decodePalette, which may change it.
     fReallyHasAlpha = false;
-    if (PNG_COLOR_TYPE_PALETTE == pngColorType) {
-        fSrcConfig = SkSwizzler::kIndex;
-        if (!this->decodePalette(kPremul_SkAlphaType == requestedInfo.alphaType(), bitDepth,
-                ctableCount)) {
-            return kInvalidInput;
-        }
-    } else if (kAlpha_8_SkColorType == requestedInfo.colorType()) {
-        // Note: we check the destination, since otherwise we would have
-        // told png to upscale.
-        SkASSERT(PNG_COLOR_TYPE_GRAY == pngColorType);
-        fSrcConfig = SkSwizzler::kGray;
-    } else if (this->getInfo().alphaType() == kOpaque_SkAlphaType) {
-        fSrcConfig = SkSwizzler::kRGBX;
-    } else {
-        fSrcConfig = SkSwizzler::kRGBA;
-    }
+
+    //srcColorType was determined in readHeader() which determined png color type
+    int srcColorType = this->getInfo().colorType();

[scroggo, 2015/06/30 15:38:37]

Why do you store these as ints instead of SkColorType?

(Also, I think they can be const)

[emmaleer, 2015/06/30 20:13:40]

Changed to const SkColorType

+    int dstColorType = requestedInfo.colorType();
+        switch (dstColorType) {

[scroggo, 2015/06/30 15:38:37]

nit: this should line up with the line before it.

Also, I think you want to switch on the srcColorType?

[emmaleer, 2015/06/30 20:13:40]

I switch on dstColorType, since libpng will convert the srcColorType to the
dstColorType (except for premultiplying)
Then the swizzler gets the data in the dstColorType format

This may be confusing for the reader. I've added a comment about this.

+        case kIndex_8_SkColorType:
+            //decode palette to Skia format
+            fSrcConfig = SkSwizzler::kIndex;
+            if (!this->decodePalette(kPremul_SkAlphaType == requestedInfo.alphaType(), fBitDepth,
+                    ctableCount)) {
+                return kInvalidInput;
+            }
+            break;
+        case kGray_8_SkColorType:
+            fSrcConfig = SkSwizzler::kGray;
+            break; 
+        case kRGBA_8888_SkColorType:
+            if (this->getInfo().alphaType() == kOpaque_SkAlphaType) {
+                    fSrcConfig = SkSwizzler::kRGBX;
+                } else {
+                    fSrcConfig = SkSwizzler::kRGBA;
+            }
+        default:
+        //dstColorType == kBGRA_8888_SkColorType
+            if (this->getInfo().alphaType() == kOpaque_SkAlphaType) {
+                fSrcConfig = SkSwizzler::kBGRX;
+            } else {
+                fSrcConfig = SkSwizzler::kBGRA;
+            }
+    }  
 
     // 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 = fColorTable ? fColorTable->readColors() : NULL;
     fSwizzler.reset(SkSwizzler::CreateSwizzler(fSrcConfig, colors, requestedInfo,
             dst, rowBytes, options.fZeroInitialized));
     if (!fSwizzler) {
         // FIXME: CreateSwizzler could fail for another reason.
         return kUnimplemented;
     }
 
     // FIXME: Here is where we should likely insert some of the modifications

[scroggo, 2015/06/30 15:38:37]

I think this FIXME has been resolved?

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

     // made in the factory.
-    png_read_update_info(fPng_ptr, fInfo_ptr);
 
     return kSuccess;
 }
 
-bool SkPngCodec::handleRewind() {
+SkCodec::Result SkPngCodec::setDataFormat(const SkImageInfo& requestedInfo) {
+    if (setjmp(png_jmpbuf(fPng_ptr))) {
+        SkCodecPrintf("setjmp long jump!\n");

[scroggo, 2015/06/30 15:38:37]

I know this is the same message that is printed elsewhere (blame me...), but
this should perhaps have a more informative error message (e.g. the function
where it was called).

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

+        return kInvalidInput;
+    }
+
+    fNumberPasses = png_set_interlace_handling(fPng_ptr);
+
+     //srcColorType was determined in readHeader() which determined png color type
+    int srcColorType = this->getInfo().colorType();

[scroggo, 2015/06/30 15:38:37]

Again, I think these should be SkColorType instead of int (and can be const).

[emmaleer, 2015/06/30 20:13:40]

Acknowledged.

+    int dstColorType = requestedInfo.colorType();
+    if (kRGBA_8888_SkColorType == dstColorType || kBGRA_8888_SkColorType == dstColorType) {
+            //dstColorType either equals kRGBA_8888_SkColorType or kBGRA_8888_SkColorType

[scroggo, 2015/06/30 15:38:37]

This seems to just add nothing new to the line above?

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

+            if (kIndex_8_SkColorType == srcColorType) {

[scroggo, 2015/06/30 15:38:37]

nit: This should only be indented 4 spaces from the if statement.

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

+                //convert paletted images to RGBA
+                png_set_palette_to_rgb(fPng_ptr);
+                if (png_get_valid(fPng_ptr, fInfo_ptr, PNG_INFO_tRNS)) {
+                    png_set_tRNS_to_alpha(fPng_ptr);
+                } else {
+                    png_set_filler(fPng_ptr, 0xff, PNG_FILLER_AFTER);
+                }
+            } else if (kGray_8_SkColorType == srcColorType) {
+                //convert from Gray to RGBA (need to add filler for alpha as gray has no alpha)
+                png_set_gray_to_rgb(fPng_ptr);
+                png_set_filler(fPng_ptr, 0xff, PNG_FILLER_AFTER);
+            }
+            if (kBGRA_8888_SkColorType == dstColorType) {
+                //convert RGBA to BGRA 
+                png_set_bgr(fPng_ptr);
+            }
+    }
+    png_read_update_info(fPng_ptr, fInfo_ptr);  
+    return kSuccess;
+}
+
+bool SkPngCodec::handleRewind(const SkImageInfo& requestedInfo) {
     switch (this->rewindIfNeeded()) {
         case kNoRewindNecessary_RewindState:
+            //set transforms needed for requestedInfo format
+            if (kSuccess != setDataFormat(requestedInfo)) {

[scroggo, 2015/06/30 15:38:38]

nit: this->setDataFormat

[emmaleer, 2015/06/30 20:13:40]

Acknowledged.

+                return false;

[scroggo, 2015/06/30 15:38:37]

Interestingly, we've changed the nature of the error. setDataFormat will fail
with kInvalidInput if we hit a longjmp, but we'll ultimately return
kCouldNotRewind. That's probably okay though.

(If we do think it's important, we could make handleRewind return a Result, and
if it's kSuccess the caller can continue, otherwise return the Result returned
by handleRewind.)

[emmaleer, 2015/06/30 20:13:40]

I think it's important to return the correct result, otherwise it is misleading
to the user
I think I will change handleRewind to return a Result, but I'll do it in a
separate CL, since this one is already quite large

+            }
             return true;
         case kCouldNotRewind_RewindState:
             return false;
         case kRewound_RewindState: {
             // This sets fPng_ptr and fInfo_ptr to NULL. If read_header
             // succeeds, they will be repopulated, and if it fails, they will
             // remain NULL. Any future accesses to fPng_ptr and fInfo_ptr will
             // come through this function which will rewind and again attempt
             // to reinitialize them.
             this->destroyReadStruct();
             png_structp png_ptr;
             png_infop info_ptr;
-            if (read_header(this->stream(), &png_ptr, &info_ptr, NULL)) {
+            if (read_header(this->stream(), &png_ptr, &info_ptr, NULL, NULL)) {
                 fPng_ptr = png_ptr;
                 fInfo_ptr = info_ptr;
+                //set transforms needed for requestedInfo format
+                if (kSuccess != setDataFormat(requestedInfo)) {

[scroggo, 2015/06/30 15:38:37]

this->setDataFormat

[emmaleer, 2015/06/30 20:13:40]

Acknowledged.

+                    return false;
+                }
                 return true;
             }
             return false;
         }
         default:
             SkASSERT(false);
             return false;
     }
 }
 
 SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void* dst,
                                         size_t rowBytes, const Options& options,
                                         SkPMColor ctable[], int* ctableCount) {
-    if (!this->handleRewind()) {
+    if (!this->handleRewind(requestedInfo)) {

[scroggo, 2015/06/30 15:38:38]

I notice that setDataFormat never cares if it sees a color type that we do not
support (e.g. 4444, which will never support). We'll fail down below in
conversion_possible though. Not sure whether the order matters.

[emmaleer, 2015/06/30 20:13:41]

I changed it so that conversion_possible is called first, then the scaling is
checked, and then handleRewind is called
The order doesn't matter for the implementation
I think it's better calling handleRewind last, as it avoids rewinding the entire
stream on the cases we will fail one step later anyways

         return kCouldNotRewind;
     }
     if (requestedInfo.dimensions() != this->getInfo().dimensions()) {
         return kInvalidScale;
     }
     if (!conversion_possible(requestedInfo, this->getInfo())) {
         return kInvalidConversion;
     }
 
     // Note that ctable and ctableCount may be modified if there is a color table
     const Result result = this->initializeSwizzler(requestedInfo, dst, rowBytes,
                                                    options, ctable, ctableCount);
-
     if (result != kSuccess) {
         return result;
     }
-
+    

[scroggo, 2015/06/30 15:38:38]

It looks like you added some whitespace?

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

     // 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;
     }
 
     SkASSERT(fNumberPasses != INVALID_NUMBER_PASSES);
     SkAutoMalloc storage;
     if (fNumberPasses > 1) {
@@ skipping 45 matching lines @@
     /* read rest of file, and get additional chunks in info_ptr - REQUIRED */
     png_read_end(fPng_ptr, fInfo_ptr);
 }
 
 class SkPngScanlineDecoder : public SkScanlineDecoder {
 public:
     SkPngScanlineDecoder(const SkImageInfo& dstInfo, SkPngCodec* codec)
         : INHERITED(dstInfo)
         , fCodec(codec)
         , fHasAlpha(false)
+        , fSrcRowBytes(dstInfo.minRowBytes())

[scroggo, 2015/06/30 15:38:37]

Why do we need to store this? It seems like we only use it in the constructor?

[emmaleer, 2015/06/30 20:13:40]

I removed this

     {
-        fStorage.reset(dstInfo.width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig));
+        fStorage.reset(dstInfo.width() * fSrcRowBytes);

[scroggo, 2015/06/30 15:38:37]

I don't think this is what you want. minRowBytes() is width * bytesPerPixel. So
now this is width * width * bytesPerPixel.

[emmaleer, 2015/06/30 20:13:41]

Yes, this is wrong.  I changed this back to using width *
SkSwizzler::BytesPerPixel, since we are still using the swizzler

         fSrcRow = static_cast<uint8_t*>(fStorage.get());
     }
 
     SkImageGenerator::Result onGetScanlines(void* dst, int count, size_t rowBytes) override {
         if (setjmp(png_jmpbuf(fCodec->fPng_ptr))) {
             SkCodecPrintf("setjmp long jump!\n");
             return SkImageGenerator::kInvalidInput;
         }
 
         for (int i = 0; i < count; i++) {
@@ skipping 25 matching lines @@
         fCodec->finish();
     }
 
     bool onReallyHasAlpha() const override { return fHasAlpha; }
 
 private:
     SkPngCodec*         fCodec;     // Unowned.
     bool                fHasAlpha;
     SkAutoMalloc        fStorage;
     uint8_t*            fSrcRow;
+    size_t              fSrcRowBytes;
 
     typedef SkScanlineDecoder INHERITED;
 };
 
 
 class SkPngInterlacedScanlineDecoder : public SkScanlineDecoder {
 public:
     SkPngInterlacedScanlineDecoder(const SkImageInfo& dstInfo, SkPngCodec* codec)
         : INHERITED(dstInfo)
         , fCodec(codec)
         , fHasAlpha(false)
         , fCurrentRow(0)
         , fHeight(dstInfo.height())
         , fSrcRowBytes(dstInfo.minRowBytes())
         , fRewindNeeded(false)
+        , fDstInfo(dstInfo)
     {
         fGarbageRow.reset(fSrcRowBytes);
         fGarbageRowPtr = static_cast<uint8_t*>(fGarbageRow.get());
     }
 
-    SkImageGenerator::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override {
+ SkImageGenerator::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override {

[scroggo, 2015/06/30 15:38:38]

This should be indented 4 spaces.

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

         //rewind stream if have previously called onGetScanlines, 
         //since we need entire progressive image to get scanlines
+        //need to reset transforms in setDataFormat if stream is rewound
         if (fRewindNeeded) {
-            if(false == fCodec->handleRewind()) {
+            if(false == fCodec->handleRewind(fDstInfo)) {

[scroggo, 2015/06/30 15:38:38]

Alternatively, you could use this->dstInfo(), which might be better.

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

                 return SkImageGenerator::kCouldNotRewind;
             }
         } else {
             fRewindNeeded = true;
         }
         if (setjmp(png_jmpbuf(fCodec->fPng_ptr))) {
             SkCodecPrintf("setjmp long jump!\n");
             return SkImageGenerator::kInvalidInput;
         }
         const int number_passes = png_set_interlace_handling(fCodec->fPng_ptr);
@@ skipping 21 matching lines @@
         for (int y = 0; y < count; y++) {
             fCodec->fSwizzler->setDstRow(dst);
             fHasAlpha |= !SkSwizzler::IsOpaque(fCodec->fSwizzler->next(srcRow));
             dst = SkTAddOffset<void>(dst, dstRowBytes);
             srcRow += fSrcRowBytes;
         }
         fCurrentRow += count;
         return SkImageGenerator::kSuccess;
     }
 
-    SkImageGenerator::Result onSkipScanlines(int count) override {
-        //when ongetScanlines is called it will skip to fCurrentRow
-        fCurrentRow += count;
-        return SkImageGenerator::kSuccess;
+    SkImageGenerator::Result onSkipScanlines(int count) override {   

[scroggo, 2015/06/30 15:38:37]

More added whitespace?

+        //when ongetScanlines is called it will skip to fCurrentRow    
+        fCurrentRow += count;  
+        return SkImageGenerator::kSuccess; 
     }
 
     void onFinish() override {
         fCodec->finish();
     }
 
     bool onReallyHasAlpha() const override { return fHasAlpha; }
 
 private:
     SkPngCodec*         fCodec;     // Unowned.
     bool                fHasAlpha;
     int                 fCurrentRow;
     int                 fHeight;
     size_t              fSrcRowBytes;
     bool                fRewindNeeded;
     SkAutoMalloc        fGarbageRow;
     uint8_t*            fGarbageRowPtr;
-    
-    
-    
-    
-
+    const SkImageInfo&  fDstInfo;
     typedef SkScanlineDecoder INHERITED;
 };
 
 
 SkScanlineDecoder* SkPngCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
         const Options& options, SkPMColor ctable[], int* ctableCount) {
-    if (!this->handleRewind()) {
+    if (!this->handleRewind(dstInfo)) {
         return NULL;
     }
 
     // Check to see if scaling was requested.
     if (dstInfo.dimensions() != this->getInfo().dimensions()) {
         return NULL;
     }
 
     if (!conversion_possible(dstInfo, this->getInfo())) {
         SkCodecPrintf("no conversion possible\n");
         return NULL;
     }
 
-    // Note: We set dst to NULL since we do not know it yet. rowBytes is not needed,
+   // Note: We set dst to NULL since we do not know it yet. rowBytes is not needed,

[scroggo, 2015/06/30 15:38:37]

nit: 4 space indent.

[emmaleer, 2015/06/30 20:13:41]

Acknowledged.

     // since we'll be manually updating the dstRow, but the SkSwizzler requires it to
     // be at least dstInfo.minRowBytes.
     if (this->initializeSwizzler(dstInfo, NULL, dstInfo.minRowBytes(), options, ctable,
             ctableCount) != kSuccess) {
         SkCodecPrintf("failed to initialize the swizzler.\n");
         return NULL;
     }
 
     SkASSERT(fNumberPasses != INVALID_NUMBER_PASSES);
     if (fNumberPasses > 1) {
         // interlaced image
         return SkNEW_ARGS(SkPngInterlacedScanlineDecoder, (dstInfo, this));
     }
 
     return SkNEW_ARGS(SkPngScanlineDecoder, (dstInfo, this));
 }