1311196 - Treeherder navigation between jobs is jumpy on recent nightlies; animations and transitions stutter / flicker

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P1)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm in only-unclassified mode and I'm jumping from job to job with the n and p keyboard shortcuts. When I do this, the enlarged selected job appears to jump back and forth a few times between the previously selected job and the newly selected job.

I don't remember it doing this in the past, and I'm not sure when exactly it started doing this, but it had to be one of the recent prod pushes this week or last week.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Also seems to do the jumpy thing when I click to select a different job, and it happens regardless of being in only-unclassified mode.

Comment 2

[Phil Ringnalda (:philor)]

You started seeing it when you installed Monday morning's nightly, it's apparently an animation regression somewhere between the second Friday nightly and then.

Comment 3

[William Lachance (:wlach)]

(In reply to Phil Ringnalda (:philor) from comment #2)
> You started seeing it when you installed Monday morning's nightly, it's
> apparently an animation regression somewhere between the second Friday
> nightly and then.

Has a bug been filed for this?

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Phil, do you know details about the patch as landed, or if a regression bug has already been filed for that (beside this TH instance)?

Comment 5

[Phil Ringnalda (:philor)]

No idea, the full, complete, entire extent of the info I possess is "huh, over the weekend? bet that's Fx, I should try Chrome, not affected, I should try last Friday's Fx nightly, not affected."

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I assume this has been caused buy bug 1223658. I will continue tomorrow to finish the regression range. Maybe Hiroyuki can already have a look at.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Regressed by this. https://hg.mozilla.org/mozilla-central/rev/00799db70975

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Attachment: Use fill:both for transition on the compositor

It seems to me that this bug is somewhat related to async animation issues we've been seeing (e.g. graphical gaps).  In this particular case, it seems to happen on the end of transitions.  
I do want to investigate in further detail with a minimized test case, but I can't create the case yet. If someone provides a minimized test case, it would be much appreciated.

Comment 9

[Markus Stange [:mstange]]

I also see this on https://www.youtube.com/ when clicking the hamburger button in the top left corner. When the side bar fades in, it sometimes flickers during the transition.
It also sometimes happens in cleopatra during the scale animation when the profile is shown, e.g. on https://cleopatra.io/#report=07b15e5c88a431b4fa5e41982caeb4c92003cae1

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Thank you Markus!  This bug happens more frequent than I thought initially.  I am convinced that in case of transition passing fill:both to the compositor is a right approach to fix this bug.  The only one thing I am concerned when we pass fill:both is that transition keeps running on the compositor or even the transition is finished.  We can write a test case to check it with getOMTAStyle(). 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f337621a63a

This is another try which run the test case 20times.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d79fece0e234&duplicate_jobs=visible

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1311196 - Part 1: Add isOMTAEnabled() in testcommon.js.

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

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

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

Comment 13

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bug 1311196 - Part 3: Adjust fill mode to fill forwards for all animations on the compositor.

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

Comment 14

[Brian Birtles (:birtles)]

Thanks for looking into this! This hopefully fixes the glitches I'm seeing on Netflix too. Building now to check.

Comment 15

[Brian Birtles (:birtles)]

Comment on attachment 8803731 [details]
Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

https://reviewboard.mozilla.org/r/87854/#review86818

Why do we need this test? It seems like test_animations_omta.html covers this case but using a different mechanism--advancing the refresh driver? Also, this test doesn't act as a regression test for this bug, right? (Since it passes without the fix in part 3.)

If we don't need this test I'd rather not add it since each test has a cost (production time, review time, local test run time, try run time, automation test run time, intermittent orange filing/debugging/fixing/reviewing/landing time, maintenance time when test APIs change etc.)

Or is this test because we would like to move away from manipulating the refresh driver? If so, I think we should probably do that more systematically.

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #15)
> Comment on attachment 8803731 [details]
> Bug 1311196 - Part 2: Test that transition is pulled from the compositor
> when the transition is finished.
> 
> https://reviewboard.mozilla.org/r/87854/#review86818
> 
> Why do we need this test? It seems like test_animations_omta.html covers
> this case but using a different mechanism--advancing the refresh driver?
> Also, this test doesn't act as a regression test for this bug, right? (Since
> it passes without the fix in part 3.)
> 
> If we don't need this test I'd rather not add it since each test has a cost
> (production time, review time, local test run time, try run time, automation
> test run time, intermittent orange filing/debugging/fixing/reviewing/landing
> time, maintenance time when test APIs change etc.)
> 
> Or is this test because we would like to move away from manipulating the
> refresh driver? If so, I think we should probably do that more
> systematically.

