222176 - Animated GIF loops 1 time too many

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Donovan Warren]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.4) Gecko/20030624

Loading an animated GIF set to loop one time is looping two times.

Reproducible: Always

Steps to Reproduce:
Load an animated GIF image which is set to loop only once, and watch it loop
twice and then stop.




I have checked in IE 5.5 and 6, Safari, Opera, and Netscape 7, and all of these
browsers ran the animated GIF only one time.

Comment 1

[Donovan Warren]

Attachment: animated GIF set to 1x loop

This is an animated GIF which demonstrates the issue.

Comment 2

[tor]

Attachment: fix off-by-one

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 133345 [details] [diff] [review]
fix off-by-one

+      if (mAnimationMode == kLoopOnceAnimMode || (mLoopCount-1) == 0) {

why not mLoopCount == 1?

Comment 4

[tor]

Attachment: more fiddling with the fenceposts

Paper, come back before we break GIF even more!  :)

Comment 5

[ArronM (:paper)]

hmm, linux only? or a misunderstanding perhaps?

GIF is set to loop once.  Which means 2 iterations.    Image plays twice on
Mozilla, NS 7, and IE 6 on my Win2k machine.

So, the question is, is the animation seen 3 times on linux, or just twice?

Comment 6

[tor]

I think we may just be reading the specification differently.  I read it as
"loop once" == the animation plays exactly once (which in mozilla happens
to be the initial decode/display).

The test gif plays twice in an unpatched trunk build.

Comment 7

[ArronM (:paper)]

Yes, it can get confusing.  I'm basing my version of the specs on JASC's
Animation Shop, in which I created an animation that displays twice.  It made a
NETSCAPE2.0 block with a loop count of 1.  So I assume it means, "play once,
then loop (once), the play again"

The version of Operat 7 I have only plays it once however.  I think it's wrong ;)

Comment 8

[Gervase Markham [:gerv]]

This is an automated message, with ID "auto-resolve01".

This bug has had no comments for a long time. Statistically, we have found that
bug reports that have not been confirmed by a second user after three months are
highly unlikely to be the source of a fix to the code.

While your input is very important to us, our resources are limited and so we
are asking for your help in focussing our efforts. If you can still reproduce
this problem in the latest version of the product (see below for how to obtain a
copy) or, for feature requests, if it's not present in the latest version and
you still believe we should implement it, please visit the URL of this bug
(given at the top of this mail) and add a comment to that effect, giving more
reproduction information if you have it.

If it is not a problem any longer, you need take no action. If this bug is not
changed in any way in the next two weeks, it will be automatically resolved.
Thank you for your help in this matter.

The latest beta releases can be obtained from:
Firefox:     http://www.mozilla.org/projects/firefox/
Thunderbird: http://www.mozilla.org/products/thunderbird/releases/1.5beta1.html
Seamonkey:   http://www.mozilla.org/projects/seamonkey/

Comment 9

[Gervase Markham [:gerv]]

This bug has been automatically resolved after a period of inactivity (see above
comment). If anyone thinks this is incorrect, they should feel free to reopen it.

Comment 10

[Martijn Wargers (dead)]

So what's the status of this bug? Is this a valid bug? 
For me the animated image loops twice in IE6 (as current trunk builds of Mozilla
do).

Comment 11

[Max Stepin]

If you look at SetLoopCount(), it's all pretty clear:

nsGIFDecoder2.cpp:
    mImageContainer->SetLoopCount(mGIFStruct.loop_count);

nsPNGDecoder.cpp:
    decoder->mImage->SetLoopCount(num_plays - 1);

RasterImage.cpp:
void RasterImage::SetLoopCount(PRInt32 aLoopCount)
{
  if (mError)
    return;

  // -1  infinite
  //  0  no looping, one iteration
  //  1  one loop, two iterations
  //  ...
  mLoopCount = aLoopCount;
}

Comment 12

[Martijn Wargers (dead)]

Max, can you attach a patch for this?
The question is, what should happen, though? IE8 also plays the image twice. Opera and Chrome plays only once, so I guess it's indeed a bug in Mozilla.

Comment 13

[Max Stepin]

IE is buggy then. Windows internal image viewer plays it once, as well as Opera and Chrome. Logically speaking, there should be an option to play GIF only once. 

