1177587 - Detect use of <img> elements for animation and change behavior to avoid flicker

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect)

Description

[Seth Fowler [:seth] [:s2h]]

It happens that sites use JavaScript to change the src attribute of <img> elements on a timer, as a primitive form of animation. The <img> element will switch to the next image as soon as its size is available, and it will then draw the image progressively. That means that unless the decode finishes very quickly, we are likely to draw the next image before it's completely decoded, resulting in flicker. The test-case in bug 1164748 is an excellent example of this problem.

This is both a quality issue and a Blink/WebKit parity issue, since their different painting behavior lets them avoid flicker in this case.

I see two ways to eliminate the flicker in Gecko.

(1) If we detect that the |src| attribute is changing faster than a certain rate, start sync decoding in nsImageFrame::PaintImage. This is extremely straightforward, but it does have the major disadvantage that it leads us to do more work on the main thread.

(2) If we detect that the |src| attribute is changing faster than a certain rate, continue to paint the previous image until the next image is fully decoded. (In other words, disable progressive display.) This is trickier to implement, and care must be taken to ensure that we don't freeze the animation or start dropping lots of frames on slower machines because decoding gets behind.

Given that this is something of an edge case (i.e., most web pages do not do this) and that generally these animations always loop, so that even if we sync decode for the first pass through the animation we'll stop doing so afterwards, I am inclined to start with (1) and follow up with (2) if we actually see issues with (1). Especially in an e10s world, I'd expect (1) to be an acceptable solution in practice.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Detect use of <img> elements for animation and use sync decoding to reduce flicker

Here's the patch. We detect animation of the |src| attribute by checking the
length of time between successive calls to OnSizeAvailable for the same
nsImageFrame. We don't use NotifyNewCurrentRequest for this because it can be
called more than once for the same request in some cases, which would throw off
the heuristic, and because waiting until the image is loaded to record the
timestamp may to some extent unfairly penalize larger images which take longer
to load.

The patch adds a new pref, "image.infer-src-animation.threshold-ms", which
controls this feature. The value of the pref is the threshold for entering
"animation mode"; if the length of time between successive calls to
OnSizeAvailable is less than this threshold, then we enter "animation mode" and
start sync decoding. We don't stay in "animation mode" permanently - if the rate
of change of the |src| attribute slows down later, then we return to our normal
behavior.

The default threshold is 2s, which should give us enough leeway to detect
animation at ~1FPS. We can always tune this later, but I think this is a good
initial value. For reference, the testcase in bug 1164748 runs at 0.8FPS by
default. This patch completely eliminates the flickering in that testcase.

Comment 2

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8626652 [details] [diff] [review]
Detect use of <img> elements for animation and use sync decoding to reduce flicker

Might it be better to detect this in nsImageLoadingContent? When we get a new pending request for example.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Attachment: Detect use of <img> elements for animation and use sync decoding to reduce flicker

Yeah, I agree, nsImageLoadingContent would be preferable. By doing it there, we
can take the network out of the equation totally.

This new version of the patch simplifies the nsImageFrame component to a simple
SetForceSyncDecoding() method and appropriate code in PaintImage(), and does the
detection in nsImageLoadingContent::PrepareNextRequest().

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8627468 [details] [diff] [review]
Detect use of <img> elements for animation and use sync decoding to reduce flicker

This looks good.

Have you tested this out to see how often the sync decode kicks in in non-animation cases? ie maybe images start out with an about:blank like request and then get moved to their src? or maybe a common pattern is to set the image src to something blank and then their final source for web authors?

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Timothy Nikkel (:tn) from comment #4)
> Have you tested this out to see how often the sync decode kicks in in
> non-animation cases? ie maybe images start out with an about:blank like
> request and then get moved to their src? or maybe a common pattern is to set
> the image src to something blank and then their final source for web authors?

What I did was surf around to various top internet sites (Facebook, Twitter, Reddit, Gawker, Ars Technica...) and verify that the sync decode was never triggered. In my testing I never saw it happen on these sites, even though it kicks in with 100% reliability for the test case in bug 1164748.

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8627468 [details] [diff] [review]
Detect use of <img> elements for animation and use sync decoding to reduce flicker

Okay, thanks.

Comment 7

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad4b72849ee

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ad4b72849ee