GIF decode: optional error messages and fault tolerance.

src/images/SkImageDecoder_libgif.cpp

 /*
  * Copyright 2006 The Android Open Source Project
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
-
 #include "SkColor.h"
 #include "SkColorPriv.h"
 #include "SkColorTable.h"
 #include "SkImageDecoder.h"
+#include "SkRTConf.h"
 #include "SkScaledBitmapSampler.h"
 #include "SkStream.h"
 #include "SkTemplates.h"
 
 #include "gif_lib.h"
 
 class SkGIFImageDecoder : public SkImageDecoder {
 public:
     virtual Format getFormat() const SK_OVERRIDE {
         return kGIF_Format;
     }
 
 protected:
     virtual bool onDecode(SkStream* stream, SkBitmap* bm, Mode mode) SK_OVERRIDE;
 
 private:
     typedef SkImageDecoder INHERITED;
 };
 
 static const uint8_t gStartingIterlaceYValue[] = {
     0, 4, 2, 1
 };
 static const uint8_t gDeltaIterlaceYValue[] = {
     8, 8, 4, 2
 };
 
+SK_CONF_DECLARE(bool, c_suppressGIFImageDecoderWarnings,
+                "images.gif.suppressDecoderWarnings", true,
+                "Suppress GIF warnings and errors when calling image decode "
+                "functions.");
+
+
 /*  Implement the GIF interlace algorithm in an iterator.
     1) grab every 8th line beginning at 0
     2) grab every 8th line beginning at 4
     3) grab every 4th line beginning at 2
     4) grab every 2nd line beginning at 1
 */
 class GifInterlaceIter {
 public:
     GifInterlaceIter(int height) : fHeight(height) {
         fStartYPtr = gStartingIterlaceYValue;
@@ skipping 91 matching lines @@
                 }
                 break;
             }
         }
     }
     return transpIndex;
 }
 
 static bool error_return(GifFileType* gif, const SkBitmap& bm,
                          const char msg[]) {
-#if 0
-    SkDebugf("libgif error <%s> bitmap [%d %d] pixels %p colortable %p\n",
-             msg, bm.width(), bm.height(), bm.getPixels(), bm.getColorTable());
-#endif
+    if (!c_suppressGIFImageDecoderWarnings) {
+        SkDebugf("libgif error [%s] bitmap [%d %d] pixels %p colortable %p\n",
+                 msg, bm.width(), bm.height(), bm.getPixels(), bm.getColorTable());
+    }
     return false;
 }
