Finite length animation is still playing after changing the tabs

NEW
Assigned to

Status

()

Core
ImageLib
12 days ago
an hour ago

People

(Reporter: roxana.leitan@softvision.ro, Assigned: tnikkel, NeedInfo)

Tracking

55 Branch
Points:
---

Firefox Tracking Flags

(firefox53 unaffected, firefox54 unaffected, firefox55 affected)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

12 days ago
Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0
Build ID: 20170511063838

[Affected versions]:
Nightly 55.0a1

[Affected platforms]:
Windows 10, Mac OS X 10.12

[Steps to reproduce]:
1.Launch Nightly 55.0a1 with a new profile
2.Navigate to https://bug1110686.bmoattachments.org/attachment.cgi?id=8535538
3.Open a new tab
4.Open a new window and navigate to about:memory
5.Click "Minimize memory usage" button
6.Wait 2 minutes until the animation get to the end
7.Go to the tab with the animation(opened at step 2)

[Expected result]:
The image should not animate and display the correct final frame

[Actual result]:
The animation is still playing
(Reporter)

Comment 1

7 days ago
Running mozregression I got:

Last good revision: d16bffb1859d7120bc82c0b39bb7d9fc664e8816
First bad revision: 50aee6c869012859a3395ddfda2aa4fd62fff1c5

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d16bffb1859d7120bc82c0b39bb7d9fc664e8816&tochange=50aee6c869012859a3395ddfda2aa4fd62fff1c5
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox55: --- → affected
(Assignee)

Comment 2

7 days ago
This happens because we have code that lets us advance the timeline by the length of one full loop of an animated gif.

https://dxr.mozilla.org/mozilla-central/rev/e66dedabe582ba7b394aee4f89ed70fe389b3c46/image/FrameAnimator.cpp#336

This works great for infinite gifs. But in this case it is being applied to a gif with only one loop. So instead of advancing to the last frame we basically continue where we left off from.
(Assignee)

Comment 3

an hour ago
Created attachment 8870529 [details] [diff] [review]
patch

The new behaviour almost seems better in this situation. But I still want to fix this logic error. And I don't want to have any unintended changes from bug 686905 if possible.
Assignee: nobody → tnikkel
Attachment #8870529 - Flags: review?(aosmond)
Comment on attachment 8870529 [details] [diff] [review]
patch

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

::: image/FrameAnimator.cpp
@@ +342,2 @@
>    FrameTimeout loopTime = aState.LoopLength();
> +  if (loopTime != FrameTimeout::Forever() && aState.LoopCount() < 0) {

Won't this cause it to block the main thread in FrameAnimator::RequestRefresh, looping/compositing until it decrements the loop? Couldn't this get expensive depending on how many loops and frames there are?

It seems to me that the loop advancement logic could be moved out of FrameAnimator::AdvanceFrame into its own function (say AdvanceLoops) which it called at the start of RequestRefresh, and if the loop count is not infinite, we clamp the number of loops we advance by aState.mLoopRemainingCount, and then decrement mLoopRemainingCount by the clamped value?
Flags: needinfo?(tnikkel)
You need to log in before you can comment on or make changes to this bug.