617817 - SMIL animation resamples <set> animation (and animation with dur="indefinite") way more frequently than it needs to.

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Right now, standard <set> animation doesn't benefit from our "has anything in our sandwich changed" optimizations.

In particular, <set> animations end up triggering  "mHasChanged" on every single sample of their duration, even though nothing has actually changed.  That happens in nsSMILAnimationFunction::SampleAt -- in that function, we see that we're sampling at a new time ("mSampleTime != aSampleTime"), so we assume that our output value may have changed.

We actually know that a new time in our simple duration result will *not* produce a different result, though, for:
 - <set> (which applies the same value for the whole simple duration)
 - any animation with an indefinite duration (in which case the sample-time does not matter, once we've entered the duration)

=== STEPS TO REPRODUCE ===
Add a printf in nsSVGLength2::SMILLength::SetAnimValue, and load testcase.
EXPECTED RESULTS: A single instance of that printf.
ACTUAL RESULTS: Continuously-spammed printf.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (indefinite duration)

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: reference case (using fill="freeze")

We handle this nicely for fill="freeze", as shown by this testcase -- i.e. we recognize that a frozen animation shouldn't be considered to have always changed.

I'd like for the testcases to behave more like this case. :)

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: debugging patch: spam NS_WARNING in SMILLength::SetAnimValue

(In reply to comment #0)
> === STEPS TO REPRODUCE ===
> Add a printf in nsSVGLength2::SMILLength::SetAnimValue, and load testcase.

For convenience, here's a patch that does that.

Comment 4

[Daniel Holbert [:dholbert]]

This bug should also apply for the <animate values="justOneValue"> case.

Taking -- this should be pretty easy to fix.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 6

[Daniel Holbert [:dholbert]]

This patch adds one PRPackedBool to nsSMILAnimationFunction, so I took the liberty of grouping all the other PRPackedBool vars together at the end of the class definition, and making them actually 1 bit each.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v1a

(removed one unnecessary comment from previous patch -- "mSimpleDuration.IsIndefinite() ||  // Indefinite duration.")

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: fix v1b

(...and one more repost -- last version had a build error due to missing ")" -- I thought I'd recompiled one last time before posting, but I guess not. :))

Comment 9

[Daniel Holbert [:dholbert]]

FWIW, I ran this through reftests, crashtests, & mochitests on TryServer (all platforms), and it passed.

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 496405 [details] [diff] [review]
fix v1b

Looks great! Love these performance wins.

Just one question:

> void
> nsSMILAnimationFunction::SampleAt(nsSMILTime aSampleTime,
>                                   const nsSMILTimeValue& aSimpleDuration,
>                                   PRUint32 aRepeatIteration)
> {
>-  if (mHasChanged || mLastValue || mSampleTime != aSampleTime ||
>-      mSimpleDuration != aSimpleDuration ||
>-      mRepeatIteration != aRepeatIteration) {
>+  if (!mHasChanged &&
>+      (mLastValue || // Were we previously sampling a fill="freeze" final val?
>+       (!IsValueFixedForSimpleDuration() && // Are we sampling at a new point
>+        (mSampleTime != aSampleTime ||      // in simple duration (if we care)?
>+         mSimpleDuration != aSimpleDuration)) ||
>+       (mRepeatIteration != aRepeatIteration && // Are we on a new repeat and
>+        GetAccumulate()))) {                    // accumulating across repeats?
>     mHasChanged = PR_TRUE;
>   }

This is a bit hard to read with the nested brackets and comments snuggling up
alongside.

Could we do this instead?

// Were we previously sampling a fill="freeze" final val?
mHasChanged |= mLastValue;

// Are we sampling at a new point in simple duration? And do we care?
mHasChanged |=
  (mSampleTime != aSampleTime || mSimpleDuration != aSimpleDuration) &&
  !IsValueFixedForSimpleDuration();

// Are we on a new repeat and accumulating across repeats?
mHasChanged |= mRepeatIteration != aRepeatIteration && GetAccumulate();


I personally find that much easier to read and debug (since it's easier to work
out what step caused the value to change when you step through the code).

r=me with that

Comment 11

[Daniel Holbert [:dholbert]]

Agreed -- that's much clearer.  Thanks!

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: fix v1c [r=birtles]

Here's the fix with comment 10's suggestion applied.  Requesting approval to land -- no anticipated functional effect, aside from a perf improvement.  Certain types of fixed-value animations were unwittingly disabling our "nothing's changed so we don't need to recompose" optimization (see comment 0, comment 4), and this patch fixes that.

Comment 13

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/89e633e31dad