Bug 20745 - REGRESSION: Animated GIF not animated
Summary: REGRESSION: Animated GIF not animated
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.5
: P1 Normal
Assignee: Peter Kasting
URL: http://castle104.com/images/spinner.gif
Keywords: InRadar
Depends on:
Reported: 2008-09-09 07:33 PDT by Naofumi Kagami
Modified: 2008-09-22 15:02 PDT (History)
3 users (show)

See Also:

Partial patch v1 (5.49 KB, patch)
2008-09-16 14:00 PDT, Peter Kasting
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Naofumi Kagami 2008-09-09 07:33:53 PDT
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.
Comment 1 Naofumi Kagami 2008-09-09 07:37:54 PDT
MIght be related to bug #16177
Comment 2 Francis Lewis 2008-09-10 11:33:54 PDT
This is not a problem created when submitting a form via javascript.
I'll give you an example

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.
Comment 3 Naofumi Kagami 2008-09-10 23:15:53 PDT
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.
Comment 4 Gavin Sherlock 2008-09-10 23:37:18 PDT


is the culprit
Comment 5 Peter Kasting 2008-09-13 09:27:19 PDT
This is probably my fault.
Comment 6 mitz 2008-09-14 16:15:04 PDT
Comment 7 Peter Kasting 2008-09-16 11:13:19 PDT
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.
Comment 8 Dave Hyatt 2008-09-16 12:04:18 PDT
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.
Comment 9 Peter Kasting 2008-09-16 12:46:07 PDT
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.
Comment 10 Peter Kasting 2008-09-16 14:00:34 PDT
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 :(
Comment 11 Dave Hyatt 2008-09-17 14:48:53 PDT
Comment on attachment 23486 [details]
Partial patch v1

Comment 12 Eric Seidel (no email) 2008-09-18 16:08:10 PDT
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.
Comment 13 Dave Hyatt 2008-09-19 21:41:37 PDT
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.
Comment 14 Dave Hyatt 2008-09-19 21:51:06 PDT
This patch caused


And was rolled out in r36702.

Comment 15 Dave Hyatt 2008-09-19 22:43:20 PDT
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:


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

Comment 16 Dave Hyatt 2008-09-19 22:45:34 PDT
There's an additional issue of multi-frame image formats other than GIF being falsely considered to be "animatable."  That needs to be fixed.

Comment 17 Peter Kasting 2008-09-20 09:39:55 PDT
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.
Comment 18 Peter Kasting 2008-09-22 14:49:18 PDT
Closing, since the original patch that caused this regression has now been backed out.
Comment 19 Francis Lewis 2008-09-22 14:57:51 PDT
This is still an issue in the latest nightly build (r36766)
Comment 20 Peter Kasting 2008-09-22 15:02:49 PDT
The revert didn't happen until r36781, have patience :)