1208411 - CSS animation: box-shadow and translateX flicker with steps() timing function

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[James Wheare]

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/

Comment 1

[James Wheare]

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

Comment 2

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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

Comment 3

[Markus Stange [:mstange]]

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?

Comment 4

[Markus Stange [:mstange]]

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).

Comment 5

[Mason Chang [Inactive] [:mchang]]

(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.

Comment 6

[Brian Birtles (:birtles, Feb 26~ parental leave)]

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.

Comment 7

[Bas Schouten (:bas.schouten)]

(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? ;-)

Comment 8

[Mason Chang [Inactive] [:mchang]]

(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.

Comment 9

[Markus Stange [:mstange]]

(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?

Comment 10

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(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.)

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1208411 - Delay vsync timestamps on the compositor to make async animations more sync.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

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

Comment 13

[Mason Chang [Inactive] [:mchang]]

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?

Comment 14

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

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

Comment 15

[Markus Stange [:mstange]]

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.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 17

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

(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.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 19

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

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

Comment 20

[Mason Chang [Inactive] [:mchang]]

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 21

[Markus Stange [:mstange]]

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

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Attachment: A compasison video

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

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Attachment: Another comparison

Here is another video recorded an example in bug 1255646.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 25

[Mason Chang [Inactive] [:mchang]]

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?

Comment 26

[Hiroyuki Ikezoe (:hiro)]

(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 27

[Hiroyuki Ikezoe (:hiro)]

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 28

[Hiroyuki Ikezoe (:hiro)]

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/

Comment 29

[Mason Chang [Inactive] [:mchang]]

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?

Comment 30

[Hiroyuki Ikezoe (:hiro)]

(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

Comment 31

[Mason Chang [Inactive] [:mchang]]

(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 32

[Hiroyuki Ikezoe (:hiro)]

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 33

[Mason Chang [Inactive] [:mchang]]

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 34

[Markus Stange [:mstange]]

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

Comment 35

[Pulsebot]

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

Comment 36

[Carsten Book [:Tomcat]]

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