Swizzler changes Index8 and 565

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 110 matching lines @@
 // the case here.
 bool SkPngCodec::decodePalette(bool premultiply) {
     int numPalette;
     png_colorp palette;
     png_bytep trans;
 
     if (!png_get_PLTE(fPng_ptr, fInfo_ptr, &palette, &numPalette)) {
         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)) {
         png_get_tRNS(fPng_ptr, fInfo_ptr, &trans, &numTrans, NULL);
     } else {
         numTrans = 0;
     }
 
     // check for bad images that might make us crash
     if (numTrans > numPalette) {
@@ skipping 17 matching lines @@
         palette++;
     }
 
     fReallyHasAlpha = transLessThanFF < 0;
 
     for (; index < numPalette; index++) {
         *colorPtr++ = SkPackARGB32(0xFF, palette->red, palette->green, palette->blue);
         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;
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 // Creation
 ///////////////////////////////////////////////////////////////////////////////
 
 #define PNG_BYTES_TO_CHECK 4
 
 bool SkPngCodec::IsPng(SkStream* stream) {
@@ skipping 75 matching lines @@
     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.
     SkColorType skColorType;
     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;
         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;
@@ skipping 135 matching lines @@
     } 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;
     }
-    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.
         return kUnimplemented;
     }
 
     // FIXME: Here is where we should likely insert some of the modifications
     // made in the factory.
     png_read_update_info(fPng_ptr, fInfo_ptr);
 
@@ skipping 34 matching lines @@
     if (!this->handleRewind()) {
         return kCouldNotRewind;
     }
     if (requestedInfo.dimensions() != this->getInfo().dimensions()) {
         return kInvalidScale;
     }
     if (!conversion_possible(requestedInfo, this->getInfo())) {
         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) {
         return result;
     }
 
     // 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");
@@ skipping 133 matching lines @@
 
     SkASSERT(fNumberPasses != INVALID_NUMBER_PASSES);
     if (fNumberPasses > 1) {
         // We cannot efficiently do scanline decoding.
         return NULL;
     }
 
     return SkNEW_ARGS(SkPngScanlineDecoder, (dstInfo, this));
 }