Animated GIFs with finite looping are falling one loop short

• Component: Images
• Status: RESOLVED FIXED

Created attachment 334654 [details]
Animated .GIF set to play 3 times.

A GIF created in Photoshop to loop X times is actually playing X-1 times in Safari.
As with the current Chrome release, Webkit is failing to count the initial playback before applying the loop count.

ie
Photoshop (or any GIF) set to play 3 times
Result is GIF with "2 loops"
aka. Intial playback + 2 loops = 3

In Safari for MacOS and iOS, the playback count is failing to include the initial play-through. It falls one loop short.
See Chromium Bug (fix implemented) here: https://bugs.chromium.org/p/chromium/issues/detail?id=815038

These animations play correctly in IE and Firefox browsers

[Radar WebKit Bug Importer]

<rdar://problem/37928153>

This issue is now fixed in Chrome (Canary) and will be rolled out in the upcoming v65 Official Build release. Details available on the previously posted bugs.chromium.org link.

[Said Abou-Hallawa]

I am not convinced that the Chrome behavior and the old Safari are correct.

I opened the attached GIF file as a binary file and I can confirm that the loop count saved in this file is 2. I attached a screenshot showing the image in its binary format. The screenshot highlights the Netscape Looping Application Extension block. The red rectangle is the 2 bytes (the second and the third bytes from the end) representing the animation loop count.

I tried hard to find a detailed description for the Netscape Looping Application Extension block but I could not find enough information. The W3C specs https://www.w3.org/Graphics/GIF/spec-gif89a.txt does not mention it. Wikipedia https://en.wikipedia.org/wiki/GIF has some info but not it is enough. I found this link  http://www.vurdalakov.net/misc/gif/netscape-looping-application-extension useful and it matches what I see in the attached screenshot. But neither this page nor the Wikipedia says that we need to animate the GIF image one extra iteration if the LoopCount is not zero.

What Chrome does to fix this issue was to increment the repetitionCount by 1 if the loopCount is not equal to zero. And this is what WebKit was doing in the past. See the comment above cAnimationLoopOnce from this link: https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/graphics/ImageSource.h?rev=198655.

Also the APNG file format https://wiki.mozilla.org/APNG_Specification does not say, the image should be animated (num_plays + 1) if num_plays is not equal to zero. I think this because APNG does not special case the num_plays == 1 as the GIF does. The GIF image does not include the loopCount in the Netscape Looping Application Extension block if it is equal to 1.

[Said Abou-Hallawa]

Created attachment 337815 [details]
Screen shot (GIF with loopCount=2)

The fact that there is a value for "loop count" infers that an initial single playback is the default behaviour. An animated Gif with three frames set to 0 loops, would still play once (and does). 

Loopcount is not designed to be a total playback count. Else multi-frame GIFs set to loopcount=0 would not play through at all.

The world (ie, Adobe, Google, Microsoft, Mozilla) disagrees with your flawed logic.
Fix your code.

[Said Abou-Hallawa]

