If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Reduce the need to flush animations for SMIL

RESOLVED FIXED in Firefox 19

Status

()

Core
Layout: Misc Code
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

16 Branch
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox19 fixed, firefox20 fixed, b2g18 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
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?
(Assignee)

Comment 2

5 years ago
Created attachment 684911 [details] [diff] [review]
WIP patch

This patch causes SMIL to never flush animations, which is presumably broken, but should make b2g pretty snappy.
Assignee: nobody → ncameron
(Assignee)

Comment 3

5 years ago
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...?)
(Assignee)

Comment 9

5 years ago
Created attachment 685994 [details] [diff] [review]
patch
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
(Assignee)

Comment 11

5 years ago
try push: https://tbpl.mozilla.org/?tree=Try&rev=a56fee435b7d
(Assignee)

Comment 12

5 years ago
Created attachment 686289 [details] [diff] [review]
patch

patch with all requested changes, carrying r=dholbert
Attachment #685994 - Attachment is obsolete: true
Attachment #686289 - Flags: review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/2db54808d08e
https://hg.mozilla.org/mozilla-central/rev/2db54808d08e
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/0946afe0c53f
(Assignee)

Comment 16

5 years ago
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
(Assignee)

Comment 19

5 years ago
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.

Updated

5 years ago
status-firefox19: --- → fixed
(Assignee)

Comment 21

5 years ago
https://hg.mozilla.org/releases/mozilla-b2g18/rev/d3ebbae46c23
status-b2g18: --- → fixed
status-firefox18: --- → fixed
status-firefox18: fixed → ---
status-firefox20: --- → fixed
Depends on: 822392
No longer depends on: 822392
You need to log in before you can comment on or make changes to this bug.