828173 - Do we still need to force re-rendering the layer when flushing throttled transitions?

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Nick Cameron [:nrc]]

See the FIXMEs after bug 822721 landed.

Comment 1

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

Attachment: testcase

Comment 2

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

Comment on attachment 700699 [details]
testcase

er, sorry, wrong bug (meant bug 822891)

Comment 3

[Nick Cameron [:nrc]]

The answer is 'yes', we do still need to force rebuilding the layer tree for animated elements. Without this we freeze after the first in a series of transitions, just like we did before.

Comment 4

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

I adjusted the comments to point here in https://hg.mozilla.org/integration/mozilla-inbound/rev/74ec9c7f21b8

I may want to re-investigate this for bug 977991, so I can share the miniflush code.

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/74ec9c7f21b8

Comment 6

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

Attachment: one testcase showing the problem

So if I remove the code I'd like to remove, I sometimes see a problem moving the mouse out of the middle box in this testcase -- the transition doesn't start reversing until it completes (or at least makes considerable forward progress past where it should have reversed).

Comment 7

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

Attachment: second (more obvious) testcase showing the problem

This is the one Nick described, and is really obvious, since it just stops going around.

(I also filed bug 978919 on this.)

Comment 8

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

So, looking into the code a bit more:  what ForceLayerRerendering does is it calls layer->RemoveUserData(nsIFrame::LayerIsPrerenderedDataKey()).  It does this on both opacity and transform layers, although all the other code that manipulates this piece of user data does it only for transform layers, so I think the code is really relevant only to transform layers.  The property was initially created in https://hg.mozilla.org/mozilla-central/rev/1cceedd227a7 .

The only piece of code that behaves differently as a result of removing this user data property is nsIFrame::TryUpdateTransformOnly, which in turn has only one caller, DoApplyRenderingChangeToTree in RestyleManager.cpp.  That, in turn, uses the function call to decide which parameter to pass here:
    aFrame->SchedulePaint(needInvalidatingPaint ?
                          nsIFrame::PAINT_DEFAULT :
                          nsIFrame::PAINT_COMPOSITE_ONLY);

So as far as I can tell, the effect of this change is to call SchedulePaint() with PAINT_DEFAULT rather than PAINT_COMPOSITE_ONLY.

My guess, though I haven't verified it yet, is that that difference leads us to call nsDisplayTransform::BuildLayer which both (a) resets the LayerIsPrerenderedDataKey and (b) calls AddAnimationsAndTransitionsToLayer.  If that's the case, it might be better to make the UpdateTransformLayer handling in ApplyRenderingChangeToTree directly call AddAnimationsAndTransitionsToLayer.

Comment 9

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

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #7)
> Created attachment 8384813 [details]
> second (more obvious) testcase showing the problem
> 
> This is the one Nick described, and is really obvious, since it just stops
> going around.

Though I actually sometimes see bugs in this testcase even with the current code, as long as I don't move the mouse.

Comment 10

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

So I'm going to reopen this and take it; I have a patch series that removes this code and replaces it with something that I think is better, although the patch series ended up being a little more complicated than I was expecting it to be at the start.  Further comments coming in the patch descriptions.

Also, please do review carefully; this involved a bit of poking around in code I don't have much experience with.

Comment 11

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

Attachment: patch 1: Add nsLayoutUtils::GetReferenceFrame

Comment 12

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

Attachment: patch 2: Make Layer::AddAnimation have the caller fill in the data instead of taking parameters

(This is a bigger simplification later in the patch queue, when I add a
variant of AddAnimation called AddAnimationForNextTransaction.)

Comment 13

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

Attachment: patch 3: Add a concept of pending animations to Layer, like pending transform

Comment 14

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

Attachment: patch 4: Expose AddAnimationsAndTransitionsToLayer and allow it to be called from style change handling

Comment 15

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

Attachment: patch 5: Call AddAnimationsAndTransitionsToLayer when we choose not to repaint when handling an UpdateTransformLayer hint, so that any new animations resulting from the style change get sent to the layer

