386269 - GIF animation throttling doesn't match IE/Safari

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[tor]

Attachment: use ie/safari threshhold mapping

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

Comment 1

[tor]

Attachment: checkin version

Comment 2

[Alfred Kayser]

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

[Jesse Ruderman]

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

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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...

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

What's the status of this bug?  Not sure if what was checked in needs to be revised/backed out, or what...

Comment 7

[Alfred Kayser]

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.

Comment 8

[tor]

(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

[Boris Zbarsky [:bzbarsky]]

> 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

[Stuart Parmenter]

so looking at this I believe that we should match the behavior we had in Firefox2.

Comment 12

[tor]

Attachment: ff2 behavior - back to Alfred's patch of removing redundant clipping

Comment 13

[Alfred Kayser]

With approval from pavlov and tor (and me) this should be ok to be checked in?

Comment 14

[tor]

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.

Comment 15

[Boris Zbarsky [:bzbarsky]]

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.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.

Comment 17

[Alfred Kayser]

Marking as Verified.
Tested with current nightly with https://bugzilla.mozilla.org/skins/custom/images/mozchomp.gif