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..b8a7fd0dc5247a9cc4d5c4e59881b8ae4e79da2a 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* bitDepthPtr) {
     // 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);
@@ -252,6 +252,8 @@ static bool read_header(SkStream* stream, png_structp* png_ptrp,
     png_get_IHDR(png_ptr, info_ptr, &origWidth, &origHeight, &bitDepth,
                  &colorType, int_p_NULL, int_p_NULL, int_p_NULL);
 
+    bitDepthPtr = &bitDepth;

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

This is going to do the wrong thing. I believe you want to assign the value of
bitDepth to the variable pointed to by bitDepthPtr. In order to do that you
would do the following:

if (bitDepthPtr) { // don't forget to check for NULL
  *bitDepthPtr = bitDepth;
}

What you've done here is made bitDepthPtr point to the memory address of
bitDepth. The variable it used to point to remains unchanged.

Let me know if this does not make sense and we can talk about it more in person.

+
     // sanity check for size
     {
         int64_t size = sk_64_mul(origWidth, origHeight);
@@ -277,8 +279,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 +288,51 @@ 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 (has_transparency_in_palette(png_ptr, info_ptr)) {
+                //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 = kN32_SkColorType;
             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 (has_transparency_in_palette(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
+                png_set_tRNS_to_alpha(png_ptr);
+                png_set_gray_to_rgb(png_ptr);
+                skColorType = kN32_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:
+            //FIXME: support gray with alpha as a color type 
+            //convert to RGBA 
             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);
-        }
+            skColorType = kN32_SkColorType;
+            skAlphaType = kUnpremul_SkAlphaType;
+            break;
+        case PNG_COLOR_TYPE_RGBA:
+            skColorType = kN32_SkColorType;
+            skAlphaType = kUnpremul_SkAlphaType;
+            break;
+        default:
+            //all the color types have been covered above
+            SkASSERT(false);
     }
 
     // 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 +349,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)) {
+        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 +411,11 @@ 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:
+        case kRGBA_8888_SkColorType:
+            return true;
+        case kBGRA_8888_SkColorType:
             return true;
         default:
             return dst.colorType() == src.colorType();
@@ -436,33 +434,40 @@ 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
+    const SkColorType srcColorType = this->getInfo().colorType();
+    const SkColorType dstColorType = requestedInfo.colorType();
+        //set swizzler fSrcConfig to dstColorType, as libbpng will convert from 

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

nit: Maybe specify that libpng will do the conversion thanks to the transforms
we have specified?

+        //srcColorType to dstColorType.  Swizzler still needed for premultiplying

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

The swizzler is only needed in one case: the client requested kPremul.

If the image is opaque (and known to be) or if the client requested unpremul, we
should skip the swizzler entirely.

+        switch (dstColorType) {

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

nit: This (and the comments above) should line up with the code above it (4
spaces, rather than 8)

+        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

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

Again, I think it would be better to make this a case, and then handle the rest
in default.

(It seems like we have not covered all the cases though. Maybe the default
should be SkASSERT(false) with a comment that we would have exited before now if
the colortype is one of the remaining cases.)

+            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);
@@ -475,17 +480,50 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
         // FIXME: CreateSwizzler could fail for another reason.
         return kUnimplemented;
     }
+    return kSuccess;
+}
 
-    // FIXME: Here is where we should likely insert some of the modifications
-    // made in the factory.
-    png_read_update_info(fPng_ptr, fInfo_ptr);
+SkCodec::Result SkPngCodec::setDataFormat(const SkImageInfo& requestedInfo) {
+    if (setjmp(png_jmpbuf(fPng_ptr))) {
+        SkCodecPrintf("setjmp long jump in SkPngCodec::setDataFormat()\n");
+        return kInvalidInput;
+    }
 
+    fNumberPasses = png_set_interlace_handling(fPng_ptr);
+
+     //srcColorType was determined in readHeader() which determined png color type
+    const SkColorType srcColorType = this->getInfo().colorType();
+    const SkColorType dstColorType = requestedInfo.colorType();
+    if (kRGBA_8888_SkColorType == dstColorType || kBGRA_8888_SkColorType == dstColorType) {
+        if (kIndex_8_SkColorType == srcColorType) {
+            //convert paletted images to RGBA
+            png_set_palette_to_rgb(fPng_ptr);
+            if (png_get_valid(fPng_ptr, fInfo_ptr, PNG_INFO_tRNS)) {

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

Should this call has_transparency_in_palette, too?

+                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() {
+bool SkPngCodec::handleRewind(const SkImageInfo& requestedInfo) {
     switch (this->rewindIfNeeded()) {
         case kNoRewindNecessary_RewindState:
+            //set transforms needed for requestedInfo format
+            if (kSuccess != this->setDataFormat(requestedInfo)) {
+                return false;
+            }
             return true;
         case kCouldNotRewind_RewindState:
             return false;
@@ -498,9 +536,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 != this->setDataFormat(requestedInfo)) {
+                    return false;
+                }
                 return true;
             }
             return false;
@@ -514,24 +556,22 @@ 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()) {
-        return kCouldNotRewind;
+    if (!conversion_possible(requestedInfo, this->getInfo())) {
+        return kInvalidConversion;
     }
     if (requestedInfo.dimensions() != this->getInfo().dimensions()) {
         return kInvalidScale;
     }
-    if (!conversion_possible(requestedInfo, this->getInfo())) {
-        return kInvalidConversion;
+    if (!this->handleRewind(requestedInfo)) {
+        return kCouldNotRewind;
     }
 
     // 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;
     }
-
     // FIXME: Could we use the return value of setjmp to specify the type of
     // error?
     if (setjmp(png_jmpbuf(fPng_ptr))) {
@@ -657,9 +697,9 @@ public:
         , fHasAlpha(false)
         , fCurrentRow(0)
         , fHeight(dstInfo.height())
-        , fSrcRowBytes(dstInfo.minRowBytes())
         , fRewindNeeded(false)
     {
+        fSrcRowBytes = dstInfo.width() * SkSwizzler::BytesPerPixel(fCodec->fSrcConfig);
         fGarbageRow.reset(fSrcRowBytes);
         fGarbageRowPtr = static_cast<uint8_t*>(fGarbageRow.get());
     }
@@ -667,8 +707,9 @@ public:
     SkImageGenerator::Result onGetScanlines(void* dst, int count, size_t dstRowBytes) override {
         //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(this->dstInfo())) {
                 return SkImageGenerator::kCouldNotRewind;
             }
         } else {
@@ -710,7 +751,7 @@ public:
         return SkImageGenerator::kSuccess;
     }
 
-    SkImageGenerator::Result onSkipScanlines(int count) override {
+    SkImageGenerator::Result onSkipScanlines(int count) override { 
         //when ongetScanlines is called it will skip to fCurrentRow
         fCurrentRow += count;
         return SkImageGenerator::kSuccess;
@@ -731,28 +772,21 @@ private:
     bool                fRewindNeeded;
     SkAutoMalloc        fGarbageRow;
     uint8_t*            fGarbageRowPtr;
-    
-    
-    
-    
-
     typedef SkScanlineDecoder INHERITED;
 };
 
 
 SkScanlineDecoder* SkPngCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo,
         const Options& options, SkPMColor ctable[], int* ctableCount) {
-    if (!this->handleRewind()) {
+    if (!conversion_possible(dstInfo, this->getInfo())) {
+        SkCodecPrintf("no conversion possible\n");
         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");
+    if (!this->handleRewind(dstInfo)) {
         return NULL;
     }