Closed Bug 222176 Opened 21 years ago Closed 14 years ago

Animated GIF loops 1 time too many

Categories

(Core :: Graphics: ImageLib, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b10

People

(Reporter: aramis1250, Assigned: newstop)

Details

Attachments

(6 files, 7 obsolete files)

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.
This is an animated GIF which demonstrates the issue.
Attached patch fix off-by-one (obsolete) — Splinter Review
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?
Attachment #133345 - Flags: review?(paper)
Paper, come back before we break GIF even more! :)
Attachment #133345 - Attachment is obsolete: true
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?
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.
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
Closed: 19 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: pavlov → nobody
Product: Core → Core Graveyard
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.
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.
Attached patch proposed patch (obsolete) — Splinter Review
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
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: --- → ?
(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+
Attached patch proposed patch, v2 (obsolete) — Splinter Review
Attachment #468932 - Attachment is obsolete: true
Attachment #469134 - Flags: review+
Does it need sr+ or can it be checked in now?
Keywords: checkin-needed
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.
Attachment #469134 - Flags: approval2.0?
status2.0: ? → ---
Attachment #469134 - Flags: approval2.0? → approval2.0+
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. :(
Attached patch proposed patch, v3 (obsolete) — Splinter Review
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+
Attached patch proposed patch, v4 (obsolete) — Splinter Review
bitrot-corrected
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp Hunk #1 FAILED at 138 Hunk #2 FAILED at 863
Keywords: checkin-needed
I can't find what's wrong with my patch...
Attached patch proposed patch, v5 (obsolete) — Splinter Review
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
Attachment #490063 - Flags: review?(joe)
Attachment #490063 - Flags: review?(joe)
Attachment #490063 - Flags: review+
Attachment #490063 - Flags: approval2.0+
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp Hunk #1 FAILED at 140 Hunk #2 FAILED at 865
Keywords: checkin-needed
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.
The same patch, but with with LF line endings.
Attachment #490063 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 19 years ago14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Attached image screenshot
regression. gif (not animation gif) flicker. (loop reload) http://www.kent-web.com/count/daycount.html
(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.
Any simpler examples? I'm not sure this daycount.cgi script generates correct GIFs or not.
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.
in error console, Error: Image corrupt or truncated: <unknown> Source File: <unknown> Line: 0
Attached image 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.
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.
I will need some time to investigate why it worked before, and how it works in other browsers.
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
Attached patch additional patch (obsolete) — Splinter Review
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.
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.
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
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
all fine again. thank you.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: