Avoid doing repaints if animated properties haven't actually changed

RESOLVED FIXED in Firefox 64

Status

()

P2
normal
RESOLVED FIXED
9 months ago
4 months ago

People

(Reporter: jrmuizel, Assigned: sotaro)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {power})

unspecified
mozilla64
power
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 12 obsolete attachments)

(Reporter)

Description

9 months ago
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.
Keywords: power
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
Blocks: 1461311
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).
Blocks: 1386669
Priority: -- → P2
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.
No longer blocks: 1461311
CCing mconley since this is exactly what he really wanted.
Thanks, hiro! Is this just for WebRender, or would fixing this impact old-school Layers-mode too?
Flags: needinfo?(hikezoe)
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.
Flags: needinfo?(hikezoe)
(Reporter)

Comment 7

6 months ago
Is layer tree invalidation keeping the frame-rate low on trunk?
Flags: needinfo?(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.
Flags: needinfo?(mstange)
(Assignee)

Updated

5 months ago
Assignee: nobody → sotaro.ikeda.g
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment hidden (obsolete)
Hi Sotaro!  What's the status of this bug?  Are you likely to work on it next week?
Flags: needinfo?(sotaro.ikeda.g)
(Assignee)

Comment 18

5 months ago
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.
Flags: needinfo?(sotaro.ikeda.g)
Comment hidden (obsolete)
(Assignee)

Comment 21

5 months ago
The talos performance regression seems to be addressed on Windows.
(Assignee)

Updated

5 months ago
Blocks: 1416645
(Assignee)

Updated

5 months ago
Depends on: 1495352
(Assignee)

Comment 22

5 months ago
Created attachment 9013895 [details] [diff] [review]
wip

Rebased.
Attachment #9012798 - Attachment is obsolete: true
(Assignee)

Comment 24

5 months ago
Created attachment 9013956 [details] [diff] [review]
patch - change to WebRender
(Assignee)

Comment 25

5 months ago
Created attachment 9013958 [details] [diff] [review]
patch - Change to gecko
(Assignee)

Comment 26

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

:nical, can you feedback to the patch?
Attachment #9013956 - Flags: feedback?(nical.bugzilla)
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.
(Assignee)

Comment 28

5 months ago
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 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.
Attachment #9013956 - Flags: feedback?(nical.bugzilla) → feedback+
(Assignee)

Comment 30

5 months ago
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 hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Updated

5 months ago
Attachment #9014332 - Flags: review?(nical.bugzilla)
(Assignee)

Updated

5 months ago
Attachment #9014333 - Flags: review?(nical.bugzilla)
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.
Attachment #9014332 - Flags: review?(nical.bugzilla) → review+
Attachment #9014333 - Flags: review?(nical.bugzilla) → review+
(Assignee)

Updated

5 months ago
QA Contact: mreavy
Comment hidden (obsolete)
(Assignee)

Updated

5 months ago
Blocks: 1496670
(Assignee)

Comment 35

5 months ago
Created attachment 9014996 [details] [diff] [review]
patch - Use InvalidateRenderedFrame() when necessary
Attachment #9014333 - Attachment is obsolete: true
Attachment #9014996 - Flags: review+
(Assignee)

Updated

5 months ago
Attachment #9014698 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #9013895 - Attachment is obsolete: true
(Assignee)

Updated

5 months ago
Attachment #9014996 - Attachment description: patch - Change to gecko → patch - Use InvalidateRenderedFrame() when necessary

Comment 36

4 months ago
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bc87d3ddef61
Use InvalidateRenderedFrame() when necessary r=nical
(Reporter)

Updated

4 months ago
Blocks: 1497768
No longer blocks: 1497768

Comment 37

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bc87d3ddef61
Status: NEW → RESOLVED
Last Resolved: 4 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
(Assignee)

Updated

4 months ago
Depends on: 1498092
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
(Assignee)

Updated

4 months ago
Depends on: 1500821
You need to log in before you can comment on or make changes to this bug.