1363830 - Async animations with webrender should use a one-frame-old timestamp

Status: RESOLVED FIXED

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

Description

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

The non-webrender codepath uses the timestamp from the previous frame when advancing animations [1]. However the WR codepath just uses the current timestamp [2]. Presumably the WR codepath should also use an older timestamp.

[1] http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/gfx/layers/composite/AsyncCompositionManager.cpp#1360
[2] http://searchfox.org/mozilla-central/rev/7057a51c5cf29b5b115b1db19ace2cfe3fd83a0e/gfx/layers/wr/WebRenderBridgeParent.cpp#701

Comment 1

[Peter Chang[:pchang]]

Thanks for filing the bug, I will work on this and see any test case we can add to address the sampling timestamp.

Comment 2

[Peter Chang[:pchang]]

Attachment: Bug 1363830 - use previous frame time when advancing animations,

Review commit: https://reviewboard.mozilla.org/r/158224/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/158224/

Comment 3

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

Comment on attachment 8887378 [details]
Bug 1363830 - use previous frame time when advancing animations,

https://reviewboard.mozilla.org/r/158224/#review163576

Can we not just subtract one vsync interval from the the current timestamp? Seems like that would be simpler and would avoid the boundary conditions.

Comment 4

[Peter Chang[:pchang]]

Comment on attachment 8887378 [details]
Bug 1363830 - use previous frame time when advancing animations,

https://reviewboard.mozilla.org/r/158224/#review163576

I'm fine to subtract one vsync interval from the current timestamp. BTW, I found APZ uses the next vsync for animations which is more reasonable for me. How about we use next vsync for both?

Comment 5

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

I'm not sure the APZ code is correct in that respect. And Botond recently changed some of that behaviour in non-WR code (bug 1375949).

But looking at the code in AsyncCompositionManager I guess it also uses the mPreviousFrameTimeStamp and has the same behaviour as your patch, so let's just go with what you have now.

Comment 6

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

Comment on attachment 8887378 [details]
Bug 1363830 - use previous frame time when advancing animations,

https://reviewboard.mozilla.org/r/158224/#review164234

Comment 7

[Mozilla Autoland]

hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 2 changes to 2 files
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Error accessing https://api.pub.build.mozilla.org/treestatus/trees/autoland :
remote: HTTP Error 500: INTERNAL SERVER ERROR
remote: Unable to check if the tree is open - treating as if CLOSED.
remote: To push regardless, include "CLOSED TREE" in your push comment.
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.a_treeclosure hook failed
abort: push failed on remote

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9cbe3a1e132
use previous frame time when advancing animations, r=kats

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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