125137 - Minimum frame rate delay on animated images

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[ArronM (:paper)]

IE seems to have a minumum frame delay of about 0.1 seconds.  Thus, any frames
set to 0.01 through 0.09 (1 - 9 in .gif "display time" terms) gets slowed down
to 0.1.

Mozilla should have a Minumum Frame Delay Setting, and it possibly should be
defaulted to 0.1.  The major reason for a 0.1 default is that many animated .gif
makers set their .gifs to 0.01, test it on IE, and see it going the speed they
want it to go.  Little do they realize IE is slowing down their animation. 
Then, when someone loads the .gif in Mozilla, it goes the proper 100 frames a
second, and the animation blazes by, sometimes causing high CPU usage.

This feature would effect the following bugs:

Bug 116853 - animated gif too fast when in background of inactive window.
Bug 105851 - no (incorrect) delay in gif animation -- speedy -- when moving
cursor around.
Bug 99636 - animated gif causes high (sometimes 100%) CPU usage.
Bug 86319 - (large) animated GIF has terrible performance

This feature request is very similar but not the same as Bug 71829 ([rfe] Let
the user set a default delay for animated gifs that don't specify one). Bug
71829 requests a default delay, and this bug requests a minimum delay.  These
two could be combined into a "minumum/default delay" setting and kill both birds
with one stone.

When users see their CPU hit 99%, they generally blame the browser instead of
the real culprit, the web designer that thought IE actually displayed their
animation at 0.01 (setting 1 in most animation programs)

The URL shows 4 animated gifs, each at 10, 50, 100, and 200ms delay
respectively.  IE displays the first 3 at the same speed, and the last one takes
twice as long.  Mozilla (Build 2002021203, Win2k) displays the gif (close to)
the real speed.

Comment 1

[Stuart Parmenter]

Attachment: Patch to set a lower bounds on the animation timeouts

Comment 2

[Stuart Parmenter]

Can you try this patch out and post the results?  It seems to make things 
better for me.

Comment 3

[ArronM (:paper)]

Excellent! 
I don't have the source to compile.. could someone else try it out?

From the looks of it, that will probably fix it up to "looking like IE".  It
still would be nice to allow faster animation.. mind you, I can't think of any
reason one would need 100 frames per second.  But I can see some people wanting
30fps.  If we are looking for a quick fix, how about something like:

  if (mTimeout = 0)
     *aTimeout = 100; // Ensure a minimal time between updates so we don't
throttle the UI thread.
  elseif (mTimeout < 30)
     *aTimeout = 30; // Set minimum frame display length to save CPU
  else
     *aTimeout = mTimeout;

(excuse my bad C grammer, it's been a while)
This would force each frame to be shown at least for 30ms, which gives the user
approx. 33 1/3 FPS.  Mozilla would be more capable than IE, but limiting
(hopefully) enough to save the CPU.

The preferable method of course is to have a configurable setting, but that's
hardly a priority.

Comment 4

[ArronM (:paper)]

Can this bug be moved under Bug 119597 (animated GIF tracking bug) if my guess
of where it belongs is correct?

First glaring bug in my previous comment is the first line of code.  It should
be of course:
  if (mTimeout == 0)

Would gfxImageFrameWin.cpp also need this patch?

Comment 5

[ArronM (:paper)]

sorry for the spam, I just noticed my edit to your patch would introduce a horid
bug.

Here's my 3rd try:

  if (mTimeout == 0)
    *aTimeout = 100; // Ensure a minimal time between updates so we don't
throttle the UI thread.
  elseif (mTimeout >= 0 && mTimeout < 30)
    *aTimeout = 30; // Set a minimum delay of 30ms
  else
    *aTimeout = mTimeout;

I really need to figure out how to make patch files someday :)

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

A proper "minimum" timeout is a tricky thing.  Some people may make anims with a
rate of around screen-refresh rate to get smooth animations.  (See bug 124999).

30ms ~= 30Hz, which is approaching smooth, but still way under refresh rates of
CRT monitors of 60-90+ Hz (mostly 72-85).  LCD's are 25-40Hz generally.  60Hz
would be around 16ms; I'd be tempted to target that and see how it affects the
bugs reported.

I have a patch that makes it a pref, but we have a lot of prefs in the tree....

I agree if the time isn't specified, set it to something safe, like 50 or 100ms.
(100 is good I think)

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Updated patch

Sets default of 100ms if animtime <= 0, otherwise limits animation speed to no
faster than 20ms (50Hz).  If someone wants, I can create prefs for this
trivially (I have them in my local tree).

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Nominating - trivial patch.  We can change the defaults if other values are
preferred.

Comment 9

[ArronM (:paper)]

I don't know how much effect having a minimum of 20ms will be on large animated
gifs.  Most of them are using 99% CPU @ 10ms, and will continue to use 99% @
20ms due to the sheer volume of the image trying to be moved.

However, it's a good starting point, as as pointed out, it can be tweaked if
need be later.  Let's get it into a build and see what it does :D

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Since this can eat 100% cpu and cause UI starvation with anims with <10-15 ms
repeat times, raising from enhancement to normal.  Already nominated for 0.9.9 -
r/sr?  we could change the max rate to 100ms (10Hz, same as IE), 16ms (60+Hz) or
10ms (100Hz) if people feel that's needed.  Read the initial report for details.

Comment 11

[Sebastian Späth]

Mmmh, I feel we should limit the frame rate if no value is specified, but not
try to "improve" the animation settings to what we believe the creator wanted
them to be in other cases. 
It looks to me like a rather bad, Microsoftish approach. There *may* be
reasonable uses for higher frame rates (e.g. by improved monitors in the future).

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

Part of the problem is that there are many animations out there that animate
faster with mozilla than the author intended; far more than where the author
intended it animate faster than IE lets it (100ms).

As I said, we can quibble over whether the limit should be 10, 16, 20, 50 or 100
(etc) ms.  We could make it a pref (I have the code, it's trivial).  But I think
we should check this fix in with some delay > 10ms.  The current limitation is
set by the GIF decoder at 10ms (#define MINIMUM_DELAY_TIME    10).  Thus far,
there are no other sources of animated frames, but I'm sure eventually some
other anim format will appear, given GIF's inadequacies.

Comment 13

[ArronM (:paper)]

I believe IE and Opera set the minimum delay to 100ms for a reason.  That reason
being anything shorter uses up way too much CPU (not because of the monitor). 
That was a while ago.  Today, we have faster computers, and probably can afford
less of a minumum delay.  But not 10ms as it's set to now.  (See the bugs in the
first comment -- they all use 99% CPU).

10ms = bad, 99% CPU
100ms = like Microsoft & Opera, but gif makers are still in ignorant bliss about
 correct settings for delay.
<100ms = better than other browsers.  Gif makers will learn they can do more
with Mozilla.  (Although IMO, if they are making high frame rate animations,
they should be using something more designed for it -- like flash or quicktime)

Let's get the patch in :)

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

Attachment: random gif

Example of a gif I ran across today that animates so fast the red-and-blue
flashing appears purple on my machine (I'm running a 10ms limit).  Obviously
not as the author intended.

The point that lots of images around the web cause 99% cpu usage and possible
UI starvation is a very strong one.  I advise nothing below 20ms, and I think
40ms (same as cheaper LCD displays) or 50ms may make sense - unless we want to
go with IE's default of 100ms, given that that's what all authors use/assume in
their testing, whether they know it or not.  If we want to preserve the ability
for other (future) formats to provide faster animation, we can move the limit
from here into the gif decoder (where it's already a #define).

Comment 15

[Brendan Eich [:brendan]]

IE has created a _de facto_ standard.  I think we can live with it.  We might
argue that to beat the 10Hz perceptual limit, we should use 50ms, but at this
point, why be different?

/be

Comment 16

[Jo Hermans]

We can always add a preference to specify the minimum frame delay, with a
default of 100 mSec, just like IE.

I think this is real low hanging fruit, which can dramatically improve the
performance of Zilla (together with bug 125025). In fact, it's so low that you
need a dig a hole to grab the fruit!

Comment 17

[Adam D. Moss]

I agree somewhat with comment #11.  Or... at the very least, 100ms is
too tardy a compromise.

Comment 18

[ArronM (:paper)]

Okay, the patch has been around for 2 weeks now, and has been tested with Bug
99636 (reducing the CPU usage of the bug's gif from 88% to 50% if I read it
correctly)

What's the next step? Can we get this patch into Mozilla?

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

retargeting for 1.0, we really should check in a fix for this.

Comment 20

[Stuart Parmenter]

Comment on attachment 70180 [details] [diff] [review]
Updated patch

r=pavlov

Comment 21

[Stuart Parmenter]

This is a simple patch which we should check in for 1.0

Comment 22

[Stuart Parmenter]

reassigning to rjesup

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Updated patch for 100ms as per pavlov

Updated patch for 100ms minimum delay (IE/etc compatible), per discussion with
pavlov.  r/sr please.

Comment 24

[Stuart Parmenter]

Comment on attachment 78242 [details] [diff] [review]
Updated patch for 100ms as per pavlov

r=pavlov

Comment 25

[Randell Jesup [:jesup] (needinfo me)]

Taking bug

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 78242 [details] [diff] [review]
Updated patch for 100ms as per pavlov

sr=jst

Comment 27

[ArronM (:paper)]

New Patch may cause problems.

if (mTimeout < 100) // 100 is compatible with IE and Opera among others

should be:

if (mTimeout >= 0 && mTimeout < 100) // 100 is compatible with IE and
Opera among others

If I read the code correctly, functions such as
imgContainer::AppendFrame(gfxIImageFrame *item) and
imgContainer::StartAnimation() want a -1 returned from GetTimeout if the frame
should be displayed forever, so we shouldn't set timeout to 100 if it's < 0


And, as I asked earlier, will the GetTimeout function in
gfx2/src/windows/gfxImageFrameWin.cpp need updating too?

Comment 28

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Allow negative values through, fix windows as well

ArronM is correct (-1 is a flag to display forever, and gfx2/windows has a copy
of the routine (why?  I don't care right now).	Updated patch to match. Fast
r/sr and I'll get an a= from Asa

Comment 29

[Johnny Stenback (:jst)]

Comment on attachment 78254 [details] [diff] [review]
Allow negative values through, fix windows as well

sr=jst

Comment 30

[Stuart Parmenter]

Comment on attachment 78254 [details] [diff] [review]
Allow negative values through, fix windows as well

r=pavlov  (fyi, the windows gfx2 code isn't built)

Comment 31

[Asa Dotzler [:asa]]

Comment on attachment 78254 [details] [diff] [review]
Allow negative values through, fix windows as well

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 32

[Randell Jesup [:jesup] (needinfo me)]

Fix checked in

Comment 33

[Mark Bitterling]

I have not been following this bug because I had never experienced any problems
with GIF animations eating too much CPU time on any of my Macs.  I am wonder if
this was a Windows only problem? 

This morning I looked at http://animecity.nu/Bugzilla/Anim.html with OS X
Mozilla build 2002040512, IE 5.1 for Mac OS X, and iCab 2.7 for OS X. All of
them displayed the 4 images at http://animecity.nu/Bugzilla/Anim.html at the
frame rates specified by the animations (or at least a close approximation since
I have no way to determine the actual frame rate that I am seeing).  Of course
Mozilla build 20020400903 which has the patch applied now shows the first 3
animations at the 100ms speed.

Since IE 5.1 and iCab do not follow the "de facto standard" of minimum of 100ms
 and since it looks like Mac builds of Mozilla may never have had the problems
that this patch addresses, I am wondering if the Mac's ability to show
animations at the author's requested speed should be restricted.

Someone might also want to check and see if this was an issue for Linux.

I am happy for all the Windows users whose system performance issues were aided
by this patch, but as a Mac user who likes 30fps animations to be displayed at
30fps, I hope that someone will consider adjusting this patch.

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

It's a tough issue.  Lots of web designers never realize that their animations
are being played back at up to 10x the intended speed.  I've seen very few anims
that purposely selected values less than 100ms. If they did, they'd then have a
problem with Win32-IE, Opera, etc (plus in most cases ns4.x) users being annoyed
that the animations are slower than intended.

We talked about different values.  Increasing from 10ms (for GIFs) to 20, 30, 50
and 100 were discussed.  The final determination was to go with 100ms for
compatibility.  We also could put in Yet Another Pref (in fact, I have done that
for the wgate product because I need to crank it _down_ to 2000 or 3000ms).  Too
many prefs is a bad thing though.

If you'd like to request 100 be changed to something smaller, open a bug,
reference this one, and suggest reasoning for a different limit.  One argument
is that we should respect the author's request, though it may have been
unintentional.

Comment 35

[Boris Zbarsky [:bzbarsky]]

*** Bug 105851 has been marked as a duplicate of this bug. ***

Comment 36

[Jon Henry]

*** Bug 205170 has been marked as a duplicate of this bug. ***

Comment 37

[Jon Henry]

*** Bug 210626 has been marked as a duplicate of this bug. ***

Comment 38

[Mark Bitterling]

For reference purposes :

Bug 139677 : Request to Remove Unnecessary Minimum frame rate delay for GIF anim on OS X 
since Mozilla is the only Mac browser imposing this restriction.

Bug 71829 : Request for a pref to allow the user set a default delay for animated gifs that do not 
specify one.

Comment 39

[Dave Townsend [:mossop] (PTO until Aug 21st)]

*** Bug 342680 has been marked as a duplicate of this bug. ***