767779 - Animated gif stops animating

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[andershol]

User Agent: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20120623 Firefox/15.0a2
Build ID: 20120623042007

Steps to reproduce:

To reset: clear cache for the site (Ctrl+h, search for "dmi.dk", right-click a page from the "DMI" site and choose "Forget About This Site")

Goto http://www.dmi.dk/dmi/index/danmark/radar.htm
 or directly http://www.dmi.dk/dmi/radar-animation640.gif



Actual results:

The animation might play correctly at first and perhaps even after a reload, but after a shift-reload (also try shift-reload followed by reload) the image will either,
- not loop (stop at last frame)
- loop once and then be an white image
- just show a white image (no animation)


Expected results:

The animation should loop repeatedly (with a small pause after the last frame).

Comment 1

[Jesper Hansen]

WFM in current nightly.
Might have been a single fluke as well. 
There is no white frame according to http://gif-explode.com/

Comment 2

[andershol]

Thanks for checking it out. I did not mean to imply that the animation had a white frame in it. Checking the nightly (I had only observed it in Aurora with my normal profile) I do however also see the problem there, so I take the liberty of re-unconfirming for reconsideration. A bisect using mozregression gives (bug 733553 in the range could sound related):

Last good nightly: 2012-05-19
First bad nightly: 2012-05-20

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=642d1a36702f&tochange=0e2cc686b07b

Comment 3

[Jesper Hansen]

Something specific to XP?
2012-06-23 is good on 7

Comment 4

[Loic]

