Closed Bug 814921 Opened 7 years ago Closed 7 years ago

Reduce the need to flush animations for SMIL

Categories

(Core :: Layout, defect)

16 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20
blocking-basecamp +
Tracking Status
firefox19 --- fixed
firefox20 --- fixed
b2g18 --- fixed

People

(Reporter: nrc, Assigned: nrc)

References

Details

Attachments

(1 file, 2 obsolete files)

For the animations and transitions on b2g, throttling of OMTA flushing is let down by unthrottable flushes due to SMIL. We should not flush when we don't have to.
Comment by dholbert from bug 780692 (see that bug for a little more discussion):

(In reply to Daniel Holbert [:dholbert] from comment #166)
> d'oh... So I just remembered some caveats to comment 164 that make this
> trickier. (Sorry, it's been a little while since I thought in depth about
> animations, so it took a bit to fully page the complex bits back in.)
> 
> I think we technically *do* need to flush CSS animations during SMIL
> samples, for correctness, after all.
> 
> If you have a CSS animation that targets one property, and then a SMIL
> animation targeting a *different* property who is influenced by the
> CSS-animated property, then we need CSS animations to be up-to-date in order
> for the SMIL animation to be correct.
> 
> One example of this would be a CSS animation on "font-size", while a SMIL
> animation changes some length from "5em" to "10em". (The meanings of those
> "em" units would be dependent on the CSS font-size animation.)  Another
> example would be a CSS animation on "color" while a SMIL animation takes
> "fill" to the currentColor keyword. (which computes to the current value of
> the "color" property)  You can get similar interactions w/ inheritance, too.
> (if you have a CSS animation on a parent and a SMIL animation w/ "inherit"
> on a child)
> 
> Sorry for not remembering that earlier. Does that complicate things here?
Attached patch WIP patch (obsolete) — Splinter Review
This patch causes SMIL to never flush animations, which is presumably broken, but should make b2g pretty snappy.
Assignee: nobody → ncameron
cjones: how do we use SMIL in Gaia? Does it have any interaction with CSS animations that you know of? Is it used where we care about transition performance? Thanks!
Flags: needinfo?(jones.chris.g)
We use it to animate the new lockscreen "unlock" interaction

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/index.html#L741

That animation is very perf sensitive.  It doesn't directly interact with CSS transitions or animations, but there are CSS transitions/animations that will run in parallel.

There was a bug previously where this SMIL animation would continue to run even when the lock screen was dismissed (bug 814076).  This has been fixed.

Do *inactive* SMIL animations cause us to flush?
Flags: needinfo?(jones.chris.g)
Yeah, it looks like these animations should not be running, and while they're not running we should not be calling DoSample!
I'm not sure what "these animations" is specifically here.

There are some cases where we continue composing animations even after they're finished. For example:

  <animate attributeName="x" by="1em" dur="1s" fill="freeze"/>

Here, even after the animation is finished the resulting animation value could change if the font-size changes.

For some cases like this we keep composing forever. See bug 533291 comment 18 (and comments 19-20). I think this also applies to animations on the 'display' property due to bug 536660.

(Which is one reason, amongst others, why it's better to animate visibility.)
(In reply to Brian Birtles (:birtles) from comment #6)
> I'm not sure what "these animations" is specifically here.

I think roc's referring to the ones in the github link in comment 4.  Those aren't fill="freeze", so I don't think there's any reason we'd be sampling them after they complete.
(though the second-to-last paragraph of comment 4 makes it sound like everything's OK w/ those animations now, I think...?)
Attached patch patch (obsolete) — Splinter Review
Attachment #684911 - Attachment is obsolete: true
Attachment #685994 - Flags: review?(dholbert)
Comment on attachment 685994 [details] [diff] [review]
patch


>   // Set running sample flag -- do this before flushing styles so that when we
>   // flush styles we don't end up requesting extra samples
>   mRunningSample = true;
>-  nsCOMPtr<nsIDocument> kungFuDeathGrip(mDocument);  // keeps 'this' alive too
>-  mDocument->FlushPendingNotifications(Flush_Style);
> 
>   // WARNING: 
>   // WARNING: the above flush may have destroyed the pres shell and/or
>   // WARNING: frames and other layout related objects.
>   // WARNING:

Move those "WARNING" lines, too -- they go with the flush.

>+  if (currentCompositorTable->Count() == 0) {
>+    mLastCompositorTable = nullptr;
>+    mRunningSample = false;
>+    return;
>+  }

Ah, good catch @ turning off mRunningSample.

Rather than explicitly un-setting it here (and at the end, and in any other early-returns we add in the future), could you add an AutoRestore helper-variable, like the one we use here:
 https://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILInstanceTime.cpp#92

(but with "using namespace mozilla" added up top, instead of the mozilla:: prefixing)

>   currentCompositorTable->EnumerateEntries(DoComposeAttribute, nullptr);
>   mRunningSample = false;

(...and we can remove this "mRunningSample = false" line, once we've got that AutoRestore helper set up.)

r=me with that.
Attachment #685994 - Flags: review?(dholbert) → review+
Depends on: 592477
Attached patch patchSplinter Review
patch with all requested changes, carrying r=dholbert
Attachment #685994 - Attachment is obsolete: true
Attachment #686289 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2db54808d08e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Hmm, now that I've landed this on aurora, I realise I don't actually have approval. But it is logically part of bug 780692 (and used to be actually part of) which does, there is no point in landing that without this, so I hope it is OK, but asking for b-b+ now to be proper. Obviously, I can backout if there is a problem with landing this.
blocking-basecamp: --- → ?
Comment on attachment 686289 [details] [diff] [review]
patch

Probably best to back out for now.

This is part of the package with bug 780692.  There's no point in taking either without the other.
Attachment #686289 - Flags: approval-mozilla-b2g18?
Attachment #686289 - Flags: approval-mozilla-aurora?
Comment on attachment 686289 [details] [diff] [review]
patch

Bug 780692 is now blocking, so I'm going to carry that flag over to here.
Attachment #686289 - Flags: approval-mozilla-b2g18?
Attachment #686289 - Flags: approval-mozilla-aurora?
blocking-basecamp: ? → +
Keywords: checkin-needed
landed on Aurora already, will land on b2g18 with 780692, not worth checking in separately
Keywords: checkin-needed
BTW, I rebased this too, if you haven't already.
No longer depends on: 822392
Product: Core → Core Graveyard
Component: Layout: Misc Code → Layout
Product: Core Graveyard → Core
You need to log in before you can comment on or make changes to this bug.