Right, I think we should test it on normal refresh driver since test_animations_omta.html could neither catch bug 1229283 nor detect that the bug has been fixed.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #15)

> Also, this test doesn't act as a regression test for this bug, right? (Since
> it passes without the fix in part 3.)

Right, I gave up writing the regression test for now since I've not succeeded to write a minimized case yet.

Comment 18

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> (In reply to Brian Birtles (:birtles, high review load) from comment #15)
> > Or is this test because we would like to move away from manipulating the
> > refresh driver? If so, I think we should probably do that more
> > systematically.
> 
> Right, I think we should test it on normal refresh driver since
> test_animations_omta.html could neither catch bug 1229283 nor detect that
> the bug has been fixed.

Ok, if we want to rewrite those tests, let's do that in a separate bug.

Comment 19

[Brian Birtles (:birtles)]

Comment on attachment 8803731 [details]
Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

https://reviewboard.mozilla.org/r/87854/#review86822

Comment 20

[Brian Birtles (:birtles)]

Comment on attachment 8803732 [details]
Bug 1311196 - Part 3: Adjust fill mode to fill forwards for all animations on the compositor.

https://reviewboard.mozilla.org/r/87856/#review86820

Is the problem that we're not getting the updated underlying value to the compositor and hence when the transition ends it temporarily flickers back to the old underlying value before being removed from the compositor?

If that's the case, it seems like we could have the same problem for CSS animations? Particularly if they use the FLIP technique[1]?

I think this approach doesn't work if we substitute in a different KeyframeEffect and clear the fill mode but that's an edge case we can probably address later.

Unfortunately, this patch doesn't fix the flicker I'm seeing on Netflix. I filed bug 1312307 for that but it may be that we duplicate it to this bug and fix it here.

I guess I'd like to understand what the issue actually is.

[1] https://aerotwist.com/blog/flip-your-animations/

::: layout/base/nsDisplayList.cpp:434
(Diff revision 1)
> +  animation->fillMode() = aAnimation->AsCSSTransition()
> +  ? static_cast<uint8_t>(dom::FillMode::Both)
> +  : static_cast<uint8_t>(timing.mFill);

Should this be:

  animation->fillMode() = aAnimation->AsCSSTransition()
                          ? static_cast<uint8_t>(dom::FillMode::Both)
                          : static_cast<uint8_t>(timing.mFill);

Comment 21

[Boris Chiou [:boris]]

Comment on attachment 8803730 [details]
Bug 1311196 - Part 1: Add isOMTAEnabled() in testcommon.js.

https://reviewboard.mozilla.org/r/87852/#review86830

Looks good to me.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #20)
> Comment on attachment 8803732 [details]
> Bug 1311196 - Part 3: Use fill:both for transition on the compositor.
> 
> https://reviewboard.mozilla.org/r/87856/#review86820
> 
> Is the problem that we're not getting the updated underlying value to the
> compositor and hence when the transition ends it temporarily flickers back
> to the old underlying value before being removed from the compositor?

I am not sure.  Can you see the old underlying value while glitching?  I can't tell it's the old value (initial style of the transition).  I did also try to see it with lower playback rate on devtools, but then I can't see any glitches at all.

> If that's the case, it seems like we could have the same problem for CSS
> animations? Particularly if they use the FLIP technique[1]?

Yes, likely.  The Netflix case is the same cause.

> Unfortunately, this patch doesn't fix the flicker I'm seeing on Netflix. I
> filed bug 1312307 for that but it may be that we duplicate it to this bug
> and fix it here.
> 
> I guess I'd like to understand what the issue actually is.

Me too.

Comment 23

[Marcia Knous [:marcia]]

Tracking 52+ as this affects animations and transitions stutter / flicker on sites such as Netflix.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

Attachment: An example to demonstrate this bug

This is a minimized test case that reproduce the same symptoms on treeherder or youtube, I believe.

I think a problem on these site is that compositor process goes ahead while heavy load jobs are processed on the main thread.

In transition case, I think using fill:both on the compositor is a proper fix.  But what I still don't understand is that the fix is not effective for the Netflix case (bug 1312307).

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803730 [details]
Bug 1311196 - Part 1: Add isOMTAEnabled() in testcommon.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87852/diff/1-2/

Comment 26

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803731 [details]
Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87854/diff/1-2/

Comment 27

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803732 [details]
Bug 1311196 - Part 3: Adjust fill mode to fill forwards for all animations on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87856/diff/1-2/

Comment 28

[Hiroyuki Ikezoe (:hiro)]

I still don't understand what's going on Netfix, but apart from that, I think we can fix this bug separately.
The test case in part 2 was re-written completely, the test fails without part 3.
Brian?

Comment 29

[Brian Birtles (:birtles)]

Ok, I'm a little concerned that if script substitutes in a different transition effect with no backwards fill we'll produce the wrong result with this patch. We could fix that, I suppose by just doing a mapping:

  none/forwards => forwards
  backwards/both => both

But I'm still curious to know what the Netflix bug is in case there's a more general solution available. I'll set you up with a Netflix account and let's hold off landing this until we've had a chance to look into that.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #29)
> But I'm still curious to know what the Netflix bug is in case there's a more
> general solution available. I'll set you up with a Netflix account and let's
> hold off landing this until we've had a chance to look into that.

Thanks! I was afraid that I can not fix that bug within a month which is the free period of Netflix. ;-)

