Swizzler changes Index8 and 565

src/codec/SkCodec_libgif.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_libgif.h"
 #include "SkCodecPriv.h"
 #include "SkColorPriv.h"
@@ skipping 113 matching lines @@
             break;
         }
     }
 
     // Use maximum unsigned int (surely an invalid index) to indicate that a valid
     // index was not found.
     return SK_MaxU32;
 }
 
 /*
+ * 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.
+ */
+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 false;
+    }
+
+    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;
+}
+
+/*
  * 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) {
-    // 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;
+    SkCodec* codec = NULL;
+    if (ReadHeader(stream, &codec)) {
+        return codec;
     }
-
-    // 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;
-    }
-
-    // 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()));
+    return NULL;
 }
 
 SkGifCodec::SkGifCodec(const SkImageInfo& srcInfo, SkStream* stream,
                        GifFileType* gif)
     : INHERITED(srcInfo, stream)
     , fGif(gif)
 {}
 
 /*
  * Checks if the conversion between the input image and the requested output
  * image has been implemented
  */
 static bool conversion_possible(const SkImageInfo& dst,
                                 const SkImageInfo& src) {
     // Ensure that the profile type is unchanged
     if (dst.profileType() != src.profileType()) {
         return false;
     }
 
     // Check for supported color and alpha types
     switch (dst.colorType()) {
         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;
     }
 }
 
 /*
  * Initiates the gif decode
  */
 SkCodec::Result SkGifCodec::onGetPixels(const SkImageInfo& dstInfo,
                                         void* dst, size_t dstRowBytes,
-                                        const Options& opts, SkPMColor*, int*) {
+                                        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 (!this->rewindIfNeeded()) {
-        return kCouldNotRewind;
-    }
     if (dstInfo.dimensions() != this->getInfo().dimensions()) {
         return gif_error("Scaling not supported.\n", kInvalidScale);
     }
     if (!conversion_possible(dstInfo, this->getInfo())) {
         return gif_error("Cannot convert input type to output type.\n",
                 kInvalidConversion);
     }
 
     // Use this as a container to hold information about any gif extension
     // blocks.  This generally stores transparency and animation instructions.
@@ skipping 59 matching lines @@
                     innerHeight = height;
                     imageTop = 0;
                 } else if (imageTop + innerHeight > height) {
                     gif_warning("Shifting inner image up to fit.\n");
                     imageTop = height - innerHeight;
                 } else if (imageTop < 0) {
                     gif_warning("Shifting image down to fit\n");
                     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) {
                     colorMap = fGif->SColorMap;
                 }
                 if (NULL != colorMap) {
                     colorCount = colorMap->ColorCount;
                     SkASSERT(colorCount ==
                             (unsigned) (1 << (colorMap->BitsPerPixel)));
                     SkASSERT(colorCount <= 256);
@@ skipping 37 matching lines @@
                 }
 
                 // Fill in the color table for indices greater than color count.
                 // This allows for predictable, safe behavior.
                 for (uint32_t i = colorCount; i < maxColors; i++) {
                     colorTable[i] = colorTable[fillIndex];
                 } 
 
                 // 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
                     const SkImageInfo subsetDstInfo =
                             dstInfo.makeWH(innerWidth, innerHeight);
 
                     // Fill the destination with the fill color
                     // 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:
                                 sk_memset32((SkPMColor*) dst,
                                     colorTable[fillIndex],
                                     ((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;
                         }
                     }
 
                     // Modify the dst pointer
                     const int32_t dstBytesPerPixel =
                             SkColorTypeBytesPerPixel(dstColorType);
                     void* subsetDst = SkTAddOffset<void*>(dst,
@@ skipping 36 matching lines @@
                                     y, height - 1).c_str(), kIncompleteInput);
                         }
                         swizzler->next(buffer.get(), iter.nextY());
                     }
                 } else {
                     // Standard mode
                     for (int32_t y = 0; y < innerHeight; y++) {
                         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",
                                     y, height - 1).c_str(), kIncompleteInput);
                         }
                         swizzler->next(buffer.get());
                     }
                 }
 
                 // FIXME: Gif files may have multiple images stored in a single
@@ skipping 69 matching lines @@
                 // giflib returns an error code if the record type is not known.
                 // We should catch this error immediately.
                 SkASSERT(false);
                 break;
         }
     } while (TERMINATE_RECORD_TYPE != recordType);
 
     return gif_error("Could not find any images to decode in gif file.\n",
             kInvalidInput);
 }