Swizzler changes Index8 and 565

src/codec/SkCodec_libgif.cpp

Index: src/codec/SkCodec_libgif.cpp
diff --git a/src/codec/SkCodec_libgif.cpp b/src/codec/SkCodec_libgif.cpp
index 2a1d81fc00f99a71f3453579ee1ba49ad764442b..aa06f0d94707c12e981754949ea172f8ca91d7b1 100644
--- a/src/codec/SkCodec_libgif.cpp
+++ b/src/codec/SkCodec_libgif.cpp
@@ -131,41 +131,58 @@ static uint32_t find_trans_index(const SavedImage& image) {
 }
 
 /*
- * Assumes IsGif was called and returned true
- * Creates a gif decoder
- * Reads enough of the stream to determine the image format
+ * Read enough of the stream to initialize the SkGifCodec.
+ * Returns a bool representing success or failure.
+ * If it returned true, and codecOut was not NULL,
+ * it will be set to a new SkGifCodec.
  */
-SkCodec* SkGifCodec::NewFromStream(SkStream* stream) {
+bool SkGifCodec::ReadHeader(SkStream* stream, SkCodec** codecOut) {
     // Read gif header, logical screen descriptor, and global color table
     SkAutoTCallIProc<GifFileType, CloseGif> gif(open_gif(stream));
 
     if (NULL == gif) {
         gif_error("DGifOpen failed.\n");
-        return NULL;
+        return false;
     }
 
-    // Get fields from header
-    const int32_t width = gif->SWidth;
-    const int32_t height = gif->SHeight;
-    if (width <= 0 || height <= 0) {
-        gif_error("Invalid dimensions.\n");
-        return NULL;
+    if (NULL != codecOut) {
+        // Get fields from header
+        const int32_t width = gif->SWidth;
+        const int32_t height = gif->SHeight;
+        if (width <= 0 || height <= 0) {
+            gif_error("Invalid dimensions.\n");
+            return false;
+        }
+
+        // Return the codec
+        // kIndex is the most natural color type for gifs, so we set this as
+        // the default.
+        // Many gifs specify a color table index for transparent pixels.  Every
+        // other pixel is guaranteed to be opaque.  Despite this, because of the
+        // possiblity of transparent pixels, we cannot assume that the image is
+        // opaque.  We have the option to set the alpha type as kPremul or
+        // kUnpremul.  Both are valid since the alpha component will always be
+        // 0xFF or the entire 32-bit pixel will be set to zero.  We prefer
+        // kPremul because we support kPremul, and it is more efficient to
+        // use kPremul directly even when kUnpremul is supported.
+        const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
+                kIndex_8_SkColorType, kPremul_SkAlphaType);
+        *codecOut = SkNEW_ARGS(SkGifCodec, (imageInfo, stream, gif.detach()));
     }
+    return true;
+}
 
-    // Return the codec
-    // kIndex is the most natural color type for gifs, so we set this as
-    // the default.
-    // Many gifs specify a color table index for transparent pixels.  Every
-    // other pixel is guaranteed to be opaque.  Despite this, because of the
-    // possiblity of transparent pixels, we cannot assume that the image is
-    // opaque.  We have the option to set the alpha type as kPremul or
-    // kUnpremul.  Both are valid since the alpha component will always be
-    // 0xFF or the entire 32-bit pixel will be set to zero.  We prefer
-    // kPremul because we support kPremul, and it is more efficient to
-    // use kPremul directly even when kUnpremul is supported.
-    const SkImageInfo& imageInfo = SkImageInfo::Make(width, height,
-            kIndex_8_SkColorType, kPremul_SkAlphaType);
-    return SkNEW_ARGS(SkGifCodec, (imageInfo, stream, gif.detach()));
+/*
+ * Assumes IsGif was called and returned true
+ * Creates a gif decoder
+ * Reads enough of the stream to determine the image format
+ */
+SkCodec* SkGifCodec::NewFromStream(SkStream* stream) {
+    SkCodec* codec = NULL;
+    if (ReadHeader(stream, &codec)) {
+        return codec;
+    }
+    return NULL;
 }
 
 SkGifCodec::SkGifCodec(const SkImageInfo& srcInfo, SkStream* stream,
@@ -190,6 +207,9 @@ static bool conversion_possible(const SkImageInfo& dst,
         case kN32_SkColorType:
             return kPremul_SkAlphaType == dst.alphaType() ||
                     kUnpremul_SkAlphaType == dst.alphaType();
+        case kIndex_8_SkColorType:
+            return kPremul_SkAlphaType == dst.alphaType() ||
+                    kUnpremul_SkAlphaType == dst.alphaType();
         default:
             return false;
     }
@@ -200,11 +220,20 @@ static bool conversion_possible(const SkImageInfo& dst,
  */
 SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                                         void* dst, size_t dstRowBytes,
-                                        const Options& opts, SkPMColor*, int*) {
-    // Check for valid input parameters
-    if (!this->rewindIfNeeded()) {
+                                        const Options& opts,
+                                        SkPMColor* inputColorPtr,
+                                        int* inputColorCount) {
+    // Rewind if necessary
+    SkCodec::RewindState rewindState = this->rewindIfNeeded();
+    if (rewindState == kCouldNotRewind_RewindState) {
         return kCouldNotRewind;
+    } else if (rewindState == kRewound_RewindState) {
+        if (!ReadHeader(this->stream(), NULL)) {
+            return kCouldNotRewind;
+        }
     }
+
+    // Check for valid input parameters
     if (dstInfo.dimensions() != this->getInfo().dimensions()) {
         return gif_error("Scaling not supported.\n", kInvalidScale);
     }
@@ -284,11 +313,15 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                     imageTop = 0;
                 }
 
+                // Create a color table to store colors the giflib colorMap
+                SkPMColor alternateColorPtr[256];
+                SkPMColor* colorTable = get_color_table_ptr(dstInfo.colorType(),
+                        inputColorPtr, inputColorCount, alternateColorPtr);
+
                 // Set up the color table
                 uint32_t colorCount = 0;
                 // Allocate maximum storage to deal with invalid indices safely
                 const uint32_t maxColors = 256;
-                SkPMColor colorTable[maxColors];
                 ColorMapObject* colorMap = fGif->Image.ColorMap;
                 // If there is no local color table, use the global color table
                 if (NULL == colorMap) {
@@ -346,6 +379,7 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
 
                 // Check if image is only a subset of the image frame
                 SkAutoTDelete<SkSwizzler> swizzler(NULL);
+                SkColorType dstColorType = dstInfo.colorType();
                 if (innerWidth < width || innerHeight < height) {
 
                     // Modify the destination info
@@ -356,7 +390,6 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                     // FIXME: This may not be the behavior that we want for
                     //        animated gifs where we draw on top of the
                     //        previous frame.
-                    SkColorType dstColorType = dstInfo.colorType();
                     if (fillBackground) {
                         switch (dstColorType) {

[msarett, 2015/04/02 17:26:29]

I've been looking at factoring this into the swizzler with some sort of fill()
method (in my jpeg cl that is in progress).  I think it could be useful to gif,
bmp, and jpeg, but it is tricky because we still have to explain whether the
input parameter is an index or a color.

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

That should be determined by the SrcConfig, correct?

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

I added a SkSwizzler::Fill() function.  It turns out that using SrcConfig as a
field does not solve the difficulties with this routine.

I needed to make it static so that:
(1) It was usable by decodeRLE() in SkBmpCodec which does not use a swizzler
object.
(2) It was usable in SkGifCodec where we create a swizzler for only a subset of
the image (but want to fill the entire image).

I thought neither of these cases were compelling on their own, but because they
are 2 of the 3 use cases for this function (although there are more coming with
jpeg), it should be static.

The result is that the Fill() function needs a ton of parameters.  I still think
it is better to have it than not because it consolidates complexity to a single
place (which will only get worse if we support more color types).

                             case kN32_SkColorType:
@@ -365,6 +398,11 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                                     ((int) dstRowBytes) * height
                                     / sizeof(SkPMColor));
                                 break;
+                            case kIndex_8_SkColorType:
+                                memset((SkPMColor*) dst,
+                                    fillIndex,
+                                    ((int) dstRowBytes) * height);

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

Why did you cast to an int? (Sorry if this came up last time for the N32
case...)

Also, as mentioned in BMP, this is more than we can safely set.

It would probably be good to write a test for this. The test would *not* use a
tight row packing (i.e. it would use something larger than minRowBytes), but it
would allocate getSafeSize(largeRowBytes) bytes. ASAN would catch us writing
beyond the bounds of allocated memory.

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

The cast was added for sk_memset32 which takes an int (Windows bots were
failing).  Nice catch, I believe it is unnecessary for memset.

I will add the test you described in a subsequent CL if that seems like a
reasonable approach.  I think I fixed this bug everywhere, but I agree we need
to verify.

+                                break;
                             default:
                                 SkASSERT(false);
                                 break;
@@ -421,11 +459,22 @@ SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                         if (GIF_ERROR == DGifGetLine(fGif, buffer.get(),
                                 innerWidth)) {
                             if (fillBackground) {
-                                SkPMColor* dstPtr = (SkPMColor*) SkTAddOffset
-                                        <void*>(dst, y * dstRowBytes);
-                                sk_memset32(dstPtr, colorTable[fillIndex],
-                                        (height - y) * ((int) dstRowBytes)
-                                        / sizeof(SkPMColor));
+                                switch (dstColorType) {
+                                    case kN32_SkColorType:
+                                        sk_memset32((SkPMColor*) dst,
+                                            colorTable[fillIndex],
+                                            (height - y) * ((int) dstRowBytes)

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

Again, for both of these, we need to be careful about not going beyond the end
of the allocated memory.

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

Acknowledged.

+                                            / sizeof(SkPMColor));
+                                        break;
+                                    case kIndex_8_SkColorType:
+                                        memset((SkPMColor*) dst,
+                                            fillIndex,
+                                            (height - y) * ((int) dstRowBytes));
+                                        break;
+                                    default:
+                                        SkASSERT(false);
+                                        break;
+                                }
                             }
                             return gif_error(SkStringPrintf(
                                     "Could not decode line %d of %d.\n",