http://codepen.io/anon/pen/JYRQPd?editors=110 The linked example above shows two animation techniques to achieve the same effect, one using positioning and one using translation. The box-shadow flickers noticeably when using `translateX`. Given advice around potential performance gains[1] from using translate instead of positioning for these sorts of animations, this could lead to developer frustration. Also, using translation for the animation allows you to use positioning to move the entire animated element independent of the animation. [1] http://www.paulirish.com/2012/why-moving-elements-with-translate-is-better-than-posabs-topleft/

Tested in Stable 41.0 and Nightly 44.0a1 (2015-09-24)

Looks like a compositor animations bug. I'm seeing the same on Windows.

This flickers because there are two animations running in parallel, one on the compositor and one on the main thread. The test relies on those staying in sync - animation updates that happen at the same time are expected to be composited in the same frame. At the moment, both main thread refresh driver ticks and compositor thread compositions happen on every vsync - however, what's being composited is whatever the main thread calculated at the *previous* vsync. So in order for the two animations to be in sync, the main thread animation update needs to happen one vsync tick before it happens on the compositor. This could be achieved by adjusting the animation sample timestamp, such that all inputs to a composited frame are using the same sample time. Do we already try to do something like that?

To be clear, the two animations are the box-shadow animation (which requires main thread repaints) and the transform animation (which can run on the compositor).

