Last Comment Bug 767779 - Animated gif stops animating
: Animated gif stops animating
Status: RESOLVED FIXED
[qa-]
: regression, relnote
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: 15 Branch
: x86 Windows XP
: -- normal with 1 vote (vote)
: mozilla17
Assigned To: Adam [:hobophobe]
:
Mentors:
Depends on: 776249 819412
Blocks: 733553
  Show dependency treegraph
 
Reported: 2012-06-24 02:08 PDT by andershol
Modified: 2012-12-07 09:38 PST (History)
16 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified
+
fixed
fixed


Attachments
Explicitly check for multipart before cleanup. (1.14 KB, patch)
2012-06-25 16:41 PDT, Adam [:hobophobe]
joe: review+
Details | Diff | Splinter Review
Fix the problem for multipart too. (1.50 KB, patch)
2012-07-16 19:15 PDT, Adam [:hobophobe]
no flags Details | Diff | Splinter Review
Test slow-loading GIF with trailing garbage (5.49 KB, patch)
2012-07-18 14:29 PDT, Adam [:hobophobe]
no flags Details | Diff | Splinter Review
Forgot to include the GIF in the patch (5.95 KB, patch)
2012-07-18 14:34 PDT, Adam [:hobophobe]
joe: review+
Details | Diff | Splinter Review
Use mDecoded to properly detect decoded state (1.46 KB, patch)
2012-07-21 18:39 PDT, Adam [:hobophobe]
joe: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description andershol 2012-06-24 02:08:51 PDT
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 2012-06-24 02:46:49 PDT
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 2012-06-24 03:32:32 PDT
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 2012-06-24 03:44:20 PDT
Something specific to XP?
2012-06-23 is good on 7
Comment 4 Loic 2012-06-25 05:45:55 PDT
(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] 2012-06-25 05:49:05 PDT
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 2012-06-25 05:55:36 PDT
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 2012-06-25 06:14:11 PDT
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 2012-06-25 06:24:34 PDT
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 2012-06-25 07:34:44 PDT
(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 [:hobophobe] 2012-06-25 16:41:14 PDT
Created attachment 636542 [details] [diff] [review]
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) 2012-07-16 11:24:19 PDT
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 [:hobophobe] 2012-07-16 19:15:07 PDT
Created attachment 642835 [details] [diff] [review]
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 2012-07-17 15:34:17 PDT
(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 [:hobophobe] 2012-07-18 14:29:02 PDT
Created attachment 643588 [details] [diff] [review]
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 [:hobophobe] 2012-07-18 14:34:55 PDT
Created attachment 643590 [details] [diff] [review]
Forgot to include the GIF in the patch
Comment 16 Joe Drew (not getting mail) 2012-07-19 13:21:02 PDT
Comment on attachment 643590 [details] [diff] [review]
Forgot to include the GIF in the patch

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

Excellent!
Comment 19 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-07-21 04:09:51 PDT
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] 2012-07-21 04:38:57 PDT
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] 2012-07-21 05:44:54 PDT
Should we be at all disturbed that none of our other reftests caught this?
Comment 22 Adam [:hobophobe] 2012-07-21 18:39:50 PDT
Created attachment 644704 [details] [diff] [review]
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 (⌚ UTC+1/+2) 2012-07-24 14:24:16 PDT
https://tbpl.mozilla.org/?tree=Try&rev=3d2d00f1bd57
Comment 24 Adam [:hobophobe] 2012-07-25 13:57:12 PDT
Try results look good, thanks Ms2ger!
Comment 27 Alex Keybl [:akeybl] 2012-07-30 16:34:47 PDT
Please nominate for uplift if deemed low risk, given this is a new regression in FF15.
Comment 28 Joe Drew (not getting mail) 2012-08-07 05:58:40 PDT
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 2012-08-07 07:41:34 PDT
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 31 Geoff Brown [:gbrown] 2012-08-10 13:23:59 PDT
There was an Android Ts regression on mozilla-beta blamed on this check-in: see bug 781917.
Comment 32 Paul Silaghi, QA [:pauly] 2012-08-13 07:27:23 PDT
(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

Note You need to log in before you can comment on or make changes to this bug.