Bug 54919 – WebCore should retrieve unclamped frame delays from ImageIO

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mark Rowe (bdash)]

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.

[Mark Rowe (bdash)]

Created attachment 83246 [details]
Patch

[Darin Adler]

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.

[Alexey Proskuryakov]

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

[Mark Rowe (bdash)]

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

[Mark Rowe (bdash)]

Fixed in r79277.