948245 - min attribute does not extend animation active interval

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

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

Attachment: min.svg

In working on bug 941315 I noticed min wasn't working as I expected. I dug into it and it turns out we don't handle min correctly when it is used to extend the active interval. In that case SMIL says we should stretch the interval and use the fill mode for the interval in between.

In the little test case attached a blue box animates for 3s but has min=6s. A second yellow box should start moving when the the blue box has finished its interval.

Currently in Gecko the yellow box starts moving after 3s, not after 6s.

In Blink, WebKit and Safari it works as expected, i.e. starts moving after 6s.

Comment 1

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

Attachment: Part 1 - Rework repeat duration calculation

Comment 2

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

Attachment: Part 2 - Implement extending the active duration with min

Comment 3

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

Try run results: https://tbpl.mozilla.org/?tree=Try&rev=36942cc457fb

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8345166 [details] [diff] [review]
Part 1 - Rework repeat duration calculation

Part 1 looks good! Just a few nits:

>+++ b/content/smil/nsSMILRepeatCount.h
>-  operator double() const { return mCount; }
>+  operator double() const {
>+    NS_ABORT_IF_FALSE(IsDefinite(),
>+      "Converting indefinite or unset repeat count to double");
>+    return mCount;

Let's use MOZ_ASSERT here. (it's shorter, it's the New Hotness, and it has the same aborting behavior)

>+++ b/content/smil/nsSMILTimedElement.cpp
> nsSMILTimedElement::GetRepeatDuration() const
> {
>+  nsSMILTimeValue multipliedDuration(nsSMILTimeValue::Indefinite());
>+  if (mRepeatCount.IsDefinite() && mSimpleDur.IsDefinite()) {
>+    multipliedDuration.SetMillis(
>+      nsSMILTime(mRepeatCount * double(mSimpleDur.GetMillis())));
>   }

Rather than passing nsSMILTimeValue::Indefinite() into the constructor, I'd marginally prefer we just declare multipliedDuration without any constructor params, and then call SetIndefinite() in an "else" clause here.  (This is at least as efficient, and it saves us from needing to instantiate an extra helper-nsSMILTimeValue inside of nsSMILTimeValue::Indefinite()).

>+  if (mRepeatDur.IsResolved() && mRepeatCount.IsSet()) {
>+    repeatDuration = std::min(multipliedDuration, mRepeatDur);
>+  } else if (mRepeatDur.IsResolved()) {
>+    repeatDuration = mRepeatDur;

These first two clauses can be collapsed together into:
  if (mRepeatDur.IsResolved()) {
    repeatDuration = std::min(multipliedDuration, mRepeatDur);

...because if mRepeatCount isn't set, multipliedDuration will be indefinite -- and nsSMILTimeValue::CompareTo considers indefinite values as "larger than" all resolved values, so the ::min expression will just do the right thing and return mRepeatDur).

(This simplification also saves us from having to redundantly check mRepeatDur.IsResolved() twice in a row.)

>diff --git a/content/smil/test/test_smilRepeatDuration.html b/content/smil/test/test_smilRepeatDuration.html
>+<div id="content" style="display: none">
>+<svg width="100%" height="1" onload="this.pauseAnimations()">

These seem like odd width and height attributes (because the width=100% is already the default, height=1 seems very tiny, and moreover this is in a display:none div so sizing specifics don't matter.

So: can we just drop 'height' and 'width' here?

>+  // Tests the calculation of the repeat duration which is one of the steps
>+  // towards determining the active duration.
>+  //
>+  // The repeat duration is determined by the following three attributes:
>+  //
>+  // dur:         may be definite (e.g. '2s') or 'indefinite'
>+  // repeatCount: may be set (e.g. '2.5'), 'indefinite', or not set
>+  // repeatDur:   may be set (e.g. '5s'), 'indefinite', or not set

You want "may be definite", not "may be set", on that last line. (Right now, the phrasing makes it sound like "set" and "indefinite" are exclusive.)

Also, maybe add "(the default)" at the end of the "dur" line, to make it clear why "not set" isn't an option there.

(I suppose it's not worth explicitly testing the "dur not set" option, since we already have tests that verify that unset-dur is treated as "indefinite"?)


>+  var testCases =
>+    [
[...]
>+      // 10. repeatDur: definite, repeatCount: definite, dur: definite
>+      { repeatDur: 15, repeatCount: 2, dur: 'indefinite', result: 15 },

The comment for 10 is incorrect -- should say "dur: indefinite" at the end.

>+  // Sometimes the repeat duration is defined to be 'indefinite'. In this case
>+  // calling getStartTime on b will throw an exception so we need to catch that
>+  // exception and translate it in 'indefinite' as follows:

s/translate it in/translate it to/
(I think?)

>+  window.addEventListener("load", runTests, false);
>+
>+  // Run through each of the test cases
>+  function runTests() {

Please add a line like...
  ok(svg.animationsPaused(), "should be paused by <svg> load handler");
...to the beginning of runTests() to verify that the onload handlers are firing in the order we expect.
(We have a sanity-check like this in a lot of the other SMIL mochitests.)

r=me with that.

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> >+  // dur:         may be definite (e.g. '2s') or 'indefinite'
> >+  // repeatCount: may be set (e.g. '2.5'), 'indefinite', or not set
> >+  // repeatDur:   may be set (e.g. '5s'), 'indefinite', or not set
> 
> You want "may be definite", not "may be set", on that last line. (Right now,
> the phrasing makes it sound like "set" and "indefinite" are exclusive.)

To clarify slightly -- it looks like [set, indefinite, not-set] is a correct breakdown for repeatCount, based on
  http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILRepeatCount.h#18
...but it's not the right breakdown for time values like 'dur' and 'repeatDur', based on
  http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILTimeValue.h#33

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8345167 [details] [diff] [review]
Part 2 - Implement extending the active duration with min

>+++ b/content/smil/nsSMILTimedElement.cpp
>+    // If the interval's repeat duration was shorter than its active duration
>+    // use the end of the repeat duration

nit: add comma at the end of first line, and period at end of second.

(And maybe add something like "to determine the frozen animation's state" at the end, for clarify about what you mean by "use" here)

>+  } else if (mElementState == STATE_ACTIVE) {
>+    // If we are being asked to sample the fill value while we *must* have
>+    // a repeat duration shorter than the active duration so use that.

I think you meant to say s/while/while active/?

(Otherwise, I don't quite understand this sentence)

>+        nsSMILTime nextRepeatActiveTime
>+          = (mCurrentRepeatIteration + 1) * mSimpleDur.GetMillis();

Pull the "=" up to the end of the previous line

>+        // Check that the repeat fits within the repeat duration
>+        if (nsSMILTimeValue(nextRepeatActiveTime) < GetRepeatDuration()) {
>+          nextRepeat.SetMillis(
>+            mCurrentInterval->Begin()->Time().GetMillis() +
>+            nextRepeatActiveTime);

Observation: you don't technically need a newline after "SetMillis".
(i.e. if you remove that newline and reindent, you'll still be within the 80-character limit.)

But this is fine as-is, too, if you think it's more readable this way.

>+++ b/content/smil/test/test_smilMinTiming.html
>+<div id="content" style="display: none">
>+<svg width="100%" height="1" onload="this.pauseAnimations()">

As noted for the other patch, we probably should just drop the width & height attributes here.

>+  function runTests() {
>+    // In the extended period (t=150s) we should not be animating or filling
>+    // since the default fill mode is "none".
>+    animation.ownerSVGElement.setCurrentTime(150);

As noted for the other patch, let's double-check that we're actually paused, with an ok(), before seeking & testing anything here.

>+    ise(rect.x.animVal.value, 0, "Shouldn't fill in extended period");
[...]
>+    animation.setAttribute("fill", "freeze");
>+    ise(rect.x.animVal.value, 100, "Should fill in extended period");

These two messages will appear as consecutive messages in mochitest log, and they appear to contradict each other, which is confusing.

Maybe add "with fill unset" to the first one, and "with fill='freeze'" to the second?

>+    // Check that if we seek past the interval we fill with the value at the end
>+    // of the _repeat_duration_ not the value at the end of the
>+    // _active_duration_.
>+    animation.setAttribute("repeatCount", "1.5");
>+    animation.ownerSVGElement.setCurrentTime(250);
>+    ise(rect.x.animVal.value, 50,
>+        "Should fill with the end of the repeat duration value");

Nit: Might be worth considering a time like "225" instead of "250", because with certain classes of bugs (i.e. if we incorrectly repeated the animation after we reached 200 for some reason), we would end up at an animated value purely by accident here and pass this check (simply because we'd be halfway through a simple duration).

It's a long shot, but might as well avoid it entirely by picking a different time like 225 or 230 (or anything other than 250).

>+++ b/layout/reftests/svg/smil/min-1.svg
>+       1. A rectangle is set to lime.
[...]
>+       At time t=15s we should still be in the first animation's active
>+       interval with its fill mode applied (i.e. background is green). -->

s/green/lime/ (for consistency with the color name you used in the prose up in step (1)

>+  <rect width="100%" height="100%" fill="lime">
>+    <set attributeName="fill" to="orange" dur="10s" min="20s" id="a"/>

Please add begin="100s" and change the setTimeAndSnapshot to seek to time 115, per bug 781362.

(If this worries you that the test will incorrectly render as lime, even if we took the snapshot without seeking, we could address that by making the rect start out as purple, and set it to lime only after 100s with an additional <set attributeName="fill" to="lime" begin="100s" dur="20s"/>.  But it's also fine without that tweak, IMHO.)

r=me with the above addressed.

Comment 7

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

Attachment: Part 1 - Rework repeat duration calculation, v1b r=dholbert

Address review feedback.

Comment 8

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

Attachment: Part 2 - Implement extending the active duration with min, v1b r=dholbert

Comment 9

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

Thanks Daniel for the reviews! That all makes sense to me.

Comment 10

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc1aa4cdac80
https://hg.mozilla.org/integration/mozilla-inbound/rev/63737099fabf

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc1aa4cdac80
https://hg.mozilla.org/mozilla-central/rev/63737099fabf