(In reply to andershol from comment #0)
> The animation might play correctly at first and perhaps even after a reload,
> but after a shift-reload (also try shift-reload followed by reload) the
> image will either,
> - loop once and then be an white image
I'm observing this situation on Win 7 and directly after the 1st load.

It works fine in IE9, the animation plays.

Comment 5

[(mostly gone) XtC4UaLL [:xtc4uall]]

Confirmed against Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120624030537 i.e. on Win 7 x64 too.

I used CTRL + F5 for hard Reload. The following F5 produces a White Picture.

Comment 6

[Jesper Hansen]

ah, I have layers.acceleration.disabled set to true
If I set it to false, then I am able to reproduce this

So if someone can reproduce my observation then I'd say we're pretty close to saying "grrrr, hardware acceleration again"

Comment 7

[Loic]

Mozregression for m-i:
last good=2012-05-19
fisrt bad=2012-05-20
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=175bcd2c7912&tochange=443c0c79e54f

So same observation than andershol, suspected bug is:
Adam Dane [:hobophobe] — Bug 733553 - Allow multipart image streams to cope with stream changes. r=joe

Comment 8

[Loic]

Another observation:
when the animated GIF is stuck at the 1st frame, changing the focus (like switching to another tab or another application) forces the blank white frame to appear.

Comment 9

[andershol]

(In reply to Jesper Hansen from comment #6)
> ah, I have layers.acceleration.disabled set to true
> If I set it to false, then I am able to reproduce this
> 
> So if someone can reproduce my observation then I'd say we're pretty close
> to saying "grrrr, hardware acceleration again"

I can reproduce with and without layers.acceleration.disabled, but my laptop is so old, I believe it don't have acceleration in any case ("GPU Accelerated Windows" is blocked under Troubleshooting Information). I don't have a Win7, but I tried a Win8 which worked with a old Aurora (14.0a2 2012-05-11) and showed the bug on a current Aurora (15.0a2 2012-06-24) (again layers.acceleration.disabled made no difference, but this machine is also old and have "GPU Accelerated Windows", as well as "Direct2D Enabled", blocked). But if the bug is timing related, it could be indirectly caused by acceleration. Since it happens in relation to reload, disk speed could be a factor (I have old machines with non-solid-state-disks on a slow internet connection), otherwise I don't know why you don't see the problem.

I tried a mozregression on Win8 which gave the same interval, i.e.:
Last good nightly: 2012-05-19
First bad nightly: 2012-05-20
(Loic's infomation is more precise, but I had already started the bisect when I saw those results)

Comment 10

[Adam]

Attachment: Explicitly check for multipart before cleanup.

This was my bad assumption about when |mBytesDecoded == 0| in |AddSourceData|.  When we're storing the source data, |mBytesDecoded| alone doesn't reliably signal that we're starting a new part/can clean up from the previous animation.

Fix is to check for |mMultipart| there.

Comment 11

[Joe Drew (not getting mail)]

Comment on attachment 636542 [details] [diff] [review]
Explicitly check for multipart before cleanup.

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

Can we add a test for this?

Comment 12

[Adam]

Attachment: Fix the problem for multipart too.

I looked at writing a test for this, and found the previous patch isn't comprehensive enough.  I'll add a test soon.  

The GIFs in question contain trailing garbage.  I'm not familiar enough with GIFs to know why they have the garbage, but using |gifsicle --info| says: 

> gifsicle: While reading 'radar-animation640.gif' frame #7:
> gifsicle:   warning: trailing garbage after GIF ignored

That's only a problem over the network: my initial attempts to recreate locally failed because the data came too fast.  So I used |iprelay| to simulate a slow network with a local webserver to recreate this.

After the valid data is loaded, the decoder is finished.  But Necko doesn't know the extra data is garbage, so |AddSourceData| gets called anyway.  

The fix is to politely return |NS_OK| when the decoder isn't open for business, as the data is useless to us.  Also keeping the |mMultipart| check for the cleanup, because that should be true whenever the cleanup is needed.

Comment 13

[andershol]

(In reply to Adam [:hobophobe] from comment #12)
> The GIFs in question contain trailing garbage.

Note that the gif is replaced about every 10 minutes, so the garbage might not always be present.

If it is a persistent problem, we could try reaching out to the site. The image does however load correctly in e.g. IE. The site is the 3th largest in the country (according to http://fdim.dk/ ), so I assume most browsers handle the images.

Comment 14

[Adam]

Attachment: Test slow-loading GIF with trailing garbage

Separate patch because I wanted to test without the fix to make sure this failed properly on the bad code.

Test adds animated GIF with trailing garbage, then sends the good data from it at once, followed by chunks of the garbage.  The timing of the data should mean that this should always properly fail on the bad code.

Comment 15

[Adam]

Attachment: Forgot to include the GIF in the patch

Comment 16

[Joe Drew (not getting mail)]

Comment on attachment 643590 [details] [diff] [review]
Forgot to include the GIF in the patch

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

Excellent!

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2d53ed0df7ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0652135a7f

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2d53ed0df7ef
https://hg.mozilla.org/mozilla-central/rev/3e0652135a7f

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/446b788ab99d
Backed out because of major image painting problems
(or perhaps images aren't loaded fully or something).

Comment 20

[Nick Thomas [:nthomas] (UTC+12)]

For testing, the first build from inbound where the problem was observed was
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-win32/1342830043/
(should be able to substitute macosx64, linux, linux64 for win32 to get other platforms)

The full set of changes for that build includes this bug - 
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=139a8f2a8538&tochange=045c11dd41a6

The behaviour we were seeing was that only part of an image was being displayed, typically the top was present and the bottom issing. http://www.neowin.net was one site this was observed on.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Should we be at all disturbed that none of our other reftests caught this?

Comment 22

[Adam]

Attachment: Use mDecoded to properly detect decoded state

Sorry about that.  We can lack a decoder (|!mDecoder|) when storing data, as it looks like a worker handles decodes in that case.  Normally it's not busy enough for that to be a problem, which is why only image-heavy/generally heavy pages like neowin and yahoo! had the problem.

This changes the condition to |mDecoded| which will work for both normal and multipart images (the latter will work because |NewSourceData| will reset it before calling |AddSourceData|).

I've tried to create a test for this, using the same sort of slow-loading as the test I added, but haven't successfully created a failing test.  I'll try to poke around with that a bit more, maybe add some more weight and see if that bogs it down enough to reproduce in a test scenario?

Comment 23

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://tbpl.mozilla.org/?tree=Try&rev=3d2d00f1bd57

Comment 24

[Adam]

Try results look good, thanks Ms2ger!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Take two!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ebf03b8f1fda
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4183e6918aa

Comment 26

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/ebf03b8f1fda
https://hg.mozilla.org/mozilla-central/rev/c4183e6918aa

Comment 27

[Alex Keybl [:akeybl]]

Please nominate for uplift if deemed low risk, given this is a new regression in FF15.

Comment 28

[Joe Drew (not getting mail)]

Comment on attachment 644704 [details] [diff] [review]
Use mDecoded to properly detect decoded state

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 733553
User impact if declined: Certain animated GIFs will stop animating instead of looping
Testing completed (on m-c, etc.): On m-c for 10 days or so
Risk to taking this patch (and alternatives if risky): Could cause different brokenness with regard to multipart/x-mixed-replace webcams.
String or UUID changes made by this patch: none

Comment 29

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 644704 [details] [diff] [review]
Use mDecoded to properly detect decoded state

Let's get this landed today so it's in the beta this week for more user coverage to shake out any issues.  Since this is a regression in 15 it's worth trying to get it in there.

Comment 30

[Joe Drew (not getting mail)]

http://hg.mozilla.org/releases/mozilla-beta/rev/fae49a9eda43
http://hg.mozilla.org/releases/mozilla-aurora/rev/daa838a23b97

Comment 31

[Geoff Brown [:gbrown]]

There was an Android Ts regression on mozilla-beta blamed on this check-in: see bug 781917.

Comment 32

[Paul Silaghi, QA [:pauly]]

(In reply to andershol from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 5.1; rv:15.0) Gecko/20120623
> Firefox/15.0a2
> Build ID: 20120623042007
> 
> Steps to reproduce:
> 
> To reset: clear cache for the site (Ctrl+h, search for "dmi.dk", right-click
> a page from the "DMI" site and choose "Forget About This Site")
> 
> Goto http://www.dmi.dk/dmi/index/danmark/radar.htm
>  or directly http://www.dmi.dk/dmi/radar-animation640.gif
> 
> 
> 
> Actual results:
> 
> The animation might play correctly at first and perhaps even after a reload,
> but after a shift-reload (also try shift-reload followed by reload) the
> image will either,
> - not loop (stop at last frame)
> - loop once and then be an white image
> - just show a white image (no animation)
> 
> 
> Expected results:
> 
> The animation should loop repeatedly (with a small pause after the last
> frame).

Able to see the issue on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0b3.
Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20100101 Firefox/15.0b4