So let's treat "1" in GIF the same way we treat "1" in APNG, by playing the animation once.

Let's use SetLoopCount(mGIFStruct.loop_count-1) 
Then the comment in SetLoopCount() where it says "one iteration" will be true.

Comment 14

[Max Stepin]

Attachment: proposed patch

Comment 15

[Martijn Wargers (dead)]

Thanks for the patch!

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 468932 [details] [diff] [review]
proposed patch

(In reply to comment #13)
> Logically speaking, there should be an option to play GIF only once. 
> 
> So let's treat "1" in GIF the same way we treat "1" in APNG, by playing the
> animation once.
> 
> Let's use SetLoopCount(mGIFStruct.loop_count-1) 
> Then the comment in SetLoopCount() where it says "one iteration" will be true.

Great analysis - I agree.

This patch won't apply cleanly to trunk, because I just moved the GIF decoder from decoders/gif/ to decoders/. So please rebase it. Other than that, r=bholley.

Comment 17

[Bobby Holley (:bholley)]

Requesting approval2.0.

This single-line change fixes a correctness issue that we've had in the GIF decoder for years. Different browsers handle the testcase differently, but Max provided compelling reasons for why we're doing it wrong. I just inspected all of the possible consequences of this code, and can vouch for its safety.

The patch was written by Max (community member), so I think we should take it, even though it's not a blocker.

Comment 18

[Max Stepin]

(In reply to comment #16)
> This patch won't apply cleanly to trunk, because I just moved the GIF decoder
> from decoders/gif/ to decoders/.

Will do. I also just realized that while parsing GIF structure, this thing will not be needed (lines 1022):

          /* Zero loop count is infinite animation loop request */
          if (mGIFStruct.loop_count == 0)
            mGIFStruct.loop_count = -1;
  
I'll remove it, ok?

Comment 19

[Bobby Holley (:bholley)]

(In reply to comment #18)

> I also just realized that while parsing GIF structure, this thing will
> not be needed (lines 1022):
> 
>           /* Zero loop count is infinite animation loop request */
>           if (mGIFStruct.loop_count == 0)
>             mGIFStruct.loop_count = -1;
> 
> I'll remove it, ok?

r+

Comment 20

[Max Stepin]

Attachment: proposed patch, v2

Comment 21

[Martijn Wargers (dead)]

Does it need sr+ or can it be checked in now?

Comment 22

[Bobby Holley (:bholley)]

(In reply to comment #21)
> Does it need sr+ or can it be checked in now?

Does not need sr+, but needs a2.0.

Comment 23

[Dão Gottwald [:dao]]

patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 270
Hunk #2 FAILED at 960

Comment 24

[Bobby Holley (:bholley)]

(In reply to comment #23)
> patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
> Hunk #1 FAILED at 270
> Hunk #2 FAILED at 960

Ah, sorry max, probably needs rebasing after some more recent changes I made to the gif decoder. :(

Comment 25

[Max Stepin]

Attachment: proposed patch, v3

Comment 26

[Bobby Holley (:bholley)]

Comment on attachment 470209 [details] [diff] [review]
proposed patch, v3

r+. This should apply now. a2.0=joe

Comment 27

[Max Stepin]

Attachment: proposed patch, v4

bitrot-corrected

Comment 28

[Dão Gottwald [:dao]]

patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 138
Hunk #2 FAILED at 863

Comment 29

[Max Stepin]

I can't find what's wrong with my patch...

Comment 30

[Max Stepin]

Attachment: proposed patch, v5

Let's try one more time. This one works for me with the current source code.

Comment 31

[Dão Gottwald [:dao]]

patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 140
Hunk #2 FAILED at 865

Comment 32

[Max Stepin]

Ah, this problem again...?

I run that command inside mozilla-central directory, and it works fine:
patch.exe -p1 --input=v5-222176.diff

I don't have a lot of experience making patches, so I would appreciate an advice on this.

Comment 33

[Dão Gottwald [:dao]]

(In reply to comment #32)
> Ah, this problem again...?
> 
> I run that command inside mozilla-central directory, and it works fine:
> patch.exe -p1 --input=v5-222176.diff
> 
> I don't have a lot of experience making patches, so I would appreciate an
> advice on this.

It looks like the patch contains DOS line endings, maybe that's the problem.

Comment 34

[Max Stepin]

Attachment: [checked in] proposed patch, v6

The same patch, but with with LF line endings.

Comment 35

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/8f8108abcff2

Comment 36

[pal-moz]

Attachment: screenshot

regression.

gif (not animation gif) flicker. (loop reload)

http://www.kent-web.com/count/daycount.html

Comment 37

[pal-moz]

(In reply to comment #36)
> Created attachment 506055 [details]
> screenshot
> 
> regression.
> 
> gif (not animation gif) flicker. (loop reload)
> 
> http://www.kent-web.com/count/daycount.html

same with 4.0 beta 10 candidate build 1

should be fixed. or backout the patch.

Comment 38

[Max Stepin]

Any simpler examples?

I'm not sure this daycount.cgi script generates correct GIFs or not.

Comment 39

[pal-moz]

gif files are not generated files, they are in folder.


"file directory"(example) is
public_html / index.html
          |
          +-- daycount / daycount.cgi
                 |       daycount.dat
                 |       gifcat.pl
                 |
                 +-- gif1 / 0.gif, 1.gif, ... 9.gif
                 |
                 +-- gif2 / 0.gif, 1.gif, ... 9.gif


and another example is
http://www.digitalcamera.jp/
see top-left counter.

this site use http://www.kent-web.com/count/dream.html
download/unzip http://www.kent-web.com/cgi/down.cgi?name=DreamCounter&file=zip
there is "gif1" folder and gif files are in this folder.

Comment 40

[pal-moz]

in error console,

Error: Image corrupt or truncated: <unknown>
Source File: <unknown>
Line: 0

Comment 41

[Max Stepin]

Attachment: digitalcamera.gif

Counter GIF saved from http://www.digitalcamera.jp/

It doesn't have NETSCAPE2.0 block, so I guess it means it *should* be in infinite loop.

Comment 42

[pal-moz]

then you think this is intended behavior ?

before this checkin, all fine. no flicker.
try with build without this checkin, if you can.

many user use this script in Japan.
so if this is not fixed, (many) new bug will be filed.

Comment 43

[Max Stepin]

I will need some time to investigate why it worked before, and how it works in other browsers.

Comment 44

[Max Stepin]

OK, that GIF is unusual - with multiple frames, but NETSCAPE2.0 block is missing.
In all browsers I tried such GIFs animate just once. 
So we would want to be consistent, and keep that behavior.

Comment 45

[Max Stepin]

Attachment: additional patch

In the rare case when NETSCAPE2.0 block is missing, we want default mGIFStruct.loop_count value to be initialized as "animate once", to keep the current behavior.

Comment 46

[Joe Drew (not getting mail)]

Comment on attachment 506153 [details] [diff] [review]
additional patch

It's not clear to me how the previous patch removed the "loop once" behaviour if no NETSCAPE2.0 extension existed. Can you explain that to me?

Also, we should include a comment saying why loop_count is 1 by default.

Comment 47

[Joe Drew (not getting mail)]

Comment on attachment 506153 [details] [diff] [review]
additional patch

Removing approval until we get the updated patch with comments.

Comment 48

[Joe Drew (not getting mail)]

Oh, and finally, we should work out some way of testing this behaviour, both the NETSCAPE2.0 case (ensure gifs don't loop 1 time too many) and the non-NETSCAPE2.0 case (ensure only one loop). I'm ok with it if that's a new bug with a followup patch.

Comment 49

[Max Stepin]

That's how it worked before: mGIFStruct.loop_count was initialized as zero by clearing the whole mGIFStruct, so it would stay zero if no NETSCAPE2.0 found.

Then this line:
  mImage->SetLoopCount(mGIFStruct.loop_count);
would set RasterImage::mLoopCount as zero, which means "animate once".

But since the previous patch changed this line to
  mImage->SetLoopCount(mGIFStruct.loop_count - 1);
we need to compensate by initializing mGIFStruct.loop_count default as 1.

Comment 50

[Max Stepin]

Attachment: additional patch, with comments

commented on why we need 1 to be default "animate once" value.

Comment 51

[Joe Drew (not getting mail)]

Comment on attachment 506211 [details] [diff] [review]
additional patch, with comments

Thanks, Max!

Comment 52

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/6149b3a93e1b

Comment 53

[pal-moz]

all fine again.

thank you.