1364365 - Finite length animation is still playing after changing the tabs

Status: VERIFIED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[roxleitan]

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

Comment 1

[roxleitan]

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

Comment 2

[Timothy Nikkel (:tnikkel)]

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.

Comment 3

[Timothy Nikkel (:tnikkel)]

Attachment: 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.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

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?

Comment 5

[Timothy Nikkel (:tnikkel)]

(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.

Comment 6

[Timothy Nikkel (:tnikkel)]

Attachment: patch v2

Comment 7

[Andrew Osmond [:aosmond] (he/him)]

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.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
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

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bf91589e3d3c

Comment 10

[roxleitan]

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.