Bug 19273 – BitmapImage.cpp erroneously assumes repetition count is available once size is available

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

[Peter Kasting]

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.

[Peter Kasting]

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

[Dave Hyatt]

Comment on attachment 21404 [details]
patch v1

r=me

[Alp Toker]

Landed in r34200.

[Peter Kasting]

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.