1324591 - Jank (rather than checkerboard) async animations when we reach the edge of the pre-rendered area

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P1)

Description

[Botond Ballo [:botond]]

In bug 1321412, I implemented a mechanism for partial pre-rendering of large content elements with an animated transform.

This allows the animation to be sampled asynchronously (on the compositor thread) in spite of not having the entire content pre-rendered.

The question arises what we should do in the compositor if we reach the edge of the pre-rendered area (because the main thread hasn't provided us with a new pre-rendered region in time). The options are:

  (1) Sample the animation anyways, showing blank space where we don't
      have content to show (i.e. "checkerboarding").

  (2) Do not sample the animation on this composite, thereby janking it.

(2) is the behaviour that happens right now, when the animation isn't async at all. As per bug 1100357 comment 18, this is the behaviour we'd like to preserve.

(1) is the behaviour you get right now if you enable partial pre-rendering. (Just because that's the behaviour that falls out of the existing compositor code.)

This bug tracks implementing behaviour (2) for partial pre-rendering.

Comment 1

[Botond Ballo [:botond]]

A rough outline of what needs to be done here:

  - Ship enough information to the compositor so that it knows whether
    we are in a partial pre-rendering situation, and if so, what the
    pre-rendered region is. (It's possible this can be inferred from
    information the compositor already has, but this needs to be
    verified.)

  - On each sample, compute whether the region we'd like to show on
    that sample is contained within the pre-rendered region. If not, 
    abort the sample.

Comment 2

[Botond Ballo [:botond]]

I'd like to get around to working on this, but I'm busy with other work at the moment, so I'm going to unassign myself to reflect that.

If someone else would like to take it up, I'd be happy to provide guidance.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: A recording to show partial prerendering results

Left: nightly with enabling partial prerender pref (checkerboarding)
Center: with the patch to stop compositing (completely janky)
Right: with an additional patch to report the jank back to the main-thread and re-prerener there (still janky)

Apparently the right side looks much better than the center one, but still it doesn't look good.
To mitigate the jank in the right example, we probably should;

1. have some margins for the prerendered area so that we can notify that we will try to composite outside of the prerender area before the animation reached to the edge of the prerender area
2. calculate a future transform value (at two or three frames ahead) and use it to tell the jank will happen

I think both is worth trying (I guess we should have both and both should be defined by prefs).

Also I think the current layout.animation.prerender.viewport-ratio-limit-{x,y} is too small for this kind of rotate animations. I'd like to use a greater value (given that we cap the value to layout.animation.prerender.absolute-limit-{x,y} which is 4096).

Comment 4

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #3)

1. have some margins for the prerendered area so that we can notify that we will try to composite outside of the prerender area before the animation reached to the edge of the prerender area

It turns out that this margin doesn't bring us better results at all, it often causes janks, and choosing a reasonable value would be quite difficult. I did choose 100px initially and the result looks bad, it often causes janks, if I choose a smaller value, say 10px, it doesn't help to pre-detect janks if the animation has short duration (i.e. faster moving animations).

For 2), I did try to use one frame ahead time stamp to pre-detect the jank (it's easily done because we use a previous time stamp for animation), to be honest I don't see much differences there on my machine, so I am going to defer this to a follow-up bug.

Anyways, with changes in bug 1634616, simple rotation animations look quite nice.

Also I am going to defer the implementation making jank on WebRender into another bug.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1324591 - A simple mochitest to make sure partial pre-rendered transform animations run on the compositor. r?boris

We just need this regardless of whether there appears checkerboarding or jank.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1324591 - Store the corresponding LayersId for each animation on the compositor. r?kats

Depends on D75727

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1324591 - Store PrerenderDecision in nsDisplayTransform. r?botond

Depends on D75728

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1324591 - Inform the partial prerender rect, the overflowed SideBits, the scroll id of the nearest scrollable frame and its rectable to the compositor. r?botond

nsDisplayTransform doesn't have enough room to store the partial prerender rect
and the overflowed SideBits unfortunately (it will exceed 512 bytes limit [1]).
So we get the partial prerender rect and calculate the overflowed SideBits just
before we send transform animation information to the compositor.

[1] https://searchfox.org/mozilla-central/rev/fa52bedc4b401c12251513fa1c9df1753a29abb2/layout/painting/nsDisplayList.cpp#7318

Depends on D75729

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1324591 - Jank if we are trying to composite area which is outside of the the partial prerender rect on non WebRender. r?botond

Depends on D75730

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1324591 - Report janked animations to the main-thread and update them on the main-thread. r?botond!,boris!

The machinery to report janked animations is;

1. Store the partial pre-rendered animation id and the Animation object in a
   hashtable in LayerManager
2. Store the animation id in the Animation object as well.
3. When we detect jank, we send the animation id to the main-thread via an IPC
   call
4. Find the Animation object with the id in the hashtable and update the
   Animaiton.
5. Whenever the partial pre-rendered Animation stop running on the compositor
   i.e. the Animation finished normall, the Animation's target element is
   changed, etc. etc., remove the Animation from the hashtable.

Depends on D75731

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: screencast.webm

I've finished writing patches mostly, but there are a couple of questions/things we need to agree on.

1. I used GetCombinedClipRect to get the clip rectangle to check whether the pre-rendered area is not overflowed or not. Does this sound a reasonable way? I understand the clip rect doesn't contain any APZ transforms by APZ, but unfortunately at the time when we sample animations by CSS/Web-APIs we haven't done sampling animatiosn by APZ, so it's hard to factor APZ transforms into it. I am attaching a screencast to see a result with the current setup. As you can see there are checkerboard-ish things and flickers happens. I believe some of them were caused by the periodical 200m unthrottling for transform animations (I'd like to drop it in bug 1638717 though), so probably it's fine?

2. There is a corner case that the jank detection function (ShouldBeJank) doesn't detect jank properly that is that the transform animation jumps to a position beyond the pre-rendered boundary in a single vsync tick. partial-prerender-translate-3.html is the reftest for this case. I am not sure we need to handle this case properly since even if there are such cases in the wild, it's probably hard to be noticeable since in such cases the expected visual result is "there is nothing to be composited" which mean it's the same result in the case we fail to detect the jank.

3. There is another corner case that the jank detection fails. The case is position:absolute animation in a transformed element like below case. I have no idea how to handle this case properly. I'd like to defer this case in a later bug since I've never seen this case in test cases/real sites in blocking bugs of bug 1100357.

<div style="transform: translateX(400px);">
  <div id="animating" style="position:absolute; top: 200px;"></div>
</div>

Botond, what do you think about these?

Comment 12

[Hiroyuki Ikezoe (:hiro)]

CAUSION! The video in comment 11 may make you feel dizzy (it does make me dizzy). Please don't watch it repeatedly.

Comment 13

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

2. There is a corner case that the jank detection function (ShouldBeJank) doesn't detect jank properly that is that the transform animation jumps to a position beyond the pre-rendered boundary in a single vsync tick. partial-prerender-translate-3.html is the reftest for this case. I am not sure we need to handle this case properly since even if there are such cases in the wild, it's probably hard to be noticeable since in such cases the expected visual result is "there is nothing to be composited" which mean it's the same result in the case we fail to detect the jank.

"since in such cases the expected visual result is "there is nothing to be composited" which mean it's the same result in the case we fail to detect the jank." this explanation is wrong. This is explaining about partial-prerender-translate-1.html, partial-prerender-translate-3.html would be visually noticeable, it's hard to, I am guessing.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

3. There is another corner case that the jank detection fails. The case is position:absolute animation in a transformed element like below case. I have no idea how to handle this case properly. I'd like to defer this case in a later bug since I've never seen this case in test cases/real sites in blocking bugs of bug 1100357.

<div style="transform: translateX(400px);">
  <div id="animating" style="position:absolute; top: 200px;"></div>
</div>

I noticed we need to factor transforms relative to the active scrolled root?
(and I noticed I didn't set NI)

Comment 15

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #11)

1. I used GetCombinedClipRect to get the clip rectangle to check whether the pre-rendered area is not overflowed or not. Does this sound a reasonable way? I understand the clip rect doesn't contain any APZ transforms by APZ, but unfortunately at the time when we sample animations by CSS/Web-APIs we haven't done sampling animatiosn by APZ, so it's hard to factor APZ transforms into it. I am attaching a screencast to see a result with the current setup. As you can see there are checkerboard-ish things and flickers happens. I believe some of them were caused by the periodical 200m unthrottling for transform animations (I'd like to drop it in bug 1638717 though), so probably it's fine?

2. There is a corner case that the jank detection function (ShouldBeJank) doesn't detect jank properly that is that the transform animation jumps to a position beyond the pre-rendered boundary in a single vsync tick. partial-prerender-translate-3.html is the reftest for this case. I am not sure we need to handle this case properly since even if there are such cases in the wild, it's probably hard to be noticeable since in such cases the expected visual result is "there is nothing to be composited" which mean it's the same result in the case we fail to detect the jank.

3. There is another corner case that the jank detection fails. The case is position:absolute animation in a transformed element like below case. I have no idea how to handle this case properly. I'd like to defer this case in a later bug since I've never seen this case in test cases/real sites in blocking bugs of bug 1100357.

I found ways to solve 1 and 2. But I found an issue on transform animations on position:fixed/stickyer. I will figure out a way to handle them. Then I am going to send review requests.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

I've decided to defer the position sticky case into bug 1642547. As for the position fixed case, I noticed that it doesn't matter other than the case where position:fixed layers are moved along with the dynamic toolbar on Fenix, even so it will not be a big problem either since the dynamic toolbar transition is quite small.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)

