1255497 - GIF animation loops once less than other browsers

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Urvang]

Attachment: full2loop.gif

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.116 Safari/537.36

Steps to reproduce:

- Open attached GIF in Firefox.
- Observe the number of times this animation loops (repeats).


Actual results:

Animation loops 2 times.


Expected results:

Animation should loop 3 times.

I tested on in Chromium (Chrome & Opera), Internet explorer and Safari -- all of these loop this animation 3 times.

More info here: https://bugs.chromium.org/p/chromium/issues/detail?id=592735#c5

Comment 1

[Loic]

It doesn't work in very old versions of Firefox like 12, old bug and likely a dupe.

Comment 2

[Peter Kasting]

This is not a dupe.  Read the linked comment ( https://bugs.chromium.org/p/chromium/issues/detail?id=592735#c5 ) for detail.

This is a case where Firefox' behavior changed a couple of years ago, but doesn't match any other browser or native image viewer that we can find, and should be changed back for web compat reasons.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Sheesh. This behavior makes no sense, but if everyone else is doing it, we should match it for web compat. Let's make the change.

Comment 4

[Max Stepin]

I guess the patch I made for bug 222176 was based on incorrect assumptions about the meaning of repetitions/loops, so I support reverting it back. Now everyone seems to agree on this interpretation:

Value 0 in "NETSCAPE2.0" means loop forever.
Missing "NETSCAPE2.0" block means 0 repetitions = loop once.
Value 1 in "NETSCAPE2.0" means 1 repetition = 2 loops.
Value 2 in "NETSCAPE2.0" means 2 repetitions = 3 loops.

Comment 5

[Max Stepin]

*play* once.

Comment 6

[Timothy Nikkel (:tnikkel)]

Max, do you want to take this?

Comment 7

[Max Stepin]

I'll take a look.

Comment 8

[Max Stepin]

Attachment: v01.patch

Reverting the changes I made in bug 222176.

Comment 10

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8735303 [details] [diff] [review]
v01.patch

Review of attachment 8735303 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Max!

::: image/decoders/nsGIFDecoder2.cpp
@@ +808,5 @@
>            // loop count during the first iteration.
>            mGIFStruct.loop_count = GETINT16(q + 1);
> +
> +          // Zero loop count is infinite animation loop request
> +          if (mGIFStruct.loop_count == 0)

I realize this is just a revert, but it'd be nice to have braces around this |if| block per current Mozilla style.

Comment 11

[Max Stepin]

Attachment: v02.patch

style fix

Comment 12

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8736869 [details] [diff] [review]
v02.patch

Review of attachment 8736869 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 13

[Max Stepin]

Attachment: v03.patch

Patch updated to reflect recent changes in original nsGIFDecoder2.cpp

Comment 14

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8762375 [details] [diff] [review]
v03.patch

Review of attachment 8762375 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, Max. You don't need to rerequest review for the tiny change below. Please just upload a revised patch and then add "checkin-needed" to the Keywords field at the top of the bug. This will let the sheriffs know that your patch is ready to be landed. I'm sorry I didn't point that out last time.

::: image/decoders/nsGIFDecoder2.cpp
@@ +738,5 @@
>      case NETSCAPE_LOOPING_EXTENSION_SUB_BLOCK_ID:
>        // This is looping extension.
>        mGIFStruct.loop_count = LittleEndian::readUint16(aData + 1);
> +
> +      // Zero loop count is infinite animation loop request

Tiny nit: please add a period at the end of this comment.

Comment 15

[Max Stepin]

Attachment: v04.patch

Comment fix

Comment 16

[Max Stepin]

(In reply to Seth Fowler [:seth] [:s2h] from comment #14)

> and then add "checkin-needed" to the
> Keywords field at the top of the bug.

Sorry, can't figure out how to add keywords...

Comment 17

[Emanuel Hoogeveen [:ehoogeveen]]

Do you have the editbugs permission on Bugzilla? Unfortunately that's required for both of those changes.

Comment 18

[Max Stepin]

Attachment: v05.patch

OK, let's try one more time.
Same patch, recreated on top of the current codebase.

Comment 19

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8867353 [details] [diff] [review]
v05.patch

Thanks!

Do you need someone to land this?

Comment 21

[Max Stepin]

Yes!

Land it if you can.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f67316c8b3d5
Change the number of iterations we do for finite length animated gif to match what everyone else does. r=seth

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f67316c8b3d5

Comment 24

[Farhadur Reja Fahim]

I have reproduced this bug with Nightly 48.0a1 (2016-03-10) on Ubuntu 16.04, 64 bit!

The bug's fix is now verified on latest Beta 55.0b2

Build ID 	20170615133456
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday 20170614]

Comment 25

[Saheda Reza [:Antora]]

I have reproduced this bug on Nightly according to (2016-03-10)

Fixing bug is verified on Latest Beta -- Build ID: 20170615063713 , User Agent: Mozilla/5.0 (Windows NT 6.1; rv:55.0) Gecko/20100101 Firefox/55.0

Tested OS-- Windows7 32bit
[bugday-20170614]

Comment 26

[Mihai Boldan, Desktop QA [:mboldan]]

Thanks  Saheda Reza for testing this.
I also confirm that the issue is no longer reproducible on Firefox 56.0a1 (2017-06-20), or on Firefox 55.0b3. 
Tests were performed under Ubuntu 16.04x64 and under Mac OS X 10.12.5.