71829 - Let the user set a default delay for animated gifs that don't specify one

Status: NEW

(Core :: Graphics: ImageLib, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

Currently, Mozilla displays animated gifs with no delay set as fast as it can. 
It would be nice if the user could set a default delay to be used for such gifs.

See bug 68104 and its dups

Comment 1

[pnunn]

Nice idea for the future.
Lets get the basics working first.
-p

Comment 2

[pnunn]

All pnunn bugs reassigned to Pav, who is taking over
the imglib.

Comment 3

[Terri Preston]

changing status to [imglib]

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

So... we have a default delay w/o pref for animated gifs now...

should we keep this bug open for the pref?

Comment 5

[Boris Zbarsky [:bzbarsky]]

Actaully, I'd prefer it if we made the pref work and just set the default to
100ms.  That would let embedders set the delay as they see fit.  For example,
Camino may choose to set a delay of 0ms (the actual delay would be the PR_MAX of
what the gif has and the pref value), since no other mac browser has a delay on
gif animations....

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

Comment 7

[Steve Chapel]

Attachment: GIF animation with many frames that have 0 ms delays

Comment 8

[Steve Chapel]

Attachment: patch to make Gecko honor all GIF delays greater than 0 ms

This patch makes Gecko honor all specified delays of greater than 0 ms in GIF animations. This change could be considered a stepping stone to fixing this bug; if there is a default delay substituted for 0 ms delays, there should not be a delay substituted for delays that are specified. This will cause some GIF animations on the web to run very fast, just as when bug 207059 was fixed, and there will be complaints such as those in bug 232769, but the GIFs will be fixed just as before and the complaints should die down.

Comment 9

[Steve Chapel]

Comment on attachment 426450 [details] [diff] [review]
patch to make Gecko honor all GIF delays greater than 0 ms

I have been browsing the web for the past week with this patch, and I have noticed that this affects very few GIFs. The only ones I've seen are on sites dedicated to animated GIFs, and only about 5% of those GIFs run faster than in other browsers.

Comment 10

[Joe Drew (not getting mail)]

Comment on attachment 426450 [details] [diff] [review]
patch to make Gecko honor all GIF delays greater than 0 ms

tor doesn't do much work on Gecko these days, but I own imagelib. I don't think this is a good idea, though I would totally accept a patch to make the default (zero) timeout pref-able.

Comment 11

[Steve Chapel]

I think the current behavior is that GIF frames with delays of 10 ms or less become changed to 100 ms. The last time I looked at the code, it seems as though this also applies to APNGs.

If we change delays of 0 ms to a pref-able timeout, that will leave the other delays change to 100 ms. That doesn't seem right. Maybe the default should apply to all frames with delays of 10 ms or less, so that whenever the delay is changed, it's changed to the same value. It would also make sense to hardcode the default delay to be constrained between 10 ms to 100 ms inclusive, so animations never go slower than they currently do, nor faster than specified in the animation. Alternatively, we could use the default only when it is greater than the specified delay.

The one concern I have with this scheme is that changing the default delay would then change all animations with delays of 10 ms or less. If someone would like to allow 10 ms frame delays, but keep the default 100 ms delay for 0 ms frames, that wouldn't be allowed. There may be many times more GIF animations using a delay of 0 ms than use 10 ms, changing the default to 10 ms could speed up lots of GIFs that are intended to run at 100 ms. We could add two defaults, one for 0 ms delays and one for non-zero delays of 10 ms or less. We would use the default delay only if it is greater than the specified delay. Then 10 ms delays could be changed to 20 ms to display them closer to their specified speed, and 0 ms delays could still be displayed at 100 ms.

I was trying to avoid this complication with my previous patch of honoring non-zero delays, then adding a pref for zero delays. Some users would complain about GIFs running too fast, as did the last time Mozilla began honoring delays of 20-90 ms, but then GIFs that ran too fast would be fixed as also happened the last time. If this isn't an option, then I think the two-pref option (one for 0 ms delays and one for non-zero delays under 10 ms) would be my next favorite.

Comment 12

[Steve Chapel]

Attachment: patch for default timeout preference

After reading the other bug reports mentioned in the code for the timeout, I think it makes the most sense to separate the ideas of a minimum timeout and a default timeout.

This patch provides the preference image.default_timeout which takes effect as soon as it is changed. The default value is 50 ms, which seems to make animations with a 0 ms timeout appear smoother than when they use the old hard-coded default of 100 ms.

The minimum timeout is 10 ms, which is the largest minimum we can use without Firefox slowing down animations that it has been animating at the speed they specify. This minimum timeout also applies to the default specified by the user, so the user can't specify an unreasonable timeout such as 5 ms (200 fps!).

Comment 13

[Steve Chapel]

The latest patch will also help with bug 531802. It will cause Firefox to honor timeouts of 10 ms on APNGs, and use the minimum timeout of 10 ms on APNGs with positive timeouts less than 10 ms. For APNGs with timeouts of 0 ms, it will use the default timeout. Currently, I think Firefox changes timeouts of 10 ms and under to 100 ms, which is just confusing.

Comment 14

[Joe Drew (not getting mail)]

Comment on attachment 555176 [details] [diff] [review]
patch for default timeout preference

Review of attachment 555176 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/libpr0n/src/imgFrame.cpp
@@ +92,5 @@
> +static PRBool gRegisteredPrefObserver = PR_FALSE;
> +
> +// Reloader
> +static void
> +ReloadPrefs()

Honestly, the body of this function could go inside imgFramePrefObserver::Observe; there's not really a lot of code here, after all. 

That being said, I'm not actually picky one way or the other.

@@ +95,5 @@
> +static void
> +ReloadPrefs()
> +{
> +  PRInt32 defaultTimeout;
> +  if (NS_SUCCEEDED(mozilla::Preferences::GetInt(DEFAULT_TIMEOUT_PREF, &defaultTimeout))) {

Here, use

  defaultTimeout = mozilla::Preferences::GetInt(DEFAULT_TIMEOUT_PREF, MINIMUM_TIMEOUT)
  if (defaultTimeout < MINIMUM_TIMEOUT)
     defaultTimeout = MINIMUM_TIMEOUT;

(i.e., don't bother with the nsresult-returning version).

@@ +104,5 @@
> +      gDefaultTimeout = defaultTimeout;
> +  }
> +
> +  // Discard reload timeout
> +  mozilla::imagelib::DiscardTracker::ReloadTimeout();

Why reload the discard timeouts when the animation pref has changed?

@@ +787,5 @@
> +  // See bug 71829, bug 125137, bug 139677, and bug 207059.
> +  if (mTimeout == 0)
> +    return gDefaultTimeout;
> +  else if (mTimeout < MINIMUM_TIMEOUT)
> +    return MINIMUM_TIMEOUT;

As below, we should make this patch behaviour-neutral, so this should be reworked to ensure that animations from 1 to 10ms timeouts go 100ms.

::: modules/libpref/src/init/all.js
@@ +3258,5 @@
>  // The maximum source data size for which we auto sync decode
>  pref("image.mem.max_bytes_for_sync_decode", 150000);
>  
> +// The default delay used for image animations, in ms
> +pref("image.default_timeout", 50);

This should be 100, not 50, as that's what we had before. If we really want to change what the default is, we should do so in a separate patch; this one should be behaviour-neutral.

Comment 15

[Benoit Girard (:BenWa)]

Attachment: reworked attachment 555176

Comment 16

[Benoit Girard (:BenWa)]

Attachment: reworked attachment 555176

Comment 17

[Joe Drew (not getting mail)]

Comment on attachment 592266 [details] [diff] [review]
reworked attachment 555176 [details] [diff] [review]

Review of attachment 592266 [details] [diff] [review]:
-----------------------------------------------------------------

::: image/src/imgFrame.cpp
@@ +161,5 @@
>        gDisableOptimize = true;
>      }
>      hasCheckedOptimize = true;
>    }
> +  mozilla::Preferences::AddIntVarCache(&gDefaultTimeout, "image.default_timeout");

The imgFrame constructor occurs too frequently for this to be called inside it.

Comment 18

[Benoit Girard (:BenWa)]

Boris can you confirm that you still want this patch? Will anyone even use this feature?

Comment 19

[Benoit Girard (:BenWa)]

Attachment: Let the user set a default delay for animated gifs that don't specify one

Comment 20

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.

Comment 23

[Sylvestre Ledru [:Sylvestre]]

No assignee, updating the status.