1461239 - Avoid doing repaints if animated properties haven't actually changed

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P2)

Description

[Jeff Muizelaar [:jrmuizel]]

The caret in Google docs has an animation. With WebRender we end up running it at 60fps compared to 12-14fps on trunk. The animated properties isn't changing for a reasonable portion of the time so we if we don't repaint if the value hasn't changed we should save a bunch of energy.

Comment 1

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

Presumably this is also a result of the SchedulePaint call that hiro linked to at https://bugzilla.mozilla.org/show_bug.cgi?id=1437036#c37

Comment 2

[Aria Beingessner [:Gankra]]

we can probably get away with not fixing this for a while but probably need it for prime-time, so going with wr-trains (P2).

Comment 3

[Hiroyuki Ikezoe (:hiro)]

The animation in question runs on the compositor completely.  So we have to do the optimization on the compositor.  The keyframe is;

@keyframes docs-text-ui-fadeoutin {
  from { opacity: 1 }
  13%  { opacity: 0 }
  50%  { opacity: 0 }
  63%  { opacity: 1 }
  to   { opacity: 1 }
}

What we could do there is that when we received this animation on the compositor, we precompute change hint-ish information between each keyframes (I guess we might need a subset of CalcStyleDifference, but for now we can simply compare values in the case of opacity), then we also precompute progress value for the animation on each tick before a transaction starts, then if the progress value is inside a keyframe which doesn't generate any changes, we can skip calculating the animation value and skip setting the value to WebRender  (And if there is no animation that generate changes, we can completely skip the transaction).  This might be a bit opposed to bug 1460759.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

CCing mconley since this is exactly what he really wanted.

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Thanks, hiro! Is this just for WebRender, or would fixing this impact old-school Layers-mode too?

Comment 6

[Hiroyuki Ikezoe (:hiro)]

This bug is for WebRender, but yeah, the approach in comment 3 should work for non-WebRender theoretically.  We need to modify CompositorBridgeParent.cpp for that.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Is layer tree invalidation keeping the frame-rate low on trunk?

Comment 8

[Markus Stange [:mstange]]

It should: On the compositor side, when we compare the layer trees, we will see that the opacity has not changed an compute an empty invalid region, and then we'll abort the composite.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: wip

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: wip

Comment 11

[Sotaro Ikeda [:sotaro]]

talos with attachment 9008686 [details] [diff] [review]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0fafb94d49c1770961331d9427e646e9fb5d805c&newProject=try&newRevision=04e2d200e9c161925a8e92e563b277c187611a97&framework=1

Comment 12

[Sotaro Ikeda [:sotaro]]

Attachment: wip

Comment 13

[Sotaro Ikeda [:sotaro]]

talos with attachment 9009848 [details] [diff] [review]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=0a4bd7e81d73f57bad53f2fdc06f2770c27f8113&newProject=try&newRevision=ca3a443129501856f51f6176adb71203688d292e&framework=1

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: wip

Code clean up.

Comment 15

[Sotaro Ikeda [:sotaro]]

talos with attachment 9010177 [details] [diff] [review]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=a55fefce43267c728edbb48ae59fdd2f6d7e589a&newProject=try&newRevision=ba3f6c308d1a55abfdb97dc7b346de146cf6a0e9&framework=1

Comment 16

[Sotaro Ikeda [:sotaro]]

Attachment: wip

Comment 17

[Maire Reavy [:mreavy]]

Hi Sotaro!  What's the status of this bug?  Are you likely to work on it next week?

Comment 18

[Sotaro Ikeda [:sotaro]]

Thanks for the notification. I am going to work next week. I wanted to work since yesterday, but I helped to look into another P1 bug.

Comment 19

[Sotaro Ikeda [:sotaro]]

Attachment: wip

Rebased.

Comment 20

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=209843d015625121b6d67bf41c5d9b0c30e582f9&newProject=try&newRevision=f044ed518d18408238107b902ac3579074c21ce9&framework=1

