Closed Bug 828173 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla30
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: nrc, Assigned: dbaron)

References

Details

Attachments

(9 files, 1 obsolete file)

720 bytes, text/html
Details
845 bytes, text/html
Details
2.68 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.14 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.50 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
13.17 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.60 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.74 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.70 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
See the FIXMEs after bug 822721 landed.
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
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.
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).
This is the one Nick described, and is really obvious, since it just stops going around.

(I also filed bug 978919 on this.)
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.
(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.
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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Attachment #8385154 - Flags: review?(matt.woodrow)
(This is a bigger simplification later in the patch queue, when I add a
variant of AddAnimation called AddAnimationForNextTransaction.)
Attachment #8385155 - Flags: review?(matt.woodrow)
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.
Attachment #8385159 - Flags: review?(matt.woodrow)
Attachment #8385155 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8385154 - Flags: review?(matt.woodrow) → review+
Attachment #8385156 - Flags: review?(matt.woodrow) → review+
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.
Attachment #8385157 - Flags: review?(matt.woodrow) → review+
Attachment #8385158 - Flags: review?(matt.woodrow) → review+
Attachment #8385159 - Flags: review?(matt.woodrow) → review+
(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.
(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.
Attachment #8385752 - Flags: review?(matt.woodrow) → review+
and a bustage fix for non-unified builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd0fa3e4a8e
Blocks: 975261
needed for bug 978712
blocking-b2g: --- → 1.3T+
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.
You need to log in before you can comment on or make changes to this bug.