Bug 20745 – REGRESSION: Animated GIF not animated

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Naofumi Kagami]

Confirmed on Webkit SVN r38283, MacOS X 10.5.4, MacBook (Intel Core Duo 2GHz).
This bug was not present on the shipping version of Safari 3.1.2(5525.20.1).

On Webkit SVN r38283, the animated GIF in the URL is not animated immediately after being loaded. It stays frozen at one frame. If you resize the window, the GIF starts. You can also get it to start by keeping the mouse button pressed on the Webkit window (this is however unreliable and doesn't always work). If you hit the reload button (Command + R), then the animation stops again.

This problem does not occur on the current shipping version of Safari 3.1.2.

[Naofumi Kagami]

MIght be related to bug #16177

[Francis Lewis]

This is not a problem created when submitting a form via javascript.
I'll give you an example
http://cache.hyvz.com/images/smilies/default/smiley_byebye.gif

That is an animated gif that doesn't move in webkit.

This problem started with a build less than 2 weeks ago from today… I update weekly and this appeared on my last update.

[Naofumi Kagami]

Did some detective work to find out when this bug appeared on the nightlies.
Before WebKit-SVN-r36053 was OK.
After WebKit-SVN-r36078 has this bug.

[Gavin Sherlock]

Possibly:

http://trac.webkit.org/changeset/36069

is the culprit

[Peter Kasting]

This is probably my fault.

<rdar://problem/6218856>

[Peter Kasting]

OK, so far it looks like my patch is tickling a probably-previously-unnoticed bug in the CG decoder (which I can't debug).  When the image is loaded, setData(..., ..., true) is called, meaning all the data is available; but when startAnimation() asks if the first frame is complete, the CG decoder says "no".  So the animation code doesn't begin the animation.

bdash mentioned on IRC that he'd glance at this.

Either the CG code needs to be fixed to properly report frame completion status, or I'm going to have to figure out how to work around this in the animation code, which I'm not sure how to do.

Unassigning for now as I can't proceed further until I hear more about the CG side of things.

[Dave Hyatt]

You need to cache the frame before you can ask if it's complete (at least for CG).  We should just add a frameIsCompleteAtIndex to BitmapImage (that is like the other methods) that caches the frame and then asks the source if the frame is complete after doing the caching.

[Peter Kasting]

Ah, I see what type of thing you're suggesting.  Let me try and whip that up.

I wonder if this will be a perf hit.  There's some code in the animation functions that tries to aggressively throw away decoded data for large images, and we may be fighting that, especially when startAnimation() goes into a loop looking through the frames to see how far we need to go to catch up.  Please review this carefully when I get it done.

[Peter Kasting]

Created attachment 23486 [details]
Partial patch v1

This patch makes life better, but does not fix all the problems.

I elected to cache "m_isComplete" on the FrameData to keep things in parallel with how the duration and alpha work.  Someone should yell if this is invalid, say because the completion status of a frame can change without cacheFrame() being called a second time.

With this patch, the image on the URL animates on load.  However, some fiddling with the resize corner can eventually make it stop animating.  What I think is happening is that we're falling into the "too far behind, skip the current frame" case in startAnimation().  In this case, internalAdvanceAnimation() changes the image and notifies observers, expecting this to eventually result in another call to BitmapImage::draw(), which in turn will call back to startAnimation() and result in a timer getting set for the next frame.  But draw() is never reached after observer notification.  I don't know why.  We mark parts of the backing store dirty but beyond that my ability to trace through this code diminishes.  Help wanted :(

[Dave Hyatt]

Comment on attachment 23486 [details]
Partial patch v1

r=me

[Eric Seidel (no email)]

Comment on attachment 23486 [details]
Partial patch v1

Landed as r36628

Clearing review flag, since this is only part 1 of 2 for the fix.

[Dave Hyatt]

This fix I suggested is not right.  Fully decoding every frame just to catch up is leading to even basic animated GIFs consuming 100% CPU.

[Dave Hyatt]

This patch caused

https://bugs.webkit.org/show_bug.cgi?id=20954

And was rolled out in r36702.

[Dave Hyatt]

So here are the interesting issues that I see:

(1) cacheFrame forces a decode of an image frame.  This can potentially be very expensive.
(2) CG won't report a frame is complete unless you have in fact tried to decode an image frame.
(3) Resizing on OS X (mousing down in the resizer) pauses all animations.  That means when you lift the mouse up the "catch up" code is presumably going to run.
(4) startAnimation() got moved to the top of the draw function in ImageCG.cpp.  This means the frame you are trying to draw may be falsely reported as incomplete.
(5) (Cross-platform issue) We are willing to draw incomplete frames.  An image like:

http://img2.abload.de/img/almost_failedovs.gif

flashes to white a bunch as incomplete frames render. (When the image is uncached.)

[Dave Hyatt]

There's an additional issue of multi-frame image formats other than GIF being falsely considered to be "animatable."  That needs to be fixed.

[Peter Kasting]

If you've rolled back this patch, please also roll back r36069 and reopen bug 19663.  Until these issues are addressed, having that checked in puts the tree in a broken state; animated images just don't work right.

The patch on bug 20945 addresses the problem where images are falsely reported as animatable.

As for the numeric list you give, my thoughts:
(2) seems like something that could be improved in CG, honestly.  For example, if m_allDataReceived is true, you know all frames are complete.  I think the open-source decoders (which Chromium is using) may do better here.  Fixing this would probably allow me to rewrite the code in a way that doesn't cacheFrame() so aggressively (i.e. by not making the fix on this bug).  Still, offhand it doesn't seem like we should really be eating tons of extra CPU here.  I think I need to discuss this more.

(3) seems like desired behavior.  (I would like even more for resizing not to pause animations, but I assume that's platform-standard or something.)  Unless resizing also halts videos, plugins, timers, sound, etc., animated images _should_ catch up.  (Of course, if it halts all those things too, I retract my statement.)

(4) shouldn't be an issue, I don't think (?), unless (2) remains unfixed and something like this patch doesn't go in.  As-is, you have to decode the frame to draw anyway, so all this is doing is making us decode it at a slightly earlier point in draw() in order to have correct frame completion info.

As for (5), I'd need to take a look at my code to ensure it's not trying to skip through incomplete frames; that's the first possibility that comes to mind.

[Peter Kasting]

Closing, since the original patch that caused this regression has now been backed out.

[Francis Lewis]

This is still an issue in the latest nightly build (r36766)

[Peter Kasting]

The revert didn't happen until r36781, have patience :)