1279859 - no longer animate with certain gif

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Alice0775 White]

[Tracking Requested - why for this release]: no longer animate with certain gif

Steps To Reproduce:
1. Open attachment 641410 [details] of Bug 773203


Actual Results:
No animated

Expected Results:
Flashing in order

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8dc18bf5abac83c7adb2791eb582f1722a244666&tochange=026cf6432f4473549aa6193272764a06be33a1c3

Regressed by: Bug 1204392

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Let's stop this from getting into Aurora.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Hmm. I'm not seeing the failure to animate. Plays back fine here. However, I know Alice0775 White generally tests on Windows, and with content acceleration, invalidation behaves different. So I'd strongly suspect this is an invalidation-related bug.

Comment 3

[Seth Fowler [:seth] [:s2h]]

I take it back; I do see it. Nightly failed to update to the latest version for me and I had to update it manually. Will get this fixed ASAP.

Comment 4

[Seth Fowler [:seth] [:s2h]]

So check out this *great* code from the previous version of the GIF decoder:

>    case gif_sub_block:	
>       mGIFStruct.count = *q;	
>       if (mGIFStruct.count) {	
>         // Still working on the same image: Process next LZW data block	
>         // Make sure there are still pixels left. If the GIF data	
>         // is corrupt, we may not get an explicit terminator.	
>         if (mGIFStruct.pixels_remaining <= 0) {	
> #ifdef DONT_TOLERATE_BROKEN_GIFS	
>           mGIFStruct.state = gif_error;	
>           break;	
> #else	
>           // This is an illegal GIF, but we remain tolerant.	
>           GETN(1, gif_sub_block);	

Hmm, looks like in this situation we should read one byte and return to the
gif_sub_block state, right?

> #endif	
>           if (mGIFStruct.count == GIF_TRAILER) {	
>             // Found a terminator anyway, so consider the image done	
>             GETN(1, gif_done);	
>             break;	
>           }	
>         }	

Well, if we didn't hit a GIF_TAILER, control flow reaches here:

>         GETN(mGIFStruct.count, gif_lzw);	

Looks like we're actually going to read |count| bytes in the gif_lzw state! The
previous GETN() actually did nothing! Nice.

>       } else {	
>         // See if there are any more images in this sequence.	
>         EndImageFrame();	
>         GETN(1, gif_image_start);	
>       }	
> break;

There's the bug. The new code behaves as though GETN(1, gif_sub_block) was the
right thing to do, but actually we want to do the equivalent of
GETN(mGIFStruct.count, gif_lzw).

Patch incoming.

Comment 5

[Seth Fowler [:seth] [:s2h]]

Attachment: Correctly skip over extra image sub blocks in the GIF decoder.

Here's the patch, with accompanying test. (Alas the test is a bit verbose since
we actually need an Image object to check this one.)

Comment 7

[Gérard Talbot]

Hello,

I'm not an ImageLib expert. Is bug 1280781 a DUPLICATE of this bug?

Is this bug a Windows only bug? Just asking...

Thank you for your assistance here. 

Gérard

Comment 8

[Gérard Talbot]

I am a Linux user [Linux 3.13.0-85-generic x86_64, Qt: 4.8.6, KDE 4.13.3; Kubuntu (trusty) 14.04.4 LTS] and I get actual results when loading attachment 641410 [details] in Firefox 50.0a1 buildID=20160618024606. 

From what I can see, this bug fits the description and what I see in bug 1280781. 
Resolving accordingly.

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8763296 [details] [diff] [review]
Correctly skip over extra image sub blocks in the GIF decoder.

Review of attachment 8763296 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/test/gtest/TestDecoders.cpp
@@ +363,5 @@
> +    ImageFactory::CreateAnonymousImage(nsDependentCString(testCase.mMimeType));
> +  ASSERT_TRUE(!image->HasError());
> +
> +  nsCOMPtr<nsIInputStream> inputStream = LoadFile(testCase.mPath);
> +  ASSERT_TRUE(inputStream != nullptr);

Just do ASSERT_TRUE(inputStream); ?

@@ +409,5 @@
> +                         RasterSurfaceKey(imageSize,
> +                                          DefaultSurfaceFlags(),
> +                                          /* aFrameNum = */ 0));
> +  EXPECT_EQ(MatchType::EXACT, firstFrameLookupResult.Type());
> +                                                             

Trailing whitespace.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: Correctly skip over extra image sub blocks in the GIF decoder.

Thanks for the review! Here's an updated version of the patch.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/629faa5d9254
Correctly skip over extra image sub blocks in the GIF decoder. r=njn

Comment 13

[Marcia Knous [:marcia]]

Tracking 50+ for this gif regression

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/629faa5d9254

Comment 15

[Gérard Talbot]

When I load attachment 641410 [details] into Firefox 50.0a1 buildID=20160622030210, I get expected results (Flashing in order); I also get expected results when I load the animated GIF from bug 1280781.

Thank you for the bug fix, Seth Fowler

Marking this bug as VERIFIED