STR: 1. Load "testcase 1" in a mozilla-central build (with "svg.smil.enabled" pref turned on) EXPECTED RESULTS: Document should contain a blue square (50x50) ACTUAL RESULTS: Document contains a wide blue rectangle (200x50), indicating that we don't correctly handle seeking backwards. This appears to be specifically because nsSMILTimedElement doesn't handle seeking backwards to a time before the current interval. More details to follow. NOTE: Opera 9.64 and Midori 0.1.2 (based on webkit trunk) both give me expected results.

This second testcase appears to work correctly (i.e. it renders as a square), but that's only because we fail an NS_ENSURE_TRUE check for non-negative sample times and bail out of attempting to apply animation effects. It triggers this warning over and over: > WARNING: NS_ENSURE_TRUE(mSampleTime >= 0) failed: file /mozilla/content/smil/nsSMILAnimationFunction.cpp, line 252 (Note that in the case of testcase 1, we seek to *exactly* one duration before the start time, which essentially corresponds to a sample time of "0" in an illegal negative-time iteration of the animation, and so it passes the above-quoted NS_ENSURE_TRUE test.)

Like you said, I'm pretty sure this is just because we don't handle seeking backwards yet. In fact, I think this is on my todo list as something to look into after syncbase timing. I think in implementing syncbase timing we'll end up reworking the data structures related to storing intervals (which is what is needed for backwards seek) so I'd rather defer this until after implementing syncbase timing if possible.

Cool, thanks Brian! In the meantime, here's a patch that adds some assertions so we catch this earlier (and so we catch it at all in the case of testcase 1), plus some reftests for situations like this.

(oops, I got my comments switched in reftests "a" & "b" -- fixed now.)

Was this fixed elsewhere? The testcase seems to pass now.