(In reply to aedge from comment #5)
> The fact that there is a value for "loop count" infers that an initial
> single playback is the default behaviour. An animated Gif with three frames
> set to 0 loops, would still play once (and does). 
> 

This is not true. I changed your image by setting the LoopCount bytes to "0" (attached) and the image animates indefinitely in Safari, Chrome and FireFox.

> Loopcount is not designed to be a total playback count. Else multi-frame
> GIFs set to loopcount=0 would not play through at all.
> 

My understanding is:

LoopCount = 0, means "animate the frames indefinitely".
LoopCount missing, means "animate the frames only once".
LoopCount > 0, "animate the frames 'LoopCount' times". I could not find any documentation or specs that say, we should animate the frames (LoopCount + 1) times.

> The world (ie, Adobe, Google, Microsoft, Mozilla) disagrees with your flawed
> logic.

I would appreciate if you can provide a single documentation for the correct logic. It will also be helpful if you can provide the same information for the APGN. I tried but I could not.

> Fix your code.

It is just a single line of code in ImageDecoderCG::repetitionCount(). Instead of returning loopCount, I can change it to return loopCount + 1. But I need a proof that your claim is correct.

[Said Abou-Hallawa]

Created attachment 337852 [details]
Animated .GIF set to play indefinitely.

[Jon Lee]

As Said said, a loopCount of 0 represents infinite looping, not 1. That means it is possible that loopCount could mean "loops in addition to the normal 1" or "total loops".

If the expectation is that in order to never have looping you don't include the extension header at all, then the former interpretation seems right. But if no such expectation exists, then the latter is also plausible.

The Chrome bug includes a comment about the difference between official and "street" specs. So it's possible there's just no good documentation of what's expected. For compatibility, we'll want to match the other browsers, but I wish we had a library of looping GIFs to ensure we're on the same page.

I found this old open bug with the same report, but it looks like they addressed it somewhere else:
https://bugzilla.mozilla.org/show_bug.cgi?id=805392

Some additional samples:
http://www.soniacoleman.com/Tutorials/PowerPoint/animated_gifs/gifs.htm

Either way, being a jerk doesn't help get bugs fixed any faster.

Hey folks, Sorry to have been so abrupt, but I thought my indications regarding Adobe, Chrome, Firefox and IE in my initial post would have proven helpful to getting a "standard" playback experience into production sooner for Safari. 
Having waited so long for a response and to have the ticket shut down, despite my research and time spent to document the issue, was a shock to me.
I am not a developer. I have never read the documentation, I do not know the standards, I have made sweeping assumptions about every aspect of my report and I apologise about my ignorance.
That said, I am a graphic designer who has been producing animated GIFs since the mid 90s. They have worked the way I have described they should, for about 25 years. Adobe Photoshop says "play 3 times" and the resulting GIF plays 3 times. Except recently in Google Chrome, where they rewrote everything recently, and made an error. And also, as I have stated, in Safari. I do not know how long Safari has had the issue.

Thanks for setting me straight on the loopcount=0 equals infinite looping logic, Said.
Apologies for guessing the logic at work and leading us down the wrong path.

I believe the Chrome Team may have provided a more complete explanation in their tickets for this issue.
https://chromium-review.googlesource.com/c/chromium/src/+/934968

(In reply to Said Abou-Hallawa from comment #7)

> My understanding is:
> 
> LoopCount = 0, means "animate the frames indefinitely".
> LoopCount missing, means "animate the frames only once".
> LoopCount > 0, "animate the frames 'LoopCount' times". I could not find any
> documentation or specs that say, we should animate the frames (LoopCount +
> 1) times.

I guess my faulty argument about "loopcount=0 and loopcount=1 should not be the same thing" is still the crux of the problem here (though its not loopcount=0, it's "loopcount missing" we should be talking about).

LoopCount missing (resulting in a 'play once' behaviour) and LoopCount=1 should not have an identical result. If they did, we'd need to start a discussion with the image creation tool providers (ie, Adobe) to change they way they count at the file-creation end. This would result in some big issues for online advertisers as Total Run Time is strictly policed in banner advertising.

[Said Abou-Hallawa]

Created attachment 337902 [details]
Patch

[Jon Lee]

Comment on attachment 337902 [details]
Patch

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

> Source/WebCore/ChangeLog:21
> +        the image designer wants the image to loop a GIF "n" times, the GIF generator

which one?

> Source/WebCore/ChangeLog:25
> +        the agreed-upon behavior the specs here.

you should include who is agreeing.

> LayoutTests/ChangeLog:21
> +x

Extra x.

I'd like to see the test augmented to include the gif without the header (go through one), and a gif with it set to 0 (and do 7 or 10 frames, so that more than 2 iterations is our proxy for "infinite").

[Said Abou-Hallawa]

Created attachment 337918 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 337902 [details]
Patch

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

>> Source/WebCore/ChangeLog:21
>> +        the image designer wants the image to loop a GIF "n" times, the GIF generator
> 
> which one?

I added Adobe Photoshop and https://ezgif.com/maker for GIF generators and Chrome 65.0.3325 181 and FireFox Quantum 59.0.2 for GIF viewers. But I disclaim that I tried Adobe Photoshop myself.

>> Source/WebCore/ChangeLog:25
>> +        the agreed-upon behavior the specs here.
> 
> you should include who is agreeing.

I referred to the other browsers Chrome 65.0.3325 181 and FireFox Quantum 59.0.2 which took the same approach.

>> LayoutTests/ChangeLog:21
>> +x
> 
> Extra x.
> 
> I'd like to see the test augmented to include the gif without the header (go through one), and a gif with it set to 0 (and do 7 or 10 frames, so that more than 2 iterations is our proxy for "infinite").

animated-red-green-blue-repeat-1.gif does not contain the Netscape block. This should animate only once.
animated-red-green-blue-repeat-2.gif was changed to have loopCount = 1. This should animate twice with the new patch.
animated-red-green-blue-repeat-inifinte.gif, which has loopCount = 0, was added. This should animate indefinitely. I capture the first frame after the last frame of the second loop. If if the first frame that means the image is animating for the third time. And as you suggested I consider this as a proxy for "infinite".

[Simon Fraser (smfr)]

Comment on attachment 337918 [details]
Patch

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

> Source/WebCore/platform/graphics/cg/ImageDecoderCG.cpp:239
> +    static std::tuple<CFStringRef, CFStringRef, bool> repetitionKeys[] = {
> +        std::make_tuple(kCGImagePropertyGIFDictionary, kCGImagePropertyGIFLoopCount, true),
> +        std::make_tuple(kCGImagePropertyPNGDictionary, WebCoreCGImagePropertyAPNGLoopCount, false)
> +    };

I don't find this tuple stuff much clearer than the existing code, especially as you have to add the hard-to-grok bool at the end.

[Said Abou-Hallawa]

Created attachment 338056 [details]
Patch

[Said Abou-Hallawa]

Created attachment 338057 [details]
Patch

[Said Abou-Hallawa]

Created attachment 338065 [details]
Patch

[EWS Watchlist]

Comment on attachment 338065 [details]
Patch

Attachment 338065 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7337897

New failing tests:
animations/needs-layout.html

[EWS Watchlist]

Created attachment 338070 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Said Abou-Hallawa]

animations/needs-layout.html has been flaky. See https://bugs.webkit.org/show_bug.cgi?id=184638.

[WebKit Commit Bot]

Comment on attachment 338065 [details]
Patch

Clearing flags on attachment: 338065

Committed r230712: <https://trac.webkit.org/changeset/230712>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.