876626 - 598726-1.html reftest fails with async animations enabled

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Always redraw whole canvas

This test currently passes on b2g, but fails with some of my changes from bug 873944.

That bug changes the DidPaint (and MozAfterPaint event) call to happen earlier. It now gets called directly after we finish painting ThebesLayers and sending them to the compositor, rather than the next widget paint event.

This shouldn't change anything fundamental, it just upsets some fragile timings.

Reftest failure log (with the test modified to use a 2.0 scale so that it's more obvious): https://tbpl.mozilla.org/php/getParsedLog.php?id=23428779&tree=Try&full=1

As discussed on irc, the reason for the failure is that invalidation (and MozAfterPaint events) are using the client side transform values for the layer, which isn't necessarily what the compositor actually draws and copies back for us.

The attached patch makes us ignore the MozAfterPaint rects (since they're often wrong) for tests with async animation, and instead copies back the entire frame.

With this patch applied we get one three results (since to be entirely random as to which happens).

1) Test passes. In this case layout has detected that the transition has completed and no longer creates an nsDisplayTransform.

2) Test looks correct-ish, but fails. In this case we're drawing the content at the right scale, but we've still got an nsDisplayTransform which is downscaling the up-scaled content. We clamped the content scale to the highest value encountered during the transition, and we're still using it here. This obviously gives minor pixel differences to the reference.

3) Test fails with content drawn at the wrong scale. This appears to be because the compositor is actually *behind* layout, but it would seem that shouldn't be possible.

Log for the last drawWindow we do before ending the test (reftest-wait gets removed from the transitionend handler): http://www.pastebin.mozilla.org/2452514

It appears that layout creates a layer with 1.0 scale (and 0.5 pre-scale to cancel out the 2.0 scale on the content), but the compositor takes us a snapshot with 1.37 scale (shown as 0.687527, since it includes the pre-scale). We can see that the compositor finally reaches the expected 0.5 value pretty soon after.


Possible solutions:

*Keep the current patch, and also add a additional, delayed, final snapshot to reftests so that we give the compositor time to catch up.

*Temporarily disable compositor-side interpolation of animations when taking the synchronous snapshot, so that the compositor always draws what layout expects.

*Keep the current patch, and ensure that the compositor only ever interpolates *forward* from the values provided by layout and never backwards.

The second/third options here probably won't fix the second failure mode, but that seems like less of a concern.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Always redraw whole canvas v2

This implements the first suggestion, and waits a second before taking a final snapshot. Makes the test pass at least.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

"waiting a second before taking a final snapshot" smells orange.

I think we need to fix the "compositor goes backwards" problem, sorry.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Attachment: Schedule a paint when we post a restyle event

We need to make sure that we always schedule a repaint when an animation completes, so that the reftest harness takes a final snapshot.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Always redraw whole canvas v3

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 755141 [details] [diff] [review]
Schedule a paint when we post a restyle event

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

I don't get why this is needed. If the style change has any effect, a paint will be scheduled, won't it? This is, after all, the not-throttled case.

This is more of a Nick patch anyway.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 755143 [details] [diff] [review]
Always redraw whole canvas v3

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

::: layout/tools/reftest/reftest-content.js
@@ +240,5 @@
> +    // use getAttribute because className works differently in HTML and SVG
> +    return contentRootElement &&
> +           contentRootElement.hasAttribute('class') &&
> +           contentRootElement.getAttribute('class').split(/\s+/)
> +                             .indexOf("reftest-snapshot-all") != -1;

We should just be using contentRootElement.classList.contains("reftest-snapshot-all")!

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Schedule a paint when we post a restyle event v2

Comment 8

[Nick Cameron [:nrc]]

Comment on attachment 755804 [details] [diff] [review]
Schedule a paint when we post a restyle event v2

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

I think this is right, but I don't feel confident enough to r+, sorry. Punting to dbaron.

::: layout/style/nsTransitionManager.cpp
@@ +1102,5 @@
>          didThrottle = true;
>        }
> +      // When an async animations ends we need to force a repaint so that
> +      // reftests know to take a final snapshot. If the start and end
> +      // positions of the animation are the same, and the entire animation

