Bug 22108 – REGRESSION: Animated gifs sometimes don't animate (wunderground.com weather radar)

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Jubal Kessler]

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.

[Jubal Kessler]

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.

[Peter Kasting]

Can you find a regression window for this?

[Alexey Proskuryakov]

See also: bug 22029, bug 22154.

[Peter Kasting]

*** Bug 22154 has been marked as a duplicate of this bug. ***

[Peter Kasting]

*** Bug 22029 has been marked as a duplicate of this bug. ***

[Peter Kasting]

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).

[Duane Williams]

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.

[Scott Perry]

(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.

[Simon Fraser (smfr)]

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?

[Scott Perry]

(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.

[Peter Kasting]

Did you type those numbers correctly?  The first is larger than the second.

I'm especially curious as to whether r37612 affected this.

[Scott Perry]

(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.

[Peter Kasting]

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.

[Scott Perry]

(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.

[Jason Fritcher]

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.

[Jason Fritcher]

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.

[Jubal Kessler]

The patch from bug #19663 may have caused this regression. We still need a reduced sample case; I'll take a stab at it.

[Alexey Proskuryakov]

*** Bug 21894 has been marked as a duplicate of this bug. ***

[Alexey Proskuryakov]

<rdar://problem/6388383>

[Jim Oase]

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

[Jim Oase]

Still no animation with build r28645
http://www.wunderground.com/radar/mixedcomposite.asp?region=a5&size=2x&type=loop

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

[Peter Kasting]

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.

[Peter Kasting]

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.

[Dave Hyatt]

Comment on attachment 25693 [details]
patch v1

r=me

[Peter Kasting]

Fixed in r39309.

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. 

[Alexey Proskuryakov]

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.

[Jim Oase]

(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

[Alexey Proskuryakov]

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.

[Simon Fraser (smfr)]

See also bug 23082.

[Simon Fraser (smfr)]

And bug 22647.

[Simon Fraser (smfr)]

*** Bug 22647 has been marked as a duplicate of this bug. ***

[Simon Fraser (smfr)]

Bug 22995 tracks remaining issues.