+static void gif_warning(GifFileType* gif, const SkBitmap& bm,

[scroggo, 2013/10/09 20:12:03]

gif is never used.

(Same in error_return)

[hal.canary, 2013/10/10 16:22:11]

Good catch.

+                        const char msg[]) {
+    if (!c_suppressGIFImageDecoderWarnings) {
+        SkDebugf("libgif warning [%s] bitmap [%d %d] pixels %p colortable %p\n",
+                 msg, bm.width(), bm.height(), bm.getPixels(),
+                 bm.getColorTable());
+    }
+}
 
 /**
+ * Used several times in SkGIFImageDecoder::onDecode() to determine
+ * the index value to use when the image is truncated or doesn't fill
+ * the bitmap. */
+static int get_fill_index(int transpIndex, int colorCount, GifFileType & gif) {

[scroggo, 2013/10/09 20:12:03]

Can't gif be const?

[hal.canary, 2013/10/10 16:22:11]

good catch.

+    int fill = 0;
+    if (transpIndex >= 0) {
+        fill = transpIndex;
+    } else {
+        fill = gif.SBackGroundColor;
+    }
+    // check for valid fill index/color
+    if (static_cast<unsigned>(fill) >=
+        static_cast<unsigned>(colorCount)) {
+        return 0;
+    }
+    return fill;
+}
+
+
+/**
  *  Skip rows in the source gif image.
  *  @param gif Source image.
  *  @param dst Scratch output needed by gif library call. Must be >= width bytes.
  *  @param width Bytes per row in the source image.
  *  @param rowsToSkip Number of rows to skip.
  *  @return True on success, false on GIF_ERROR.
  */
 static bool skip_src_rows(GifFileType* gif, uint8_t* dst, int width, int rowsToSkip) {
     for (int i = 0; i < rowsToSkip; i++) {
         if (DGifGetLine(gif, dst, width) == GIF_ERROR) {
@@ skipping 19 matching lines @@
     temp_save.ExtensionBlocks=NULL;
     temp_save.ExtensionBlockCount=0;
     SkAutoTCallVProc<SavedImage, CheckFreeExtension> acp2(&temp_save);
 
     int width, height;
     GifRecordType recType;
     GifByteType *extData;
 #if GIFLIB_MAJOR >= 5
     int extFunction;
 #endif
     int transpIndex = -1;   // -1 means we don't have it (yet)

[scroggo, 2013/10/09 20:12:03]

Would it make sense to define a fill color here, and calculate it once in the
colortable block, and then use it from then on? Then you can remove the
function.

Not sure it's better; just a thought.

[hal.canary, 2013/10/10 16:22:11]

I was thinking about the waste of extra logic that is only used sometimes.  But
that would be a lot cleaner and the overhead is minimal.

 
     do {
         if (DGifGetRecordType(gif, &recType) == GIF_ERROR) {
             return error_return(gif, *bm, "DGifGetRecordType");
         }
 
         switch (recType) {
         case IMAGE_DESC_RECORD_TYPE: {
             if (DGifGetImageDesc(gif) == GIF_ERROR) {
                 return error_return(gif, *bm, "IMAGE_DESC_RECORD_TYPE");
             }
 
             if (gif->ImageCount < 1) {    // sanity check
                 return error_return(gif, *bm, "ImageCount < 1");
             }
 
             width = gif->SWidth;
             height = gif->SHeight;
-            if (width <= 0 || height <= 0) {
+            // if (width <= 0 || height <= 0) {

[scroggo, 2013/10/09 20:12:03]

Any reason not to remove this commented out code?

[hal.canary, 2013/10/10 16:22:11]

Good catch

+            //     return error_return(gif, *bm, "invalid dimensions");
+            // }
+            SavedImage* image = &gif->SavedImages[gif->ImageCount-1];
+            const GifImageDesc& desc = image->ImageDesc;
+
+            int imageLeft = desc.Left;
+            int imageTop = desc.Top;
+            const int innerWidth = desc.Width;
+            const int innerHeight = desc.Height;
+            if (innerWidth <= 0 || innerHeight <= 0) {
                 return error_return(gif, *bm, "invalid dimensions");
             }
 
+            // check for valid descriptor
+            if (innerWidth > width) {
+                gif_warning(gif, *bm, "image too wide, expanding output to size");
+                width = innerWidth;
+                imageLeft = 0;
+            } else if (imageLeft + innerWidth > width) {
+                gif_warning(gif, *bm, "shifting image left to fit");
+                imageLeft = width - innerWidth;
+            } else if (imageLeft < 0) {
+                gif_warning(gif, *bm, "shifting image right to fit");
+                imageLeft = 0;
+            }
+
+
+            if (innerHeight > height) {
+                gif_warning(gif, *bm, "image too tall,  expanding output to size");
+                height = innerHeight;
+                imageTop = 0;
+            } else if (imageTop + innerHeight > height) {
+                gif_warning(gif, *bm, "shifting image up to fit");
+                imageTop = height - innerHeight;
+            } else if (imageTop < 0) {
+                gif_warning(gif, *bm, "shifting image down to fit");
+                imageTop = 0;
+            }
+
             // FIXME: We could give the caller a choice of images or configs.
             if (!this->chooseFromOneChoice(SkBitmap::kIndex8_Config, width, height)) {
                 return error_return(gif, *bm, "chooseFromOneChoice");
             }
 
             SkScaledBitmapSampler sampler(width, height, this->getSampleSize());
 
             bm->setConfig(SkBitmap::kIndex8_Config, sampler.scaledWidth(),
                           sampler.scaledHeight());
 
             if (SkImageDecoder::kDecodeBounds_Mode == mode) {
                 return true;
             }
 
-            SavedImage* image = &gif->SavedImages[gif->ImageCount-1];
-            const GifImageDesc& desc = image->ImageDesc;
-
-            // check for valid descriptor
-            if (   (desc.Top | desc.Left) < 0 ||
-                    desc.Left + desc.Width > width ||
-                    desc.Top + desc.Height > height) {
-                return error_return(gif, *bm, "TopLeft");
-            }
 
             // now we decode the colortable
             int colorCount = 0;
             {
+                SkAutoTMalloc<SkPMColor> colorStorage;  // declare here for scope.

[scroggo, 2013/10/09 20:12:03]

Mike's patch (https://codereview.chromium.org/26572006/) switches to putting an
array on the stack to avoid malloc.

[hal.canary, 2013/10/10 16:22:11]

That's a good idea.  1kb on the stack is a lot cheaper.

                 const ColorMapObject* cmap = find_colormap(gif);
-                if (NULL == cmap) {
-                    return error_return(gif, *bm, "null cmap");
+                if (cmap != NULL) {
+                    colorCount = cmap->ColorCount;
+                    colorStorage.reset(colorCount);
+                    for (int index = 0; index < colorCount; index++) {
+                        colorStorage[index] = SkPackARGB32(0xFF,
+                                                           cmap->Colors[index].Red,
+                                                           cmap->Colors[index].Green,
+                                                           cmap->Colors[index].Blue);
+                    }
+                } else {
+                    // find_colormap() returned NULL.  Some GIFs don't
+                    // have a color table, so we force one.
+                    gif_warning(gif, *bm, "missing colormap");
+                    colorCount = 256;
+                    colorStorage.reset(colorCount);
+                    for (int i = 0; i < colorCount; ++i) {
+                        colorStorage[i] = SK_ColorWHITE;

[scroggo, 2013/10/09 20:12:03]

Why did you pick white? I thought you said this happened when the intent was to
be transparent? I suppose we just have to arbitrarily pick something either way.

nit: Could this be done using memset?

[hal.canary, 2013/10/10 16:22:11]

In every example I saw, the only index used was the transpIndex.  I used the
white in case the transpIndex was -1, in which case I am trying to return an
opaque bitmap.
 
I'll use sk_memset32().

+                    }
                 }
-
-                colorCount = cmap->ColorCount;
-                SkAutoTMalloc<SkPMColor> colorStorage(colorCount);
-                SkPMColor* colorPtr = colorStorage.get();
-                for (int index = 0; index < colorCount; index++) {
-                    colorPtr[index] = SkPackARGB32(0xFF,
-                                                   cmap->Colors[index].Red,
-                                                   cmap->Colors[index].Green,
-                                                   cmap->Colors[index].Blue);
-                }
-
                 transpIndex = find_transpIndex(temp_save, colorCount);
                 bool reallyHasAlpha = transpIndex >= 0;
                 if (reallyHasAlpha) {
-                    colorPtr[transpIndex] = SK_ColorTRANSPARENT; // ram in a transparent SkPMColor
+                    // ram in a transparent SkPMColor
+                    colorStorage[transpIndex] = SK_ColorTRANSPARENT;
                 }
-
-                SkAutoTUnref<SkColorTable> ctable(SkNEW_ARGS(SkColorTable, (colorPtr, colorCount)));
+                SkAutoTUnref<SkColorTable> ctable(SkNEW_ARGS(SkColorTable,
+                                                             (colorStorage.get(), colorCount)));
                 ctable->setIsOpaque(!reallyHasAlpha);
                 if (!this->allocPixelRef(bm, ctable)) {
                     return error_return(gif, *bm, "allocPixelRef");
                 }
             }
 
-            const int innerWidth = desc.Width;
-            const int innerHeight = desc.Height;
-
             // abort if either inner dimension is <= 0
             if (innerWidth <= 0 || innerHeight <= 0) {
                 return error_return(gif, *bm, "non-pos inner width/height");
             }
 
             SkAutoLockPixels alp(*bm);
 
             SkAutoMalloc storage(innerWidth);
             uint8_t* scanline = (uint8_t*) storage.get();
 
             // GIF has an option to store the scanlines of an image, plus a larger background,
             // filled by a fill color. In this case, we will use a subset of the larger bitmap
             // for sampling.
             SkBitmap subset;
             SkBitmap* workingBitmap;
             // are we only a subset of the total bounds?
-            if ((desc.Top | desc.Left) > 0 ||
+            if ((imageTop | imageLeft) > 0 ||
                  innerWidth < width || innerHeight < height) {
-                int fill;
-                if (transpIndex >= 0) {
-                    fill = transpIndex;
-                } else {
-                    fill = gif->SBackGroundColor;
-                }
-                // check for valid fill index/color
-                if (static_cast<unsigned>(fill) >=
-                        static_cast<unsigned>(colorCount)) {
-                    fill = 0;
-                }
+                int fill = get_fill_index(transpIndex, colorCount, *gif);
                 // Fill the background.
                 memset(bm->getPixels(), fill, bm->getSize());
 
                 // Create a subset of the bitmap.
-                SkIRect subsetRect(SkIRect::MakeXYWH(desc.Left / sampler.srcDX(),
-                                                     desc.Top / sampler.srcDY(),
+                SkIRect subsetRect(SkIRect::MakeXYWH(imageLeft / sampler.srcDX(),
+                                                     imageTop / sampler.srcDY(),
                                                      innerWidth / sampler.srcDX(),
                                                      innerHeight / sampler.srcDY()));
                 if (!bm->extractSubset(&subset, subsetRect)) {
                     return error_return(gif, *bm, "Extract failed.");
                 }
                 // Update the sampler. We'll now be only sampling into the subset.
                 sampler = SkScaledBitmapSampler(innerWidth, innerHeight, this->getSampleSize());
                 workingBitmap = &subset;
             } else {
                 workingBitmap = bm;
             }
 
             // bm is already locked, but if we had to take a subset, it must be locked also,
             // so that getPixels() will point to its pixels.
             SkAutoLockPixels alpWorking(*workingBitmap);
 
             if (!sampler.begin(workingBitmap, SkScaledBitmapSampler::kIndex, *this)) {
                 return error_return(gif, *bm, "Sampler failed to begin.");
             }
 
             // now decode each scanline
             if (gif->Image.Interlace) {
                 // Iterate over the height of the source data. The sampler will
                 // take care of skipping unneeded rows.
                 GifInterlaceIter iter(innerHeight);
-                for (int y = 0; y < innerHeight; y++){
+                for (int y = 0; y < innerHeight; y++) {
                     if (DGifGetLine(gif, scanline, innerWidth) == GIF_ERROR) {
-                        return error_return(gif, *bm, "interlace DGifGetLine");
+                        gif_warning(gif, *bm, "interlace DGifGetLine");
+                        int fill = get_fill_index(transpIndex, colorCount, *gif);

[scroggo, 2013/10/09 20:12:03]

Maybe this suggestion introduces complexity without much benefit: if we already
filled (due to smaller innerWidth/innerHeight), we can skip this.

[hal.canary, 2013/10/10 16:22:11]

But we aren't guaranteed to have done that.

+                        for (; y < innerHeight; y++) {
+                            memset(scanline, fill, innerWidth);

[scroggo, 2013/10/09 20:12:03]

This can be done once outside the loop. (sampleInterlaced does not modify
scanline)

[hal.canary, 2013/10/10 16:22:11]

good catch.

+                            sampler.sampleInterlaced(scanline, iter.currY());
+                            iter.next();
+                        }
+                        return true;

[scroggo, 2013/10/09 20:12:03]

Should we say 'goto DONE;' to fit with the surrounding code? (Or maybe change
the other goto DONE?)

[hal.canary, 2013/10/10 16:22:11]

Since there is no cleanup code, let's just get rid of the goto.  Dijkstra will
be happy.

                     }
                     sampler.sampleInterlaced(scanline, iter.currY());
                     iter.next();
                 }
             } else {
                 // easy, non-interlace case
+                bool getLineErrorState = false;
+                int fill = 0;
                 const int outHeight = workingBitmap->height();
                 skip_src_rows(gif, scanline, innerWidth, sampler.srcY0());
                 for (int y = 0; y < outHeight; y++) {
-                    if (DGifGetLine(gif, scanline, innerWidth) == GIF_ERROR) {
-                        return error_return(gif, *bm, "DGifGetLine");
+                    if (!getLineErrorState) {

[scroggo, 2013/10/09 20:12:03]

This is a little complicated to follow. Why not follow a similar format as the
interlaced case? (i.e. loop through outHeight and return true.)

[hal.canary, 2013/10/10 16:22:11]

I'll change it.

+                        if (DGifGetLine(gif, scanline, innerWidth) == GIF_ERROR) {
+                            getLineErrorState = true;
+                            gif_warning(gif, *bm, "DGifGetLine");
+                            fill = get_fill_index(transpIndex, colorCount, *gif);
+                            memset(scanline, fill, innerWidth);
+                        }
+                    } else {
+                        memset(scanline, fill, innerWidth);

[scroggo, 2013/10/09 20:12:03]

This step is unnecessary for each line past the first one that failed.

[hal.canary, 2013/10/10 16:22:11]

I'll change it.

                     }
                     // scanline now contains the raw data. Sample it.
                     sampler.next(scanline);
                     if (y < outHeight - 1) {
                         skip_src_rows(gif, scanline, innerWidth, sampler.srcDY() - 1);

[scroggo, 2013/10/09 20:12:03]

If getLineErrorState is true, both this and the next skip_src_rows should be
skipped (skip_src_rows calls DGifGetLine). More reason to follow the pattern in
the interlaced case and return true before now.

[hal.canary, 2013/10/10 16:22:11]

I'll change it.

                     }
                 }
                 // skip the rest of the rows (if any)
                 int read = (outHeight - 1) * sampler.srcDY() + sampler.srcY0() + 1;
                 SkASSERT(read <= innerHeight);
                 skip_src_rows(gif, scanline, innerWidth, innerHeight - read);
             }
             goto DONE;
             } break;
 
@@ skipping 68 matching lines @@
 static SkImageDecoder_DecodeReg gReg(sk_libgif_dfactory);
 
 static SkImageDecoder::Format get_format_gif(SkStreamRewindable* stream) {
     if (is_gif(stream)) {
         return SkImageDecoder::kGIF_Format;
     }
     return SkImageDecoder::kUnknown_Format;
 }
 
 static SkImageDecoder_FormatReg gFormatReg(get_format_gif);