Closed Bug 617817 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b8

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(5 files, 3 obsolete files)

Attached image 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.
Component: Document Navigation → SVG
QA Contact: docshell → general
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. :)
(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.
This bug should also apply for the <animate values="justOneValue"> case.

Taking -- this should be pretty easy to fix.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attached patch fix v1 (obsolete) — Splinter Review
Attachment #496396 - Flags: review?
Attachment #496396 - Flags: review? → review?(birtles)
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.
Attached patch fix v1a (obsolete) — Splinter Review
(removed one unnecessary comment from previous patch -- "mSimpleDuration.IsIndefinite() ||  // Indefinite duration.")
Attachment #496396 - Attachment is obsolete: true
Attachment #496397 - Flags: review?(birtles)
Attachment #496396 - Flags: review?(birtles)
Attached patch fix v1b (obsolete) — Splinter Review
(...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. :))
Attachment #496397 - Attachment is obsolete: true
Attachment #496397 - Flags: review?(birtles)
Attachment #496405 - Flags: review?(birtles)
FWIW, I ran this through reftests, crashtests, & mochitests on TryServer (all platforms), and it passed.
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
Attachment #496405 - Flags: review?(birtles) → review+
Agreed -- that's much clearer.  Thanks!
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.
Attachment #496753 - Flags: review+
Attachment #496753 - Flags: approval2.0?
Attachment #496405 - Attachment is obsolete: true
Landed: http://hg.mozilla.org/mozilla-central/rev/89e633e31dad
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Depends on: 678822
You need to log in before you can comment on or make changes to this bug.