1744499 - A single frame 'animated' GIF favicon causes 60Hz refresh driver ticks until the tab is closed

Status: NEW

(Core :: Graphics, defect, P3)

Description

[Florian Quèze [:florian]]

Attachment: Test animated favicon

See attached reduced testcase.

Here are the debug info of the gif image according to https://onlinegiftools.com/analyze-gif

GIF Information
---------------
GIF size: 559 bytes
GIF length: 0.1 second(s)
GIF width/height: 16×16 pixels
number of frames: 1
number of colors: 128
loop count: not present (loops once)

Frame #1:
---------
x: 2
y: 0
width: 13
height: 16
delay: 100ms
disposal: 0
transparent: true
transparency index: 127
transparency color: #ffffff

My guess is that the "100ms" delay makes it go through the animated code path, but there's no real animation happening.

Here's a profile: https://share.firefox.dev/31z5sDs

Comment 1

[Florian Quèze [:florian]]

Andrew, what would be the expected behavior in this case? Should we treat the image as non-animated, or animate for 100ms and then stop the refresh driver?

Comment 2

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

It seems we do not propagate the animation finished state to the refresh driver?

Presumably the same thing could happen if we had multiple frames, but the image set a maximum loop count as well. We should (if not actually) stop the animation internally at this point because there is no next frame.

I suspect because we don't special case a single frame, it may try to reloop onto the "first" frame again:
https://searchfox.org/mozilla-central/rev/3407e72ceb5039da514c03ae61bd279b1725c3b2/image/FrameAnimator.cpp#210

Because of decoding failures and lack of trustworthy metadata, it is hard for the decoder to know that there is only a single frame in the animation. In addition, due to the fixed loop count situation, I would expect us to treat this as animated until the animation has finished.

Comment 3

[Timothy Nikkel (:tnikkel)]

Not sure it's a special problem with 1 frame animated images, the refresh driver never stops ticking for all finite length animated images.

Comment 4

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

Did we not used to pass up a timeout period for when the frame would advance? We certainly do our best to calculate it.

Comment 5

[Timothy Nikkel (:tnikkel)]

The refresh driver pulls the first frame delay when an image gets added to it

https://searchfox.org/mozilla-central/rev/eea0fac2cb8cae1e5a2e38959eeaa0b7f6d23456/layout/base/nsRefreshDriver.cpp#1355

and uses that to put it in the "start table". We then "start" the images in the start table on a refresh driver tick after the right amount of time and move the images to regular mRequests array

https://searchfox.org/mozilla-central/rev/eea0fac2cb8cae1e5a2e38959eeaa0b7f6d23456/layout/base/nsRefreshDriver.cpp#2443
https://searchfox.org/mozilla-central/rev/eea0fac2cb8cae1e5a2e38959eeaa0b7f6d23456/layout/base/nsRefreshDriver.cpp#2624

We call RequestRefresh on every element in mRequests on every refresh driver tick

https://searchfox.org/mozilla-central/rev/eea0fac2cb8cae1e5a2e38959eeaa0b7f6d23456/layout/base/nsRefreshDriver.cpp#2472

At least that's my understanding from reading the code.