Swizzler changes Index8 and 565

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 33111cee67841938335fd6f7b0862d5f9ec0a9c6..7c5dd914517bdcdc09efbb3fb8289706fd03dc6c 100644
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -128,16 +128,8 @@ bool SkPngCodec::decodePalette(bool premultiply) {
         return false;
     }
 
-    /*  BUGGY IMAGE WORKAROUND
-
-        We hit some images (e.g. fruit_.png) who contain bytes that are == colortable_count
-        which is a problem since we use the byte as an index. To work around this we grow
-        the colortable by 1 (if its < 256) and duplicate the last color into that slot.
-    */
-    const int colorCount = numPalette + (numPalette < 256);

[scroggo, 2015/04/02 19:20:31]

Part of the contract of onGetPixels is that we will set the passed in color
pointer to the actual number of colors. We need to support that.

[msarett, 2015/04/03 18:01:32]

Didn't know this.  I think I now handle this properly in bmp, gif, and png.

     // Note: These are not necessarily SkPMColors.
-    SkPMColor colorStorage[256];    // worst-case storage
-    SkPMColor* colorPtr = colorStorage;
+    SkPMColor* colorPtr = fColorTable;
 
     int numTrans;
     if (png_get_valid(fPng_ptr, fInfo_ptr, PNG_INFO_tRNS)) {
@@ -175,12 +167,16 @@ bool SkPngCodec::decodePalette(bool premultiply) {
         palette++;
     }
 
-    // see BUGGY IMAGE WORKAROUND comment above
+    /*  BUGGY IMAGE WORKAROUND

[scroggo, 2015/04/02 19:20:31]

As long as you're touching this code, you might as well add the fix from
https://codereview.chromium.org/948163002, to SkImageDecoder.

[msarett, 2015/04/03 18:01:32]

Done.

+
+        We hit some images (e.g. fruit_.png) who contain bytes that are == colortable_count
+        which is a problem since we use the byte as an index. To work around this we grow
+        the colortable by 1 (if its < 256) and duplicate the last color into that slot.
+    */
     if (numPalette < 256) {
         *colorPtr = colorPtr[-1];
     }
 
-    fColorTable.reset(SkNEW_ARGS(SkColorTable, (colorStorage, colorCount)));
     return true;
 }
 
@@ -276,10 +272,7 @@ static bool read_header(SkStream* stream, png_structp* png_ptrp,
     SkAlphaType skAlphaType;
     switch (colorType) {
         case PNG_COLOR_TYPE_PALETTE:
-            // Technically, this is true of the data, but I don't think we want
-            // to support it.
-            // skColorType = kIndex8_SkColorType;
-            skColorType = kN32_SkColorType;
+            skColorType = kIndex_8_SkColorType;
             skAlphaType = has_transparency_in_palette(png_ptr, info_ptr) ?
                     kUnpremul_SkAlphaType : kOpaque_SkAlphaType;
             break;
@@ -435,8 +428,7 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
     } else {
         fSrcConfig = SkSwizzler::kRGBA;
     }
-    const SkPMColor* colors = fColorTable ? fColorTable->readColors() : NULL;
-    fSwizzler.reset(SkSwizzler::CreateSwizzler(fSrcConfig, colors, requestedInfo,
+    fSwizzler.reset(SkSwizzler::CreateSwizzler(fSrcConfig, fColorTable, requestedInfo,
             dst, rowBytes, options.fZeroInitialized));
     if (!fSwizzler) {
         // FIXME: CreateSwizzler could fail for another reason.
@@ -491,6 +483,11 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
         return kInvalidConversion;
     }
 
+    // Get a valid color table pointer
+    SkPMColor ctableAlternate[256];
+    fColorTable = get_color_table_ptr(requestedInfo.colorType(), ctable, ctableCount,

[scroggo, 2015/04/02 19:20:31]

Once again, we're potentially setting the member pointer to something that will
disappear when this function ends. This is dangerous.

[msarett, 2015/04/03 18:01:32]

This has been fixed.

+            ctableAlternate);
+
     const Result result = this->initializeSwizzler(requestedInfo, dst, rowBytes,
                                                    options);
     if (result != kSuccess) {