1419851 - Off-main-thread animations do not throttle the main thread refresh driver with WebRender

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P1)

Description

[Markus Stange [:mstange]]

Bug 1305976 is not reproducible when WebRender is enabled. You'd think that's a good thing, but it probably means that we run the refresh driver at its full rate even when all the animations that are running are running on the compositor.

Comment 1

[Ethan Lin[:ethlin]]

This bug may affect animation performance. I take a look.

Comment 2

[Ethan Lin[:ethlin]]

Attachment: Bug 1419851 - Part1. Add a static function to get AnimationInfo.

Review commit: https://reviewboard.mozilla.org/r/202366/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202366/

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

https://reviewboard.mozilla.org/r/202366/#review207826

Looks reasonable to me, but I'm that familiar with this code.

::: dom/animation/KeyframeEffectReadOnly.cpp:1467
(Diff revision 1)
> -      return false;
> +        return false;
> -    }
> +      }
> +    } else {
> +      nsPoint offset;
> +      RefPtr<layers::LayerManager> widgetLayerManager;
> +      if (nsIWidget* widget = nsContentUtils::GetWidget(GetPresShell(), &offset)) {

You can pass nullptr instead of &offset since you don't need the result

::: dom/animation/moz.build:71
(Diff revision 1)
>      '/dom/base',
>      '/layout/base',
>      '/layout/style',
>  ]
>  
> +include('/ipc/chromium/chromium-config.mozbuild')

Is this change required? It's not clear to me why it would be. If it is you'll need to get a build peer review on this

Comment 4

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Comment on attachment 8931240 [details]
> Bug 1419851 - Handle OMTA throttle for webrender.
> 
> https://reviewboard.mozilla.org/r/202366/#review207826
> 
> Looks reasonable to me, but I'm that familiar with this code.
> 
> ::: dom/animation/KeyframeEffectReadOnly.cpp:1467
> (Diff revision 1)
> > -      return false;
> > +        return false;
> > -    }
> > +      }
> > +    } else {
> > +      nsPoint offset;
> > +      RefPtr<layers::LayerManager> widgetLayerManager;
> > +      if (nsIWidget* widget = nsContentUtils::GetWidget(GetPresShell(), &offset)) {
> 
> You can pass nullptr instead of &offset since you don't need the result
> 
> ::: dom/animation/moz.build:71
> (Diff revision 1)
> >      '/dom/base',
> >      '/layout/base',
> >      '/layout/style',
> >  ]
> >  
> > +include('/ipc/chromium/chromium-config.mozbuild')
> 
> Is this change required? It's not clear to me why it would be. If it is
> you'll need to get a build peer review on this

Yes, it's necessary, or I get the compilation error:

 0:56.29 In file included from /Volumes/firefoxos2/graphics/obj-x86_64-apple-darwin16.5.0/dom/animation/Unified_cpp_dom_animation0.cpp:128:
 0:56.29 In file included from /Volumes/firefoxos2/graphics/dom/animation/KeyframeEffectReadOnly.cpp:23:
 0:56.29 In file included from /Volumes/firefoxos2/graphics/obj-x86_64-apple-darwin16.5.0/dist/include/mozilla/layers/WebRenderLayerManager.h:16:
 0:56.29 In file included from /Volumes/firefoxos2/graphics/obj-x86_64-apple-darwin16.5.0/dist/include/mozilla/layers/APZTestData.h:18:
 0:56.29 /Volumes/firefoxos2/graphics/obj-x86_64-apple-darwin16.5.0/dist/include/ipc/IPCMessageUtils.h:10:10: fatal error: 'base/process_util.h' file not found
 0:56.29 #include "base/process_util.h"

Comment 5

[Ethan Lin[:ethlin]]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202366/diff/1-2/

Comment 6

[Ethan Lin[:ethlin]]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202366/diff/2-3/

Comment 7

[Brian Birtles (:birtles)]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

https://reviewboard.mozilla.org/r/202366/#review208130

::: dom/animation/KeyframeEffectReadOnly.cpp:1465
(Diff revision 3)
> +      RefPtr<layers::LayerManager> widgetLayerManager;
> +      if (nsIWidget* widget = nsContentUtils::GetWidget(GetPresShell(), nullptr)) {
> +        widgetLayerManager = widget->GetLayerManager();
> +      }
> +      if (layers::WebRenderLayerManager *wrlm = widgetLayerManager ?
> +            widgetLayerManager->AsWebRenderLayerManager() : nullptr) {
> +        RefPtr<layers::WebRenderAnimationData> animationData = wrlm->CommandBuilder()
> +            .GetWebRenderUserData<layers::WebRenderAnimationData>(frame, (uint32_t)record.mLayerType);
> +        if (!animationData ||
> +            animationData->GetAnimationInfo().GetAnimationGeneration() != effectSet->GetAnimationGeneration()) {
> +          return false;
> +        }
> +      } else {
> +        return false;
> +      }
> +    }

