1358955 - stylo: Update SMIL animations that depend on the cascade

Status: ASSIGNED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Brian Birtles (:birtles)]

SMIL has this (horribly inefficient) setup where, if it sees an animation value that depends on its context (e.g. an em-based length), it sets a flag and recomputes it on each tick.

Unless we set this to true all the time at least at least the following tests from test_smilChangeAfterFrozen.xhtml fail:

    failed | Checking animated font-size=2em after updating context - got "20px", expected "40px"
    failed | Checking animated font-size=150% after updating context - got "15px", expected "30px"
    failed | Checking animated font-size > 20px after updating context
    failed | Checking animated font-weight = 900 after updating context - got 400, expected 900
    failed | Checking animated calc font-size == 24px after updating context - got 12, expected 24

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I just looked this, the relevant part of gecko is here:
https://hg.mozilla.org/mozilla-central/file/120d8562d4a5/layout/style/StyleAnimationValue.cpp#l3497

I haven't looked into the implementation in detail, but hmm, I think we should not mimic this, we should figure out other ways.

Comment 2

[Brian Birtles (:birtles)]

I think we might be able to do this without detecting context-sensitive values.

The approach I have in mind is:

1. Add some FFI for detecting if we have frozen SMIL animations in the document

   The tricky bit here is that normally we detect animations per element but in SMIL we normally do it per document and
   looking it up per element can be expensive. That limits our ability to re-use the "update effect properties" sequential
   task.

2. Add a new "update SMIL animations" task

3. Dispatch it any time we update the cascade anywhere

4. In that task, call into a new method on the SMIL animation controller that iterates through the registered
   animation elements. For each element:
   - Get the animation function
   - If the animation function is frozen and if its values are CSS values, reset its mHasChanged member.

5. If any animation function is newly "changed" (i.e. mHasChanged is newly-true), set the mResampleNeeded member of the
   animation controller to true, and, if necessary, mark the presshell as needing a style flush.

If that works, it's probably better than the current setup where we reparse the string on every single tick while frozen.

The cost is that for a document with frozen SMIL animations of CSS properties that are *not* context-sensitive, we'll end up doing some (fairly inexpensive) extra work each time the cascade is updated.

In terms of performance, I suspect we could just drop (1) and always do (2). That is, rather than doing an FFI call as part of each traversal in order to see if we have SMIL animations, just do that same check in the post traversal or some other sequential task.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

(In reply to Brian Birtles (:birtles) from comment #2)
> I think we might be able to do this without detecting context-sensitive
> values.
> 
> The approach I have in mind is:
> 
> 1. Add some FFI for detecting if we have frozen SMIL animations in the
> document
> 
>    The tricky bit here is that normally we detect animations per element but
> in SMIL we normally do it per document and
>    looking it up per element can be expensive. That limits our ability to
> re-use the "update effect properties" sequential
>    task.
> 
> 2. Add a new "update SMIL animations" task
> 
> 3. Dispatch it any time we update the cascade anywhere
> 
> 4. In that task, call into a new method on the SMIL animation controller
> that iterates through the registered
>    animation elements. For each element:
>    - Get the animation function
>    - If the animation function is frozen and if its values are CSS values,
> reset its mHasChanged member.
> 
> 5. If any animation function is newly "changed" (i.e. mHasChanged is
> newly-true), set the mResampleNeeded member of the
>    animation controller to true, and, if necessary, mark the presshell as
> needing a style flush.
> 
> If that works, it's probably better than the current setup where we reparse
> the string on every single tick while frozen.
> 
> The cost is that for a document with frozen SMIL animations of CSS
> properties that are *not* context-sensitive, we'll end up doing some (fairly
> inexpensive) extra work each time the cascade is updated.
> 
> In terms of performance, I suspect we could just drop (1) and always do (2).
> That is, rather than doing an FFI call as part of each traversal in order to
> see if we have SMIL animations, just do that same check in the post
> traversal or some other sequential task.

I guess the check can't work if it's done in the other sequential task since there might be cases that we don't create the sequential task for the SMIL animations, e.g. the document has only one context-sensitive SMIL animation.