(In reply to comment #5) > Was this fixed elsewhere? The testcase seems to pass now. Ooops. Sorry was not testing in a trunk build. The build I was using contained other SMIL fixes for bugs 474049, 474739, 483584, and 506096 though so one of them fixes this.

Confirmed, the *first* testcase does pass, when using my patch queue applied up through "smil_css" (while stock mozilla-central is still broken on that testcase). I think this was probably due to the recent changes in handling NS_ERROR_FAILURE return values more gracefully in nsSMILAnimationFunction, per birtles' suggestion in bug 474049 comment 67. The changeset was: http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/1e9c3cfd5fcc However, the second testcase still triggers the warning mentioned in comment 1, and the reftests anim-x-seek-1a.svg and anim-x-seek-1b.svg (included in the attached patch) trigger this assertion (which is also added in the patch): > ###!!! ASSERTION: Sample time should be within current interval: 'aDocumentTime >= beginTime', file /mozilla/content/smil/nsSMILTimedElement.cpp, line 282 So, overall, this still isn't fixed.

This patch is just a draft mochitest but I think it demonstrates some of the issues involved in doing this properly.

Previously I'd planned to just accumulate old intervals and then roll them back. In discussion with dholbert I decided this was a really bad idea since in some cases this will lead to accumulating intervals and instance times indefinitely and this would be particularly bad for long-running applications like dashboards and the like. So this first patch attempts to filter out unneeded intervals and instance times. Actual backwards seeking support is yet to come but will build on top of this. I'm not putting this patch up for review just yet since it will likely change once backwards seeking is added.

Backwards seeking implementation. This patch incorporates the tests and assertions from attachment 376828 [details] [diff] [review] and the tests from attachment 419735 [details] [diff] [review]. With this patch applied the tests in attachment 376821 [details] and attachment 376822 [details] should pass. It should also fix bug 550978.

Comment on attachment 451448 [details] [diff] [review] Part 1: Instance and interval time filtering v1b >diff --git a/content/smil/nsSMILInterval.h b/content/smil/nsSMILInterval.h >+ // Indicates if the end points of the interval are fixed or not. >+ // >+ >+ // Note that this is not the same as having an end point whose TIME >+ // (nsSMILInstanceTime) is fixed. It is possible to have an end point with >+ // a fixed time and yet still update the end point to refer to a different >+ // nsSMILInstanceTime object. However, if the end point of the interval is >+ // fixed then both the nsSMILInstanceTime object returned for that interval >+ // and it's time value will not change. >+ PRPackedBool mBeginFixed; >+ PRPackedBool mEndFixed; s/fixed then/fixed, then/ s/it's/its/ Also, this could use some clarification on the meaning of "fixed" at different points, to be absolutely clear. In particular, when you say... > It is possible to have an end point with a fixed time ...I'd suggest specifically mentioning "nsSMILInstanceTime::IsFixed()" (assuming that's the type of 'fixed' you mean there) And then later, when you say... > However, if the end point of the interval is fixed ...I'd specifically mention the mEndFixed/mBeginFixed variables. (assuming that's the type of 'fixed' you mean there) >+ // Indicates that this instance time was generated by an event or a DOM >+ // call. Such instance times require special handling when the owning >+ // element is reset and when a backwards seek is performed and the timing >+ // model is reconstructed. >+ kDynamic = 1, In that last sentence there, the "...and...and...and..." is a little confusing. I think it would be clearer with this (correct me if I'm misunderstanding): s/element is reset and when/element is reset, and also when/ >diff --git a/content/smil/nsSMILTimedElement.cpp b/content/smil/nsSMILTimedElement.cpp >+nsSMILTimedElement::ApplyEarlyEnd(const nsSMILTimeValue& aSampleTime) [...] >+ nsRefPtr<nsSMILInstanceTime> newEarlyEnd = >+ new nsSMILInstanceTime(earlyEnd->Time()); >+ if (!newEarlyEnd) { >+ NS_WARNING("Failed to allocate memory for new early end"); There's no need to check for |new| failures anymore -- we've got infallible-new now. There are also some already-existing |new| failure-checks in this file -- would you mind either fixing those in this patch (since you're already refactoring some stuff in the file) or filing a new bug to get rid of them? >+ for (PRInt32 i = count - 2 /*skip prev interval*/; i >= 0; --i) { >+ nsSMILInterval* interval = mOldIntervals[i]; >+ if (i < threshold || !interval->IsDependencyChainLink()) { >+ interval->Unlink(PR_TRUE /*filtered, not deleted*/); >+ mOldIntervals.RemoveElementAt(i); >+ } >+ } Hmm... So this patch adds a number of loops like this, which could all potentially have O(n^2) performance characteristics (where n = arrayLength), since RemoveElementAt involves shifting everything after the removed index. It'd probably be better to do this with O(n) performance -- one way would be to use a new array, with something like the following: nsTArray<FOO> newArray; for (i = highest_idx_that_we_might_remove; i >=0; --i) { if (existingArray[i] should be removed) { existingArray[i]->Unlink(); } else { newArray.AppendElement(existingArray(i)); } } existingArray.Clear(); existingArray.SwapElements(newArray); existingArray.Reverse(); // (since we iterated & appended in reverse order) I think that should work & would have O(n) behavior. (Note that nsTArray::Reverse() doesn't exist, but if we need it, it shouldn't be hard to implement & it would have O(n) behavior too) If the above makes sense, it might even be useful to outsource this pseudocode to a templated helper function, since we use the "loop across nsTArray and remove some of its elements" pattern in many places within this file, all of which could stand some O(n^2)-fixing-sauce, I think. (The helper would take a nsTArray<FOO> & a callback to determine whether a particular element should be removed.)

Comment on attachment 451449 [details] [diff] [review] Part 2: Backwards seeking v1a Not done with part 2 yet, but here's one minor note on a mochitest change: >diff --git a/content/smil/test/test_smilSetCurrentTime.xhtml b/content/smil/test/test_smilSetCurrentTime.xhtml >-const gTimes = [0, 1.5, 0.2, 0.99, -400.5, 10000000, -1]; >+const gTimes = [0, 1.5, 0.2, 0.99, 10000000]; // No negative times as we adopt >+ // the SVGT1.2 behavior of >+ // clamping negative times to 0 Rather than removing these negative times from the test, I'd prefer that we keep them & update the test to know that they're expected to produce a resulting time of 0. That is, after the test has seeked to gTimes[i], can you change it to do something like: if (gTimes[i] >= 0.0) { assert that getCurrentTime() == gTimes[i]; } else { // We adopt the SVG1.2 behavior of clamping negative times to 0 assert that getCurrentTime() == 0.0; } (The first clause there is what the test does right now.)

+ // Additional reference count to determine if this time is currently used as + // a fixed endpoint in any intervals. Such instance times should not be + // filtered. Clarify what 'filtered' refers to here? + void ApplyEarlyEnd(const nsSMILTimeValue& aSampleTime); + void Reset(); + void FilterHistory(); + void FilterIntervals(); + void FilterInstanceTimes(InstanceTimeList& aList); Could use some documentation.

Address review feedback from comment 12 and comment 14. (In reply to comment #12) > There are also some already-existing |new| failure-checks in this file -- would > you mind either fixing those in this patch (since you're already refactoring > some stuff in the file) or filing a new bug to get rid of them? Fixed within nsSMILTimedElement.cpp > Hmm... So this patch adds a number of loops like this, which could all > potentially have O(n^2) performance characteristics (where n = arrayLength), > since RemoveElementAt involves shifting everything after the removed index. Yeah, thanks for that. I keep forgetting RemoveElementAt at is O(n). > If the above makes sense, it might even be useful to outsource this pseudocode > to a templated helper function, since we use the "loop across nsTArray and > remove some of its elements" pattern in many places within this file, all of > which could stand some O(n^2)-fixing-sauce, I think. (The helper would take a > nsTArray<FOO> & a callback to determine whether a particular element should be > removed.) Done.

* Rebase off changes to underlying Part 1 * Address review feedback from comment 13

Comment on attachment 455042 [details] [diff] [review] Part 1: Instance and interval time filtering v1c Looks great!

Pushed: http://hg.mozilla.org/mozilla-central/rev/5dbb57834cac http://hg.mozilla.org/mozilla-central/rev/2e0a75bcb159