975261 - CSS Animations with OMTA sometimes don't start, especially when there is a delay

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

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

Attachment: Test case

With OMTC and OMTA enabled, if you load the attached test case in Windows 7 in a nightly build and don't touch the mouse the animation does not start. It sits at 30px. If you move the mouse during the first 10s (which is the duration of the animation) then it will pick up from mid-way through the animation.

I've noticed that this *only* happens when there is already a specified value of 'transform' on the element.

Comment 1

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

(In reply to Brian Birtles (:birtles) from comment #0)
> I've noticed that this *only* happens when there is already a specified
> value of 'transform' on the element.

If we have a specified value of transform then we create the transform layer when the style is first applied. Otherwise, we create the layer at the end of the delay. As a result CommonElementAnimationData::CanThrottleAnimation will return true during the delay phase if there is a specified transform and false otherwise.

Comment 2

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

I can verify that if I simply put something like the following in ElementAnimations::EnsureStyleRuleFor:

      if (positionInIteration < 0.1) {
        aIsThrottled = false;
        break;
      }

then the problem disappears. I guess that doing this means that we'll generate a new style rule until just after the animation starts so FlushAnimations will trigger PostRestyleForAnimation and we'll get the paint that adds the animation to the layer... or something like that.

Obviously that's not the right way to fix it. We really want to know what the last sample time was so that we can trigger the paint that updates the layer exactly when we finish the delay. Alternatively, we can just put the animation on the layer, delay-and-all, to begin with. That seems preferable to me but is there something I'm missing?

Comment 3

[David Zbarsky (:dzbarsky)]

(In reply to Brian Birtles (:birtles) from comment #2)
> I can verify that if I simply put something like the following in
> ElementAnimations::EnsureStyleRuleFor:
> 
>       if (positionInIteration < 0.1) {
>         aIsThrottled = false;
>         break;
>       }
> 
> then the problem disappears. I guess that doing this means that we'll
> generate a new style rule until just after the animation starts so
> FlushAnimations will trigger PostRestyleForAnimation and we'll get the paint
> that adds the animation to the layer... or something like that.
> 
> Obviously that's not the right way to fix it. We really want to know what
> the last sample time was so that we can trigger the paint that updates the
> layer exactly when we finish the delay. Alternatively, we can just put the
> animation on the layer, delay-and-all, to begin with. That seems preferable
> to me but is there something I'm missing?

I didn't put the animation on the layer originally because I assumed it would get added when the delay ended and didn't want to do unnecessary sampling on the compositor.  I guess with the main thread throttling this is no longer true; I think it should be fine to add the animation with the delay to the layer.

Comment 4

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

(In reply to David Zbarsky (:dzbarsky) from comment #3)
> I didn't put the animation on the layer originally because I assumed it
> would get added when the delay ended and didn't want to do unnecessary
> sampling on the compositor.  I guess with the main thread throttling this is
> no longer true; I think it should be fine to add the animation with the
> delay to the layer.

I just realised that this is related to bug 880596 comment 35. Basically, we have special handling for transitions that detects the end of the delay and adds the transition to the layer then but we don't have it for animations. So, an alternative way of fixing this is to mirror what we do in transitions (i.e. add mIsRunningOnCompositor).

I'll wait to see what Nick thinks about adding animations that are still in their delay phase to the compositor.

Comment 5

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

I made a test for this and fixed it only to discover that the original problem remains. When I add printfs to see what is happening it starts working again :/

After making IsRunning return true we *are* adding the animation to the layer during the delay phase. But is something else needed to trigger a sync to the compositor thread? It seems like we often don't end up queuing an initial animation restyle event--is that what is needed?

Note that in the test case if you move the mouse we end up calling FlushAnimations (from FlushPendingNotifications) with Cannot_Throttle which in turn triggers the animation restyle and the animation starts on the compositor thread. That's as far as I've gotten. Any clues about what is needed to trigger the sync to the compositor thread or where the race condition may be coming from would be helpful.

Comment 6

[David Zbarsky (:dzbarsky)]

Are you forcing a flush in your test?

Comment 7

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

(In reply to David Zbarsky (:dzbarsky) from comment #6)
> Are you forcing a flush in your test?

The test is attachment 8379457 [details]. There's no script, just markup.

Comment 8

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

I fixed this properly finally made a test that fails correctly (paint_listener.js triggering extra flushes really tricked me!)

I'll tidy it up and post patch and test tomorrow.

Comment 9

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

Since fixing this I found two other closely related bugs so I'm going to tackle them in the same bug.

The two other bugs relate to transform handling (clearing LayerIsPrerenderedDataKey when starting a new transform animation) and backwards fill handling (ensuring animation restyle happens even when the old and new style rules match). I have tests for both, a patch for one, and a pretty good idea about how to fix the other one.

Comment 10

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

So far I have identified three cases where animations fail to start on the compositor:

(1) When they have a delay. We don't do a restyle for animations so, in the absence of other actions like UI events that trigger flushes, we never build the layer and send the animation to the compositor thread because we throttle the main thread.

(2) When we have a transform animation with a delay that targets an element that already has a transform set by underlying style. In this case the LayerIsPrerenderedDataKey is set on the layer because in nsDisplayTransform::ShouldPrerenderTransformedContent we only check for the *presence* of an animation, not whether it is running. Then in RestyleManager::DoApplyRenderingChangeToTree we don't do an invalidating paint because TryUpdateTransformOnly() returns true since it looks at LayerIsPrerenderedDataKey.

(3) When we have a backwards fill and we sample at *exactly* the start of the animation. In this case, even if we address (1) and (2), on the next refresh driver tick, when we get to RestyleManager::ComputeStyleChangeFor (or more specifically ElementRestyler::CaptureChange) we notice that the style hasn't changed (since the first frame of the animation produces the same value as the backwards fill) and end up with an empty change list. As a result we never schedule a view manager flush and rebuild the layer. Hence, the animation never gets sent to the compositor thread. On the next tick we're already throttling the main thread.

This sounds bad but in practice other things in the system like UI events causes flushes and we end up seeing the animation anyway. That said,
- (1) is easy to reproduce in real usage (just load the attachment and don't move the mouse).
- I've seen (2) happen in real usage when I have the debugger attached, but not otherwise.
- I can only reproduce (3) in a test so far.

(1) can be fixed by just using mLastNotification to detect the first sample after a delay and turn off throttling for that sample.

(2) can be fixed by forcing layer re-rendering for the first sample after a delay to clear the LayerIsPrerenderedDataKey key.

(3) is more tricky because it applies to the *next* tick. Possible options include:
 (a) toggle something so that ElementRestyler::CaptureChange produces a non-empty change list in this case
 (b) directly call ScheduleViewManagerFlush from the animation manager when we detect this kind of case
 (c) add mIsRunningOnCompositor to animations too and use that to turn off throttling as needed

I'm going to try (c). I think it may complicate testing because when you sample at the very beginning of a delayed animation sometimes you'll see a result on the compositor thread and sometimes you won't (depending on backwards fill). I think that's ok though because long-term I'm hoping to put animations on the compositor sooner (and do backwards filling there too).

Comment 11

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

Attachment: part 1 - Add test for OMTA animations that start with a delay

Comment 12

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

Attachment: part 2 - Trigger animations with a delay

Animations with a delay are not put on the compositor thread until the end of
the delay phase. However, there is currently nothing that explicitly triggers
this transaction. It may occur due to flushes that arise from UI events but it
is not guaranteed.

This patch detects the end of a delay phase and turns off throttling for that
sample. It re-uses the mLastNotification member which is not ideal but
a subsequent patch in this queue removes this and replaces it with the approach
used for transitions.

Comment 13

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

Attachment: part 3 - Add test for transforms

Adds a test for transform animations with a delay where there is already
a transform specified on the element.

This test used to fail but bug 828173 fixed it. It is included here as
a regression test.

(Prior to bug 828173 we were able to fix this by triggering
ForceLayerRerendering inside nsAnimationManager::nsFlushAnimations whenever we
detected *different* style rules but canThrottleTick=true)

The issue stemmed from the fact that when an element has a transform property,
LayerIsPrerenderedDataKey would get set on the layer because in
nsDisplayTransform::ShouldPrerenderTransformedContent we would only check for
the *presence* of an animation, not whether it is running. Then in
RestyleManager::DoApplyRenderingChangeToTree we wouldn't do an invalidating
paint because TryUpdateTransformOnly() returns true since it looks at
LayerIsPrerenderedDataKey.

Bug 828173 fixes this, at least in part, by checking if an animation is running.
This bug may resurface if we put animations with a delay on the compositor
thread hence it is worth including here.

Comment 14

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

Attachment: part 4 - Add test for starting OMTA animations with backwards fill

This test reproduces a bug where we don't send an animation to the compositor
thread. The important step is the sample precisely at the start of the animation
interval.

Comment 15

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

Attachment: part 5 - Fix OMTA animations with backwards fill

When we have a backwards fill and we sample at *exactly* the start of the
animation on the next refresh driver tick, when we get to
RestyleManager::ComputeStyleChangeFor (or more specifically
ElementRestyler::CaptureChange) we notice that the style hasn't changed (since
the first frame of the animation produces the same value as the backwards fill)
and end up with an empty change list. As a result we never schedule a view
manager flush and rebuild the layer. Hence, the animation never gets sent to the
compositor thread. On the next tick we're already throttling the main thread.

This patch fixes this by applying the same approach as is used for transitions,
that is, explicitly marking which animations are running on the compositor
thread so we know if we need to trigger a layer transaction or not. This should
not only be more robust than the previous code but also facilitate aligning
animations and transitions code (bug 880596).

Comment 16

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

Comment on attachment 8387246 [details] [diff] [review]
part 1 - Add test for OMTA animations that start with a delay

Hi David, sorry about all the reviews. If you don't have time, feel free to re-assign to dbaron.

Comment 17

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

Attachment: part 5 - Fix OMTA animations with backwards fill

Comment 18

[David Zbarsky (:dzbarsky)]

Comment on attachment 8387301 [details] [diff] [review]
part 5 - Fix OMTA animations with backwards fill

Review of attachment 8387301 [details] [diff] [review]:
-----------------------------------------------------------------

Nice!

Comment 19

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

Went to land this but try reports test failures on B2G ICS Emulator:
https://tbpl.mozilla.org/?tree=Try&rev=bec9de21fab5

I'm wondering if the ICS Emulator doesn't support OMTC. Currently tweaking the tests for check for DOMWindowUtils.layerManagerRemote == true.

Comment 20

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

(In reply to Brian Birtles (:birtles) from comment #19)
> I'm wondering if the ICS Emulator doesn't support OMTC. Currently tweaking
> the tests for check for DOMWindowUtils.layerManagerRemote == true.

This fails to fix the problem (layerManagerRemote returns true). I'm not sure why we're not getting values on the layer in this case.

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a380efaa88d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a8a2e851e1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4ca3c2bb7f
https://hg.mozilla.org/integration/mozilla-inbound/rev/33536a62f534
https://hg.mozilla.org/integration/mozilla-inbound/rev/430db7206b0c

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a380efaa88d5
https://hg.mozilla.org/mozilla-central/rev/7a8a2e851e1a
https://hg.mozilla.org/mozilla-central/rev/7a4ca3c2bb7f
https://hg.mozilla.org/mozilla-central/rev/33536a62f534
https://hg.mozilla.org/mozilla-central/rev/430db7206b0c

Comment 23

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

This bug is also covered by the tests in bug 964646 (test_animations_omta.html), specifically the fill mode tests.