Comment 21

[Sotaro Ikeda [:sotaro]]

The talos performance regression seems to be addressed on Windows.

Comment 22

[Sotaro Ikeda [:sotaro]]

Attachment: wip

Rebased.

Comment 23

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=5374d3ef4e74ec6256038f6f7cbf701d4936f00d&newProject=try&newRevision=d357d169b8bba9ee3ca83da96e037deffe13d137&framework=1

Comment 24

[Sotaro Ikeda [:sotaro]]

Attachment: patch - change to WebRender

Comment 25

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Change to gecko

Comment 26

[Sotaro Ikeda [:sotaro]]

Comment on attachment 9013956 [details] [diff] [review]
patch - change to WebRender

:nical, can you feedback to the patch?

Comment 27

[Nicolas Silva [:nical]]

Let's talk about this after the weekly meeting. I'm not sure I fully understand the overall strategy in these patches, but I was hoping we could either detect that the frame is valid in webrender and skip rendering, or detect it in gecko and not schedule generating the frame. Either way it would be great if we could avoid adding some state that overrides the decision of whether or not to render.

Comment 28

[Sotaro Ikeda [:sotaro]]

OK. Basic idea is the following.

As a place of detecting if animation value is updated, there were 2 candidates.
- WebRenderBridgeParent::MaybeGenerateFrame()
- RenderBackend::update_document()

Then I chose RenderBackend::update_document() as a place of detecting if animation value is updated. It is because RenderBackend already checks if dynamic properties were changed by doc.dynamic_properties.flush_pending_updates(). We do not need additional checks of detecting if animation value is updated. And it could also optimize APZ scroll use case.

Current WR always do frame composition during generating frame when doc.has_pixels() is true. But it is not necessary if there is no update.

we could skip frame composition when doc.build_frame() is not called during generating frame in update_document() except the following use cases.
- Video frame is updated by reusing same ExternalImageId for different video frames. see Bug 1477608.
- Platform needs re-composite if pixel become stale (like wakeup from standby).
   It is notified by WebRenderBridgeParent::RecvScheduleComposite().

The patches implement the above and skip frame composition if possible.

Non-WR case, LayerManagerComposite::UpdateAndRender() also skips composition if there is no update.

>  if (invalid.IsEmpty() && !mWindowOverlayChanged) {
>    // Composition requested, but nothing has changed. Don't do any work.
>    mClonedLayerTreeProperties = LayerProperties::CloneFrom(GetRoot());
>    return;
>  }

https://dxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.cpp#534

Comment 29

[Nicolas Silva [:nical]]

Comment on attachment 9013956 [details] [diff] [review]
patch - change to WebRender

Review of attachment 9013956 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot for the details, it's clearer now. I agree with the approach.
I'm going to suggest some different naming but this is really only cosmetic: I'd like things related to the renderer to use "render" in their names to keep the vocabulary a simple and consistent, and also stay similar to the existing frame_is_valid mechanism. Other than that I like this patch.

::: gfx/webrender/src/render_backend.rs
@@ +132,5 @@
>      /// Track whether the last built frame is up to date or if it will need to be re-built
>      /// before rendering again.
>      frame_is_valid: bool,
>      hit_tester_is_valid: bool,
> +    composite_needed: bool,

How about naming this "rendered_frame_is_valid" (and invert the meaning of true/false)?

@@ +1003,5 @@
>          doc_resource_updates: Option<DocumentResourceUpdates>,
>          mut frame_ops: Vec<FrameMsg>,
>          mut notifications: Vec<NotificationRequest>,
>          mut render_frame: bool,
> +        request_composite: bool,

how about naming this: "invalidate_rendered_frame" ?

::: gfx/webrender/src/scene_builder.rs
@@ +43,5 @@
>      pub frame_ops: Vec<FrameMsg>,
>      pub notifications: Vec<NotificationRequest>,
>      pub set_root_pipeline: Option<PipelineId>,
>      pub render_frame: bool,
> +    pub request_composite: bool,

