Closed Bug 885277 Opened 6 years ago Closed 6 years ago

Pausing and resuming animated images skips frames

Categories

(Core :: ImageLib, defect)

24 Branch
x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox23 --- unaffected
firefox24 --- ?

People

(Reporter: simon.lindholm10, Assigned: simon.lindholm10)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

STR:
1. Install Toggle Animated GIFs from AMO.
2. Visit http://i.imgur.com/jBG8fcN.gif and press Ctrl+M twice (this sets animationMode = 1, then animationMode = 0).
=> Frames will be skipped in between pausing and unpausing!

Regression range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=00b264c7cced&tochange=df526497d949

I'll bet that this is due to introducing the `(mAnim->currentAnimationFrameTime.IsNull())` check in http://hg.mozilla.org/mozilla-central/rev/420fe3994edf.
Maybe?
Joe Drew — Bug 867758 - Don't start images' animation until they've been explicitly RequestRefreshed by the refresh driver. r=seth
I'm not surprised - we track actual time, not "gif time", now. Is this something that you really need to work?
I see. Yes, my extension quite fundamentally needs this behavior, or an alternative way of achieving it. (New alternative APIs are okay, even if that would mean losing out on the ability of nsIDOMWindowUtils.imageAnimationMode to affect imgIContainers of background images, which I believe isn't otherwise possible from JS.)
But given that resetting GIFs to frame 0 at any time is fine, is there actually a problem with the wanted behavior of resetting a GIF to frame `mAnim->currentAnimationFrameIndex`?
Attached patch proposed patch (obsolete) — Splinter Review
Handles ShouldAnimate() changing from true to false (and perhaps some StartAnimate failure cases) in RequestRefresh. Restart from the beginning of frame when !mAnimating in StartAnimation. Remove an optimization from ResetAnimation, which is invalid in the case of setting animationMode = kNormalAnimMode and calling resetAnimation() before the repaint.
Attachment #790132 - Flags: review?(joe)
Attachment #790132 - Flags: review?(joe) → review?(seth)
Comment on attachment 790132 [details] [diff] [review]
proposed patch

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

::: image/src/RasterImage.cpp
@@ +530,5 @@
>  RasterImage::RequestRefresh(const mozilla::TimeStamp& aTime)
>  {
> +  EvaluateAnimation();
> +
> +  if (!mAnimating) {

I am not against this change but I want to understand how it's connected to the rest of the patch. Was there something about the logic of the old version that failed with the rest of your patch applied?
Yes - without that change, StopAnimation() (and later StartAnimation()) is never called when animationMode changes from kNormalAnimMode to kDontAnimMode, because ShouldAnimate() returns false if animationMode == kDontAnimMode, and that prevents EvaluateAnimation() from getting called. Then, when setting animationMode back to kNormalAnimMode it continues as if pausing never happened (i.e. skips frames).
Sigh. This is yet another patch that has gotten bitrotted by bug 899861. Do you still observe the problem with current nightly? If so, this patch needs a rebase. Sorry about that - disabling FrameAnimator affected a lot of work in progress.
Flags: needinfo?(simon.lindholm10)
Blocks: 867758
Seth, I expect the "fix" to bug 899861 to be backed out this week.
Thanks Milan. That's good news. Simon, let's hold off on this until the backout of bug 899861.
Flags: needinfo?(simon.lindholm10)
I just made it depend on bug 905678 which should get the code back to the original state.
Depends on: 905678
Bug 905678 has landed, so the patch should apply again now. (The problem still reproduces.) It could perhaps use a try run, but I don't have the privileges.
Assignee: nobody → simon.lindholm10
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OK, let's get this thing finished up. If this is green on try I'll do a final review and push this in tomorrow.

Try job here: https://tbpl.mozilla.org/?tree=Try&rev=4f65340d9b81
Attachment #790132 - Flags: review?(seth) → review+
Attached patch patch v2 (obsolete) — Splinter Review
Whelp. Apparently I totally missed the SetAnimationStartTime thing happening before animation start... This should work better, I hope.
Attachment #790132 - Attachment is obsolete: true
Attachment #797567 - Flags: review?(seth)
Comment on attachment 797567 [details] [diff] [review]
patch v2

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

Seems OK. Please update the commit message to include "r=seth".
Attachment #797567 - Flags: review?(seth) → review+
Attached patch patch v3Splinter Review
Done. Try run?
Attachment #797567 - Attachment is obsolete: true
Attachment #797768 - Flags: review+
I've got one going here:

https://tbpl.mozilla.org/?tree=Try&rev=cd9df304e794

Will trigger a bunch of mochitest-oth to verify that there's no more issue there.
Looks good on try. I am about to run out the door so I don't have time to push this right now, but if this remains unpushed on Tuesday I'll take care of it.
https://hg.mozilla.org/mozilla-central/rev/810c464191d4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 956460
You need to log in before you can comment on or make changes to this bug.