This code doesn't belong in CanThrottle. Can you factor out a GetAnimationData helper (preferably somewhere in graphics code, if possible--hopefully after doing that you won't need to touch dom/animation/moz.build, nor include WebRender headers here). I guess it will need to take a pres context, layer type, and nsIFrame.

I'm pretty sure the code will get a lot more readable if you do that since you'll be able to use early returns instead of using null checks.

(Also, please wrap lines to 80 chars.)

Comment 8

[Ethan Lin[:ethlin]]

Attachment: Bug 1419851 - Part2. Handle OMTA throttle for webrender.

Review commit: https://reviewboard.mozilla.org/r/202866/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/202866/

Comment 9

[Ethan Lin[:ethlin]]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202366/diff/3-4/

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

https://reviewboard.mozilla.org/r/202366/#review208204

::: gfx/layers/AnimationInfo.h:64
(Diff revision 4)
>    AnimationArray& GetAnimations() { return mAnimations; }
>    bool ApplyPendingUpdatesForThisTransaction();
>    bool HasOpacityAnimation() const;
>    bool HasTransformAnimation() const;
>  
> +  static AnimationInfo* GetFromFrame(nsIFrame* aFrame,

Add a comment here warning that the returned pointer might be turned into garbage if the frame is destroyed (or possibly other conditions), and so callers should manage the lifetime of the pointer carefully.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8931728 [details]
Bug 1419851 - Part2. Handle OMTA throttle for webrender.

https://reviewboard.mozilla.org/r/202866/#review208206

::: dom/animation/KeyframeEffectReadOnly.cpp:1464
(Diff revision 1)
> +    if (!animationInfo ||
>          effectSet->GetAnimationGeneration() !=
> -          layer->GetAnimationGeneration()) {
> +        animationInfo->GetAnimationGeneration()) {
>        return false;
>      }
>  

It might be a good idea to explicitly null out the animationInfo pointer here after you're done with it, with a comment to indicate that it shouldn't be used beyond this point without verifying that it will not be garbage.

Comment 12

[Brian Birtles (:birtles)]

Comment on attachment 8931728 [details]
Bug 1419851 - Part2. Handle OMTA throttle for webrender.

https://reviewboard.mozilla.org/r/202866/#review208292

This looks better, thanks.

If all the raw pointer handling seems a bit dangerous and all the call sites of this function are only currently interested in the animation generation then I guess you could just scope the function to returning the animation generation.

Comment 13

[Ethan Lin[:ethlin]]

I'll set the animationInfo pointer to nullptr after using it for now. I will check if we can use this static function for other places to simplify our code, and see if we should change the return value of the static function.

Comment 14

[Ethan Lin[:ethlin]]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202366/diff/4-5/

Comment 15

[Ethan Lin[:ethlin]]

Comment on attachment 8931728 [details]
Bug 1419851 - Part2. Handle OMTA throttle for webrender.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202866/diff/1-2/

Comment 16

[Code Review Bot [:reviewbot]]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

https://reviewboard.mozilla.org/r/202366/#review208334


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: gfx/layers/AnimationInfo.cpp:176
(Diff revision 5)
> +{
> +  layers::Layer* layer =
> +    FrameLayerBuilder::GetDedicatedLayer(aFrame, aDisplayItemKey);
> +  if (layer) {
> +    return &layer->GetAnimationInfo();
> +  } else {

Warning: Do not use 'else' after 'return' [clang-tidy: readability-else-after-return]

Comment 17

[Ethan Lin[:ethlin]]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202366/diff/5-6/

Comment 18

[Ethan Lin[:ethlin]]

Comment on attachment 8931728 [details]
Bug 1419851 - Part2. Handle OMTA throttle for webrender.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202866/diff/2-3/

Comment 19

[Ethan Lin[:ethlin]]

According to the try result[1], our OMTA doesn't work correctly. Also we have memory leakage problem. I'll have another bug to fix memory leakage bug first. About the OMTA problem, I think it might be the invalidation region problem. I couldn't set fails-if to the tests since they are intermittent...

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=35212eaa22650f3675baadef3e520a0009d0a44c&selectedJob=147755124

Comment 20

[Ethan Lin[:ethlin]]

After fixing the memory leakage problem, I got this try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99f26c6306b1b57df8e36632f28494d6bcbc2477. (I also tried to change the annotations to make them passed.)

Comment 21

[Jeff Muizelaar [:jrmuizel]]

Fixing this should help page load times.

Comment 22

[Peter Chang[:pchang]]

I'm checking the try failures from comment 20.

Comment 23

[Peter Chang[:pchang]]

Looks like the intermittent failures are related to how we deal with 'reftest-wait' with WR. Still checking the reftest sequence between good and bad ones.

Comment 24

[Ethan Lin[:ethlin]]

Attachment: Bug 1419851 - Part3. Update reftest annotations.

Review commit: https://reviewboard.mozilla.org/r/205316/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205316/

Comment 25

[Ethan Lin[:ethlin]]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202366/diff/6-7/

Comment 26

[Ethan Lin[:ethlin]]

Comment on attachment 8931728 [details]
Bug 1419851 - Part2. Handle OMTA throttle for webrender.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/202866/diff/3-4/

Comment 27

[Ethan Lin[:ethlin]]

Currently OMTA works fine with the patches. But WR path doesn't know when to stop the reftest test since the animation is still playing. I don't have a good idea to fix it right now, so I want to change the annotations for those failures.

Here is the try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d17eb2a4c97f2c0390ea7b05f24eaeb47f4289d1

Comment 28

[Kartikaya Gupta (email:[e-mail])]

I'm pretty reluctant to do this as it could very well mask other OMTA breakage. Can you describe in more detail why the reftest doesn't stop?

Comment 29

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #28)
> I'm pretty reluctant to do this as it could very well mask other OMTA
> breakage. Can you describe in more detail why the reftest doesn't stop?

The problem is similar to bug 1404091. We need to decide whether to notify invalidation or not in the end of the transaction. If we notify invalidation, that means there are invalidation regions and the test will keep running. WR path cannot detect the real invalidation region. In bug 1404091, we notify invalidation if the transaction is triggered by scheduled flush since normally the scheduled flush means there are changes. For animation, it's also triggered by scheduled flush. But sometimes the animation may have no real invalidation region if it doesn't change anything.
For example, the test[1] have a animation that will change the opacity 100s later, but the test removes the reftest-wait when receiving requestAnimationFrame event. Non-WR will stop the test immediately because there is no real change in 100s. WR will wait 100s until the animation ends and may get wrong screenshot. Another test[2] has an infinite animation so I have to add skip-if for it.
Before this patch, WR also has the invalidation detection problem for animation but I think some redundant transactions which not from scheduled flush save it. After we apply the throttle mechanism, those redundant transactions are throttled so the problem is revealed.

[1] layout/reftests/css-animations/stacking-context-opacity-wins-over-transition.html
[2] layout/reftests/css-animations/opacity-animation-in-fixed-opacity-parent.html

Comment 30

[Kartikaya Gupta (email:[e-mail])]

Thanks for the explanation, that makes sense. It seems to me that we need to separate the cases where the flush is scheduled via animations from the other cases, and only set the mHasScheduleFlush (which was added in bug 1404091) in the "other" cases. That way animation-only flushes will not trigger the invalidation. We'll have to be careful about separating the cases where nothing changes (because the animation is done entirely on the compositor) from the cases where stuff does change on the main thread. Do you have a sense of how hard that would be?

Comment 31

[Ethan Lin[:ethlin]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #30)
> Thanks for the explanation, that makes sense. It seems to me that we need to
> separate the cases where the flush is scheduled via animations from the
> other cases, and only set the mHasScheduleFlush (which was added in bug
> 1404091) in the "other" cases. That way animation-only flushes will not
> trigger the invalidation. We'll have to be careful about separating the
> cases where nothing changes (because the animation is done entirely on the
> compositor) from the cases where stuff does change on the main thread. Do
> you have a sense of how hard that would be?

I thought this way before. I feel it's not easy. I think currently we don't know if the animation will cause any change unless we start to cache the last animation computed value. I'm also not sure how to distinguish an animation-only flush. It looks like the ScheduleFlush comes from ServoRestyleManager::DoProcessPendingRestyles[1]. We may need to create a new nsChangeHint[2] for animation since there is no change hint for animation, but it may conflict with the original restyle change hint.

[1] https://dxr.mozilla.org/mozilla-central/rev/79d3e25106f8eb369dde6a47199547d131af8d3d/layout/base/ServoRestyleManager.cpp#1161
[2] https://dxr.mozilla.org/mozilla-central/rev/79d3e25106f8eb369dde6a47199547d131af8d3d/layout/base/nsChangeHint.h#20

Comment 32

[Kartikaya Gupta (email:[e-mail])]

Sorry for the delay here, I've been trying to think of a better way to do this but haven't had enough time to really dig into the code. It might be something worth discussing with Matt Woodrow during the all hands if you get a chance.

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8934396 [details]
Bug 1419851 - Part3. Update reftest annotations.

https://reviewboard.mozilla.org/r/205316/#review213486

Dropping this review flag for now, since we have an alternate approach that we're looking into. Also just to note - if we had landed this, I wouldn't have caught some regressions in the patches I was working on for bug 1424686, so there is definitely value in making sure the tests stay enabled and passing as much as possible.

Comment 34

[Jeff Muizelaar [:jrmuizel]]

Any update on this?

Comment 35

[Ethan Lin[:ethlin]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #34)
> Any update on this?

It's blocked by bug 1425315. I'm still working on the reftest harness. Currently I only have a prototype. My latest try[1] shows that there is one test still has problem.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cd9b58fb9b8c0bc286d4780d22a59573bc1f7da&selectedJob=155272147

Comment 36

[Ethan Lin[:ethlin]]

Oh...after some investigations, the reftest failure is because it really has a changing transformation. But the change is too small to display. I think we can keep using fails-if for this reftest. I'll start to clean up my codes.

Comment 37

[Hiroyuki Ikezoe (:hiro)]

I just noticed that dom/animation/test/mozilla/test_restyles.html is disabled for WebRender;

https://hg.mozilla.org/mozilla-central/file/32b850fa28ae/dom/animation/test/mochitest.ini#l117

The test should work once OTMA works fine, if it's not it there must be other issues for OMTA.

Comment 38

[Kartikaya Gupta (email:[e-mail])]

I did a try push with the first two patches on this bug, rebased to tip

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6623029dfca685d709b8cea6c0078f1e8e33fc6a

As expected, there are still reftest failures.

I read through this bug again and gave it some more thought. The approach I outlined in comment 30 still feels right to me. Ethan said it wouldn't be easy to do (in comment 31) but I'd like to give it another shot and see if I can make it work.

Comment 39

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #38)
> I read through this bug again and gave it some more thought. The approach I
> outlined in comment 30 still feels right to me. Ethan said it wouldn't be
> easy to do (in comment 31) but I'd like to give it another shot and see if I
> can make it work.

The approach in comment 30 sounds reasonable to me.

As for stacking-context-transform-changing-display-property.html failure, I suspect there is another underlying problem regardless of webrender.  The test changes 'display' property from 'none' to 'block', at the moment, as far as I can tell, we should do RequestRestyle(Layer), but it seems not.  I did push a try run [1] to make the RequestRestyl(Layer) happens (it's a really dirty hack though).  The test still fails intermittently but not a perma anymore.  I haven't yet checked other failure tests, but there probably are other underlying issues. 

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=086fab8f4d2fd8c8313d9e685f9a996756c6655f

Comment 40

[Hiroyuki Ikezoe (:hiro)]

I found another underlying issue, it's bug 1388557.  stacking-context-transform-none-animation-before-appending-element.html failed because of that bug.

FWIW, here is a try with a dirty hack for bug 1388557.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75e3036135f1194e65a3275e5b8b802d6f33c7c9

Comment 41

[Hiroyuki Ikezoe (:hiro)]

Here is a new try based on bug 1447874 and bug 1447870.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f96c4bdb2eb53902ad44aef55f502877a5edbf45

There are still four failures in the try.
1) animate-preserve3d-parent.html
2) animate-preserve3d-child.html
3) table-overflowed-by-animation.html
4) continuation-opacity.html 

As for the first three they don't run on the compositor yet (bug 779598 and bug 1320608). there are still something broken.

For the last one, it's bit complicated, it's related to continuation frames, I guess we need to tweak GetWebRenderUserData for continuations?

Comment 42

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8931240 [details]
Bug 1419851 - Part1. Add a static function to get AnimationInfo.

https://reviewboard.mozilla.org/r/202366/#review236330

::: gfx/layers/AnimationInfo.cpp:184
(Diff revision 7)
> +      RefPtr<WebRenderAnimationData> animationData = wrlm->CommandBuilder()
> +          .GetWebRenderUserData<WebRenderAnimationData>(aFrame, (uint32_t)aDisplayItemKey);
> +      if (animationData) {
> +        info = &animationData->GetAnimationInfo();

I noticed that nobody holds this animationData reference here.

Comment 43

[Kartikaya Gupta (email:[e-mail])]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #42)
> I noticed that nobody holds this animationData reference here.

Yeah, but as long as the frame is alive the animation data should be valid. Anyway I rebased these patches (part 1 and part 2) and changed the function to return the generation (as a Maybe<uint64_t>) instead of the animation info which is safer and more tightly scoped (because we only need the generation at the call site anyway).

I'm building it now and will do a try push shortly to see how things stand now.

Comment 44

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=960cfc605927ad61142c6dc2b71c20f59533daa1

Comment 45

[Kartikaya Gupta (email:[e-mail])]

That one didn't build. Here's a better one: https://treeherder.mozilla.org/#/jobs?repo=try&revision=00bca153ae56a64022d6c69523188e36115d0c96

It looks green, so maybe this is good to go now!

Comment 46

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1419851 - Handle OMTA throttling for webrender.

Original patch by Ethan Lin <[e-mail]>.

Review commit: https://reviewboard.mozilla.org/r/234538/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/234538/

Comment 47

[Kartikaya Gupta (email:[e-mail])]

The new patch is effectively the same as the old part 1 + part 2, but rebased to m-c and simplified somewhat.

Comment 48

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8965694 [details]
Bug 1419851 - Handle OMTA throttling for webrender.

https://reviewboard.mozilla.org/r/234538/#review240176

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c721699dba5
Handle OMTA throttling for webrender. r=jrmuizel

Comment 50

[Noemi Erli[:noemi_erli]]

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

Comment 51

[Hiroyuki Ikezoe (:hiro)]

\o/