Bug 22108 - REGRESSION: Animated gifs sometimes don't animate (wunderground.com weather radar)
Summary: REGRESSION: Animated gifs sometimes don't animate (wunderground.com weather r...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P1 Minor
Assignee: Peter Kasting
URL: http://www.wunderground.com/radar/mix...
Keywords: InRadar
: 21894 22029 22154 (view as bug list)
Depends on:
Blocks:
 
Reported: 2008-11-06 11:13 PST by Jubal Kessler
Modified: 2009-01-07 17:53 PST (History)
11 users (show)

See Also:


Attachments
patch v1 (28.73 KB, patch)
2008-12-02 17:05 PST, Peter Kasting
hyatt: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jubal Kessler 2008-11-06 11:13:19 PST
For some reason, this page's animated GIF doesn't loop on the page. If I context-menu click the image and open it in a new tab, it will animate. I don't notice this problem with regular Safari 3.1.2.
Comment 1 Jubal Kessler 2008-11-06 11:18:07 PST
Additionally, if I reload the image while it is already standalone in its own tab (and is actually animating), the animation fails to happen. I have to repeat the context-menu-click-and-move-to-new-tab action to get it to animate again.
Comment 2 Peter Kasting 2008-11-10 11:04:17 PST
Can you find a regression window for this?
Comment 3 Alexey Proskuryakov 2008-11-10 13:16:01 PST
See also: bug 22029, bug 22154.
Comment 4 Peter Kasting 2008-11-10 13:31:12 PST
*** Bug 22154 has been marked as a duplicate of this bug. ***
Comment 5 Peter Kasting 2008-11-10 13:32:04 PST
*** Bug 22029 has been marked as a duplicate of this bug. ***
Comment 6 Peter Kasting 2008-11-10 13:33:15 PST
From the duped bugs, here are other pages that exhibit problems:

http://radar.weather.gov/ridge/radar_lite.php?rid=ICT&product=NCR&loop=yes
http://hurricane.accuweather.com/hurricane/satellite.asp?region=hpac&anim=1&type=ei&basin=epacific