please clarify the comment by explaining that this happens when we have a reversing transition triggered by the end of another transition. This will cause a mini-flush but not (presumably) schedule a paint.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Nick Cameron [:nrc] from comment #8)
> Comment on attachment 755804 [details] [diff] [review]
> > +      // When an async animations ends we need to force a repaint so that
> > +      // reftests know to take a final snapshot. If the start and end
> > +      // positions of the animation are the same, and the entire animation
> 
> please clarify the comment by explaining that this happens when we have a
> reversing transition triggered by the end of another transition. This will
> cause a mini-flush but not (presumably) schedule a paint.

This doesn't make sense to me.  If there was a transition reversing, then the current value in style data will be at-the-oldest the value that the animation had when it reversed (since computing the reversing involves computing style data).  That's either a different value from the endpoint or the animation didn't move at all.

So I guess I don't understand why this is needed, which makes me somewhat hesitant to r+ it.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And I was thinking I was being clever and using needinfo?assignee...

Comment 11

[Matt Woodrow (:mattwoodrow)]

Ok, so most of this bug seems to have resolved itself.

The only failure now is where the test version is drawn with active layers, and the reference isn't. The two images look similar, but fail on a bunch of pixel differences.

We definitely get a style flush after the transition ends, but sometimes it detects no changes (probably depending on the timing of throttling?).

If we force an explicit paint after the animation finishes, then we always get an inactive layer and the test passes.

We seem to fail the majority of the time on b2g, and very very rarely on desktop. Timing issues are fun :)

Comment 12

[Matt Woodrow (:mattwoodrow)]

Alright, did a little more debugging with printf (did I mention timing issues being fun?).