2. There is a corner case that the jank detection function (ShouldBeJank) doesn't detect jank properly that is that the transform animation jumps to a position beyond the pre-rendered boundary in a single vsync tick. partial-prerender-translate-3.html is the reftest for this case. I am not sure we need to handle this case properly since even if there are such cases in the wild, it's probably hard to be noticeable since in such cases the expected visual result is "there is nothing to be composited" which mean it's the same result in the case we fail to detect the jank.

Filed bug 1642575 for this case.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1324591 - Disable partial pre-render for frames in position:sticky subtree. r?botond

Depends on D75732

Comment 19

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1324591 - Add nsLayoutUtils::IsInPositionFixedSubtree. r?TYLin

Depends on D75729

Comment 20

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1324591 - Add APZSampler::GetCompositionBounds to get the composition bounds for a given pair of LayersId and scrollId. r?botond

Depends on D75730

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bc284863aff
A simple mochitest to make sure partial pre-rendered transform animations run on the compositor. r=boris
https://hg.mozilla.org/integration/autoland/rev/8e6d4e9f5aa0
Store the corresponding LayersId for each animation on the compositor. r=kats
https://hg.mozilla.org/integration/autoland/rev/e6c74ba41007
Store PrerenderDecision in nsDisplayTransform. r=botond
https://hg.mozilla.org/integration/autoland/rev/84657a3522fb
Add nsLayoutUtils::IsInPositionFixedSubtree. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/732804c83add
Inform the partial prerender rect, the overflowed SideBits, the scroll id of the nearest scrollable frame and its rectable to the compositor. r=botond
https://hg.mozilla.org/integration/autoland/rev/4a4ae4bd95d1
Add APZSampler::GetCompositionBounds to get the composition bounds for a given pair of LayersId and scrollId. r=botond
https://hg.mozilla.org/integration/autoland/rev/fef36ff2ea3d
Jank if we are trying to composite area which is outside of the the partial prerender rect on non WebRender. r=botond,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/d6a01c6bc40e
Report janked animations to the main-thread and update them on the main-thread. r=botond,boris
https://hg.mozilla.org/integration/autoland/rev/75966ee1fe65
Disable partial pre-render for frames in position:sticky subtree. r=botond