Verified all three pages' looping images are GIFs.  It is not clear from reading these comments if the problem is that the images fail to loop (i.e. they animate exactly once) or if they fail to animate at all (only the initial frame shows).
Comment 7 Duane Williams 2008-11-11 07:32:03 PST
After clicking Animate there is no visible animation, but you do get an additional frame in the "Show the previous page" sequence.  It looks like it might be the first frame in the animation sequence.
Comment 8 Scott Perry 2008-11-11 14:19:00 PST
(In reply to comment #6)
> It is not clear from
> reading these comments if the problem is that the images fail to loop (i.e.
> they animate exactly once) or if they fail to animate at all (only the initial
> frame shows).

in my experience, only the first frame shows.  very rarely the image will animate (no obvious cause of success), and when it does it loops correctly.
Comment 9 Simon Fraser (smfr) 2008-11-11 14:26:34 PST
This bug seems sporadic, and doesn't happen in simple test cases. When a complex page with an animated GIF loads, the GIF doesn't always start animating. However, if you cause a redraw, or load the image in its own tab, then it animates fine.

Can someone try to whip up a testcase that is simpler than the URLs listed above?
Comment 10 Scott Perry 2008-11-11 14:37:48 PST
(In reply to comment #2)
> Can you find a regression window for this?

testing the nightlies, it looks like this problem surfaced between (r37605,r37469].

pretty big window, but narrows it down a bit.
Comment 11 Peter Kasting 2008-11-11 14:39:58 PST
Did you type those numbers correctly?  The first is larger than the second.

I'm especially curious as to whether r37612 affected this.
Comment 12 Scott Perry 2008-11-11 14:49:39 PST
(In reply to comment #11)
> Did you type those numbers correctly?  The first is larger than the second.

unfortunately yes.

from http://nightly.webkit.org/builds/trunk/mac/1 , the builds for r37605 and r37469 are adjacent.
Comment 13 Peter Kasting 2008-11-11 14:51:54 PST
Adjacent, in descending order.  So the range is (r37469,r37605], assuming you're saying the problem reproduces in the later build and not the earlier.  I was trying to figure out if there was a typo.

That sounds like r37612 is not at fault, which surprises me slightly.
Comment 14 Scott Perry 2008-11-11 15:23:22 PST
(In reply to comment #13)
> Adjacent, in descending order.  So the range is (r37469,r37605], assuming
> you're saying the problem reproduces in the later build and not the earlier.  I
> was trying to figure out if there was a typo.

sorry, you are correct.

> That sounds like r37612 is not at fault, which surprises me slightly.

well, I can't claim 100% accuracy, either.

r37605 works more often than not.  I only flagged it because of one animation failure (out of about 48 different tests).
that may have been a false positive as it works with all of the other cases that r37698 fails on.
Comment 15 Jason Fritcher 2008-11-16 09:09:02 PST
I use wunderground.com as well  and I can confirm that of the nightlies r37605 and previous work normally, but r37698 and newer do not animate and only show the first frame.

I am currently checking the code out of svn to do intermediate builds to narrow down which revision breaks the animation.
Comment 16 Jason Fritcher 2008-11-16 11:12:31 PST
To update my previous post, I've done a few intermediate builds and it appears that r37612 is the responsible party for breaking these animations. The animated weather radar on wunderground.com works normally in r37611 but does not animate and only shows the first frame in r37612.

I'm not familiar enough with the WebKit code base to dig into the code itself, but please let me know if there is any more detail I can provide.
Comment 17 Jubal Kessler 2008-11-16 11:33:56 PST
The patch from bug #19663 may have caused this regression. We still need a reduced sample case; I'll take a stab at it.
Comment 18 Alexey Proskuryakov 2008-11-17 08:44:44 PST
*** Bug 21894 has been marked as a duplicate of this bug. ***
Comment 19 Alexey Proskuryakov 2008-11-20 05:50:11 PST
<rdar://problem/6388383>
Comment 20 Jim Oase 2008-11-20 06:31:02 PST
Could it be that the same routines are used and causing similar problems?

http://radar.weather.gov/Conus/full_loop.php

This site does not animate when load is complete.

Using nightly build r38592
Comment 22 Peter Kasting 2008-11-21 14:01:28 PST
I'm looking at something like this on the Chromium side at the moment, though it manifests a bit differently (looping images fail to loop).  I think the underlying problem may be the same, but it's tricky to tell because the two have different ImageSource implementations/interactions with the underlying decoders.

The basic problem is that for large images, we destroy the decoder as we go, and keep re-setting the data on it.  My concern is that important state like the repetition count, or perhaps the frame count (?) is then basically uninitialized, and BitmapImage can ask the source for it and get garbage, leading to problems.

In Chromium's case, the underlying GIF decoder (available in the WebKit tree as GIFImage[Decoder,Reader].cpp) has a repetitionCount() function that simply returns the value of a member variable, which itself is not set unless we happen to run across the repetition count extension block while decoding.  So if we destroy the decoder, the value returned from this function can be wrong for an arbitrary time.  I'm seeing cases where it is, in fact, wrong, and BitmapImage erroneously believes the image loops once instead of looping forever.

This doesn't explain the CG bug described here; I'm wondering if perhaps in that case the frame count is wrong for the same reason, or something.  hyatt isn't around to chime in and I can't see into the CG decoder's source.

There are possible fixes on the Chromium side, but all involve modifying the decoder.  I can't add new functions to the CG decoder (and don't have a reference to the existing set to see if I'd need to), so I'm not sure how best to address this.
Comment 23 Peter Kasting 2008-12-02 17:05:23 PST
Created attachment 25693 [details]
patch v1

hyatt and I have already chatted some on IRC about this patch.

This fixes animations by not deleting the decoder, meaning the loop count can stay valid across frames.  I didn't actually determine for sure that this was the problem in CG WebKit, but it was the problem in Chromium, and patching the fix in here made the testcases seem to work in my Safari Win build.

It turns out the CG decoder was already doing a pretty good job of dropping frames when their external references were released, and of deleting frames internally during re-decoding, so it didn't suffer from the peak memory issues Cairo/Chromium did, and the old "delete ImageDecoder" code wasn't really necessary for it at all.  Unfortunately it also decodes from the beginning of the stream more than it needs to, so it still uses far more CPU than it should (i.e. this patch does not fix the underlying problem in bug 22280).  Perhaps hyatt can use the framework in this patch to help address things.
Comment 24 Dave Hyatt 2008-12-15 12:01:53 PST
Comment on attachment 25693 [details]
patch v1

r=me
Comment 25 Peter Kasting 2008-12-15 12:46:05 PST
Fixed in r39309.
Comment 26 pilotgi 2008-12-25 10:22:56 PST
Why has this been marked as resolved? I'm using r39469 and weather radar loops from weather.gov still don't animate. They didn't work with for me with r39309 or any subsequent builds either. 
Comment 27 Alexey Proskuryakov 2008-12-25 13:04:21 PST
This is marked as resolved because a patch that fixed some or all problems was committed. Please file a new bug if you are still seeing problems, and describe them in detail.
Comment 28 Jim Oase 2008-12-25 22:50:32 PST
(In reply to comment #27)
> This is marked as resolved because a patch that fixed some or all problems was
> committed. Please file a new bug if you are still seeing problems, and describe
> them in detail.
> 

This site still exhibits the no animation problem
http://radar.weather.gov/Conus/full_loop.php

Using WebKit-SVN-r39469

Comment 29 Alexey Proskuryakov 2008-12-25 23:31:05 PST
As I said, please file a new bug. Also, this weather.gov page is different from the one discussed here (http://radar.weather.gov/ridge/radar_lite.php?rid=ICT&product=NCR&loop=yes), and it doesn't work well even in shipping Safari/WebKit for me.
Comment 30 Simon Fraser (smfr) 2009-01-02 15:33:28 PST
See also bug 23082.
Comment 31 Simon Fraser (smfr) 2009-01-02 16:43:13 PST
And bug 22647.
Comment 32 Simon Fraser (smfr) 2009-01-02 16:47:26 PST
*** Bug 22647 has been marked as a duplicate of this bug. ***
Comment 33 Simon Fraser (smfr) 2009-01-07 17:53:53 PST
Bug 22995 tracks remaining issues.