Closed Bug 1453360 Opened 5 years ago Closed 5 years ago

Keep animation infos for multiple epochs

Categories

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

Product:
Core
Component:
Graphics: WebRender
Version:
Other Branch
Type:
enhancement

Tracking

()

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

People

(Reporter: kats, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files, 1 obsolete file)

Bug 1453360 - Rename NotifyDidCompositeToPipeline to NotifyPipelineRendered.
59 bytes, text/x-review-board-request
nical
: review+
Details
Bug 1453360 - Store the compositor animation ids to delete until the epoch is rendered.
59 bytes, text/x-review-board-request
nical
: review+
Details 
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 https://bugzilla.mozilla.org/show_bug.cgi?id=1455999#c16. 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.

https://reviewboard.mozilla.org/r/240350/#review246338

        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.

https://reviewboard.mozilla.org/r/240352/#review246340

        Attachment #8971577 -
        Flags: review?(nical.bugzilla) → review+

    
    

    
  
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/59f4e493272f
Rename NotifyDidCompositeToPipeline to NotifyPipelineRendered. r=nical
https://hg.mozilla.org/integration/autoland/rev/ed8dc255b508
Store the compositor animation ids to delete until the epoch is rendered. r=nical

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/59f4e493272f
https://hg.mozilla.org/mozilla-central/rev/ed8dc255b508
Status: NEW → RESOLVED
Closed: 5 years ago

          status-firefox61:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61

          You need to log in
          before you can comment on or make changes to this bug.