Bug 19273 - BitmapImage.cpp erroneously assumes repetition count is available once size is available
Summary: BitmapImage.cpp erroneously assumes repetition count is available once size i...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-05-27 11:51 PDT by Peter Kasting
Modified: 2009-01-07 18:12 PST (History)
1 user (show)

See Also:


Attachments
patch v1 (5.36 KB, patch)
2008-05-28 15:12 PDT, Peter Kasting
hyatt: review+
Details | Formatted Diff | Diff
Testcase (143.96 KB, image/gif)
2009-01-07 18:12 PST, Peter Kasting
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2008-05-27 11:51:55 PDT
BitmapImage.cpp:CacheFrame() sets m_repetitionCount during the first call to it, assuming that the decoder can provide a valid repetition count.  And GIFImageDecoder.cpp:repetitionCount() (which is not used in Safari) has a comment that the repetition count is available once the image size is available.

But, as far as I can tell from reading the GIF89a spec, this is not the case.

The loop count is contained in a Netscape Application Extension block, which, according the the GIF grammar, can appear anywhere in the set of blocks that makes up the "data" section (which extends nearly until the end of the file).  So, in the worst case, the loop count is unavailable until you've actually decoded the whole image.

The safest way to deal with this is as follows:
* Leave the existing call to get the repetition count in BitmapImage::cacheFrame(), which will distinguish between GIF and non-animating image formats, and will in many cases get the right repetition count immediately.
* When deciding whether to advance the animation in BitmapImage::startAnimation(), do not advance if the m_repetitionCount == cAnimationLoopOnce, m_AllDataReceived == false, and m_currentFrame >= (frameCount() - 1).  This gives us time to get the true repetition count if it appears at the end of the data stream, but does not hang the animation any more than necessary (and no more than could currently happen).
* When advancing the animation in BitmapImage:advanceAnimation(), get the repetition count again if we're advancing past the last frame, before deciding whether the loop count has been reached.
Comment 1 Peter Kasting 2008-05-28 15:12:45 PDT
Created attachment 21404 [details]
patch v1

Here's a patch to fix.  I didn't realize that the GIFImageDecoder deletes m_reader as soon as the GIF finishes decoding, so I had to add a member to GIFImageDecoder to hold the repetition count after that point.  (I didn't bother to plumb a notification from the reader up to the decoder that the count has changed, since BitmapImage.cpp will only ask for this data either very early or after the whole image has decoded, so this wouldn't have bought us anything.)
Comment 2 Dave Hyatt 2008-05-28 15:35:32 PDT
Comment on attachment 21404 [details]
patch v1

r=me
Comment 3 Alp Toker 2008-05-29 05:54:15 PDT
Landed in r34200.
Comment 4 Peter Kasting 2009-01-07 18:12:55 PST
Created attachment 26518 [details]
Testcase

(Since I needed to check this recently) here's a test image that has the repeat count stored near the end of the image.