The default bug view has changed. See this FAQ.

Animated GIF loops 1 time too many

RESOLVED FIXED in mozilla2.0b10

Status

()

Core
ImageLib
RESOLVED FIXED
14 years ago
6 years ago

People

(Reporter: Donovan Warren, Assigned: Max Stepin)

Tracking

Trunk
mozilla2.0b10
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 7 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Comment 1

14 years ago
Created attachment 133291 [details]
animated GIF set to 1x loop

This is an animated GIF which demonstrates the issue.

Comment 2

14 years ago
Created attachment 133345 [details] [diff] [review]
fix off-by-one

Updated

14 years ago
Attachment #133345 - Flags: review?(paper)
Comment on attachment 133345 [details] [diff] [review]
fix off-by-one

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

why not mLoopCount == 1?

Updated

14 years ago
Attachment #133345 - Flags: review?(paper)

Comment 4

14 years ago
Created attachment 133349 [details] [diff] [review]
more fiddling with the fenceposts

Paper, come back before we break GIF even more!  :)
Attachment #133345 - Attachment is obsolete: true

Comment 5

14 years ago
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

14 years ago
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

14 years ago
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 ;)
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/
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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → EXPIRED
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).
Status: RESOLVED → UNCONFIRMED
Resolution: EXPIRED → ---
Assignee: jdunn → pavlov

Updated

10 years ago
Assignee: pavlov → nobody
QA Contact: image.gfx

Updated

7 years ago
Component: Image: Painting → Image: Painting
Product: Core → Core Graveyard
(Assignee)

Comment 11

7 years ago
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;
}
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.
(Assignee)

Comment 13

7 years ago
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.
(Assignee)

Comment 14

7 years ago
Created attachment 468932 [details] [diff] [review]
proposed patch
Thanks for the patch!
Status: UNCONFIRMED → NEW
Component: Image: Painting → ImageLib
Ever confirmed: true
OS: Linux → All
Product: Core Graveyard → Core
QA Contact: image.gfx → imagelib

Updated

7 years ago
Attachment #468932 - Flags: review?(bobbyholley+bmo)
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.
Attachment #468932 - Flags: review?(bobbyholley+bmo) → review+
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.
status2.0: --- → ?
(Assignee)

Comment 18

7 years ago
(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?
(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+
(Assignee)

Comment 20

7 years ago
Created attachment 469134 [details] [diff] [review]
proposed patch, v2
Attachment #468932 - Attachment is obsolete: true
Attachment #469134 - Flags: review+
Does it need sr+ or can it be checked in now?
Keywords: checkin-needed

Updated

7 years ago
Assignee: nobody → newstop
(In reply to comment #21)
> Does it need sr+ or can it be checked in now?

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

Updated

7 years ago
Keywords: checkin-needed

Updated

7 years ago
Attachment #469134 - Flags: approval2.0?
status2.0: ? → ---
Attachment #469134 - Flags: approval2.0? → approval2.0+

Updated

7 years ago
Keywords: checkin-needed
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 270
Hunk #2 FAILED at 960
Keywords: checkin-needed
(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. :(
(Assignee)

Comment 25

7 years ago
Created attachment 470209 [details] [diff] [review]
proposed patch, v3
Attachment #469134 - Attachment is obsolete: true
Attachment #469134 - Attachment is obsolete: false
Comment on attachment 470209 [details] [diff] [review]
proposed patch, v3

r+. This should apply now. a2.0=joe
Attachment #470209 - Flags: review+
(Assignee)

Comment 27

7 years ago
Created attachment 474611 [details] [diff] [review]
proposed patch, v4

bitrot-corrected

Updated

7 years ago
Keywords: checkin-needed
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 138
Hunk #2 FAILED at 863
Keywords: checkin-needed
(Assignee)

Comment 29

7 years ago
I can't find what's wrong with my patch...
(Assignee)

Comment 30

7 years ago
Created attachment 490063 [details] [diff] [review]
proposed patch, v5

Let's try one more time. This one works for me with the current source code.
Attachment #469134 - Attachment is obsolete: true
Attachment #470209 - Attachment is obsolete: true
Attachment #474611 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #490063 - Flags: review?(joe)
Attachment #490063 - Flags: review?(joe)
Attachment #490063 - Flags: review+
Attachment #490063 - Flags: approval2.0+
Keywords: checkin-needed
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 140
Hunk #2 FAILED at 865
Keywords: checkin-needed
(Assignee)

Comment 32

6 years ago
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.
(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.
(Assignee)

Comment 34

6 years ago
Created attachment 505405 [details] [diff] [review]
[checked in] proposed patch, v6

The same patch, but with with LF line endings.
Attachment #490063 - Attachment is obsolete: true

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/8f8108abcff2
Status: NEW → RESOLVED
Last Resolved: 12 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10

Comment 36

6 years ago
Created attachment 506055 [details]
screenshot

regression.

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

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

Comment 37

6 years ago
(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.
(Assignee)

Comment 38

6 years ago
Any simpler examples?

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

Comment 39

6 years ago
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

6 years ago
in error console,

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

Comment 41

6 years ago
Created attachment 506094 [details]
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

6 years ago
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.
(Assignee)

Comment 43

6 years ago
I will need some time to investigate why it worked before, and how it works in other browsers.
(Assignee)

Comment 44

6 years ago
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.
Resolution: FIXED → INCOMPLETE
(Assignee)

Comment 45

6 years ago
Created attachment 506153 [details] [diff] [review]
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.
Attachment #506153 - Flags: review?(joe)
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
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.
Attachment #506153 - Flags: review?(joe)
Attachment #506153 - Flags: review+
Attachment #506153 - Flags: approval2.0+
Comment on attachment 506153 [details] [diff] [review]
additional patch

Removing approval until we get the updated patch with comments.
Attachment #506153 - Flags: approval2.0+
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.
(Assignee)

Comment 49

6 years ago
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.
(Assignee)

Comment 50

6 years ago
Created attachment 506211 [details] [diff] [review]
additional patch, with comments

commented on why we need 1 to be default "animate once" value.
Attachment #506153 - Attachment is obsolete: true
Attachment #506211 - Flags: review?(joe)
Comment on attachment 506211 [details] [diff] [review]
additional patch, with comments

Thanks, Max!
Attachment #506211 - Flags: review?(joe)
Attachment #506211 - Flags: review+
Attachment #506211 - Flags: approval2.0+
Attachment #505405 - Attachment description: proposed patch, v6 → [checked in] proposed patch, v6
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/6149b3a93e1b
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 53

6 years ago
all fine again.

thank you.
You need to log in before you can comment on or make changes to this bug.