Bug 54919 - WebCore should retrieve unclamped frame delays from ImageIO
Summary: WebCore should retrieve unclamped frame delays from ImageIO
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.6
: P2 Normal
Assignee: Mark Rowe (bdash)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-21 17:37 PST by Mark Rowe (bdash)
Modified: 2011-02-21 18:16 PST (History)
1 user (show)

See Also:


Attachments
Patch (3.20 KB, patch)
2011-02-21 17:44 PST, Mark Rowe (bdash)
darin: 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) 2011-02-21 17:37:46 PST
In bug 26455 we changed WebCore’s animated GIF frame delay clamping to match Firefox.  However, ImageIO implements its own frame delay clamping on the value it returns for the kCGImagePropertyGIFDelayTime. We should switch to retrieving the raw, unclamped frame delay from ImageIO so that we really do match Firefox when using ImageIO.
Comment 1 Mark Rowe (bdash) 2011-02-21 17:44:10 PST
Created attachment 83246 [details]
Patch
Comment 2 Darin Adler 2011-02-21 17:48:18 PST
Comment on attachment 83246 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83246&action=review

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:319
> +            // Look for the unclamped frame delay if it exists.

The phrasing here is awkward. We always look for it, but only find it if it exists.
Comment 3 Alexey Proskuryakov 2011-02-21 17:49:55 PST
Comment on attachment 83246 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83246&action=review

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:46
> +static const CFStringRef kCGImagePropertyGIFUnclampedDelayTime = CFSTR("UnclampedDelayTime");

I suggest omitting the unnecessary "static".

I see kCGImagePropertyGIFUnclampedDelayTime in some versions of public Snow Leopard headers. Will this break the build for those who have a new version?

> Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:324
> +                CFNumberGetValue(num, kCFNumberFloatType, &duration);
> +
> +            // Fall back to the clamped frame delay if the unclamped frame delay does not exist.
> +            else if (CFNumberRef num = (CFNumberRef)CFDictionaryGetValue(typeProperties, kCGImagePropertyGIFDelayTime))

The blank line could confuse someone. Maybe move comments inside the block (and add braces then)?
Comment 4 Mark Rowe (bdash) 2011-02-21 18:08:07 PST
(In reply to comment #3)
> (From update of attachment 83246 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83246&action=review
> 
> > Source/WebCore/platform/graphics/cg/ImageSourceCG.cpp:46
> > +static const CFStringRef kCGImagePropertyGIFUnclampedDelayTime = CFSTR("UnclampedDelayTime");
> 
> I suggest omitting the unnecessary "static".
> 
> I see kCGImagePropertyGIFUnclampedDelayTime in some versions of public Snow Leopard headers. Will this break the build for those who have a new version?


It will.  I’ll change this to always provide and use our own version of this constant.
Comment 5 Mark Rowe (bdash) 2011-02-21 18:16:36 PST
Fixed in r79277.