It appears that we get a MozAfterPaint event, then do a DrawWindow which computes the end values of the transition (but transitionend hasn't fired yet).

This draws the content with the existing active layers.

Then we fire the transitionend event, which doesn't trigger a repaint since the style system already has the final values, and nothing has changed.

If we forcibly trigger a new paint at this point, then we'd draw *without* the active layers and pass the test.

This seems to match the change that breaks it, which is making the MozAfterPaint event fire sooner.

Does this seem like a valid conclusion David?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What does the transitionend *event* have to do with things?  Or is it some other concept of transition ending that is relevant?

Would this patch, perhaps, fix the problem:

 bool
 ElementTransitions::HasAnimationOfProperty(nsCSSProperty aProperty) const
 {
   for (uint32_t tranIdx = mPropertyTransitions.Length(); tranIdx-- != 0; ) {
-    if (aProperty == mPropertyTransitions[tranIdx].mProperty) {
+    const ElementPropertyTransition& pt = mPropertyTransitions[tranIdx];
+    if (aProperty == pt.mProperty && !pt.IsRemovedSentinel()) {
       return true;
     }
   }
   return false;
 }

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(The reason I ask is that I *think* that function is used to determine whether layers are active, and that particular change would cause it to determine that the layer no longer needs to be active one refresh driver tick earlier.)

Comment 15

[Matt Woodrow (:mattwoodrow)]

The transitionend event is what the test is using as its end condition (where it removes the reftest-wait class).

That change doesn't fix the issue, because we've explicitly marked the layer as active.

http://mxr.mozilla.org/mozilla-central/source/layout/style/nsTransitionManager.cpp#202

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So where is the code that decides the layer should no longer be active?  It seems like, if anything, that's the code that ought to say we should repaint as a result.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Ok, so it's not actually the layer being active (since that's on a 1 second timeout and definitely won't be hit), but instead the code that decides that scale to draw the layer at.

It still thinks we're in the transition, so it draws us at the max value of the transition (1.1).

How do we compute the reversing of transitions?

It looks like ConsiderStartingTransition, but we don't get that (with shouldAnimate=true) when we reverse the transition.

In the test, we wait for the first transition to finish, and then from the transitionended event, we blur the element (which changes the style rule, and should be a new transition back to the initial values).

I see a paint scheduled by the blur() change, but no successful call to ConsiderStartingTransition. Is there any other way that the compositor could be getting the updated transitions without going through this?

Comment 18

[Matt Woodrow (:mattwoodrow)]

I set a breakpoint on AddTransformScale (interpolating transform scale changes), and filtered out calls coming from the compositor.

We only ever get it called at the start, it doesn't appear to be called at all when we start the second transition in the reverse direction.

This puts my back to my initial conclusion, that we only ever interpolate the starting value (content side at least), and that's why completing the second transition doesn't result in a repaint.

Comment 19

[Matt Woodrow (:mattwoodrow)]

Once the first transition finishes, we send the transitionend event, mark the ElementPropertyTransitions as a 'removed sentinel' and call RestyleForAnimation.

Once we process that restyle, we get UpdateAllThrottledStyles called, which gets to UpdateThrottledStyle, and then ElementTransitions::EnsureStyleRuleFor.

EnsureStyleRuleFor doesn't do anything if the ElementPropertyTransitions is marked as a removed sentinel, so we don't interpolate the new values and restyle the frame.

Removing this check (http://mxr.mozilla.org/mozilla-central/source/layout/style/nsTransitionManager.cpp#105) seems to fix it locally for me.

Comment 20

[Matt Woodrow (:mattwoodrow)]

Attachment: Fix CanAnimateProperty

Comment 21

[Matt Woodrow (:mattwoodrow)]

Attachment: Allow UpdateThrottledStyles to flush the style for transitions that have just finishe

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 758332 [details] [diff] [review]
Allow UpdateThrottledStyles to flush the style for transitions that have just finishe

This doesn't make sense; anything that's a removed sentinel shouldn't have any change since the previous cycle.  Also, the following code is likely to assert, I think.

(Looking very quickly, though, in the middle of an all-day meeting.)

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(The "removed sentinel" concept is a thing that logically, no longer exists, except that for one cycle we need to treat the possibility of starting a new transition for that element+property differently.)

Though maybe I'm missing something, but I can't look now...

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 758332 [details] [diff] [review]
Allow UpdateThrottledStyles to flush the style for transitions that have just finishe

Though this is the code that builds the style rule.  We definitely shouldn't make a transition that's already over contribute to the style rule.  That could leave an incorrect value around permanently, e.g., if transitions are interacting with animations.

Comment 25

[Matt Woodrow (:mattwoodrow)]

I'm not sure what the right solution is then.

While the transition is still running we throttle style changes, and don't flush style until the transition ends.

Once it has ended, then it's marked as being removed, and we skip over updating style for the property/element. And so nothing actually updates style, and it's still using the initial value.

Something needs to actually update style once the transition ends, if this code isn't it, what is?

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #25)
> Something needs to actually update style once the transition ends, if this
> code isn't it, what is?

FlushTransitions does:

      if (!canThrottleTick || transitionStartedOrEnded) {
        nsRestyleHint hint = et->mElementProperty == nsGkAtoms::transitionsProperty ?
          eRestyle_Self : eRestyle_Subtree;
        mPresContext->PresShell()->RestyleForAnimation(et->mElement, hint);
      } else {

which should cause the style to be updated when the transition ends.


I think the patch in attachment 755804 [details] [diff] [review] makes more sense than any of the others, but I still don't understand how we can have a transition end such that the style change posted in the code I quote above doesn't actually cause the necessary repainting.  (Could the handling of UpdateTransformLayer/UpdateOpacityLayer hints be buggy somehow?)

Comment 27

[Matt Woodrow (:mattwoodrow)]

Alright, more debugging.

It appears to be going wrong when we start the second transition (which goes from scale(2) to not transformed).

We process the style change (from js, non-animation style change)
That calls nsTransitionManager::StyleContextChanged
We call ConsiderStartingTransition, which starts the transition from scale(2) to not-transformed, and calls RestyleForAnimation.
We call 'AddValue' at the end of StyleContextChanged, which adds the scale(2) to the 'covering' rule set (which previously had no transform), and creates another new StyleContext.
This generates nsChangeHint_RepaintFrame.
We immediately then start processing animation restyles (http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp#12050)
We end up in nsTransitionManager::StyleContextChanged again for the same style context.
We *don't* call AddValue at the end this time (since it's not a new transition?), so the new style context doesn't have a transform
We generate nsChangeHint_AddOrRemoveTransform when comparing the style contexts, since the transform is gone.

Even though the style context is no longer transformed, we still build nsDisplayTransform because this code (http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#969) detects that we are using the compositor.


Why do we call RestyleForAnimation when we're already inside StyleContextChanged (and already get to modify the StyleContext being created)?

Or alternatively, is the animation restyle not meant to create a new style context? Or should the new style context contain the transition start-value rule we added to the previous one (via the 'covering' rule set)?

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So the whole "RestyleForAnimation" business is part of the design of the current code, though that part needs to get redone as part of implementing http://lists.w3.org/Archives/Public/www-style/2013Mar/0297.html .

Essentially what it's implementing is that conceptually, we have two separate styles, the style without animation, and the style with animation.  However, instead of storing these in two separate style contexts, we instead switch the style contexts from representing one or the other over time.

I didn't quite follow the previous comment yet (I'm in a someone active meeting right now and can't concentrate enough to understand all of it), but *if* you're saying that the style context has the style derived from the cover rule for the entire duration of the animation, then that seems wrong.   (The cover rule should be temporary, only while we're starting transitions -- and also a concept that should be going away with the change in architecture to implement the changes above.)

Comment 29

[Matt Woodrow (:mattwoodrow)]

No, the cover rule is only active for the start of the animation.

After that the style context doesn't have any transform property (which is the end state), so when we actually reach the end of the transition, no change hint is generated.

I would have expected the style context to have the transition start value for the duration of a throttled transition.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #27)
> We end up in nsTransitionManager::StyleContextChanged again for the same
> style context.
> We *don't* call AddValue at the end this time (since it's not a new
> transition?), so the new style context doesn't have a transform
> We generate nsChangeHint_AddOrRemoveTransform when comparing the style
> contexts, since the transform is gone.

Are the two StyleContextChanged calls for the same content node?  Two content nodes could have the same style context if they're for siblings that have the same styles.  Or does the second one (not sure if you breakpoint is at the start) hit this early return:
  if (aNewStyleContext->PresContext()->IsProcessingAnimationStyleChange()) {
    return nullptr;
  }

But the thing that should cause the with-animation style context to have the styles representing the current position of the transition isn't StyleContextChanged; it's one of the nsTransitionManager::RulesMatching methods (which all call WalkTransitionRule) (see more below).


(In reply to Matt Woodrow (:mattwoodrow) from comment #29)
> After that the style context doesn't have any transform property (which is
> the end state), so when we actually reach the end of the transition, no
> change hint is generated.
> 
> I would have expected the style context to have the transition start value
> for the duration of a throttled transition.

I'd have expected that as well.  This should happen because something in StyleContextChanged calls RestyleForAnimation, and then when we get to the for-animation restyle pass in nsCSSFrameConstructor::ProcessPendingRestyles, we should create a style context that matches a transition rule, which happens because that rule is added in nsTransitionManager::WalkTransitionRule.

Comment 31

[Matt Woodrow (:mattwoodrow)]

Right, and the actual rule is generated by EnsureStyleRuleFor.

So, the only call to EnsureStyleRuleFor that gets past the initial if happens *before* we call ConsiderStartingTransition. And in this call, mPropertyTransitions is empty, so the rule generated is empty.

The test has a transition for 'all' properties, and makes multiple changes, forcing a style flush between each.

Is it possible that we generate the rule during the first layout flush (when there are no active transitions), and when the actual transition is added, EnsureStyleRuleFor does nothing since mStyleRuleRefresh time matches?

The test looks like this: http://mxr.mozilla.org/mozilla-central/source/layout/reftests/bugs/598726-1.html?force=1

Lines 20-31 are where the problem happens, and the transition is initiated by line 30.

Maybe we need to make sure that a new style rule is generated when we start a transition?

Comment 32

[Matt Woodrow (:mattwoodrow)]

And now this sounds like it probably is a duplicate of bug 849399.

Comment 34

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2662face321
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad0fa6692a65

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a2662face321
https://hg.mozilla.org/mozilla-central/rev/ad0fa6692a65

Comment 36

[Michael Wu [:mwu]]

Comment on attachment 755143 [details] [diff] [review]
Always redraw whole canvas v3

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

::: layout/tools/reftest/reftest-content.js
@@ +789,5 @@
>      var win = content;
>      var scale = markupDocumentViewer().fullZoom;
>  
>      var rects = [ ];
> +    if (shouldSnapshotWholePage) {

Isn't this always true?

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 995410 covers fixing comment 36, which is no longer trivial according to try.