Closed
Bug 386269
Opened 17 years ago
Closed 17 years ago
GIF animation throttling doesn't match IE/Safari
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
VERIFIED
FIXED
People
(Reporter: tor, Unassigned)
References
Details
Attachments
(3 files)
(deleted),
patch
|
pavlov
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
pavlov
:
review+
|
Details | Diff | Splinter Review |
Recently the checkin for bug 196295 fixed a bug that disabled the GIF-specific animation speed throttling that has been broken for some time now (at least back to FF2). Now that we're throttling again, we should probably match IE and Safari behavior, which maps <=50ms to 100ms.
See also: http://bugs.webkit.org/show_bug.cgi?id=14413
Attachment #270247 -
Flags: review?(pavlov)
Updated•17 years ago
|
Attachment #270247 -
Flags: review?(pavlov) → review+
Comment 2•17 years ago
|
||
Another place where throttling is happening:
http://lxr.mozilla.org/mozilla/source/gfx/src/shared/gfxImageFrame.cpp#486
486 if (mTimeout >= 0 && mTimeout <= 10)
487 *aTimeout = 100;
This should be made consistent with the clipping in nsGIFDecoder.
(otherwise APNG will clip below 10, and AGIF below 50).
Comment 3•17 years ago
|
||
It's ok for APNG and AGIF to clip at different places, IMO. AGIF only clips in web browsers for historical/compatibility reasons, and it breaks some uses of AGIFs.
Comment 4•17 years ago
|
||
Er... the version we had (throttle things under 10ms to 100ms) was there for a reason. Please read the long discussions that led to that choice before changing to a different value.
In brief, the IE 50ms thing is too high and actually breaks some sites. We don't want to be doing that.
Flags: in-testsuite?
Flags: blocking1.9?
Comment 5•17 years ago
|
||
Note that I would have assumed that reading the bugs that are linked in the CVS blame is standard operating procedure for situations like this...
What's the status of this bug? Not sure if what was checked in needs to be revised/backed out, or what...
Flags: blocking1.9? → blocking1.9+
Comment 7•17 years ago
|
||
I think it awaits SR (but who?).
The patch that needs to go in is the last one.
The discussion is whether to use:
#define MINIMUM_DELAY_THRESHOLD 50
or to use:
#define MINIMUM_DELAY_THRESHOLD 10
The first is for Opera and others, but second is what it used to be in Mozilla/Firefox.
(In reply to comment #4)
> Er... the version we had (throttle things under 10ms to 100ms) was there for a
> reason. Please read the long discussions that led to that choice before
> changing to a different value.
There was a <10->100ms mapping since almost the beginning of the GIF decoder, but that was quietly changed to <100ms->100ms as part of unrelated cleanup in bug 274391 a couple years ago.
> In brief, the IE 50ms thing is too high and actually breaks some sites. We
> don't want to be doing that.
What sites? It seems best that we match the other major browsers on this rather insignificant point for compatibility, which means <=50ms->100ms.
Comment 9•17 years ago
|
||
> What sites?
You'd have to ask the people who brought that up in bug 207059 and bug 139677. I guess we _did_ match Opera at the time, so I guess if they've had to change it we might need to as well. That's too bad, really.
Comment 10•17 years ago
|
||
so looking at this I believe that we should match the behavior we had in Firefox2.
Reporter | ||
Comment 12•17 years ago
|
||
Attachment #275495 -
Flags: review?(pavlov)
Updated•17 years ago
|
Attachment #275495 -
Flags: review?(pavlov) → review+
Comment 13•17 years ago
|
||
With approval from pavlov and tor (and me) this should be ok to be checked in?
Keywords: checkin-needed
Reporter | ||
Comment 14•17 years ago
|
||
Comment on attachment 275495 [details] [diff] [review]
ff2 behavior - back to Alfred's patch of removing redundant clipping
Very low risk patch to return our GIF animation throttling to FF2 behavior.
Attachment #275495 -
Flags: approval1.9?
Comment 15•17 years ago
|
||
Comment on attachment 275495 [details] [diff] [review]
ff2 behavior - back to Alfred's patch of removing redundant clipping
This is already blocking+. It doesn't need approvals; just needs to land.
Attachment #275495 -
Flags: approval1.9?
Comment 16•17 years ago
|
||
Checked in on trunk.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 17•17 years ago
|
||
Marking as Verified.
Tested with current nightly with https://bugzilla.mozilla.org/skins/custom/images/mozchomp.gif
Status: RESOLVED → VERIFIED
Updated•17 years ago
|
Keywords: checkin-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•