If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

no longer animate with certain gif

VERIFIED FIXED in Firefox 50



a year ago
a year ago


(Reporter: Alice0775 White, Assigned: seth)



50 Branch

Firefox Tracking Flags

(firefox49 unaffected, firefox50+ fixed)


(Whiteboard: [parity-Chrome][parity-Edge])


(1 attachment, 1 obsolete attachment)



a year ago
[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:

Regressed by: Bug 1204392
Flags: needinfo?(seth)


a year ago
See Also: → bug 773203
Let's stop this from getting into Aurora.
Assignee: nobody → seth

Comment 2

a year ago
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

a year ago
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

a year ago
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) {	
>           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.
Flags: needinfo?(seth)

Comment 5

a year ago
Created attachment 8763296 [details] [diff] [review]
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.)
Attachment #8763296 - Flags: review?(n.nethercote)


a year ago
Duplicate of this bug: 1280109

Comment 7

a year ago

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. 


Comment 8

a year ago
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.
OS: Windows 10 → All
Hardware: x86 → All


a year ago
Duplicate of this bug: 1280781
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.
Attachment #8763296 - Flags: review?(n.nethercote) → review+

Comment 11

a year ago
Created attachment 8763762 [details] [diff] [review]
Correctly skip over extra image sub blocks in the GIF decoder.

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


a year ago
Attachment #8763296 - Attachment is obsolete: true

Comment 12

a year ago
Pushed by mfowler@mozilla.com:
Correctly skip over extra image sub blocks in the GIF decoder. r=njn
Tracking 50+ for this gif regression
tracking-firefox50: ? → +

Comment 14

a year ago
Last Resolved: a year ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Comment 15

a year ago
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
You need to log in before you can comment on or make changes to this bug.