814921 - Reduce the need to flush animations for SMIL

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Nick Cameron [:nrc]]

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 1

[Nick Cameron [:nrc]]

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?

Comment 2

[Nick Cameron [:nrc]]

Attachment: WIP patch

This patch causes SMIL to never flush animations, which is presumably broken, but should make b2g pretty snappy.

Comment 3

[Nick Cameron [:nrc]]

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!

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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?

Comment 5

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

Yeah, it looks like these animations should not be running, and while they're not running we should not be calling DoSample!

Comment 6

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

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.)

Comment 7

[Daniel Holbert [:dholbert]]

(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.

Comment 8

[Daniel Holbert [:dholbert]]

(though the second-to-last paragraph of comment 4 makes it sound like everything's OK w/ those animations now, I think...?)

Comment 9

[Nick Cameron [:nrc]]

Attachment: patch

Comment 10

[Daniel Holbert [: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.

Comment 11

[Nick Cameron [:nrc]]

try push: https://tbpl.mozilla.org/?tree=Try&rev=a56fee435b7d

Comment 12

[Nick Cameron [:nrc]]

Attachment: patch

patch with all requested changes, carrying r=dholbert

Comment 13

[Nick Cameron [:nrc]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2db54808d08e

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2db54808d08e

Comment 15

[Nick Cameron [:nrc]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0946afe0c53f

Comment 16

[Nick Cameron [:nrc]]

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.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

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.

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 686289 [details] [diff] [review]
patch

Bug 780692 is now blocking, so I'm going to carry that flag over to here.

Comment 19

[Nick Cameron [:nrc]]

landed on Aurora already, will land on b2g18 with 780692, not worth checking in separately

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

BTW, I rebased this too, if you haven't already.

Comment 21

[Nick Cameron [:nrc]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/d3ebbae46c23