Closed
Bug 222176
Opened 21 years ago
Closed 14 years ago
Animated GIF loops 1 time too many
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
RESOLVED
FIXED
mozilla2.0b10
People
(Reporter: aramis1250, Assigned: newstop)
Details
Attachments
(6 files, 7 obsolete files)
33.45 KB,
image/gif
|
Details | |
1.34 KB,
patch
|
Details | Diff | Splinter Review | |
1.72 KB,
patch
|
Details | Diff | Splinter Review | |
102.77 KB,
image/jpeg
|
Details | |
351 bytes,
image/gif
|
Details | |
757 bytes,
patch
|
joe
:
review+
joe
:
approval2.0+
|
Details | Diff | Splinter Review |
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•21 years ago
|
||
This is an animated GIF which demonstrates the issue.
Attachment #133345 -
Flags: review?(paper)
Comment 3•21 years ago
|
||
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
Comment 5•21 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?
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•21 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 ;)
Comment 8•19 years ago
|
||
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•19 years ago
|
||
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
Comment 10•19 years ago
|
||
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•18 years ago
|
Assignee: pavlov → nobody
QA Contact: image.gfx
Assignee | ||
Comment 11•14 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;
}
Comment 12•14 years ago
|
||
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•14 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•14 years ago
|
||
Comment 15•14 years ago
|
||
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•14 years ago
|
Attachment #468932 -
Flags: review?(bobbyholley+bmo)
Comment 16•14 years ago
|
||
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+
Comment 17•14 years ago
|
||
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.
Assignee | ||
Comment 18•14 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?
Comment 19•14 years ago
|
||
(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•14 years ago
|
||
Attachment #468932 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #469134 -
Flags: review+
Updated•14 years ago
|
Assignee: nobody → newstop
Comment 22•14 years ago
|
||
(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•14 years ago
|
Keywords: checkin-needed
Updated•14 years ago
|
Attachment #469134 -
Flags: approval2.0?
Updated•14 years ago
|
Updated•14 years ago
|
Attachment #469134 -
Flags: approval2.0? → approval2.0+
Updated•14 years ago
|
Keywords: checkin-needed
Comment 23•14 years ago
|
||
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 270
Hunk #2 FAILED at 960
Keywords: checkin-needed
Comment 24•14 years ago
|
||
(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•14 years ago
|
||
Attachment #469134 -
Attachment is obsolete: true
Updated•14 years ago
|
Attachment #469134 -
Attachment is obsolete: false
Comment 26•14 years ago
|
||
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•14 years ago
|
||
bitrot-corrected
Updated•14 years ago
|
Keywords: checkin-needed
Comment 28•14 years ago
|
||
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 138
Hunk #2 FAILED at 863
Keywords: checkin-needed
Assignee | ||
Comment 29•14 years ago
|
||
I can't find what's wrong with my patch...
Assignee | ||
Comment 30•14 years ago
|
||
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•14 years ago
|
Attachment #490063 -
Flags: review?(joe)
Updated•14 years ago
|
Attachment #490063 -
Flags: review?(joe)
Attachment #490063 -
Flags: review+
Attachment #490063 -
Flags: approval2.0+
Updated•14 years ago
|
Keywords: checkin-needed
Comment 31•14 years ago
|
||
patching file modules/libpr0n/decoders/nsGIFDecoder2.cpp
Hunk #1 FAILED at 140
Hunk #2 FAILED at 865
Keywords: checkin-needed
Assignee | ||
Comment 32•14 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.
Comment 33•14 years ago
|
||
(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•14 years ago
|
||
The same patch, but with with LF line endings.
Attachment #490063 -
Attachment is obsolete: true
Updated•14 years ago
|
Keywords: checkin-needed
Comment 35•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 19 years ago → 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b10
Comment 36•14 years ago
|
||
regression.
gif (not animation gif) flicker. (loop reload)
http://www.kent-web.com/count/daycount.html
Comment 37•14 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•14 years ago
|
||
Any simpler examples?
I'm not sure this daycount.cgi script generates correct GIFs or not.
Comment 39•14 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•14 years ago
|
||
in error console,
Error: Image corrupt or truncated: <unknown>
Source File: <unknown>
Line: 0
Assignee | ||
Comment 41•14 years ago
|
||
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•14 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•14 years ago
|
||
I will need some time to investigate why it worked before, and how it works in other browsers.
Assignee | ||
Comment 44•14 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•14 years ago
|
||
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)
Updated•14 years ago
|
Status: RESOLVED → REOPENED
Resolution: INCOMPLETE → ---
Comment 46•14 years ago
|
||
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 47•14 years ago
|
||
Comment on attachment 506153 [details] [diff] [review]
additional patch
Removing approval until we get the updated patch with comments.
Attachment #506153 -
Flags: approval2.0+
Comment 48•14 years ago
|
||
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•14 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•14 years ago
|
||
commented on why we need 1 to be default "animate once" value.
Attachment #506153 -
Attachment is obsolete: true
Attachment #506211 -
Flags: review?(joe)
Comment 51•14 years ago
|
||
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+
Updated•14 years ago
|
Attachment #505405 -
Attachment description: proposed patch, v6 → [checked in] proposed patch, v6
Updated•14 years ago
|
Keywords: checkin-needed
Comment 52•14 years ago
|
||
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Comment 53•14 years ago
|
||
all fine again.
thank you.
You need to log in
before you can comment on or make changes to this bug.
Description
•