Comment 16

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

Attachment: patch 6: Remove calls to ForceLayerRerendering from the miniflush code (UpdateThrottledStyles, which flushes animations whose main thread updates are throttled without updating any other styles)

I've been wanting to remove this code for a while.  I think this code is
problematic for three reasons:

 (1) It's in the middle of code where it doesn't belong, and which ought
     to be handling purely-style-system things.  (This is blocking me
     from reusing that code elsewhere, e.g., in bug 977991 and
     bug 960465, both of which could use it in some form.)

 (2) It defeats the optimization from bug 790505 whenever we do a
     miniflush (in other words, whenever we have any style change,
     whether or not it's related)

 (3) It means the conditions for when we decide to ship a new set of
     animation data to a layer doesn't cover all the cases the layer
     needs it.  In particular, we only run this miniflush code when we
     have a currently running animation or transition that's running on
     the compositor thread.  On the other hand, the UpdateTransformLayer
     style change handling in DoApplyRenderingChangeToTree depends on
     whether the frame currently has a transform layer, which can
     continue to be true for a bit after the animation stops.  So if we
     need to send animations to the layer because of a transform style
     change that happens soon after an animation completes, our style
     change handling will find the existing layer and call its
     SetBaseTransformForNextTransaction method but never do anything
     that triggers layer construction.  The style throttling code, in
     turn, will never stop doing main thread updates because the
     animation generation on the layer is out-of-date, and these main
     thread updates will keep the layer active, but they'll never show
     up because the stale animation data overrides the new transform
     that we've been setting.  (At least, I think that's what was
     happening; it makes sense to me and matches the behavior I was
     observing.  I didn't verify which main thread updates and which
     layer updates were actually happening, though.)

     This shows up, for example, in the animation in attachment 8384813 [details]
     just halting at a corner if I'm careful not to disturb it.  (I'm
     testing on Linux, with both accelerated layers and OMT animations
     explicitly enabled.)

I think there are probably some other things that can be removed as
followups to removing this code, because I think we made some boundary
conditions intentionally incorrect so that problem (3) above wouldn't be
as bad as it otherwise would have been.

Comment 17

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8385154 [details] [diff] [review]
patch 1:  Add nsLayoutUtils::GetReferenceFrame

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

Can we also have the nsDisplayListBuilder just call this one? That way it can just add a cache for fast lookups, rather than actually duplicating any code.

Less worried about this, but it might be nice if we also passed in the enum value for the type of display item we're about to create. That way we could move the logic from the nsDisplayTransform constructor into this function, and have all the logic for computing references frames contained to a single location.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8385157 [details] [diff] [review]
patch 4:  Expose AddAnimationsAndTransitionsToLayer and allow it to be called from style change handling

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

