Swizzling, except for premultiplying, done using libpng transforms

src/codec/SkCodec_libpng.cpp

Index: src/codec/SkCodec_libpng.cpp
diff --git a/src/codec/SkCodec_libpng.cpp b/src/codec/SkCodec_libpng.cpp
index 444e2ad87e0ae14a3bfd95746fd991f592c7ceca..5c02f438544acaa4c21cf442e107dd51b7570532 100644
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -214,7 +214,7 @@ bool SkPngCodec::IsPng(SkStream* stream) {
 // 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);
@@ -248,7 +248,7 @@ static bool read_header(SkStream* stream, png_structp* png_ptrp,
     // 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);
 
@@ -277,8 +277,7 @@ static bool read_header(SkStream* stream, png_structp* png_ptrp,
         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) {
@@ -287,57 +286,47 @@ static bool read_header(SkStream* stream, png_structp* png_ptrp,
             skAlphaType = has_transparency_in_palette(png_ptr, info_ptr) ?
                     kUnpremul_SkAlphaType : kOpaque_SkAlphaType;
             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;
+        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 {
-                skColorType = kN32_SkColorType;
+                //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;
-        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)
-            {
+        case PNG_COLOR_TYPE_GRAY:
+            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 = kGray_8_SkColorType;
                 skAlphaType = kOpaque_SkAlphaType;
             }
-            skColorType = kN32_SkColorType;
             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) {
+        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)

-        }
-
-        // 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);
-        }
+            skColorType = kRGBA_8888_SkColorType;
+            skAlphaType = kUnpremul_SkAlphaType;
+            break;
+        default:
+            //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: 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) {
@@ -354,21 +343,24 @@ SkCodec* SkPngCodec::NewFromStream(SkStream* 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() {
@@ -413,11 +405,13 @@ static bool conversion_possible(const SkImageInfo& dst, const SkImageInfo& src)
                 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();
@@ -436,33 +430,38 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
         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);
@@ -478,14 +477,52 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
 
     // 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;
@@ -498,9 +535,13 @@ bool SkPngCodec::handleRewind() {
             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;
@@ -514,7 +555,7 @@ bool SkPngCodec::handleRewind() {
 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()) {
@@ -527,11 +568,10 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
     // 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))) {
@@ -597,8 +637,9 @@ public:
         : 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());
     }
 
@@ -644,6 +685,7 @@ private:
     bool                fHasAlpha;
     SkAutoMalloc        fStorage;
     uint8_t*            fSrcRow;
+    size_t              fSrcRowBytes;
 
     typedef SkScanlineDecoder INHERITED;
 };
@@ -659,16 +701,18 @@ public:
         , 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 {
@@ -710,10 +754,10 @@ public:
         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 {
@@ -731,18 +775,14 @@ private:
     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;
     }
 
@@ -756,7 +796,7 @@ SkScanlineDecoder* SkPngCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
         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,