How about renaming it: "invalidate_rendered_frame" ?

@@ +77,5 @@
>      pub doc_resource_updates: Option<DocumentResourceUpdates>,
>      pub scene_build_start_time: u64,
>      pub scene_build_end_time: u64,
>      pub render_frame: bool,
> +    pub request_composite: bool,

("invalidate_rendered_frame" again, etc.)

::: gfx/webrender_api/src/api.rs
@@ +60,5 @@
>      use_scene_builder_thread: bool,
>  
>      generate_frame: bool,
>  
> +    request_composite: bool,

invalidate_rendered_frame

@@ +250,5 @@
>      pub fn generate_frame(&mut self) {
>          self.generate_frame = true;
>      }
>  
> +    pub fn force_generate_frame(&mut self) {

I'd like this to be named "invalidate_rendered_frame" and to be orthogonal to generating the frame. The video/resume use cases would call it, and we'd still also call generate_frame.
In the final patch we need a comment that mentions the two use cases.

Comment 30

[Sotaro Ikeda [:sotaro]]

Comment on attachment 9013956 [details] [diff] [review]
patch - change to WebRender

Review of attachment 9013956 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/webrender/src/render_backend.rs
@@ +132,5 @@
>      /// Track whether the last built frame is up to date or if it will need to be re-built
>      /// before rendering again.
>      frame_is_valid: bool,
>      hit_tester_is_valid: bool,
> +    composite_needed: bool,

Yes, it is better. I'll apply it in a next patch.

@@ +1003,5 @@
>          doc_resource_updates: Option<DocumentResourceUpdates>,
>          mut frame_ops: Vec<FrameMsg>,
>          mut notifications: Vec<NotificationRequest>,
>          mut render_frame: bool,
> +        request_composite: bool,

I'll apply it in a next patch.

::: gfx/webrender/src/scene_builder.rs
@@ +43,5 @@
>      pub frame_ops: Vec<FrameMsg>,
>      pub notifications: Vec<NotificationRequest>,
>      pub set_root_pipeline: Option<PipelineId>,
>      pub render_frame: bool,
> +    pub request_composite: bool,

I'll apply it in a next patch.

@@ +77,5 @@
>      pub doc_resource_updates: Option<DocumentResourceUpdates>,
>      pub scene_build_start_time: u64,
>      pub scene_build_end_time: u64,
>      pub render_frame: bool,
> +    pub request_composite: bool,

I'll apply it in a next patch.

::: gfx/webrender_api/src/api.rs
@@ +60,5 @@
>      use_scene_builder_thread: bool,
>  
>      generate_frame: bool,
>  
> +    request_composite: bool,

I'll apply it in a next patch.

@@ +250,5 @@
>      pub fn generate_frame(&mut self) {
>          self.generate_frame = true;
>      }
>  
> +    pub fn force_generate_frame(&mut self) {

Thanks for the comment. I'll apply it in a next patch!

Comment 31

[Sotaro Ikeda [:sotaro]]

Attachment: patch - change to WebRender

Apply the comments.

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Change to gecko

Comment 33

[Nicolas Silva [:nical]]

Comment on attachment 9014332 [details] [diff] [review]
patch - change to WebRender

Review of attachment 9014332 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks a lot for making the changes.

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: patch - change to WebRender

Comment 35

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Use InvalidateRenderedFrame() when necessary

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc87d3ddef61
Use InvalidateRenderedFrame() when necessary r=nical

Comment 37

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/bc87d3ddef61

Comment 38

[Ionuț Goldan [:igoldan]]

Either this bug or one of bug 1496670, bug 1495902 or bug 1496670 brought this perf improvement:

== Change summary for alert #16607 (as of Wed, 10 Oct 2018 00:20:39 GMT) ==

Improvements:

  5%  tscrollx linux64-qr opt e10s stylo     2.35 -> 2.25

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16607