Bug 205640 – An animated PNG plays the frames one time more than the image loopCount

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

Created attachment 386515 [details]
have to be repeated twice

APNG files are played once more than configured.

The attached file has to be repeated twice, and it does in other browsers.
It is repeated 3 times in Safari.

[Radar WebKit Bug Importer]

<rdar://problem/58258824>

[Said Abou-Hallawa]

Created attachment 394139 [details]
Patch

[Darin Adler]

Comment on attachment 394139 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Make the repetitionCount calculation for GIFs different from it for other
> +        image formats.

This seems to be a workaround for a bug in the values of the property "LoopCount" as set by the ImageIO framework. It’s OK to have a workaround in WebKit, but the whole point of this property and framework is to abstract differences between image types.

Ideally we should get this fixed in ImageIO some day and remove our workaround.

> Source/WebCore/platform/graphics/cg/UTIRegistry.h:38
> +bool iGIFImageType(const String&);

Any new function like this should use StringView instead of const String&.

Also, there’s a missing "s" here in "is".

[Said Abou-Hallawa]

Created attachment 394148 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 394139 [details]
Patch

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

>> Source/WebCore/ChangeLog:9
>> +        image formats.
> 
> This seems to be a workaround for a bug in the values of the property "LoopCount" as set by the ImageIO framework. It’s OK to have a workaround in WebKit, but the whole point of this property and framework is to abstract differences between image types.
> 
> Ideally we should get this fixed in ImageIO some day and remove our workaround.

Yes fixing the issue in ImageIO will be ideal. However I am not sure if our request this change will be accepted or not especially it is a backward compatibility issue.

>> Source/WebCore/platform/graphics/cg/UTIRegistry.h:38
>> +bool iGIFImageType(const String&);
> 
> Any new function like this should use StringView instead of const String&.
> 
> Also, there’s a missing "s" here in "is".

Fixed.

[Darin Adler]

Comment on attachment 394139 [details]
Patch

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

>>> Source/WebCore/ChangeLog:9
>>> +        image formats.
>> 
>> This seems to be a workaround for a bug in the values of the property "LoopCount" as set by the ImageIO framework. It’s OK to have a workaround in WebKit, but the whole point of this property and framework is to abstract differences between image types.
>> 
>> Ideally we should get this fixed in ImageIO some day and remove our workaround.
> 
> Yes fixing the issue in ImageIO will be ideal. However I am not sure if our request this change will be accepted or not especially it is a backward compatibility issue.

Two thoughts on that:

1) I don’t mind being unsure as long as we still take next steps to see if it can be fixed; I don’t want to assume this will fail and thus not even ask. Often I write the workaround *after* hearing from the people who I report bugs to.

2) Yes any behavior change can create a compatibility issue. On the other hand, how could any client be getting correct behavior if they are using "LoopCount"? It seems highly likely that they have the same kind of bug with non-GIF animated images. If the change is to the animated PNG case, and not the GIF case (since that is *much* less likely to be tested), the "incompatibility" is likely to simply be a bug, fixed. And the traditional tools can be used to mitigate compatibility as well, such as linked on or after checks.

So I think we need to think of this as a bug workaround.

[Darin Adler]

Comment on attachment 394148 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/UTIRegistry.cpp:105
> +bool isGIFImageType(const StringView& imageType)

Should be just StringView; actually more efficient than const&.

> Source/WebCore/platform/graphics/cg/UTIRegistry.h:38
> +bool isGIFImageType(const StringView&);

Should be just StringView.

[Said Abou-Hallawa]

Created attachment 394189 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 394139 [details]
Patch

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

>>>> Source/WebCore/ChangeLog:9
>>>> +        image formats.
>>> 
>>> This seems to be a workaround for a bug in the values of the property "LoopCount" as set by the ImageIO framework. It’s OK to have a workaround in WebKit, but the whole point of this property and framework is to abstract differences between image types.
>>> 
>>> Ideally we should get this fixed in ImageIO some day and remove our workaround.
>> 
>> Yes fixing the issue in ImageIO will be ideal. However I am not sure if our request this change will be accepted or not especially it is a backward compatibility issue.
> 
> Two thoughts on that:
> 
> 1) I don’t mind being unsure as long as we still take next steps to see if it can be fixed; I don’t want to assume this will fail and thus not even ask. Often I write the workaround *after* hearing from the people who I report bugs to.
> 
> 2) Yes any behavior change can create a compatibility issue. On the other hand, how could any client be getting correct behavior if they are using "LoopCount"? It seems highly likely that they have the same kind of bug with non-GIF animated images. If the change is to the animated PNG case, and not the GIF case (since that is *much* less likely to be tested), the "incompatibility" is likely to simply be a bug, fixed. And the traditional tools can be used to mitigate compatibility as well, such as linked on or after checks.
> 
> So I think we need to think of this as a bug workaround.

I agree it is a workaround. But I think it is a workaround for a lack of specs and bad file format design. Please see https://bugs.webkit.org/show_bug.cgi?id=183153 and <rdar://problem/60733364>.

When I said I am not sure if ImageIO should take care of removing this discrepancy I said that because I am still not sure what is the right thing to be done here:

1. Should the GIF creators (like Adobe PhotoShop) add the loopCount always to the GIF and do not depend on the notion "No loopCount => LoopCount = 1"?
2. Should the GIF decoders (like ImageIO) hide the discrepancy of the GIF images and always return a loopCount even if it does not exist in the file and adds the extra one if LoopCount > 0?
3. Should the GIF viewers (like WebKit) handles the cases of the loopCount for the GIF different from the other image formats?

I think the problem is there is no clear documentation for the animated GIF. It looks the animation part was added incrementally to the GIF image at a time there was no standardization. Multiple frames were added and the assumption was the animation would be played only once. Then the Netscape Looping Application Extension block was added to give more control over the animation. But because it had to be backward compatible, the loopCount = 1 should have been handled the same way it was handled before adding this Netscape block. And to have a single representation for loopCount = 1, an extra one is assumed to be added to the value stored in the file.

[Darin Adler]

(In reply to Said Abou-Hallawa from comment #9)
> 1. Should the GIF creators (like Adobe PhotoShop) add the loopCount always
> to the GIF and do not depend on the notion "No loopCount => LoopCount = 1"?

No.

> 2. Should the GIF decoders (like ImageIO) hide the discrepancy of the GIF
> images and always return a loopCount even if it does not exist in the file
> and adds the extra one if LoopCount > 0?

Yes, ImageIO needs to normalize loop counts so ImageIO clients do not have write format-specific code to handle looping images

I don’t know about other GIF decoders.

> 3. Should the GIF viewers (like WebKit) handles the cases of the loopCount
> for the GIF different from the other image formats?

Code like WebKit needs to handle each type of looping image correctly. Whether it needs to handle various cases depends on the behavior of the image decoding layer it uses. If it’s using one like ImageIO that strives for format-indepedence, then the goal would be to have no GIF-format-specific code.

> I think the problem is there is no clear documentation for the animated GIF.
> It looks the animation part was added incrementally to the GIF image at a
> time there was no standardization.

Yes, but that happened over 10 years ago. I am not sure why that historical fact is relevant to fixing this bug in handling of non-GIF looping images.

[EWS]

Committed r258817: <https://trac.webkit.org/changeset/258817>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 394189 [details].