GIF decode: optional error messages and fault tolerance.

GIF decode: optional error messages and fault tolerance.

Add new runtime configuration variable,
images.gif.suppressDecoderWarnings, which suppresses warning and
errors from the GIF library.  It defaults to "true", which is current
behavior.

(This setting can be changed by setting the environment variable
skia_images_gif_suppressDecoderWarnings="false".)

Some conditions which were errors before are now warnings:

-   If the image width or height is greater than the GIF screen width or
    height (respectively) we expand the screen to hold the image.

-   If the offset of the image inside the screen would place the
    image outside of the screen, we shift the image to fix this.

-   If the image lacks a color table, we create a default color table.

-   If the image is truncated, then the rest of the image is filled with
    the fill color.

In all four cases, if images.gif.suppressDecoderWarnings is set to
false, then a warning message is printed via SkDebugf.

In the event of another kind of error, SkGIFImageDecoder::onDecode()
will still return false.  But with this change, if
images.gif.suppressDecoderWarnings is set to false, a description of
the error is printed via SkDebugf.

Also, added a new unit test GifTest, which tests the deconing of both
good GIf files and corrupted files that should now work with this
change.  This unit test is disabled on Win32, iOS, and Mac.

BUG=skia:1689
R=scroggo@google.com

Committed: https://code.google.com/p/skia/source/detail?r=11734

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

I cannot see patch set 2 (upload failed) - this seems to be a spurious error of
the site that can usually be fixed by reuploading.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
File src/images/SkImageDecoder_libgif.cpp (right):

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:162: static void gif_warning(GifFileType*
gif, const SkBitmap& bm,
gif is never used.

(Same in error_return)

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:175: static int get_fill_index(int
transpIndex, int colorCount, GifFileType & gif) {
Can't gif be const?

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:231: int transpIndex = -1;   // -1 means we
don't have it (yet)
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.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:250: // if (width <= 0 || height <= 0) {
Any reason not to remove this commented out code?

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:308: SkAutoTMalloc<SkPMColor> colorStorage;
 // declare here for scope.
Mike's patch (https://codereview.chromium.org/26572006/) switches to putting an
array on the stack to avoid malloc.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:326: colorStorage[i] = SK_ColorWHITE;
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?

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:396: int fill = get_fill_index(transpIndex,
colorCount, *gif);
Maybe this suggestion introduces complexity without much benefit: if we already
filled (due to smaller innerWidth/innerHeight), we can skip this.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:398: memset(scanline, fill, innerWidth);
This can be done once outside the loop. (sampleInterlaced does not modify
scanline)

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:402: return true;
Should we say 'goto DONE;' to fit with the surrounding code? (Or maybe change
the other goto DONE?)

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:414: if (!getLineErrorState) {
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.)

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:422: memset(scanline, fill, innerWidth);
This step is unnecessary for each line past the first one that failed.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:427: skip_src_rows(gif, scanline,
innerWidth, sampler.srcDY() - 1);
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:10]

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
File src/images/SkImageDecoder_libgif.cpp (right):

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:162: static void gif_warning(GifFileType*
gif, const SkBitmap& bm,
On 2013/10/09 20:12:03, scroggo wrote:
> gif is never used.
> 
> (Same in error_return)

Good catch.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:175: static int get_fill_index(int
transpIndex, int colorCount, GifFileType & gif) {
On 2013/10/09 20:12:03, scroggo wrote:
> Can't gif be const?

good catch.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:231: int transpIndex = -1;   // -1 means we
don't have it (yet)
On 2013/10/09 20:12:03, scroggo wrote:
> 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.

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.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:250: // if (width <= 0 || height <= 0) {
On 2013/10/09 20:12:03, scroggo wrote:
> Any reason not to remove this commented out code?

Good catch

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:308: SkAutoTMalloc<SkPMColor> colorStorage;
 // declare here for scope.
On 2013/10/09 20:12:03, scroggo wrote:
> Mike's patch (https://codereview.chromium.org/26572006/) switches to putting
an
> array on the stack to avoid malloc.

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

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:326: colorStorage[i] = SK_ColorWHITE;
On 2013/10/09 20:12:03, scroggo wrote:
> 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.

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.
 
> nit: Could this be done using memset?


I'll use sk_memset32().

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:396: int fill = get_fill_index(transpIndex,
colorCount, *gif);
On 2013/10/09 20:12:03, scroggo wrote:
> Maybe this suggestion introduces complexity without much benefit: if we
already
> filled (due to smaller innerWidth/innerHeight), we can skip this.

But we aren't guaranteed to have done that.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:398: memset(scanline, fill, innerWidth);
On 2013/10/09 20:12:03, scroggo wrote:
> This can be done once outside the loop. (sampleInterlaced does not modify
> scanline)

good catch.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:402: return true;
On 2013/10/09 20:12:03, scroggo wrote:
> Should we say 'goto DONE;' to fit with the surrounding code? (Or maybe change
> the other goto DONE?)

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

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:414: if (!getLineErrorState) {
On 2013/10/09 20:12:03, scroggo wrote:
> 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.)

I'll change it.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:422: memset(scanline, fill, innerWidth);
On 2013/10/09 20:12:03, scroggo wrote:
> This step is unnecessary for each line past the first one that failed.

I'll change it.

https://codereview.chromium.org/26743002/diff/1/src/images/SkImageDecoder_lib...
src/images/SkImageDecoder_libgif.cpp:427: skip_src_rows(gif, scanline,
innerWidth, sampler.srcDY() - 1);
On 2013/10/09 20:12:03, scroggo wrote:
> 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.

I'll change it.

[hal.canary, 2013-10-10 19:45:29]

I've made some changes, please take a look.

[scroggo, 2013-10-10 20:02:12]

LGTM at patch set 5

https://codereview.chromium.org/26743002/diff/25001/tests/GifTest.cpp
File tests/GifTest.cpp (right):

https://codereview.chromium.org/26743002/diff/25001/tests/GifTest.cpp#newcode53
tests/GifTest.cpp:53: static void testGifDataNoColormap(skiatest::Reporter* r,
nit: Typically we write our static functions with names like

test_gif_data_no_colormap

[scroggo, 2013-10-11 14:56:33]

On 2013/10/10 20:02:12, scroggo wrote:
> LGTM at patch set 5
> 
> https://codereview.chromium.org/26743002/diff/25001/tests/GifTest.cpp
> File tests/GifTest.cpp (right):
> 
>
https://codereview.chromium.org/26743002/diff/25001/tests/GifTest.cpp#newcode53
> tests/GifTest.cpp:53: static void testGifDataNoColormap(skiatest::Reporter* r,
> nit: Typically we write our static functions with names like
> 
> test_gif_data_no_colormap

This one fails your new GIF test on Mac.

I should have remembered - on Windows and Mac, we use the platform's image
decoders (you can look at src/ports/SkImageDecoder_CG.cpp and
src/ports/SkImageDecoder_WIC.cpp if you're curious). We should probably just
disable this test on Windows and Mac.

(It also fails and Android here due to UploadSKImageResults, which is probably
because it doesn't behave properly for trybots.)

[hal.canary, 2013-10-11 18:21:59]

Committed patchset #8 manually as r11734 (presubmit successful).