Comment 31

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #29)
> Ok, I'm a little concerned that if script substitutes in a different
> transition effect with no backwards fill we'll produce the wrong result with
> this patch.

I think you mean the code something like this:

  div.style.transform = 'translate(100px)';
  getComputedStyle(div).transform;

  div.style.transform = 'none';
  var anim = div.getAnimations()[0];
  anim.effect.timing.fill = 'none';  // Suppose the effect is writable.

In this case, what is the proper style of the transition in the before phase?  'translate(100px)' or 'none'?  I thought it's 'translate(100px)'.  Regarding that time stamp on the compositor starts from a prior time stamp to the main thread, I think 'backwards' or 'both' is needed on the compositor.

Comment 32

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #31)
> (In reply to Brian Birtles (:birtles, high review load) from comment #29)
> > Ok, I'm a little concerned that if script substitutes in a different
> > transition effect with no backwards fill we'll produce the wrong result with
> > this patch.
> 
> I think you mean the code something like this:
> 
>   div.style.transform = 'translate(100px)';
>   getComputedStyle(div).transform;
> 
>   div.style.transform = 'none';
>   var anim = div.getAnimations()[0];
>   anim.effect.timing.fill = 'none';  // Suppose the effect is writable.

Right, specifically when you get the transition back and do transition.effect = new KeyframeEffect(...) and then set the fill mode to 'none'.

> In this case, what is the proper style of the transition in the before
> phase?  'translate(100px)' or 'none'? I thought it's 'translate(100px)'.

I'm pretty sure it should be 'none'. The underlying style is now 'none', it's just that the backwards fill of the transition normally covers it up.

> Regarding that time stamp on the compositor starts from a prior time stamp
> to the main thread, I think 'backwards' or 'both' is needed on the
> compositor.

Why is it different to animations?

Comment 34

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #32)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #31)
> > (In reply to Brian Birtles (:birtles, high review load) from comment #29)
> > > Ok, I'm a little concerned that if script substitutes in a different
> > > transition effect with no backwards fill we'll produce the wrong result with
> > > this patch.
> > 
> > I think you mean the code something like this:
> > 
> >   div.style.transform = 'translate(100px)';
> >   getComputedStyle(div).transform;
> > 
> >   div.style.transform = 'none';
> >   var anim = div.getAnimations()[0];
> >   anim.effect.timing.fill = 'none';  // Suppose the effect is writable.
> 
> Right, specifically when you get the transition back and do
> transition.effect = new KeyframeEffect(...) and then set the fill mode to
> 'none'.
> 
> > In this case, what is the proper style of the transition in the before
> > phase?  'translate(100px)' or 'none'? I thought it's 'translate(100px)'.
> 
> I'm pretty sure it should be 'none'. The underlying style is now 'none',
> it's just that the backwards fill of the transition normally covers it up.

It really surprised me. But OK, I will fix part 3.

> > Regarding that time stamp on the compositor starts from a prior time stamp
> > to the main thread, I think 'backwards' or 'both' is needed on the
> > compositor.
> 
> Why is it different to animations?

Because it's transition!  The modified transition looks animations to me!

Comment 35

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803730 [details]
Bug 1311196 - Part 1: Add isOMTAEnabled() in testcommon.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87852/diff/2-3/

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803731 [details]
Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87854/diff/2-3/

Comment 37

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803732 [details]
Bug 1311196 - Part 3: Adjust fill mode to fill forwards for all animations on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87856/diff/2-3/

Comment 38

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803730 [details]
Bug 1311196 - Part 1: Add isOMTAEnabled() in testcommon.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87852/diff/3-4/

Comment 39

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803731 [details]
Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87854/diff/3-4/

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803732 [details]
Bug 1311196 - Part 3: Adjust fill mode to fill forwards for all animations on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87856/diff/3-4/

Comment 41

[Hiroyuki Ikezoe (:hiro)]

The test case in part 2 failed intermittently without waiting for document loaded.

Comment 42

[Brian Birtles (:birtles)]

I think we should apply this same handling to animations too (i.e. not just transitions). Prior to https://hg.mozilla.org/mozilla-central/rev/00799db70975 we used a fill mode of "both" for transitions and animations. Now, however we pass the fill mode. However, we still remove transitions and animations from the compositor when they reach the end of their active interval. I think, therefore, it should be fine to force the fill mode to either both/forwards for *both* transitions and animations. That should hopefully prevent flicker with cases like FLIP animation too. In other words, just drop the check for AsCSSTransition(). Does that make sense?

Comment 43

[Brian Birtles (:birtles)]

Better still, could we pass the correct fill mode to the compositor, and then in AsyncCompositionManager (SampleAnimations) adjust it to include forwards fill before we pass it to GetComputedTimingAt ? That seems like that might be a better place to do it?

(If we pass across computedTiming.mFill in nsDisplayList.cpp we wouldn't need to check for the 'auto' value either, but maybe that's fine as-is.)

Comment 44

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #42)
> I think we should apply this same handling to animations too (i.e. not just
> transitions). Prior to
> https://hg.mozilla.org/mozilla-central/rev/00799db70975 we used a fill mode
> of "both" for transitions and animations. Now, however we pass the fill
> mode. However, we still remove transitions and animations from the
> compositor when they reach the end of their active interval. I think,
> therefore, it should be fine to force the fill mode to either both/forwards
> for *both* transitions and animations. That should hopefully prevent flicker
> with cases like FLIP animation too. In other words, just drop the check for
> AsCSSTransition(). Does that make sense?

For FLIP animations I think we need to check base style value equals to the final animation value just like I did in the try in bug 1312307 comment 2 <https://hg.mozilla.org/try/rev/eb0494d02ee447ef71452a036eef9b7e53afb4f3>.  I am also concerned that bug 1229283 re-appears.  Also we will see unwanted results on the compositor with additive or accumulate.  I can't bring a good example, but something like this:

 animate({ transform: 'translate(0px), translate(100px)' },  { duration: 100 });
 animate([{ transform: 'translate(0px)' }
          { transform: 'translate(-100px)' },
          { transform: 'translate(-200px)' }],
          { duration: 200, composite: 'add' });

These animations can be seen as staying at 0px until the first animation is finished, and jump to -100px then moving to left.
Yes, this isn't a good example, web developers will not write this kinds of jumpy animations but I guess there might be such kinds of animations without any jump.

Anyway, I'd like to handle FLIP animations after bug 1305325 landed.  Or you are suggesting that we change to use fill:both or fill:forwards here and then change it after bug 1305325?

(In reply to Brian Birtles (:birtles, high review load) from comment #43)
> Better still, could we pass the correct fill mode to the compositor, and
> then in AsyncCompositionManager (SampleAnimations) adjust it to include
> forwards fill before we pass it to GetComputedTimingAt ? That seems like
> that might be a better place to do it?

I think Layer::SetAnimations() is a good place rather than SampleAnimations() so that we don't need to change fill mode every Vsync tick on the compositor.

Comment 45

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803731 [details]
Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

Clearing review request against part 2 since it still fails intermittently.
https://treeherder.mozilla.org/logviewer.html#?job_id=29859258&repo=try#L1732

On Android, the transition is not yet running on the compositor after document loaded and a MozAfterPaint event...

Comment 46

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803732 [details]
Bug 1311196 - Part 3: Adjust fill mode to fill forwards for all animations on the compositor.

OK. I've decided to back to before.  Then I will change it after bug 1311257.

Comment 47

[Brian Birtles (:birtles)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #44)
> (In reply to Brian Birtles (:birtles, high review load) from comment #42)
> > I think we should apply this same handling to animations too (i.e. not just
> > transitions). Prior to
> > https://hg.mozilla.org/mozilla-central/rev/00799db70975 we used a fill mode
> > of "both" for transitions and animations. Now, however we pass the fill
> > mode. However, we still remove transitions and animations from the
> > compositor when they reach the end of their active interval. I think,
> > therefore, it should be fine to force the fill mode to either both/forwards
> > for *both* transitions and animations. That should hopefully prevent flicker
> > with cases like FLIP animation too. In other words, just drop the check for
> > AsCSSTransition(). Does that make sense?
> 
> For FLIP animations I think we need to check base style value equals to the
> final animation value just like I did in the try in bug 1312307 comment 2
> <https://hg.mozilla.org/try/rev/eb0494d02ee447ef71452a036eef9b7e53afb4f3>.

I think that only fixes a subset of the problem cases. There will be other cases where the updated base value is not equal to the final animation value but nor is it equal to the original base value. In those cases it seems better to hold the final animation value for a few more milliseconds then switch to the updated base value rather than jump back to the original base value and then jump again to the updated base value.

(Of course, if we could just get the updated base value to the compositor then we probably wouldn't need to worry about any of this. I'm not sure why that doesn't happen actually.)
 
> I am also concerned that bug 1229283 re-appears.  Also we will see unwanted
> results on the compositor with additive or accumulate.  I can't bring a good
> example, but something like this:
> 
>  animate({ transform: 'translate(0px), translate(100px)' },  { duration: 100
> });
>  animate([{ transform: 'translate(0px)' }
>           { transform: 'translate(-100px)' },
>           { transform: 'translate(-200px)' }],
>           { duration: 200, composite: 'add' });
> 
> These animations can be seen as staying at 0px until the first animation is
> finished, and jump to -100px then moving to left.

Yes. With the change I'm suggesting, in the case where the main thread is tied down, we'll see:

  0-100ms: Stays at 0
  100-150ms: Goes from 0 to -50px
  <sync from main thread arrives>
  150ms-200ms: Goes from -150px to -200px

That doesn't seem so bad? Or at least it seems preferable than introducing flicker to many other kinds of animations (which we expect to be more common)?

> Anyway, I'd like to handle FLIP animations after bug 1305325 landed.  Or you
> are suggesting that we change to use fill:both or fill:forwards here and
> then change it after bug 1305325?

Yes, I'd rather we apply the same behavior to animations here. That is closer to the original behavior we had so it is a less risky change. If we find problem cases after landing bug 1305325 we can fix them then.

> (In reply to Brian Birtles (:birtles, high review load) from comment #43)
> > Better still, could we pass the correct fill mode to the compositor, and
> > then in AsyncCompositionManager (SampleAnimations) adjust it to include
> > forwards fill before we pass it to GetComputedTimingAt ? That seems like
> > that might be a better place to do it?
> 
> I think Layer::SetAnimations() is a good place rather than
> SampleAnimations() so that we don't need to change fill mode every Vsync
> tick on the compositor.

Ok, I don't mind too much. Updating the fill mode before passing to GetComputedTimingAt is basically free so I don't think there's any performance reason for avoiding that. It's up to you where you think it makes sense.

Comment 48

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #45)
> Comment on attachment 8803731 [details]
> Bug 1311196 - Part 2: Test that transition keeps the final style while the
> main thread is busy even if the transition finished on the compositor.
> 
> Clearing review request against part 2 since it still fails intermittently.
> https://treeherder.mozilla.org/logviewer.html#?job_id=29859258&repo=try#L1732
> 
> On Android, the transition is not yet running on the compositor after
> document loaded and a MozAfterPaint event...

I am inclined to skip the test on Android.  On local Android emulator, it took 5s.  I don't think generating 5s-busy frame on the main thread is good idea.

Comment 49

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #47)
> > I am also concerned that bug 1229283 re-appears.  Also we will see unwanted
> > results on the compositor with additive or accumulate.  I can't bring a good
> > example, but something like this:
> > 
> >  animate({ transform: 'translate(0px), translate(100px)' },  { duration: 100
> > });
> >  animate([{ transform: 'translate(0px)' }
> >           { transform: 'translate(-100px)' },
> >           { transform: 'translate(-200px)' }],
> >           { duration: 200, composite: 'add' });
> > 
> > These animations can be seen as staying at 0px until the first animation is
> > finished, and jump to -100px then moving to left.
> 
> Yes. With the change I'm suggesting, in the case where the main thread is
> tied down, we'll see:
> 
>   0-100ms: Stays at 0
>   100-150ms: Goes from 0 to -50px
>   <sync from main thread arrives>
>   150ms-200ms: Goes from -150px to -200px
> 
> That doesn't seem so bad? 

I don't think it's so easy, considering delay, endDelay, playbackRate, etc.. Anyway we will handle those cases later.

Comment 50

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803730 [details]
Bug 1311196 - Part 1: Add isOMTAEnabled() in testcommon.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87852/diff/4-5/

Comment 51

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803731 [details]
Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87854/diff/4-5/

Comment 52

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803732 [details]
Bug 1311196 - Part 3: Adjust fill mode to fill forwards for all animations on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87856/diff/4-5/

Comment 53

[Brian Birtles (:birtles)]

Comment on attachment 8803732 [details]
Bug 1311196 - Part 3: Adjust fill mode to fill forwards for all animations on the compositor.

https://reviewboard.mozilla.org/r/87856/#review87524

::: gfx/layers/Layers.cpp:435
(Diff revision 5)
> +    // Force to append fill:forwards effect, otherwise we will see jumpy
> +    // animaiton on the end of the animation while the main thread is busy.

"Adjust fill mode to fill forwards so that if the main thread is delayed in clearing this animation we don't introduce flicker by jumping back to the old underlying value."

::: gfx/layers/Layers.cpp:437
(Diff revision 5)
> +    switch (static_cast<dom::FillMode>(animation.fillMode())) {
> +      case dom::FillMode::None:
> +        animation.fillMode() = static_cast<uint8_t>(dom::FillMode::Forwards);
> +        break;
> +      case dom::FillMode::Backwards:
> +        animation.fillMode() = static_cast<uint8_t>(dom::FillMode::Both);
> +        break;
> +      default:
> +        break;
> +    }

Unless I'm missing something in an underlying patch, we still pass the specified fill mode in [1] so we can still get "Auto" here which should behave the same as "None". So we either need to handle that case here, or (probably preferably) change [1] to pass computedTiming.mFill.

[1] http://searchfox.org/mozilla-central/rev/e3e8571c5378ac92663d4f583ccc4ad0a3019716/layout/base/nsDisplayList.cpp#434

::: gfx/layers/Layers.cpp:464
(Diff revision 5)
> -    for (uint32_t j = 0; j < mAnimations[i].segments().Length(); j++) {
> -      const AnimationSegment& segment = mAnimations[i].segments()[j];
> +    for (uint32_t j = 0; j < animation.segments().Length(); j++) {
> +      const AnimationSegment& segment = animation.segments()[j];

s/animation.segments()/segments/ ?

Comment 54

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #53)

> ::: gfx/layers/Layers.cpp:437
> (Diff revision 5)
> > +    switch (static_cast<dom::FillMode>(animation.fillMode())) {
> > +      case dom::FillMode::None:
> > +        animation.fillMode() = static_cast<uint8_t>(dom::FillMode::Forwards);
> > +        break;
> > +      case dom::FillMode::Backwards:
> > +        animation.fillMode() = static_cast<uint8_t>(dom::FillMode::Both);
> > +        break;
> > +      default:
> > +        break;
> > +    }
> 
> Unless I'm missing something in an underlying patch, we still pass the
> specified fill mode in [1] so we can still get "Auto" here which should
> behave the same as "None". So we either need to handle that case here, or
> (probably preferably) change [1] to pass computedTiming.mFill.

Ah, exactly.  I will do it in this patch.

> ::: gfx/layers/Layers.cpp:464
> (Diff revision 5)
> > -    for (uint32_t j = 0; j < mAnimations[i].segments().Length(); j++) {
> > -      const AnimationSegment& segment = mAnimations[i].segments()[j];
> > +    for (uint32_t j = 0; j < animation.segments().Length(); j++) {
> > +      const AnimationSegment& segment = animation.segments()[j];
> 
> s/animation.segments()/segments/ ?

Oh, good catch!

Comment 55

[Brian Birtles (:birtles)]

Comment on attachment 8803731 [details]
Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

https://reviewboard.mozilla.org/r/87854/#review87526

r=me but let me know what the comment is supposed to mean so we can write something a little more clear

::: dom/animation/test/mochitest.ini:105
(Diff revision 5)
> +skip-if = toolkit == 'android'
> +[mozilla/test_transition_finish_on_compositor.html]

Shouldn't this go *after* [mozilla/test_transition_finish_on_compositor.html]

::: dom/animation/test/mozilla/file_transition_finish_on_compositor.html:34
(Diff revision 5)
> +  // Force to initiate the transition to avoid missing transition on the
> +  // compositor after waitForPaint().

I don't understand this comment. Something like, "Force the transition to start so that it doesn't begin and finish while we are in waitForPaints()" ?

::: dom/animation/test/mozilla/file_transition_finish_on_compositor.html:44
(Diff revision 5)
> +    // Generate artificial busyness on the main thread for 100ms.
> +    var timeAtStart = window.performance.now();
> +    while (window.performance.now() - timeAtStart < 100) {}

This seems like a long time for a test, but I guess we have other tests that do this. We should probably try not to add too many more tests like this is we can help it.

::: dom/animation/test/mozilla/file_transition_finish_on_compositor.html:58
(Diff revision 5)
> +    assert_equals(transform, 'matrix(1, 0, 0, 1, 100, 0)',
> +                  'The final transition style is still applied on the ' +
> +                  'compositor');
> +  });
> +}, 'Transition on the compositor keeps the final style while the main thread ' +
> +   'is busy even if the transition finised on the compositor');

s/finised/finished/

Comment 56

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles, high review load) from comment #55)

> ::: dom/animation/test/mozilla/file_transition_finish_on_compositor.html:34
> (Diff revision 5)
> > +  // Force to initiate the transition to avoid missing transition on the
> > +  // compositor after waitForPaint().
> 
> I don't understand this comment. Something like, "Force the transition to
> start so that it doesn't begin and finish while we are in waitForPaints()" ?

Yes, that's right.  I saw that there is no transform on the compositor after MozAfterPaint.  As you know, MozAfterPaint is received after a paint has done on the compositor.  So I think the transition was not started yet at that time, but I saw actually it on Android emulator, so now the comment is not necessary.  I will drop it.

> ::: dom/animation/test/mozilla/file_transition_finish_on_compositor.html:44
> (Diff revision 5)
> > +    // Generate artificial busyness on the main thread for 100ms.
> > +    var timeAtStart = window.performance.now();
> > +    while (window.performance.now() - timeAtStart < 100) {}
> 
> This seems like a long time for a test, but I guess we have other tests that
> do this. We should probably try not to add too many more tests like this is
> we can help it.

Yes, I wanted a gimmick to run a script on the compositor.  Something like this;

 runScriptOnCompositorAfterSeconds(() => {
   assert_equals(transform, ...);
 }, 100);

Comment 57

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803730 [details]
Bug 1311196 - Part 1: Add isOMTAEnabled() in testcommon.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87852/diff/5-6/

Comment 58

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803731 [details]
Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87854/diff/5-6/

Comment 59

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803732 [details]
Bug 1311196 - Part 3: Adjust fill mode to fill forwards for all animations on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87856/diff/5-6/

Comment 60

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #56)
> (In reply to Brian Birtles (:birtles, high review load) from comment #55)
> 
> > ::: dom/animation/test/mozilla/file_transition_finish_on_compositor.html:34
> > (Diff revision 5)
> > > +  // Force to initiate the transition to avoid missing transition on the
> > > +  // compositor after waitForPaint().
> > 
> > I don't understand this comment. Something like, "Force the transition to
> > start so that it doesn't begin and finish while we are in waitForPaints()" ?
> 
> Yes, that's right.  I saw that there is no transform on the compositor after
> MozAfterPaint.  As you know, MozAfterPaint is received after a paint has
> done on the compositor.  So I think the transition was not started yet at
> that time, but I saw actually it on Android emulator, so now the comment is
> not necessary.  I will drop it.

Unfortunately the 'no transform on the compositor' happens on desktop if we can't received the MozAfterPaint event within 50ms.
So I added a check that the MozAfterPaint is received within 50ms, and if not, skip the test.

Brian, is this change OK with you?

Comment 61

[Brian Birtles (:birtles)]

Seems fine to me.

Comment 62

[Hiroyuki Ikezoe (:hiro)]

Thanks!

https://treeherder.mozilla.org/#/jobs?repo=try&revision=68d05e0852a7

Comment 63

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803730 [details]
Bug 1311196 - Part 1: Add isOMTAEnabled() in testcommon.js.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87852/diff/6-7/

Comment 64

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803731 [details]
Bug 1311196 - Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87854/diff/6-7/

Comment 65

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8803732 [details]
Bug 1311196 - Part 3: Adjust fill mode to fill forwards for all animations on the compositor.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/87856/diff/6-7/

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8dfb01b23512
Part 1: Add isOMTAEnabled() in testcommon.js. r=boris
https://hg.mozilla.org/integration/autoland/rev/cd9fc5466381
Part 2: Test that transition keeps the final style while the main thread is busy even if the transition finished on the compositor. r=birtles
https://hg.mozilla.org/integration/autoland/rev/c9a8a051dd30
Part 3: Adjust fill mode to fill forwards for all animations on the compositor. r=birtles

Comment 67

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8dfb01b23512
https://hg.mozilla.org/mozilla-central/rev/cd9fc5466381
https://hg.mozilla.org/mozilla-central/rev/c9a8a051dd30