Closed Bug 1453360 Opened 5 years ago Closed 5 years ago

Keep animation infos for multiple epochs


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

Other Branch



Tracking Status
firefox61 --- fixed


(Reporter: kats, Assigned: kats)


(Blocks 1 open bug)


(Whiteboard: [gfx-noted])


(2 files, 1 obsolete file)

Right now, we get updated OMTA info as part of the display list in WebRenderBridgeParent. The display list is sent to WR at the same time that mAnimationInfo is updated in WebRenderBridgeParent, and the display list is built into a scene directly on the render backend thread. This means that mAnimationInfo is always in sync with the built scene, and any time we send a GenerateFrame transaction, the dynamic properties that we send as part of that transaction correspond to the most recently built scene in WR.

However, with async scene building enabled, this is no longer the case. When we get a display list from content in WRBP, we send the display list to WR, but it doesn't get built right away - instead, it gets built on the scene builder thread asynchronously. However, we also update mAnimationInfo upon receiving the display list. So now we are out of sync - if we send a GenerateFrame transaction at this point, it will contain animation info for the new scene, which might not be ready yet. WR might end up compositing the old scene with the new animation info, with wonky results.

So we need some way to keeping the old animation info around until the scene they correspond to is built, and only at the point of the scene swap do we want to switch to the new animation info. Bug 1449982 set up the equivalent machinery for APZ. I'm not yet sure if we want to piggyback on that somehow, or essentially duplicate it, or if we can find some simpler alternative.
I realized that this is much simpler than I thought. The key is that it doesn't matter if we send *more* PropertyBindingValue entries to WR than are present in the current scene, which means we don't need to be exact with respect to which animations are for which epoch. Instead, we just need to make sure we don't delete an animation's info as long as there is still a scene somewhere that uses it. So what we can do is, whenever we get a command to delete animations, we store the epoch that's associated with, and then only delete the animations once that epoch has been rendered. This will basically ensure that we prolong the lifetime of the animation infos for as long as they are being used.
This will also allow us to move the DiscardCompositorAnimations call to the end of the transaction in which they are discarded, rather than to the beginning of the next transaction, which is what :hiro wanted in This will be safe to do because on the compositor side it won't matter when the message is received; the animations will be discarded only once the appropriate epoch has been rendered. As long as we flush the rendering pipeline, that should result in the same set of passing tests as what :hiro said in that comment.
Attached patch WIP (obsolete) — Splinter Review
Assignee: nobody → bugmail
Attachment #8971349 - Attachment is obsolete: true
Comment on attachment 8971576 [details]
Bug 1453360 - Rename NotifyDidCompositeToPipeline to NotifyPipelineRendered.
Attachment #8971576 - Flags: review?(nical.bugzilla) → review+
Comment on attachment 8971577 [details]
Bug 1453360 - Store the compositor animation ids to delete until the epoch is rendered.
Attachment #8971577 - Flags: review?(nical.bugzilla) → review+
Pushed by
Rename NotifyDidCompositeToPipeline to NotifyPipelineRendered. r=nical
Store the compositor animation ids to delete until the epoch is rendered. r=nical
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.