Index: src/images/SkImageDecoder_libgif.cpp |
diff --git a/src/images/SkImageDecoder_libgif.cpp b/src/images/SkImageDecoder_libgif.cpp |
index 08f37efced9fc8bdee36b286dc52ad44f06ac028..71469f975927ea73f89f3c8110e084b16e5821b0 100644 |
--- a/src/images/SkImageDecoder_libgif.cpp |
+++ b/src/images/SkImageDecoder_libgif.cpp |
@@ -5,11 +5,11 @@ |
* 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" |
@@ -36,6 +36,12 @@ 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 |
@@ -147,12 +153,40 @@ static int find_transpIndex(const SavedImage& image, int colorCount) { |
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. |
@@ -213,10 +247,46 @@ bool SkGIFImageDecoder::onDecode(SkStream* sk_stream, SkBitmap* bm, Mode mode) { |
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"); |
@@ -231,50 +301,45 @@ bool SkGIFImageDecoder::onDecode(SkStream* sk_stream, SkBitmap* bm, 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/2657
hal.canary
2013/10/10 16:22:11
That's a good idea. 1kb on the stack is a lot che
|
const ColorMapObject* cmap = find_colormap(gif); |
- if (NULL == cmap) { |
- return error_return(gif, *bm, "null cmap"); |
- } |
- |
- 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); |
+ 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 ha
hal.canary
2013/10/10 16:22:11
In every example I saw, the only index used was th
|
+ } |
} |
- |
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"); |
@@ -291,25 +356,15 @@ bool SkGIFImageDecoder::onDecode(SkStream* sk_stream, SkBitmap* bm, Mode mode) { |
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)) { |
@@ -335,20 +390,36 @@ bool SkGIFImageDecoder::onDecode(SkStream* sk_stream, SkBitmap* bm, Mode mode) { |
// 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 withou
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. (sampleInt
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 surroun
hal.canary
2013/10/10 16:22:11
Since there is no cleanup code, let's just get rid
|
} |
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 fo
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 fi
hal.canary
2013/10/10 16:22:11
I'll change it.
|
} |
// scanline now contains the raw data. Sample it. |
sampler.next(scanline); |