Finite length animation is still playing after changing the tabs

VERIFIED FIXED in Firefox 55

Status

()

Core
ImageLib
VERIFIED FIXED
2 months ago
27 days ago

People

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

Tracking

55 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox53 unaffected, firefox54 unaffected, firefox55 verified)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

2 months 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

a month 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

a month 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

a month 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)
(Assignee)

Comment 5

a month ago
(In reply to Andrew Osmond [:aosmond] from comment #4)
> Comment on attachment 8870529 [details] [diff] [review]
> 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?

Yes.

> 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?

I wrote a patch to do this, minus the factoring out an AdvanceLoops function because I don't want to make too many changes to the logic here, there are a lot of hidden dependencies. For example, while writing that patch I discovered that mLoopRemainingCount isn't even initialized until we do one full loop, so we have to gate it on LoopCount() and mLoopRemainingCount.
Flags: needinfo?(tnikkel)
(Assignee)

Comment 6

a month ago
Created attachment 8870610 [details] [diff] [review]
patch v2
Attachment #8870529 - Attachment is obsolete: true
Attachment #8870529 - Flags: review?(aosmond)
Attachment #8870610 - Flags: review?(aosmond)
Comment on attachment 8870610 [details] [diff] [review]
patch v2

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

(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> I wrote a patch to do this, minus the factoring out an AdvanceLoops function
> because I don't want to make too many changes to the logic here, there are a
> lot of hidden dependencies. For example, while writing that patch I
> discovered that mLoopRemainingCount isn't even initialized until we do one
> full loop, so we have to gate it on LoopCount() and mLoopRemainingCount.

Makes sense. I would not predicted us still dealing with behavioural changes for the original discard animated images patch :). Still, I think this version is less risky than the original, avoiding that particular main thread blocking scenario.
Attachment #8870610 - Flags: review?(aosmond) → review+

Comment 8

a month ago
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf91589e3d3c
Limit the number of full loops of an animated image we can skip to the number of remaining loops. r=aosmond

Comment 9

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bf91589e3d3c
Status: NEW → RESOLVED
Last Resolved: a month ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Reporter)

Comment 10

27 days ago
Verified as fixed using the latest Nightly 55.0a1 (2017-06-01) on Ubuntu 16.04 x64, Mac OS X 10.12 and Windows 10 x64.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
You need to log in before you can comment on or make changes to this bug.