Closed Bug 1418268 Opened 7 years ago Closed 6 years ago

Tweak test_restyles.html for the case where mPendingReadyTime is clamped to current time

Categories

(Core :: DOM: Animation, enhancement)

Product:
Core
Component:
DOM: Animation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(3 files)

Bug 1418268 - Make sure the animation on scrolled out element is throttled in the first place.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1418268 - Add a function to check whether there is a micro task checkpoint between animation tick and requestAnimationFrame callbacks.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1418268 - Tweak expected restyle count for the case where animation start time was clamped.
59 bytes, text/x-review-board-request
birtles
: review+
Details 
Performing micro task checkpoint in Animation::Tick causes another cumbersome problem.  The problem happens on test cases like this;

  var animation = div.animate(...);

  await animation.ready;

  var markers = await observeStyling(5);

  is(markers.length, 5, ...)


Pretty simple but this test sometimes observes 4 restyles.   What happens when the test observes 4 restyles;

1) In div.animate() we compose styles, so mProgressOnLastCompose is set to 0
2) At the next frame in Animation::Tick(), mPendingReadyTime is smaller than currentTime somehow, then mPendingReadyTime is set to the currentTime
3) As a result of 2), the progress of computed timing is 0 again
4) As a result of 3) HasComputedTimingChanged() check returns false in KeyframeEffectReadOnly::NotifyAnimationTimingUpdated()
5) As a result of 4 we skip calling CanThrottle() and RequestRestyle()
I'm pretty sure clamping is the right thing to do in the (uncommon) case that mPendingReadyTime ends up less than the timeline's current time due to vsync, and I'm pretty sure that if the progress hasn't changed, we shouldn't restyle. So I think altering the test makes most sense here.
(In reply to Brian Birtles (:birtles) from comment #1)
> I'm pretty sure clamping is the right thing to do in the (uncommon) case

Yeah, I'd like to know whether this test case is the uncommon case or not.  It actually happens frequently.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> Yeah, I'd like to know whether this test case is the uncommon case or not. 
> It actually happens frequently.

In testing, or in regular browsing? How frequently?
(In reply to Brian Birtles (:birtles) from comment #3)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #2)
> > Yeah, I'd like to know whether this test case is the uncommon case or not. 
> > It actually happens frequently.
> 
> In testing, or in regular browsing? How frequently?

In test_restyles.html.  Approximately happens on 30% runs.
Ok, that seems reasonable.
After some more trials it seems that the uncommon case happens more frequent than I thought.  If test case expects 0 restyle marker, the test case does not fail so I didn't notice that the uncommon case happened.

Anyway, as far as I can tell (in the trials), all uncommon case are actually the case where painting process took more than 16.6ms.  So clamping mPendingReadyTime is the right thing to do.  (Over 16.6ms for painting a few elements is actually not good but that's another story.)  

Also I think the uncommon case is detectable in test to check animation.currentTime == 0 and animation.startTime != null, so we can tweak test somehow.  That's said, I don't have clear idea for the tweak.

 var markers = observeStyling(5);
 is(markers.length, 5);

This case is easy to tweak, we can just ignore/skip the uncommon case frame and wait for one more frame

 var markers = observeStyling(5);
 is(markers.length, 0);

This case is not a problem.

 var markers = observeStyling(1);
 is(markers.length, 1);

This is a cumbersome case that I have no idea to tweak.  But there is no such test cases for now.

 var markers = observeStyling(1);
 is(markers.length, 0);

This is another cumbersome case.  I hope it doesn't a problem, I mean even if the test case succeeded intermittently, it's not a problem?

We might want to add another variant of observeStyling() which just waits for a single frame and add tweaks against each functions respectively?
Summary: Avoid clamping mPendingReadyTime to current time or tweak test_restyles.html → Tweak test_restyles.html for the case where mPendingReadyTime is clamped to current time
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
> Anyway, as far as I can tell (in the trials), all uncommon case are actually
> the case where painting process took more than 16.6ms.  So clamping
> mPendingReadyTime is the right thing to do.  (Over 16.6ms for painting a few
> elements is actually not good but that's another story.)  

Note that I am testing with debug build but with --enable-optimize on i7-6820HQ, it should be fast enough!
FWIW, this happens 100% on headless mode, I thought headless mode is faster than normal, but..
(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> FWIW, this happens 100% on headless mode, I thought headless mode is faster
> than normal, but..

This is wrong.  It's mostly due to bug 1416966.  Though this still happens on headless mode.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #6)
>  var markers = observeStyling(1);
>  is(markers.length, 1);
> 
> This is a cumbersome case that I have no idea to tweak.  But there is no
> such test cases for now.

There is a kind of such tests [1], i.e creating an animation and expecting a single restyle in the next frame.  The test does;

1) Create an animation on scrolled-out element
2) Wait for Animation.ready
3) Change the parent element's height to make the animating element visible
4) Check restyle marker in the next frame.

So, as for this test case, we should check the animation is surely throttled after 2), then we should change the parent style.  With this way, we don't need to worrying about clamping the animation start time.

