Closed Bug 1419851 Opened 7 years ago Closed 6 years ago

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

Categories

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

Product:
Core
Component:
Graphics: WebRender
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox59 --- unaffected
firefox61 --- fixed

People

(Reporter: mstange, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Keywords: power, Whiteboard: [wr-mvp])

Attachments

(4 files)

Bug 1419851 - Part1. Add a static function to get AnimationInfo.
59 bytes, text/x-review-board-request
kats
: review+
kats
: checkin-
Details
Bug 1419851 - Part2. Handle OMTA throttle for webrender.
59 bytes, text/x-review-board-request
birtles
: review+
kats
: review+
kats
: checkin-
Details
Bug 1419851 - Part3. Update reftest annotations.
59 bytes, text/x-review-board-request
Details
Bug 1419851 - Handle OMTA throttling for webrender.
59 bytes, text/x-review-board-request
jrmuizel
: review+
Details 
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.
Whiteboard: [wr-mvp] [triage]
This bug may affect animation performance. I take a look.
Assignee: nobody → ethlin
Blocks: 1416974
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [wr-mvp] [triage] → [wr-mvp]
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
Attachment #8931240 - Flags: review?(bugmail) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 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.)
Attachment #8931240 - Flags: review?(bbirtles)
Attachment #8931240 - Flags: review+ → review?(bugmail)
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.
Attachment #8931240 - Flags: review?(bugmail) → review+
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.
Attachment #8931728 - Flags: review?(bugmail) → review+
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.
Attachment #8931728 - Flags: review?(bbirtles) → review+
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 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]
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
Depends on: 1421200
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.)
Fixing this should help page load times.
I'm checking the try failures from comment 20.
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.
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
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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
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?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
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.
Flags: needinfo?(matt.woodrow)
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.
Attachment #8934396 - Flags: review?(bugmail)
Depends on: 1425315
Flags: needinfo?(matt.woodrow)
Any update on this?
Flags: needinfo?(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
Flags: needinfo?(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.
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.
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.
Assignee: demo99 → bugmail
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
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
See Also: → 1388557
Depends on: 1388557
Depends on: 1439279
Depends on: 1439269
Depends on: 1439803
Depends on: 1439824
Depends on: 1440556
Depends on: 1441713
Depends on: 1442063
Depends on: 1447870
Depends on: 1447874
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 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.
(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.
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!
The new patch is effectively the same as the old part 1 + part 2, but rebased to m-c and simplified somewhat.
Comment on attachment 8965694 [details]
Bug 1419851 - Handle OMTA throttling for webrender.

https://reviewboard.mozilla.org/r/234538/#review240176
Attachment #8965694 - Flags: review?(jmuizelaar) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0c721699dba5
Handle OMTA throttling for webrender. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/0c721699dba5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1453953
Depends on: 1454763
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: