Bug 35287 - ImageSourceCG::frameIsCompleteAtIndex returns false for all frames until image has completed loading
Summary: ImageSourceCG::frameIsCompleteAtIndex returns false for all frames until imag...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.6
: P2 Normal
Assignee: Nobody
URL: http://i.imgur.com/Zgc5p.gif
Keywords:
Depends on:
Blocks: 35115
  Show dependency treegraph
 
Reported: 2010-02-23 01:45 PST by Mark Rowe (bdash)
Modified: 2010-02-23 14:01 PST (History)
4 users (show)

See Also:


Attachments
Patch (4.09 KB, patch)
2010-02-23 10:36 PST, Mark Rowe (bdash)
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Rowe (bdash) 2010-02-23 01:45:50 PST
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).
Comment 1 Mark Rowe (bdash) 2010-02-23 10:36:49 PST
Created attachment 49304 [details]
Patch
Comment 2 Mark Rowe (bdash) 2010-02-23 10:39:10 PST
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.
Comment 3 Geoffrey Garen 2010-02-23 11:08:54 PST
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.
Comment 4 Darin Adler 2010-02-23 11:30:27 PST
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?
Comment 5 Mark Rowe (bdash) 2010-02-23 12:10:58 PST
(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>.
Comment 6 Mark Rowe (bdash) 2010-02-23 12:14:46 PST
(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.
Comment 7 Mark Rowe (bdash) 2010-02-23 14:01:43 PST
Landed in r55169.