Bug 39857 – GIFs loop one time too few

• Component: Images
• Status: CLOSED FIXED

[Peter Kasting]

Created attachment 57281 [details]
Image with a loop count of "1"

The Netscape extension to GIF89a that adds a loop count to animated images is interpreted differently by Firefox/IE vs. Safari/Opera.  According to http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/src/imgContainer.cpp (see SetLoopCount()), the loop count is intended to mean "how many times should this loop after animating through once".  Thus a loop count of "1" means that the animation should be displayed a total of 2 times.  Firefox and IE both behave this way.

Safari/Opera, on the other hand, treat the loop count as the number of total times to loop.  Thus they display non-infinitely-looping GIFs with one fewer cycle compared to Firefox/IE.  The attached image specifies a loop count of 1 and thus can be used to compare the browsers' behavior.

Chrome currently matches Safari/Opera; I intend to fix this in a separate bug.  This bug is about fixing Safari.

[Peter Kasting]

Oh hey wait.  Since the fix is in BitmapImage.cpp, it will actually fix things for both Safari and Chrome at the same time.

OK, taking.

[Peter Kasting]

Argh, this is really confusing.

Reading http://mxr.mozilla.org/mozilla-central/source/modules/libpr0n/decoders/gif/nsGIFDecoder2.cpp more:

-1 means "loop infinitely".
0 means "loop infinitely".
anything else means "show n + 1 total times".

The obvious question then is: how would someone specify "loop once"?  The answer is that you can't do it from this value, you just omit the Netscape extension block entirely, and the default is then to loop once.

[Peter Kasting]

Created attachment 57284 [details]
patch v1

This should fix Safari, Chrome, and all other consumers besides Qt, which was already correct.

[WebKit Review Bot]

Attachment 57284 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/graphics/BitmapImage.cpp:283:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/image-decoders/gif/GIFImageReader.cpp:692:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/graphics/cg/ImageSourceCG.cpp:213:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 3 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kasting]

Created attachment 57285 [details]
patch v1.1

Fixes the style errors in BitmapImage.cpp and ImageSourceCG.cpp, but not in GIFImageReader.cpp as that file is not in WebKit style but rather is copied from Mozilla.

[WebKit Review Bot]

Attachment 57285 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/image-decoders/gif/GIFImageReader.cpp:692:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 57285 [details]
patch v1.1

Great catch, really glad you are fixing it.

Why get rid of cAnimationLoopOnce? Changing its value to 0 would make the patch smaller, and I think having a named constant for it is still helpful even if it's only a particular special case of the general rule of "loop once more than this number". I think that changing from the constant to the magic number 0 makes the code less readable.

Please take a crack at doing this without removing cAnimationLoopOnce. The resulting patch should be a bit smaller and it will also be clearer what you are and are not changing.

[Darin Adler]

Comment on attachment 57285 [details]
patch v1.1

We also need a test case for this bug fix. If we can't come up with a DumpRenderTree test, we need a manual test.

[Peter Kasting]

(In reply to comment #7)
> Why get rid of cAnimationLoopOnce? Changing its value to 0 would make the patch smaller, and I think having a named constant for it is still helpful even if it's only a particular special case of the general rule of "loop once more than this number". I think that changing from the constant to the magic number 0 makes the code less readable.

You are right that that would make the patch smaller.  My reason for doing it was that I felt uncomfortable with having a named constant for a value that was handled identically to all the unnamed constants -- it seemed like it would cause readers to believe that had to be handled specially, which wasn't true.  OTOH, it _is_ something of a magic number since the GIF extension can't represent that value, so in that way it makes sense.  I'll change it.

Manual tests go in WebCore/manual-tests/?

[Peter Kasting]

Created attachment 57371 [details]
patch v2

This restores cAnimationLoopOnce, adds more comments, and fixes + makes available for everyone an existing Qt manual test on this.

I am concerned that, based on the comments on bug 36818, Qt may still be buggy with this patch.  If that's true, the fix may need to happen in the Qt image decoders themselves.  I will leave it to a Qt developer to address that (a few of whom are CCed on this bug).

[WebKit Review Bot]

Attachment 57371 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/platform/image-decoders/gif/GIFImageReader.cpp:693:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Peter Kasting]

Fixed in r60375.

[Simon Hausmann]

Revision r60375 cherry-picked into qtwebkit-2.0 with commit 6a7801db0fd3f747a0306f2847d5199823cc0ed7