::: layout/base/nsDisplayList.cpp
@@ +439,5 @@
> +    if (aItem) {
> +      origin = aItem->ToReferenceFrame();
> +    } else {
> +      // transform display items use a reference frame computed from
> +      // their parent

Note that this isn't strictly true (sorry, forgot to mention it), see http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#2881

If we move the nsDisplayTransform adjustment code into nsLayoutUtils::GetReferenceFrame, then we don't have to worry about it here. Otherwise we should assert that aFrame->GetParent()->Preserves3DChildren() is false (always the case with async animations iirc) or do the equivalent handling.

Comment 19

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

(In reply to Matt Woodrow (:mattwoodrow) from comment #17)
> Comment on attachment 8385154 [details] [diff] [review]
> Can we also have the nsDisplayListBuilder just call this one? That way it
> can just add a cache for fast lookups, rather than actually duplicating any
> code.

It's not quite the same, since the display list builder can be constructed with an arbitrary root determined by the caller, whereas my GetReferenceFrame assumes that you want the root that GetDisplayRootFrame would get you.  It's not clear to me if anyone is depending on the difference, but it seems like it might be some cleanup work to make sure callers never construct an nsDisplayListBuilder with something that wouldn't be returned by GetDisplayRootFrame, and than they never do anything on that builder that would involve a frame inside it.  I'm inclined to want to leave this to a separate refactoring bug, if that's ok with you?


(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> Note that this isn't strictly true (sorry, forgot to mention it), see
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp#2881
>
> If we move the nsDisplayTransform adjustment code into
> nsLayoutUtils::GetReferenceFrame, then we don't have to worry about it here.
> Otherwise we should assert that aFrame->GetParent()->Preserves3DChildren()
> is false (always the case with async animations iirc) or do the equivalent
> handling.

Oops, right.  So at a minimum I think I should move GetTransformRootFrame into nsLayoutUtils, and use it instead of a straight GetParent() call.

Comment 20

[Matt Woodrow (:mattwoodrow)]

(In reply to David Baron [:dbaron] (needinfo? me) (UTC-8) from comment #19)
 
> It's not quite the same, since the display list builder can be constructed
> with an arbitrary root determined by the caller, whereas my
> GetReferenceFrame assumes that you want the root that GetDisplayRootFrame
> would get you.  It's not clear to me if anyone is depending on the
> difference, but it seems like it might be some cleanup work to make sure
> callers never construct an nsDisplayListBuilder with something that wouldn't
> be returned by GetDisplayRootFrame, and than they never do anything on that
> builder that would involve a frame inside it.  I'm inclined to want to leave
> this to a separate refactoring bug, if that's ok with you?
> 

Yeah, that's true. We are relying on the nsDisplayListBuilder that created the nsDisplayTransform/Layer that we're modifying to have used the same reference frame as we compute in the nsLayoutUtils function. I'm 99% sure that is the case, but nothing is asserting it.

Separate bug is fine.

Comment 21

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

Attachment: patch 3.5: Move GetTransformRootFrame to nsLayoutUtils

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf28c8a3613
https://hg.mozilla.org/integration/mozilla-inbound/rev/b999b6e2ccab
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cd385901f97
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d766ce8ba36
https://hg.mozilla.org/integration/mozilla-inbound/rev/4055be226e07
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d436b8eedda
https://hg.mozilla.org/integration/mozilla-inbound/rev/55fa3c2f32aa

(see also try run at https://tbpl.mozilla.org/?tree=Try&rev=d7e48cf00e0e)

Comment 23

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

and a bustage fix for non-unified builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd0fa3e4a8e

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8bf28c8a3613
https://hg.mozilla.org/mozilla-central/rev/b999b6e2ccab
https://hg.mozilla.org/mozilla-central/rev/2cd385901f97
https://hg.mozilla.org/mozilla-central/rev/8d766ce8ba36
https://hg.mozilla.org/mozilla-central/rev/4055be226e07
https://hg.mozilla.org/mozilla-central/rev/7d436b8eedda
https://hg.mozilla.org/mozilla-central/rev/55fa3c2f32aa
https://hg.mozilla.org/mozilla-central/rev/ecd0fa3e4a8e

Comment 25

[Gregor Wagner [:gwagner]]

needed for bug 978712

Comment 26

[[:fabrice] Fabrice Desré]

remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/fa07f9042762
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/6b8096e82742
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/5ac99362000c
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/2c8b3b6b67f5
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/44a84daee42d
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/192546efaa47
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/9b5a07403fbd
remote:   https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/c07046c36310

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/7a4ca3c2bb7f added a test for one aspect of this

Comment 28

[Brian Birtles (:birtles)]

This code is somewhat tested by the tests added in bug 964646 and bug 975261.

In particular, if I comment out the following lines added in patch 5 (landed as https://hg.mozilla.org/mozilla-central/rev/7d436b8eedda):

+        if (!needInvalidatingPaint) {
+          // Since we're not going to paint, we need to resend animation
+          // data to the layer.
+          MOZ_ASSERT(layer, "this can't happen if there's no layer");
+          nsDisplayListBuilder::AddAnimationsAndTransitionsToLayer(layer,
+            nullptr, nullptr, aFrame, eCSSProperty_transform);
+        }

a number of tests in test_animations_omta.html and test_animations_omta_start.html fail.