I guess the place where we can do the check is the second EffectCompositor::PreTraverse() call, we can do the check along with the PreTraverse() in ServoStyleSet::StyleDocument().  Of course we need to call a second nsSMILAnimationController::PreTraverse().  I am not sure about other cases (e.g. frame constructions).

Comment 4

[Brian Birtles (:birtles)]

The trouble is the sequential task is per-element but the SMIL animation controller really tracks state on a per-document level so from what I can tell re-using the per-element sequential tasks would be wasteful. I think we can probably just add a hook in the post-traversal and update frozen SMIL animations there. I think its probably only when we have a post-traversal that we'd need to update this given the way SMIL animations currently update (i.e. I don't think we need to do it when styling a new/reconstructed subtree etc.).

Furthermore, SMIL animations typically only update once per tick. So, for example, if something in the animation traversal updated the font-size, in theory we should update any SMIL animations at that point (in a sequential task), but currently we don't do that in Gecko. That's less than ideal but that's the current Gecko behavior so that's all we need to match for now and so doing it in a post-traversal will hopefully work and be less expensive since we'd avoid doing it after each traversal and since we're already be in C++ code so don't need any extra FFI calls.

Hopefully in the near-ish future we can port the SMIL-CSS parts over to re-using the Web Animations setup and fix this properly, however.

Comment 5

[Brian Birtles (:birtles)]

Firstly, it's not just frozen animations we need to apply this to, but basically all animations of CSS properties. I'm not sure if it's a good idea to unconditionally mark all SMIL animations as needing an update at the end of each traversal. That seems bad. At very least we should distinguish between animation-only and full restyles.

Secondly, I'm seeing something weird with the calc() and font-size test. With my patch, the first time I call getComputedStyle() on the calc element I get 23.2167px. Which is wrong. However, if I call getComputedStyle() again I get 24.3167px each time. Gecko, however, gives me exactly 24px every time. Not sure if that's a problem with calc, font-size, or SMIL at this stage.

Comment 6

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #5)
> Secondly, I'm seeing something weird with the calc() and font-size test.
> With my patch, the first time I call getComputedStyle() on the calc element
> I get 23.2167px. Which is wrong. However, if I call getComputedStyle() again
> I get 24.3167px each time. Gecko, however, gives me exactly 24px every time.
> Not sure if that's a problem with calc, font-size, or SMIL at this stage.

Filed bug 1394302 for this since I can observe somewhat similar results with CSS animations too.

Comment 7

[Brian Birtles (:birtles)]

In terms of the simplest approach, we could just mark all SMIL animations as needing an update on every non-animation traversal. That sounds really bad, but I think it might be ok. Here's why:

* The current setup in Gecko is that any "context-sensitive" animations report themselves as needing to be updated on every single resample. Ever. That is nsSMILAnimationFunction::HasChanged *always* reports true for these animations. If you have one such animation (frozen or not) in your document, we'll update SMIL animations on every single resample.

* However, we don't actually need to mark the document as needing a flush, or even mark the animation controller as needing a resample in order to produce consistent behavior with Gecko. We simply need to mark the animation function as having changed so that when something else triggers a resample, we update the animation. (This is probably a bug in Gecko, but clearly it hasn't been a problem in the last 10 years or so, so I'm quite ok with continuing the buggy behavior as a temporary measure until we rebase this on Web Animations, especially if it makes this patch simpler, faster, and less risky.)

* This behavior would mean that we would always update SMIL animations that target CSS properties (and not just ones that use context-sensitive properties) on the resample following a restyle with non-animation changes but that seems much more reasonable than the current Gecko behavior and something we can refine step by step later if needed (e.g. start by narrowing down the set of properties that we consider context-sensitive so that, e.g., opacity animations are excluded).

Comment 8

[Brian Birtles (:birtles)]

WIP: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5487d003c354eaedf4ed2958b99e855bf0db51d4

Comment 9

[Brian Birtles (:birtles)]

