Closed
Bug 814921
Opened 12 years ago
Closed 12 years ago
Reduce the need to flush animations for SMIL
Categories
(Core :: Layout, defect)
Tracking
()
People
(Reporter: nrc, Assigned: nrc)
References
Details
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
nrc
:
review+
|
Details | Diff | Splinter Review |
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•12 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•12 years ago
|
||
This patch causes SMIL to never flush animations, which is presumably broken, but should make b2g pretty snappy.
Assignee: nobody → ncameron
Assignee | ||
Comment 3•12 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!
Comment 6•12 years ago
|
||
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•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
Attachment #684911 -
Attachment is obsolete: true
Attachment #685994 -
Flags: review?(dholbert)
Comment 10•12 years ago
|
||
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+
Assignee | ||
Comment 11•12 years ago
|
||
Assignee | ||
Comment 12•12 years ago
|
||
patch with all requested changes, carrying r=dholbert
Attachment #685994 -
Attachment is obsolete: true
Attachment #686289 -
Flags: review+
Assignee | ||
Comment 13•12 years ago
|
||
Comment 14•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Assignee | ||
Comment 15•12 years ago
|
||
Assignee | ||
Comment 16•12 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?
Updated•12 years ago
|
blocking-basecamp: ? → +
Keywords: checkin-needed
Assignee | ||
Comment 19•12 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•12 years ago
|
status-firefox19:
--- → fixed
Assignee | ||
Comment 21•12 years ago
|
||
status-b2g18:
--- → fixed
status-firefox18:
--- → fixed
Updated•12 years ago
|
status-firefox18:
fixed → ---
status-firefox20:
--- → fixed
Updated•6 years ago
|
Product: Core → Core Graveyard
Updated•6 years ago
|
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.
Description
•