534325 - SVG SMIL: Effects of "endElement()" persist, after animation has been restarted.

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STR: Load attached testcase.

ACTUAL RESULTS:
  Green square starts out covering half of red square.  No animations play.
EXPECTED RESULTS:
  Green square starts out covering half of red square. It should grow to cover 3/4 of it, then snap back to half, and finally grow to cover the full red square.


DETAILS:
Quoting http://www.w3.org/TR/smil-animation/#ResettingElementState  :
> When an element restarts, certain state is "reset". In effect, all events
> and DOM methods calls in the past are cleared.

So, I claim that in the attached testcase, the "endElement()" call should have no effect, because it should be cleared as soon as we restart with the "begin()" call.

mozilla-central shows "ACTUAL RESULTS".
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a1pre) Gecko/20091211 Minefield/3.7a1pre

Opera 10.10 shows "EXPECTED RESULTS".

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: reference case 1

This reference case is identical to the testcase, except I've commented out the "endElement()" call.  This change gives us the EXPECTED RESULTS from comment 0.

Comment 2

[Daniel Holbert [:dholbert]]

NOTE: The syncbase timing patches (bug 474743) do not fix this bug.

NOTE: The following tweaks to the testcase will make its animations play correctly:
 * reducing the "begin" attribute to 0.
 OR
 * using setTimeout to delay our "go()" call to _after_ the time-value specified in our "begin" attribute.
 OR
 * Inserting a beginElement() call before the endElement() call.

So, I think we're fine as long as the endElement() call doesn't fire before we've reached our first "begin" instance time.

Comment 3

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

I'm currently working on related issues to this as part of the backwards seeking patch. I think the SMIL spec needs clarification on some of these points.

With the original test case what should happen is unclear.

endElement adds an end instance time causing the interval to be updated but since we now have at least one end instance there's no valid end for the interval beginning at t=1s and hence there's no valid interval and the animation will not play.

Then beginElement adds a begin instance time and this instance time might have the same time as the added end instance time in which case we'll get a zero-duration interval which will then play and end. The second interval will not play, however, because there is no matching end instance for it. Hence the animation will not play a second time, and maybe not a first.

Note that no restart occurs however since the animation only plays at most once so restart semantics are irrelevant.

That said, there are some genuine issues with our reset behaviour which I have partially addressed in a yet-to-be-published patch for bug 492458.

One such issue is that a call sequence such as the following:

beginElementAt(1)
endElementAt(2)

followed, at t=3s, by

beginElementAt(1)
endElementAt(2)

will, according to the letter of the SMIL spec, create a new interval and then as soon as the interval is begun it will be deleted. This is because SMIL recognises that the begin defining the current interval shouldn't be cleared on a restart but fails to recognise that sometimes the end needs to be preserved as well.

Comment 4

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

Just to follow up, the reason why restart semantics are not employed in this case and why the animation never plays is because there's no valid interval. See in particular:

http://www.w3.org/TR/smil-animation/#Timing-BeginEnd-LC-End

Note the part in the pseudocode that reads

  // if all ends are before the begin, bad interval
  else
    return FAILURE;

So in this case the call the endElement creates an end instance before our begins so we can't create a valid interval. In the case where there are no end instances different semantics are employed.

As I've noted in our implementation, I find this to be a bit counter-intuitive:

  http://mxr.mozilla.org/mozilla-central/source/content/smil/nsSMILTimedElement.cpp#742

Anyway, I'm suggesting this isn't a bug unless we want to contest the spec. Note that SMIL 3.0 contains the same pseudocode.

I don't have my SMIL work on this computer but I'll confirm later that my explanation in the previous comment is actually what we're doing.

(Also, another known bug with the code, even in the syncbase patch but which I've addressed since, is that we don't apply restart semantics when instance times are changed whilst we're in the postactive state.)

Comment 5

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

Seeking clarification from www-smil on some of these issues:

http://lists.w3.org/Archives/Public/www-smil/2009OctDec/0002.html

Comment 6

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

Also, regarding comment 4, I've checked Batik 1.7, Opera 10.10 and a WebKit nightly (10 Dec 09) and, unlike ASV, they all seem to stick to SMIL's rule about bad intervals.

However, as described in my post to www-smil (comment 5), they don't seem to implement the reset behaviour, or at least not as I understand it.

Comment 7

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

Attachment: Patch v1a

Proposed patch.

I've presented some of these findings to www-smil who've confirmed that the spec is inconsistent on many of these points.

See: http://lists.w3.org/Archives/Public/www-smil/2009OctDec/0004.html and related messages.

I've decided to bring our reset behaviour in line with that discussion in the following ways:

1. Restart semantics should be employed when the animation restarts.

That is to say, that if we have an animation with two intervals from 1s-2s and 3s-4s, the restart behaviour is effected at t=3s. Currently we do it at t=2s.

Note that this produces inconsistent behaviour in that instance times are not cleared when the first interval is begun (t=1s) but are when the second interval is begun (t=3s). This is a recognised issue with the SMIL specification.

2. If we have no end attribute then allow for intervals to use the active
duration to determine the end of the interval rather than requiring an end
instance.

Strictly speaking this is more correct since the SMIL pseudocode contains the condition: "If there was no end attribute specified" whereas we were previously testing for both no end attribute and no end instances.

This fixes the issue in the test cases attached to this bug and is more intuitive.

One issue with this approach is that, contra SMIL, in order for many of our syncbase test cases to pass we need to handle syncbase time in a similar way to event times in that if we don't have a matching end we let it slide. Without this at least one test from the SVG 1.2 Tiny Test Suite fails to pass. (This is case (c) in the relevant comment of the attached patch).

Also, in testing this I discovered a couple of bugs with our handling of fill behaviour and have fixed them here.

Comment 8

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

I forgot to mention that this patch applies on top of the syncbase patches in bug 474743.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 419648 [details] [diff] [review]
Patch v1a

-/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Why are you taking this out?

Comment 10

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

Attachment: Patch v1b (sr=roc)

(In reply to comment #9)
> (From update of attachment 419648 [details] [diff] [review])
> -/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
> */
> 
> Why are you taking this out?
Sorry, don't know how that happened, must have been manual merge fallout. Fixed.

Comment 11

[Daniel Holbert [:dholbert]]

It looks like both loops in Reset() have O(N*M) performance right now, where N is the number of instances removed and M is the number of remaining instances.

I that fact precludes this bug's patch, though.

Comment 12

[Daniel Holbert [:dholbert]]

er s/precludes/predates/

Comment 13

[Daniel Holbert [:dholbert]]

Pushed this fix per Brian's request:
  http://hg.mozilla.org/mozilla-central/rev/ef2195ab9aba