Last Comment Bug 222176 - Animated GIF loops 1 time too many
: Animated GIF loops 1 time too many
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: ImageLib (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla2.0b10
Assigned To: Max Stepin
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-10-14 15:28 PDT by Donovan Warren
Modified: 2011-03-23 05:49 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
animated GIF set to 1x loop (33.45 KB, image/gif)
2003-10-14 15:29 PDT, Donovan Warren
no flags Details
fix off-by-one (957 bytes, patch)
2003-10-15 11:55 PDT, tor
no flags Details | Diff | Review
more fiddling with the fenceposts (1.34 KB, patch)
2003-10-15 13:08 PDT, tor
no flags Details | Diff | Review
proposed patch (932 bytes, patch)
2010-08-24 21:51 PDT, Max Stepin
bobbyholley: review+
Details | Diff | Review
proposed patch, v2 (1.79 KB, patch)
2010-08-25 12:11 PDT, Max Stepin
bobbyholley: review+
joe: approval2.0+
Details | Diff | Review
proposed patch, v3 (1.79 KB, patch)
2010-08-28 12:03 PDT, Max Stepin
bobbyholley: review+
Details | Diff | Review
proposed patch, v4 (1.83 KB, patch)
2010-09-12 21:30 PDT, Max Stepin
no flags Details | Diff | Review
proposed patch, v5 (1.83 KB, patch)
2010-11-12 04:27 PST, Max Stepin
joe: review+
joe: approval2.0+
Details | Diff | Review
[checked in] proposed patch, v6 (1.72 KB, patch)
2011-01-20 07:08 PST, Max Stepin
no flags Details | Diff | Review
screenshot (102.77 KB, image/jpeg)
2011-01-21 19:43 PST, pal-moz
no flags Details
digitalcamera.gif (351 bytes, image/gif)
2011-01-22 03:28 PST, Max Stepin
no flags Details
additional patch (665 bytes, patch)
2011-01-22 15:06 PST, Max Stepin
joe: review+
Details | Diff | Review
additional patch, with comments (757 bytes, patch)
2011-01-23 01:17 PST, Max Stepin
joe: review+
joe: approval2.0+
Details | Diff | Review

Description Donovan Warren 2003-10-14 15:28:40 PDT
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 2003-10-14 15:29:34 PDT
Created attachment 133291 [details]
animated GIF set to 1x loop

This is an animated GIF which demonstrates the issue.
Comment 2 tor 2003-10-15 11:55:20 PDT
Created attachment 133345 [details] [diff] [review]
fix off-by-one
Comment 3 Christian :Biesinger (don't email me, ping me on IRC) 2003-10-15 12:58:46 PDT
Comment on attachment 133345 [details] [diff] [review]
fix off-by-one

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

why not mLoopCount == 1?
Comment 4 tor 2003-10-15 13:08:54 PDT
Created attachment 133349 [details] [diff] [review]
more fiddling with the fenceposts

Paper, come back before we break GIF even more!  :)
Comment 5 ArronM (:paper) 2003-10-15 19:08:15 PDT
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 2003-10-15 20:07:38 PDT
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) 2003-10-15 20:12:03 PDT
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] 2005-09-27 02:02:33 PDT
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] 2005-10-13 10:35:43 PDT
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 [:mwargers] (gone per 2016-05-31 :-( ) 2005-10-21 10:05:35 PDT
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 2010-08-24 03:53:52 PDT
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 [:mwargers] (gone per 2016-05-31 :-( ) 2010-08-24 04:01:12 PDT
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 2010-08-24 21:49:21 PDT
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 2010-08-24 21:51:44 PDT
Created attachment 468932 [details] [diff] [review]
proposed patch
Comment 15 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2010-08-25 02:12:05 PDT
Thanks for the patch!
Comment 16 Bobby Holley (PTO through June 13) 2010-08-25 08:48:57 PDT
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 (PTO through June 13) 2010-08-25 08:54:39 PDT
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 2010-08-25 09:21:25 PDT
(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 (PTO through June 13) 2010-08-25 09:30:28 PDT
(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 2010-08-25 12:11:01 PDT
Created attachment 469134 [details] [diff] [review]
proposed patch, v2
Comment 21 Martijn Wargers [:mwargers] (gone per 2016-05-31 :-( ) 2010-08-26 05:41:15 PDT
Does it need sr+ or can it be checked in now?
Comment 22 Bobby Holley (PTO through June 13) 2010-08-26 08:27:56 PDT
(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] 2010-08-28 04:59:39 PDT
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 270
Hunk #2 FAILED at 960
Comment 24 Bobby Holley (PTO through June 13) 2010-08-28 10:06:25 PDT
(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 2010-08-28 12:03:45 PDT
Created attachment 470209 [details] [diff] [review]
proposed patch, v3
Comment 26 Bobby Holley (PTO through June 13) 2010-08-28 12:17:03 PDT
Comment on attachment 470209 [details] [diff] [review]
proposed patch, v3

r+. This should apply now. a2.0=joe
Comment 27 Max Stepin 2010-09-12 21:30:49 PDT
Created attachment 474611 [details] [diff] [review]
proposed patch, v4

bitrot-corrected
Comment 28 Dão Gottwald [:dao] 2010-09-13 03:40:51 PDT
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 138
Hunk #2 FAILED at 863
Comment 29 Max Stepin 2010-09-13 08:17:32 PDT
I can't find what's wrong with my patch...
Comment 30 Max Stepin 2010-11-12 04:27:40 PST
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.
Comment 31 Dão Gottwald [:dao] 2011-01-07 05:36:31 PST
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 140
Hunk #2 FAILED at 865
Comment 32 Max Stepin 2011-01-07 14:20:55 PST
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] 2011-01-20 05:52:43 PST
(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 2011-01-20 07:08:15 PST
Created attachment 505405 [details] [diff] [review]
[checked in] proposed patch, v6

The same patch, but with with LF line endings.
Comment 35 :Ehsan Akhgari (busy, don't ask for review please) 2011-01-21 12:29:47 PST
http://hg.mozilla.org/mozilla-central/rev/8f8108abcff2
Comment 36 pal-moz 2011-01-21 19:43:48 PST
Created attachment 506055 [details]
screenshot

regression.

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

http://www.kent-web.com/count/daycount.html
Comment 37 pal-moz 2011-01-21 20:16:18 PST
(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 2011-01-22 02:38:05 PST
Any simpler examples?

I'm not sure this daycount.cgi script generates correct GIFs or not.
Comment 39 pal-moz 2011-01-22 03:00:11 PST
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 2011-01-22 03:10:02 PST
in error console,

Error: Image corrupt or truncated: <unknown>
Source File: <unknown>
Line: 0
Comment 41 Max Stepin 2011-01-22 03:28:37 PST
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 pal-moz 2011-01-22 03:50:56 PST
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 2011-01-22 04:17:09 PST
I will need some time to investigate why it worked before, and how it works in other browsers.
Comment 44 Max Stepin 2011-01-22 14:58:15 PST
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 2011-01-22 15:06:44 PST
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.
Comment 46 Joe Drew (not getting mail) 2011-01-22 19:11:30 PST
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) 2011-01-22 19:12:00 PST
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) 2011-01-22 19:13:36 PST
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 2011-01-23 00:51:11 PST
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 2011-01-23 01:17:12 PST
Created attachment 506211 [details] [diff] [review]
additional patch, with comments

commented on why we need 1 to be default "animate once" value.
Comment 51 Joe Drew (not getting mail) 2011-01-23 13:35:05 PST
Comment on attachment 506211 [details] [diff] [review]
additional patch, with comments

Thanks, Max!
Comment 52 :Ehsan Akhgari (busy, don't ask for review please) 2011-01-23 20:35:20 PST
http://hg.mozilla.org/mozilla-central/rev/6149b3a93e1b
Comment 53 pal-moz 2011-01-24 06:17:27 PST
all fine again.

thank you.

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