(In reply to Brian Birtles (:birtles) from comment #7)
> In terms of the simplest approach, we could just mark all SMIL animations as
> needing an update on every non-animation traversal.

Technically, this should happen also on animation-only traversals. I wonder how important that case is, however.

Comment 10

[Brian Birtles (:birtles)]

Attachment: Test for changing the context using animation

(In reply to Brian Birtles (:birtles) from comment #9)
> (In reply to Brian Birtles (:birtles) from comment #7)
> > In terms of the simplest approach, we could just mark all SMIL animations as
> > needing an update on every non-animation traversal.
> 
> Technically, this should happen also on animation-only traversals. I wonder
> how important that case is, however.

Looks like we do need to support this case (although there are no tests for it).

Comment 11

[Brian Birtles (:birtles)]

I'm a bit stuck on this at the moment. On the one hand, we need to support changes to context from animations (which my previous patch didn't do). If we simply detect *any* changes to the cascade and then mark SMIL animations of CSS properties as needing to update on the next sample, then we will basically always mark such these anmations as needing an update (since when we *do* update the animation, we'll then mark it as needing an update again). This is basically what Gecko currently does, except Gecko only does it when it detects a context-sensitive keyword.

On the other hand, I haven't struck upon any really simple way to detect context-sensitive keywords in Servo, or at least not from within Servo_GetAnimationValues which is where we'd ideally like to do it. To do it there we'd basically want to do it from within the AnimationValueIterator, i.e. AnimationValue::from_declaration. It's easy enough to detect CSS-wide keywords like 'inherit' there, but detecting context-sensitive keywords on individual values looks invasive.

Instead, it would probably best to use compute_for_declarations and extend CascadeInfo to note when we see a font-relative length, currentcolor, or an inherit keyword like we already do for viewport units. That's probably the cleanest and simplest solutions but even that seems a little error-prone and invasive. Nevertheless, let's call that approach A for now.

As an alternative approach B, I'm wondering if we could just detect context-sensitive values by re-composing attributes whose animation values might have changed and comparing them. The tricky thing about SMIL is that we don't cache anything except the base value so we'd just have to compare against the value stored in the SMIL override style.

Basically, at the end of a cascade we'd call into the animation controller and it would:

- iterate through its animation elements,
- find the ones that target CSS properties,
- build up compositor tables for each of them,
- get them to re-calculate the final animation value,
- compare that with the value stored on the target element's SMIL override declaration
- if the values differed, mark the compositor as needing updating somehow (perhaps just
  go through and toggle mHasChanged on all the un-skipped functions in the compositor.
  We can't unfortunately, just toggle mForceCompositing on the compositor since
  that state doesn't get transferred across to new compositors.)

One tricky part is that mLastCompositor stores raw pointers to the animation functions it uses. It's not clear that those pointers would still be valid so we shouldn't just re-use mLastCompositor. Instead we'd need to build up new compositors.

That all sounds pretty involved, and I slightly prefer approach A. The advantages of approach B, however are:

1) The code changes are entirely isolated to dom/smil. (If we extend CascadeInfo, we potentially introduce some
   overhead to non-SMIL consumers unless we introduce more logic to toggle that off.)
2) It means we only update SMIL animations when some context has actually changed, unlike Gecko which will update
   such animations on every single sample regardless of whether or not something has changed.

I'll try hacking up approach B and see if it works and just how complex it is.

Comment 12

[Brian Birtles (:birtles)]

I wrote up approach B from comment 11 and it worked great, but then I discovered that SMIL actually triggers a style flush as part of resampling, so it basically meant we'd end up resampling animations for CSS properties on every sample (since, at the end of the cascade that happens *inside* the resample, we'd end up doing a mini-sample just to see if it needs to be updated or not). Which is basically no different to just setting "is context-sensitive?" to true for all SMIL animations of CSS properties.

So then I went and tried approach A from comment 11, but just now I update my tree and see that CascadeInfo no longer exists.[1]

Emilio, I was planning on using CascadeInfo to detect context-sensitive values (i.e. CSS values that depend on the cascade). Basically, SMIL has this setup where it sticks values in the SMIL override and then doesn't touch them unless the timing or animation parameters change. However, for animation parameters whose computed value might depend on the cascade (e.g. '10em', 'currentColor') it sets a flag that basically says, "Every time you resample SMIL, recalculate this animation even if its timing/values haven't changed." Anyway, now that CascadeInfo is gone, what's the easiest way to hook in to determine if a value is context-sensitive or not? Preferably, we'd like to do this inside Servo_GetAnimationValues.

If this proves all too hard, we could possibly just set the context-sensitive flag to true always for SMIL animations of CSS. It's not ideal, but in Gecko we're prepared to do that for at least some animations so it can't be that bad (we probably should see if we can preserve the throttling behavior though). It doesn't mean we recompose SMIL animations on every tick -- only every time we have a sample scheduled.

[1] https://github.com/servo/servo/pull/18268

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Brian Birtles (:birtles) from comment #12)
> I wrote up approach B from comment 11 and it worked great, but then I
> discovered that SMIL actually triggers a style flush as part of resampling,
> so it basically meant we'd end up resampling animations for CSS properties
> on every sample (since, at the end of the cascade that happens *inside* the
> resample, we'd end up doing a mini-sample just to see if it needs to be
> updated or not). Which is basically no different to just setting "is
> context-sensitive?" to true for all SMIL animations of CSS properties.
> 
> So then I went and tried approach A from comment 11, but just now I update
> my tree and see that CascadeInfo no longer exists.[1]

Whoopsies.

> Emilio, I was planning on using CascadeInfo to detect context-sensitive
> values (i.e. CSS values that depend on the cascade). Basically, SMIL has
> this setup where it sticks values in the SMIL override and then doesn't
> touch them unless the timing or animation parameters change. However, for
> animation parameters whose computed value might depend on the cascade (e.g.
> '10em', 'currentColor') it sets a flag that basically says, "Every time you
> resample SMIL, recalculate this animation even if its timing/values haven't
> changed." Anyway, now that CascadeInfo is gone, what's the easiest way to
> hook in to determine if a value is context-sensitive or not? Preferably,
> we'd like to do this inside Servo_GetAnimationValues.

It's not impossible to do this reintroducing CascadeInfo, but I'd rather don't.

These cases look unsurprisingly similar to the cacheability conditions in the rule tree. I had a patch in bug 1367635 with some similar mechanism (that one was hacky, but it worked).

Cam has been looking at that bug recently. Maybe he can upload for review that part of his patches separately, or maybe he has more useful input here.

Comment 14

[Cameron McCormack (:heycam)]

Yeah, so in bug 1367635 attachment 8902610 [details] [diff] [review] I add a trait Cacheable with a function cache_conditions() and implement it on every specified value type, which returns a bitfield of property computation dependencies (CacheConditions).  It does seem like you want to call that, and just check that return value doesn't have any bits set.  Actually I only implement it for specified value types that are used for properties in reset structs, since that's all the struct caching mechanism cares about, but we can easily fill it out for other types.

I can split this out and land it first if that's helpful.

Comment 15

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #14)
> Yeah, so in bug 1367635 attachment 8902610 [details] [diff] [review] I add a
> trait Cacheable with a function cache_conditions() and implement it on every
> specified value type, which returns a bitfield of property computation
> dependencies (CacheConditions).  It does seem like you want to call that,
> and just check that return value doesn't have any bits set.  Actually I only
> implement it for specified value types that are used for properties in reset
> structs, since that's all the struct caching mechanism cares about, but we
> can easily fill it out for other types.
> 
> I can split this out and land it first if that's helpful.

That sounds awesome. No need to rush though unless you expect that bug to drag out. Ideally I'd like to fix this bug next week (since I'm away the following week) so if your patches land in the first half of next week that should be enough.

Comment 16

[Chris Peterson [:cpeterson]]

status-firefox57=wontfix unless someone thinks this bug should block 57

Comment 17

[Cameron McCormack (:heycam)]

Brian, can you tell me if this bug is worth my prioritizing the work in comment 14, or if we can leave this in the backlog?

Comment 18

[Brian Birtles (:birtles)]

(In reply to Cameron McCormack (:heycam) from comment #17)
> Brian, can you tell me if this bug is worth my prioritizing the work in
> comment 14, or if we can leave this in the backlog?

I think this is ok to leave in the backlog. SMIL usage being what it is.

Comment 19

[Joel Maher ( :jmaher ) (UTC -8)]

should we delete the test case test_smilChangeAfterFrozen.xhtml as we don't seem to be working on this?

Comment 20

[Brian Birtles (:birtles)]

I don't see any benefit to deleting the test case. We should fix this one day.