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..819ade55df079b0d1f05de936640330f6200b3ec 100644
--- a/src/codec/SkCodec_libpng.cpp
+++ b/src/codec/SkCodec_libpng.cpp
@@ -15,6 +15,7 @@
 #include "SkSize.h"
 #include "SkStream.h"
 #include "SkSwizzler.h"
+#include "SkUtils.h"
 
 ///////////////////////////////////////////////////////////////////////////////
 // Helper macros
@@ -119,7 +120,7 @@ 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) {
+bool SkPngCodec::decodePalette(bool premultiply, int bitDepth, int* ctableCount) {
     int numPalette;
     png_colorp palette;
     png_bytep trans;
@@ -128,14 +129,7 @@ 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);
-    // Note: These are not necessarily SkPMColors.
+    // Note: These are not necessarily SkPMColors
     SkPMColor colorStorage[256];    // worst-case storage
     SkPMColor* colorPtr = colorStorage;
 
@@ -175,9 +169,24 @@ bool SkPngCodec::decodePalette(bool premultiply) {
         palette++;
     }
 
-    // see BUGGY IMAGE WORKAROUND comment above
-    if (numPalette < 256) {
-        *colorPtr = colorPtr[-1];
+    /*  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(bitDepth, 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 != NULL) {
+        SkASSERT(256 == *ctableCount);
+        *ctableCount = colorCount;
     }
 
     fColorTable.reset(SkNEW_ARGS(SkColorTable, (colorStorage, colorCount)));
@@ -276,10 +285,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;
@@ -401,7 +407,8 @@ static bool conversion_possible(const SkImageInfo& dst, const SkImageInfo& src)
 
 SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
                                                void* dst, size_t rowBytes,
-                                               const Options& options) {
+                                               const Options& options,
+                                               int* ctableCount) {
     // FIXME: Could we use the return value of setjmp to specify the type of
     // error?
     if (setjmp(png_jmpbuf(fPng_ptr))) {
@@ -422,7 +429,8 @@ SkCodec::Result SkPngCodec::initializeSwizzler(const SkImageInfo& requestedInfo,
     fReallyHasAlpha = false;
     if (PNG_COLOR_TYPE_PALETTE == pngColorType) {
         fSrcConfig = SkSwizzler::kIndex;
-        if (!this->decodePalette(kPremul_SkAlphaType == requestedInfo.alphaType())) {
+        if (!this->decodePalette(kPremul_SkAlphaType == requestedInfo.alphaType(), bitDepth,
+                ctableCount)) {
             return kInvalidInput;
         }
     } else if (kAlpha_8_SkColorType == requestedInfo.colorType()) {
@@ -491,8 +499,18 @@ SkCodec::Result SkPngCodec::onGetPixels(const SkImageInfo& requestedInfo, void*
         return kInvalidConversion;
     }
 
+    // Note that ctableCount will be modified if there is a color table
     const Result result = this->initializeSwizzler(requestedInfo, dst, rowBytes,
-                                                   options);
+                                                   options, ctableCount);
+
+    // Copy the color table to the client if necessary
+    if (kIndex_8_SkColorType == requestedInfo.colorType()) {
+        SkASSERT(NULL != ctable);
+        SkASSERT(NULL != ctableCount);
+        SkASSERT(NULL != fColorTable.get());
+        sk_memcpy32(ctable, fColorTable->readColors(), *ctableCount);

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

Is this better than using regular memcpy?

[scroggo, 2015/04/06 14:55:11]

We have MemoryBench which compares the two, and it looks like we use SSE2 if we
have it. I'm guessing that if MemoryBench did not show it to be faster, we'd
just make it call memcpy.

[msarett, 2015/04/06 18:10:55]

Acknowledged.

+    }
+
     if (result != kSuccess) {
         return result;
     }
@@ -630,7 +648,7 @@ SkScanlineDecoder* SkPngCodec::onGetScanlineDecoder(const SkImageInfo& dstInfo)
     Options opts;
     // FIXME: Pass this in to getScanlineDecoder?
     opts.fZeroInitialized = kNo_ZeroInitialized;
-    if (this->initializeSwizzler(dstInfo, NULL, dstInfo.minRowBytes(), opts) != kSuccess) {
+    if (this->initializeSwizzler(dstInfo, NULL, dstInfo.minRowBytes(), opts, NULL) != kSuccess) {

[scroggo, 2015/04/06 14:55:11]

Maybe add a FIXME acknowledging that getScanlineDecoder currently does not have
a way to get the color table information for a kIndex8 image. (It seems like the
caller cannot reasonably use a kIndex8 image from scanline decoding, since they
won't know what the colors are.)

[msarett, 2015/04/06 18:10:55]

Done.

         SkCodecPrintf("failed to initialize the swizzler.\n");
         return NULL;
     }