Closed Bug 1255497 Opened 6 years ago Closed 4 years ago

GIF animation loops once less than other browsers


(Core :: ImageLib, defect)

44 Branch
Not set



Tracking Status
firefox55 --- verified
firefox56 --- verified


(Reporter: urvang2005, Assigned: newstop)



(Whiteboard: gfx-noted)


(2 files, 4 obsolete files)

Attached image 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:
Component: Untriaged → ImageLib
Product: Firefox → Core
It doesn't work in very old versions of Firefox like 12, old bug and likely a dupe.
Whiteboard: DUPEME
This is not a dupe.  Read the linked comment ( ) 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.
Ever confirmed: true
Whiteboard: DUPEME
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.
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.
*play* once.
Max, do you want to take this?
Flags: needinfo?(newstop)
I'll take a look.
Flags: needinfo?(newstop)
Attached patch v01.patch (obsolete) — Splinter Review
Reverting the changes I made in bug 222176.
Attachment #8735303 - Flags: review?(seth)
Duplicate of this bug: 1236573
Comment on attachment 8735303 [details] [diff] [review]

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.
Attachment #8735303 - Flags: review?(seth) → review+
Attached patch v02.patch (obsolete) — Splinter Review
style fix
Attachment #8735303 - Attachment is obsolete: true
Attachment #8736869 - Flags: review?(seth)
Comment on attachment 8736869 [details] [diff] [review]

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

Attachment #8736869 - Flags: review?(seth) → review+
Attached patch v03.patch (obsolete) — Splinter Review
Patch updated to reflect recent changes in original nsGIFDecoder2.cpp
Attachment #8736869 - Attachment is obsolete: true
Attachment #8762375 - Flags: review?(seth)
Comment on attachment 8762375 [details] [diff] [review]

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 @@
>        // 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.
Attachment #8762375 - Flags: review?(seth) → review+
Attached patch v04.patch (obsolete) — Splinter Review
Comment fix
Attachment #8762375 - Attachment is obsolete: true
(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...
Do you have the editbugs permission on Bugzilla? Unfortunately that's required for both of those changes.
Attached patch v05.patchSplinter Review
OK, let's try one more time.
Same patch, recreated on top of the current codebase.
Attachment #8763766 - Attachment is obsolete: true
Attachment #8867353 - Flags: review?(tnikkel)
Comment on attachment 8867353 [details] [diff] [review]


Do you need someone to land this?
Attachment #8867353 - Flags: review?(tnikkel) → review+
Duplicate of this bug: 1364446

Land it if you can.
Pushed by
Change the number of iterations we do for finite length animated gif to match what everyone else does. r=seth
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
QA Whiteboard: [good first verify]
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]
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
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.
Assignee: nobody → newstop
You need to log in before you can comment on or make changes to this bug.