(In reply to Markus Stange [:mstange] from comment #3) > This flickers because there are two animations running in parallel, one on > the compositor and one on the main thread. The test relies on those staying > in sync - animation updates that happen at the same time are expected to be > composited in the same frame. > At the moment, both main thread refresh driver ticks and compositor thread > compositions happen on every vsync - however, what's being composited is > whatever the main thread calculated at the *previous* vsync. So in order for > the two animations to be in sync, the main thread animation update needs to > happen one vsync tick before it happens on the compositor. This could be > achieved by adjusting the animation sample timestamp, such that all inputs > to a composited frame are using the same sample time. > Do we already try to do something like that? No we don't do something like this, although we probably should.

One thing we do currently do, however, is disable compositor animations when we are animating the transform property and geometry properties like left/top/etc. on the same element. The simplest fix for this would be to apply to the same handling to box-shadow. That's obviously less than ideal, however.

(In reply to Mason Chang PTO [:mchang] from comment #5) > (In reply to Markus Stange [:mstange] from comment #3) > > This flickers because there are two animations running in parallel, one on > > the compositor and one on the main thread. The test relies on those staying > > in sync - animation updates that happen at the same time are expected to be > > composited in the same frame. > > At the moment, both main thread refresh driver ticks and compositor thread > > compositions happen on every vsync - however, what's being composited is > > whatever the main thread calculated at the *previous* vsync. So in order for > > the two animations to be in sync, the main thread animation update needs to > > happen one vsync tick before it happens on the compositor. This could be > > achieved by adjusting the animation sample timestamp, such that all inputs > > to a composited frame are using the same sample time. > > Do we already try to do something like that? > > No we don't do something like this, although we probably should. So if this is something that's technically required to be correct, are we going to schedule this work and assign it a priority? ;-)

(In reply to Bas Schouten (:bas.schouten) from comment #7) > (In reply to Mason Chang PTO [:mchang] from comment #5) > > (In reply to Markus Stange [:mstange] from comment #3) > > > This flickers because there are two animations running in parallel, one on > > > the compositor and one on the main thread. The test relies on those staying > > > in sync - animation updates that happen at the same time are expected to be > > > composited in the same frame. > > > At the moment, both main thread refresh driver ticks and compositor thread > > > compositions happen on every vsync - however, what's being composited is > > > whatever the main thread calculated at the *previous* vsync. So in order for > > > the two animations to be in sync, the main thread animation update needs to > > > happen one vsync tick before it happens on the compositor. This could be > > > achieved by adjusting the animation sample timestamp, such that all inputs > > > to a composited frame are using the same sample time. > > > Do we already try to do something like that? > > > > No we don't do something like this, although we probably should. > > So if this is something that's technically required to be correct, are we > going to schedule this work and assign it a priority? ;-) Not quite sure when actually. We've always had this problem since the compositor and refresh driver were on independent timers, even before silk. Now we just have an ordered 1 frame difference whereas before we had a random time difference. I won't have time to fix this for a while.

(In reply to Brian Birtles (:birtles) from comment #6) > One thing we do currently do, however, is disable compositor animations when > we are animating the transform property and geometry properties like > left/top/etc. on the same element. The simplest fix for this would be to > apply to the same handling to box-shadow. That's obviously less than ideal, > however. Just adding box-shadow to the list of exclusions seems somewhat futile - I can think of many other properties that would show similar problems, for example you could mix opacity and background-color... And even if the timestamp issue is fixed, small amounts of main thread jank would still cause us to hit this problem. How about we instead special-case step animations? And whenever there's a main thread animation with a step at the same time as an OMTA step, we disable the OMTA?

(In reply to Markus Stange [:mstange] from comment #9) > (In reply to Brian Birtles (:birtles) from comment #6) > > One thing we do currently do, however, is disable compositor animations when > > we are animating the transform property and geometry properties like > > left/top/etc. on the same element. The simplest fix for this would be to > > apply to the same handling to box-shadow. That's obviously less than ideal, > > however. > > Just adding box-shadow to the list of exclusions seems somewhat futile - I > can think of many other properties that would show similar problems, for > example you could mix opacity and background-color... The reason we special case geometric properties is that they are much more noticeable where they are out of sync. If you have an opacity fade and a color fade that are out of sync it's much less noticeable. > How about we instead special-case step animations? And whenever there's a > main thread animation with a step at the same time as an OMTA step, we > disable the OMTA? That would work. Let me check I understand what you mean--disable any OMTA for any animations with a step timing function when there's another running main thread animation on the same element with a step timing function? (I wouldn't want to disable step animation on the main thread in general since at lot of spinners use step timing function and they should continue uninterrupted when the main thread is busy. Also, I wouldn't want to check *where* the steps are in the different animations since that seems too fiddly. But I don't think you're suggesting either of those things.)

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

A try result with attachment 8740212 [details] seems to be good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac322273a2f4 There is a new orange on OS X 10.10 but I don't think it's related to the patch because the test is for worker. https://treeherder.mozilla.org/logviewer.html#?job_id=19268630&repo=try

Just want to make sure that APZ would be ok with this before r+. Kats, do you forsee any APZ problems with having the Compositor and Refresh driver be on the same timestamp rather than having the compositor technically be one frame ahead?

So pushing the compositor vsync timestamp backwards might cause skipped composites because of the check at [1] in certain conditions (like after a force-compose, after resuming the compositor, and anywhere else mLastCompose is set to TimeStamp::Now()). That might result in badness, but that's unrelated to APZ. I don't *think* APZ would be impacted by this change, from a quick look through it looks all the APZ animations use time deltas rather than absolute time so it should work out the same. The checkerboard recording code might report one less frame of checkerboard when APZCs are first created, but that shouldn't be a problem. However, I'm wondering if a better fix might just be to subtract off one vsync interval of time from the |aCurrentFrame| timestamp at [2], which is used to compute the OMTA position. We do a similar thing for APZ code a few lines below, except we advance the timestamp to the next vsync. [1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorBridgeParent.cpp?rev=0cfe55a2eb1f#536 [2] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?rev=0cfe55a2eb1f#1401

Kats brings up a good point. Other than that, my only issue with the patch is that it says it's doing what it's doing to "mitigate flicker". I think we should instead say something along the lines of "to make main thread animations and compositor animations more in sync with each other", and it should explain that if there is a main thread animation, what we composite at the current vsync is the result of what the main thread painted at the *previous* vsync.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14) > So pushing the compositor vsync timestamp backwards might cause skipped > composites because of the check at [1] in certain conditions (like after a > force-compose, after resuming the compositor, and anywhere else mLastCompose > is set to TimeStamp::Now()). That might result in badness, but that's > unrelated to APZ. I don't *think* APZ would be impacted by this change, from > a quick look through it looks all the APZ animations use time deltas rather > than absolute time so it should work out the same. The checkerboard > recording code might report one less frame of checkerboard when APZCs are > first created, but that shouldn't be a problem. > > However, I'm wondering if a better fix might just be to subtract off one > vsync interval of time from the |aCurrentFrame| timestamp at [2], which is > used to compute the OMTA position. We do a similar thing for APZ code a few > lines below, except we advance the timestamp to the next vsync. The reason I did not choose to subtract the interval time is that each vsync time stamp on Linux is not so accurate because it's based on software timer. To avoid any effect against APZ, AsyncCompositionManager has the previous vsync time stamp and uses it only for SampleAnimation(). Is that OK with you? One thing I am concerned about this approach is that I have no idea how to write automation tests for it though.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16) > The reason I did not choose to subtract the interval time is that each vsync > time stamp on Linux is not so accurate because it's based on software timer. Ok, that seems reasonable. > To avoid any effect against APZ, AsyncCompositionManager has the previous > vsync time stamp and uses it only for SampleAnimation(). Is that OK with > you? I'm not entirely sure what you mean by this. By "previous vsync time stamp", do you mean the timestamp that the compositor is getting in the current code (i.e. the one that matches the main-thread timestamp)? Or do you mean the timestamp that would be the "vsync timestamp" minus "one vsync interval"? > One thing I am concerned about this approach is that I have no idea how to > write automation tests for it though. I'm not sure either. I remember seeing a bunch of interesting machinery in layout/style/test/test_animations_omta.html that might be useful, but I don't really know.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #17) > (In reply to Hiroyuki Ikezoe (:hiro) from comment #16) > > The reason I did not choose to subtract the interval time is that each vsync > > time stamp on Linux is not so accurate because it's based on software timer. > > Ok, that seems reasonable. > > > To avoid any effect against APZ, AsyncCompositionManager has the previous > > vsync time stamp and uses it only for SampleAnimation(). Is that OK with > > you? > > I'm not entirely sure what you mean by this. By "previous vsync time stamp", > do you mean the timestamp that the compositor is getting in the current code > (i.e. the one that matches the main-thread timestamp)? Or do you mean the > timestamp that would be the "vsync timestamp" minus "one vsync interval"? I meant something similar to the latter. I meant vsync timestamp on the compositor matches to vsync timestamp one frame behind on the main thread like this: frame x frame x+1 frame x+2 main thread: 100 120.6 (delayed for some reason) ... compositor : 83.4 100 120.6 > > One thing I am concerned about this approach is that I have no idea how to > > write automation tests for it though. > > I'm not sure either. I remember seeing a bunch of interesting machinery in > layout/style/test/test_animations_omta.html that might be useful, but I > don't really know. Yes, there are bunch of tests but those are run on test control mode. I don't think it's suitable here.

Ok. I'm not 100% sure there won't be problems but I think it's worth trying.

Comment on attachment 8740212 [details] Bug 1208411 - Delay vsync timestamps on the compositor to make async animations more sync. Please request review after implementing comment 18. Thanks!

Comment on attachment 8740212 [details] Bug 1208411 - Delay vsync timestamps on the compositor to make async animations more sync. https://reviewboard.mozilla.org/r/45633/#review45281

Here is video to compare the effect of using the previous vsync time. Firefox on left side with a patch[1] and another firefox is nightly on 2016-06-20. Flickers apparently are less frequent on the left side firefox (but still happens, I could not record it in this video though). [1] https://hg.mozilla.org/try/rev/4818ae852617d3bf9625b9430df3acbf6aa25edf

Here is another video recorded an example in bug 1255646.

Comment on attachment 8740212 [details] Bug 1208411 - Delay vsync timestamps on the compositor to make async animations more sync. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45633/diff/1-2/

https://reviewboard.mozilla.org/r/45633/#review56994 ::: gfx/layers/composite/AsyncCompositionManager.cpp:1469 (Diff revision 2) > + // On the initial frame we use aVsyncTimestamp here so the timestamp on the > + // second frame are the same as the initial frame, but it does not matter. > + bool wantNextFrame = SampleAnimations(root, > + !mPreviousFrame.IsNull() ? mPreviousFrame : aCurrentFrame); > + > + mPreviousFrame = aCurrentFrame; I think this is mostly ok, but you will have a problem here if the compositor is idle for a while, then we start up again. The previous frame vsync will be far behind from the current frame, so you probably want to use the current frame. Maybe you can hook into when vsync is disabled or when an animation stops to reset mPreviousFrame to null?

(In reply to Mason Chang [:mchang] from comment #25) > https://reviewboard.mozilla.org/r/45633/#review56994 > > ::: gfx/layers/composite/AsyncCompositionManager.cpp:1469 > (Diff revision 2) > > + // On the initial frame we use aVsyncTimestamp here so the timestamp on the > > + // second frame are the same as the initial frame, but it does not matter. > > + bool wantNextFrame = SampleAnimations(root, > > + !mPreviousFrame.IsNull() ? mPreviousFrame : aCurrentFrame); > > + > > + mPreviousFrame = aCurrentFrame; > > I think this is mostly ok, but you will have a problem here if the > compositor is idle for a while, then we start up again. The previous frame > vsync will be far behind from the current frame, so you probably want to use > the current frame. Maybe you can hook into when vsync is disabled or when an > animation stops to reset mPreviousFrame to null? Yes, indeed! Thanks Mason!

Comment on attachment 8740212 [details] Bug 1208411 - Delay vsync timestamps on the compositor to make async animations more sync. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45633/diff/2-3/

Comment on attachment 8740212 [details] Bug 1208411 - Delay vsync timestamps on the compositor to make async animations more sync. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45633/diff/3-4/

https://reviewboard.mozilla.org/r/45631/#review56998 ::: gfx/layers/ipc/CompositorBridgeParent.cpp:536 (Diff revision 4) > CompositorVsyncScheduler::UnobserveVsync() > { > MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); > mCompositorVsyncDispatcher->SetCompositorVsyncObserver(nullptr); > mIsObservingVsync = false; > + if (mCompositorBridgeParent) { Do we need this if we already are checking in the animation? If we don't need another frame, that will already reset the previous timestamp right?

(In reply to Mason Chang [:mchang] from comment #29) > https://reviewboard.mozilla.org/r/45631/#review56998 > > ::: gfx/layers/ipc/CompositorBridgeParent.cpp:536 > (Diff revision 4) > > CompositorVsyncScheduler::UnobserveVsync() > > { > > MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); > > mCompositorVsyncDispatcher->SetCompositorVsyncObserver(nullptr); > > mIsObservingVsync = false; > > + if (mCompositorBridgeParent) { > > Do we need this if we already are checking in the animation? If we don't > need another frame, that will already reset the previous timestamp right? I am not really sure but I have an impression that there is a case that UnobserveVsync() is called even if there are running animations at https://hg.mozilla.org/mozilla-central/file/51377a641589/gfx/layers/ipc/CompositorBridgeParent.cpp#l484

(In reply to Hiroyuki Ikezoe (:hiro) from comment #30) > (In reply to Mason Chang [:mchang] from comment #29) > > https://reviewboard.mozilla.org/r/45631/#review56998 > > > > ::: gfx/layers/ipc/CompositorBridgeParent.cpp:536 > > (Diff revision 4) > > > CompositorVsyncScheduler::UnobserveVsync() > > > { > > > MOZ_ASSERT(CompositorThreadHolder::IsInCompositorThread()); > > > mCompositorVsyncDispatcher->SetCompositorVsyncObserver(nullptr); > > > mIsObservingVsync = false; > > > + if (mCompositorBridgeParent) { > > > > Do we need this if we already are checking in the animation? If we don't > > need another frame, that will already reset the previous timestamp right? > > I am not really sure but I have an impression that there is a case that > UnobserveVsync() is called even if there are running animations at > https://hg.mozilla.org/mozilla-central/file/51377a641589/gfx/layers/ipc/ > CompositorBridgeParent.cpp#l484 That shouldn't happen, otherwise the animation wouldn't continue also. If the animation is still going, shouldn't we have to composite it?

Comment on attachment 8740212 [details] Bug 1208411 - Delay vsync timestamps on the compositor to make async animations more sync. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45633/diff/4-5/

Comment on attachment 8740212 [details] Bug 1208411 - Delay vsync timestamps on the compositor to make async animations more sync. https://reviewboard.mozilla.org/r/45633/#review57272 Thanks!

Comment on attachment 8740212 [details] Bug 1208411 - Delay vsync timestamps on the compositor to make async animations more sync. https://reviewboard.mozilla.org/r/45633/#review57936

Pushed by hiikezoe@mozilla-japan.org: https://hg.mozilla.org/integration/autoland/rev/dc77b6514b80 Delay vsync timestamps on the compositor to make async animations more sync. r=mchang,mstange