Bug 35287 – ImageSourceCG::frameIsCompleteAtIndex returns false for all frames until image has completed loading

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

[Mark Rowe (bdash)]

While looking in to bug 35115 I noticed that BitmapImage::frameIsCompleteAtIndex was always returning false until the complete image had loaded.  Further analysis revealed that CGImageSourceGetStatusAtIndex seems to claim that all frames of an animated GIF are incomplete when using an incremental image source that has not yet received the complete data of the image. This certainly isn’t the behavior that the author of ImageSource::frameIsCompleteAtIndex (or any reasonable person) expects. I’m not sure if this behavior changed from Leopard to SnowLeopard, but I filed a bug against CGImageSourceGetStatusAtIndex expressing my displeasure at the misleading results that it gives (<rdar://problem/7679174>).

We should attempt to work around <rdar://problem/7679174> and have frameIsCompleteAtIndex return a more useful answer.  This will allow the fix from bug 35115 to be landed, which will fix an issue with the timing of the start of animations that are loaded from the cache (or a very quick network).

[Mark Rowe (bdash)]

Created attachment 49304 [details]
Patch

[Mark Rowe (bdash)]

One user-visible impact of this change is that animated GIFs on Mac OS X and Windows no longer wait until they have completely loaded before they begin animating.

[Geoffrey Garen]

Comment on attachment 49304 [details]
Patch

It looks like the two values we're interested in, when testing "frameStatus >= kCGImageStatusIncomplete", are:
    kCGImageStatusIncomplete = -1,
    kCGImageStatusComplete = 0

I think it would be better just to test them explicitly: "frameStatus == kCGImageStatusIncomplete || frameStatus == kCGImageStatusComplete".

This has the benefit of documenting which values we're testing for without requiring the reader to flip to another file. Also, unless there's some explicit API contract somewhere, it's probably better not to assume that statuses >= kCGImageStatusIncomplete will always be "good" statuses.

[Darin Adler]

Comment on attachment 49304 [details]
Patch

> +    // source (<rdar://problem/7679174>).  We work around this by special-casing all frames except the

We do a single space after periods.

> +    if (index < frameCount() - 1)
> +        return frameStatus >= kCGImageStatusIncomplete;

Can this ever be called if the frameCount is 0?

[Mark Rowe (bdash)]

(In reply to comment #3)
> (From update of attachment 49304 [details])
> It looks like the two values we're interested in, when testing "frameStatus >=
> kCGImageStatusIncomplete", are:
>     kCGImageStatusIncomplete = -1,
>     kCGImageStatusComplete = 0
> 
> I think it would be better just to test them explicitly: "frameStatus ==
> kCGImageStatusIncomplete || frameStatus == kCGImageStatusComplete".
> 
> This has the benefit of documenting which values we're testing for without
> requiring the reader to flip to another file. Also, unless there's some
> explicit API contract somewhere, it's probably better not to assume that
> statuses >= kCGImageStatusIncomplete will always be "good" statuses.

I was matching the manner in which the status values are tested elsewhere in ImageSourceCG.cpp: <http://trac.webkit.org/browser/trunk/WebCore/platform/graphics/cg/ImageSourceCG.cpp#L153>.

[Mark Rowe (bdash)]

(In reply to comment #4)
> (From update of attachment 49304 [details])
> > +    // source (<rdar://problem/7679174>).  We work around this by special-casing all frames except the
> 
> We do a single space after periods.

Perhaps we should put this in the style guide ;-)

> > +    if (index < frameCount() - 1)
> > +        return frameStatus >= kCGImageStatusIncomplete;
> 
> Can this ever be called if the frameCount is 0?

It can’t be called with a frameCount of zero.  I’ll add an assertion that frameCount is not zero since the code doesn’t correctly handle that case.

[Mark Rowe (bdash)]

Landed in r55169.