Comment 22

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/6bc284863aff
https://hg.mozilla.org/mozilla-central/rev/8e6d4e9f5aa0
https://hg.mozilla.org/mozilla-central/rev/e6c74ba41007
https://hg.mozilla.org/mozilla-central/rev/84657a3522fb
https://hg.mozilla.org/mozilla-central/rev/732804c83add
https://hg.mozilla.org/mozilla-central/rev/4a4ae4bd95d1
https://hg.mozilla.org/mozilla-central/rev/fef36ff2ea3d
https://hg.mozilla.org/mozilla-central/rev/d6a01c6bc40e
https://hg.mozilla.org/mozilla-central/rev/75966ee1fe65

Comment 23

[Cristian Brindusan [:cbrindusan]]

Backed out 9 changesets (bug 1324591) for linux build bustages on central on nsDisplayList.h.
https://hg.mozilla.org/integration/autoland/rev/65326426c5ecfe95b74f8097f0bdd9d8041bb42e

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&resultStatus=success%2Ctestfailed%2Cbusted%2Cexception&revision=75966ee1fe658b1767d7459256175c0662d14c25&selectedTaskRun=ea0h1eYKT22QTjYAH-D5ow.0

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=308603624&repo=mozilla-central&lineNumber=22119

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c258d455b290
A simple mochitest to make sure partial pre-rendered transform animations run on the compositor. r=boris
https://hg.mozilla.org/integration/autoland/rev/4e42d8d4df9d
Store the corresponding LayersId for each animation on the compositor. r=kats
https://hg.mozilla.org/integration/autoland/rev/b4248fe63414
Store PrerenderDecision in nsDisplayTransform. r=botond
https://hg.mozilla.org/integration/autoland/rev/2e9d5d1bd7db
Add nsLayoutUtils::IsInPositionFixedSubtree. r=TYLin
https://hg.mozilla.org/integration/autoland/rev/a77c8f5e7ec7
Inform the partial prerender rect, the overflowed SideBits, the scroll id of the nearest scrollable frame and its rectable to the compositor. r=botond
https://hg.mozilla.org/integration/autoland/rev/face25a6c173
Add APZSampler::GetCompositionBounds to get the composition bounds for a given pair of LayersId and scrollId. r=botond
https://hg.mozilla.org/integration/autoland/rev/76d8caea7f12
Jank if we are trying to composite area which is outside of the the partial prerender rect on non WebRender. r=botond,jrmuizel
https://hg.mozilla.org/integration/autoland/rev/5d5e5aafec2f
Report janked animations to the main-thread and update them on the main-thread. r=botond,boris
https://hg.mozilla.org/integration/autoland/rev/5c22eb861a63
Disable partial pre-render for frames in position:sticky subtree. r=botond

Comment 25

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c258d455b290
https://hg.mozilla.org/mozilla-central/rev/4e42d8d4df9d
https://hg.mozilla.org/mozilla-central/rev/b4248fe63414
https://hg.mozilla.org/mozilla-central/rev/2e9d5d1bd7db
https://hg.mozilla.org/mozilla-central/rev/a77c8f5e7ec7
https://hg.mozilla.org/mozilla-central/rev/face25a6c173
https://hg.mozilla.org/mozilla-central/rev/76d8caea7f12
https://hg.mozilla.org/mozilla-central/rev/5d5e5aafec2f
https://hg.mozilla.org/mozilla-central/rev/5c22eb861a63

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Finally! \o/ Thank you Botond for a long time your help!

Comment 27

[Botond Ballo [:botond]]

\o/ Thanks so much for working on this Hiro!