755209 - High CPU usage with fill="freeze" after the active duration has ended, e.g. on anim-filter-size-01.svg

Status: RESOLVED DUPLICATE of bug 758505

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

I'm seeing high CPU usage with fill="freeze" after the active duration has ended on this:

https://mxr.mozilla.org/mozilla-central/source/layout/reftests/svg/smil/anim-filter-size-01.svg?raw=1&ctype=image/svg+xml

Bug 527762 was previously filed on the same issue with a different test case, but that testcase still seems to be fixed.

Comment 1

[Daniel Holbert [:dholbert]]

I believe this is just because we're animating between percentage-values...
>    <animate attributeName="height"
>             calcMode="linear"
>             begin="0s" dur="1s"
>             from="0%" to="100%"
>             fill="freeze"/>

...and percent values prevent us from caching the animation sandwich:
http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGLength2.cpp?mark=520-521#503

...so we recompose every time.

They prevent us from caching the animation sandwich because we can't be sure that a % value will produce the same px-result as it did in the previous sample, so combining such a value with other (additive) animations in the sandwich could produce a different result than it did last time.  So we recompose the sandwich, which makes us set the animated value, which makes us invalidate.

There are a number of ways we could approach this.  Bug 758505's solution (checking to see if the animated value matches the previous one before we invalidate) would help, though it might be nice to do that up one level so we don't have to add that logic for every nsSMILType impl.

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #1)
> Bug 758505's solution
> (checking to see if the animated value matches the previous one before we
> invalidate) would help, though it might be nice to do that up one level so
> we don't have to add that logic for every nsSMILType impl.

er, s/nsSMILType/nsISMILAttr/

(and to expand on that slightly -- I was imagining we could expose a new method 'nsSMILValue nsISMILAttr::GetAnimValue()' or 'void nsISMILAttr::DoesSMILValueMatchAnimValue(const nsSMILValue& aValue)', which we'd check before calling SetAnimValue.

Or we could just do that check inside of nsISMILAttr::SetAnimValue -- maybe that's simpler.)

Comment 3

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

Yes, I think it's time to fix this. We keep getting bugs relating to this.

This is the last item listed under bug 732323.

> * be more aggressive about detecting when a style might have changed rather
> than just assuming it must have for certain types (see bug 562815. For
> example, we always recompose animations on the display property due to issues
> with <use>, bug 536660, this should be fixed)

If we can do something to fix that case with use and display:none at the same time that would be great. :)

Comment 4

[Daniel Holbert [:dholbert]]

See bug 758505 comment 11 and bug 758505 comment 12 -- once DLBI lands, it appears that invalidations from SMIL can indefinitely delay our reftest snapshot. So the unnecessary & unending invalidations from frozen animations will actually start causing reftest timeouts.

I r+'d mattwoodrow's targeted fix for nsSVGLength2 on that bug, and I suggested that we fix remaining nsISMILAttr impls over here, so as not to overload him.  (Alternately, if we end up taking a different strategy to fixing this on this bug, we can back out his nsSVGLength2 tweak when that strategy is ready to land.)

Comment 5

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

(In reply to Daniel Holbert [:dholbert] from comment #4)
> I r+'d mattwoodrow's targeted fix for nsSVGLength2 on that bug, and I
> suggested that we fix remaining nsISMILAttr impls over here, so as not to
> overload him.  (Alternately, if we end up taking a different strategy to
> fixing this on this bug, we can back out his nsSVGLength2 tweak when that
> strategy is ready to land.)

Looks good. I did a bunch of similar changes in bug 629200 when setting the base value to avoid dispatching redundant DOMAttrModified events (e.g. see nsSVGLength2::SetBaseValueInSpecifiedUnits).

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> I r+'d mattwoodrow's targeted fix for nsSVGLength2 on that bug, and I
> suggested that we fix remaining nsISMILAttr impls over here, so as not to
> overload him.

longsonr was good enough to fix the remaining instances over on that same bug (bug 758505), so I'm just duping this to that bug.