If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

ASSIGNED
Unassigned

Status

()

Core
ImageLib
--
enhancement
ASSIGNED
17 years ago
5 years ago

People

(Reporter: bz, Unassigned)

Tracking

Trunk
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [imglib], URL)

Attachments

(4 attachments, 2 obsolete attachments)

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

17 years ago
Nice idea for the future.
Lets get the basics working first.
-p
Status: NEW → ASSIGNED
Target Milestone: --- → Future

Comment 2

17 years ago
All pnunn bugs reassigned to Pav, who is taking over
the imglib.
Assignee: pnunn → pavlov
Status: ASSIGNED → NEW

Comment 3

17 years ago
changing status to [imglib]
Whiteboard: [imglib]

Updated

16 years ago
Blocks: 119597

Updated

15 years ago
Summary: [rfe] Let the user set a default delay for animated gifs that don't specify one → Let the user set a default delay for animated gifs that don't specify one
So... we have a default delay w/o pref for animated gifs now...

should we keep this bug open for the pref?
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....
*** Bug 205170 has been marked as a duplicate of this bug. ***

Updated

11 years ago
Assignee: pavlov → nobody
QA Contact: tpreston → imagelib

Comment 7

8 years ago
Created attachment 426446 [details]
GIF animation with many frames that have 0 ms delays

Comment 8

8 years ago
Created attachment 426450 [details] [diff] [review]
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

8 years ago
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.
Attachment #426450 - Flags: review?(tor)
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.
Attachment #426450 - Flags: review?(tor) → review-

Comment 11

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

Updated

6 years ago
Assignee: nobody → schapel
Target Milestone: Future → mozilla9

Comment 12

6 years ago
Created attachment 555176 [details] [diff] [review]
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!).
Attachment #426450 - Attachment is obsolete: true
Attachment #555176 - Flags: review?(joe)

Comment 13

6 years ago
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 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.
Attachment #555176 - Flags: review?(joe) → review-
Created attachment 592265 [details] [diff] [review]
reworked attachment 555176 [details] [diff] [review]
Assignee: schapel → bgirard
Status: NEW → ASSIGNED
Attachment #592265 - Flags: review?(joe)
Created attachment 592266 [details] [diff] [review]
reworked attachment 555176 [details] [diff] [review]
Attachment #592265 - Attachment is obsolete: true
Attachment #592265 - Flags: review?(joe)
Attachment #592266 - Flags: review?

Updated

6 years ago
Attachment #592266 - Flags: review? → review?(joe)
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.
Attachment #592266 - Flags: review?(joe) → review+
Boris can you confirm that you still want this patch? Will anyone even use this feature?
Created attachment 592272 [details] [diff] [review]
Let the user set a default delay for animated gifs that don't specify one

Updated

5 years ago
Assignee: bgirard → nobody
You need to log in before you can comment on or make changes to this bug.