So, the only test case that needs to be modified is something like this;

 var animation = div.animate(...);
 await animation.ready;
 var markers = await observeStyling(5);
 is(markers.length, 5, ...);;

I am going to rewrite it to;

 var animation = div.animate(...);
 await animation.ready;
 const expectedRestyleCount = wasStartTimeClamped(animation) ? 4 : 5;
 var markers = await observeStyling(5);
 is(markers.length, expectedRestyleCount, ..);

and wasStartTimeClamped is;

 function wasStartTimeClamped(aAnimation) {
   return aAnimation.startTime === aAnimation.timeline.currentTime &&
          aAnimation.currentTime === 0;
 }


[1] https://hg.mozilla.org/mozilla-central/file/f2cf6d147380/dom/animation/test/mozilla/file_restyles.html#l506
(In reply to Hiroyuki Ikezoe (:hiro) from comment #10)
> 
>  var animation = div.animate(...);
>  await animation.ready;
>  const expectedRestyleCount = wasStartTimeClamped(animation) ? 4 : 5;
>  var markers = await observeStyling(5);
>  is(markers.length, expectedRestyleCount, ..);
> 
> and wasStartTimeClamped is;
> 
>  function wasStartTimeClamped(aAnimation) {
>    return aAnimation.startTime === aAnimation.timeline.currentTime &&
>           aAnimation.currentTime === 0;
>  }

I've noticed that with the conformant Promise handling and with the new micro task checkpoint, we observe 4 restyle markers if the start time is clamped.  With the new micro task checkpoint but without the conformant Promise handling, we observe 6 restyle markers if the start time is *NOT* clamped since Promises inside requestAnimationFrame callbacks are not fulfilled soon, so there is a chance to process restyling until the Promises are fulfilled.  This is pretty complicated unfortunately, but anyway, I'd like to introduce the tweak for the new micro task checkpoint in this bug, and later I'd like to modify it for the conformant Promise handling.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b08bf01fde4c0e13fb513a99bddccbc5be95b744
Comment on attachment 8934833 [details]
Bug 1418268 - Make sure the animation on scrolled out element is throttled in the first place.

https://reviewboard.mozilla.org/r/205750/#review211302
Attachment #8934833 - Flags: review?(bbirtles) → review+
Comment on attachment 8934834 [details]
Bug 1418268 - Add a function to check whether there is a micro task checkpoint between animation tick and requestAnimationFrame callbacks.

https://reviewboard.mozilla.org/r/205752/#review211312
Attachment #8934834 - Flags: review?(bbirtles) → review+
Comment on attachment 8934835 [details]
Bug 1418268 - Tweak expected restyle count for the case where animation start time was clamped.

https://reviewboard.mozilla.org/r/205754/#review211314

::: dom/animation/test/mozilla/file_restyles.html:92
(Diff revision 1)
> +// Returns true if the given animation's start time was clamped to timeline
> +// current time due to delay on the main thread in vsync refresh rate during
> +// the initial paint for the animation.
> +// Note that this function will be inaccurate if the animation start time or
> +// current time has been modified.
> +function wasStartTimeClamped(aAnimation) {
> +  return aAnimation.startTime === aAnimation.timeline.currentTime &&

I feel a little bit uncomfortable about this method. One reason is that doesn't tell you if start time was clamped if you call it in a subsequent frame.

Could we just call it startsRightNow() ?

And then say something like:

"Returns true if |aAnimation| begins at the current timeline time. We sometimes need to detect this case because if we started an animation asynchronously (e.g. using play()) and then ended up running the next frame at precisely the time the animation started (due to aligning with vsync refresh rate) then we won't end up restyling in that frame."

?

::: dom/animation/test/mozilla/file_restyles.html:160
(Diff revision 1)
> +    // If there is a micro task checkpoint between animation tick and
> +    // requestAnimationFrame callbacks, below script is being processed in the
> +    // micro task checkpoint.  But Promises inside the callback for
> +    // requestAnimationFrame are not yet fulfilled right after the callbacks
> +    // without the conformant Promise handling, the Promises are fulfilled just
> +    // before refresh driver's next tick happens which means that we might
> +    // observe a redundant restyle marker because restyling process happens
> +    // before the callback for the Promise inside requestAnimationFrame callback
> +    // is called. But if the animation start time is clamped, we don't observe
> +    // the redandunt restyle maker since we skip restyling for the case.

I'm finding this comment hard to follow. Is there anyway to make it easier to read?

Perhaps start by saying, "Normally we expect one restyling for for each requestAnimationFrame (as called by observeRestyling) PLUS one for the current frame. However, we won't observe that initial restyling unless BOTH of the following two conditions hold:

1. We are running *before* restyling happens. This only happens if we perform a microtask checkpoint after resolving the 'ready' promise above (bug 1416966).

2. The animation actually needs a restyle because it started prior to this frame. Even if (1) is true, in some cases due to aligning with the refresh driver, the animation frame in which the ready promise is resolved happens to coincide perfectly with the start time of the animation. In this case no restyling is needed so we won't observe an additional restyle."

?
Attachment #8934835 - Flags: review?(bbirtles) → review+
Thanks for the review.  I will revise the comment tomorrow.
(In reply to Brian Birtles (:birtles) from comment #17)

> ::: dom/animation/test/mozilla/file_restyles.html:160
> (Diff revision 1)
> > +    // If there is a micro task checkpoint between animation tick and
> > +    // requestAnimationFrame callbacks, below script is being processed in the
> > +    // micro task checkpoint.  But Promises inside the callback for
> > +    // requestAnimationFrame are not yet fulfilled right after the callbacks
> > +    // without the conformant Promise handling, the Promises are fulfilled just
> > +    // before refresh driver's next tick happens which means that we might
> > +    // observe a redundant restyle marker because restyling process happens
> > +    // before the callback for the Promise inside requestAnimationFrame callback
> > +    // is called. But if the animation start time is clamped, we don't observe
> > +    // the redandunt restyle maker since we skip restyling for the case.
> 
> I'm finding this comment hard to follow. Is there anyway to make it easier
> to read?
> 
> Perhaps start by saying, "Normally we expect one restyling for for each
> requestAnimationFrame (as called by observeRestyling) PLUS one for the
> current frame. However, we won't observe that initial restyling unless BOTH
> of the following two conditions hold:

I re-read this carefully, then I noticed that this sentence has an inaccurate thing.  That is 'PLUS for the current frame',  actually it should be 'PLUS for the last frame'.  In observeRestyling() we stop observing inside the callback for a Promise that is generated in the callback for requestAnimationFrame.  So without the conformant Promise handling, the Promise callback will be processed after restyling has done in the last frame, then we will see the additional restyling there.

I will add a comment about this there.  Anyway, though this check will be more complicated once we start checking whether we have the conformant Promise handling, I really hope this confusing situation will be solved soon.
(In reply to Brian Birtles (:birtles) from comment #17)
> I'm finding this comment hard to follow. Is there anyway to make it easier
> to read?
> 
> Perhaps start by saying, "Normally we expect one restyling for for each
> requestAnimationFrame (as called by observeRestyling) PLUS one for the
> current frame. However, we won't observe that initial restyling unless BOTH
> of the following two conditions hold:
> 
> 1. We are running *before* restyling happens. This only happens if we
> perform a microtask checkpoint after resolving the 'ready' promise above
> (bug 1416966).
> 
> 2. The animation actually needs a restyle because it started prior to this
> frame. Even if (1) is true, in some cases due to aligning with the refresh
> driver, the animation frame in which the ready promise is resolved happens
> to coincide perfectly with the start time of the animation. In this case no
> restyling is needed so we won't observe an additional restyle."

I did use this comment mostly as it is.  Later for the conformant Promise handling, I will modify the comment again.  Thank you!
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/97512e9cac85
Make sure the animation on scrolled out element is throttled in the first place. r=birtles
https://hg.mozilla.org/integration/autoland/rev/13cdd8267cd6
Add a function to check whether there is a micro task checkpoint between animation tick and requestAnimationFrame callbacks. r=birtles
https://hg.mozilla.org/integration/autoland/rev/3c2576d4ea82
Tweak expected restyle count for the case where animation start time was clamped. r=birtles
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: