Closed Bug 615872 Opened 9 years ago Closed 9 years ago

"ASSERTION: Sample time should not precede current interval" with removed documentElement

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b9

People

(Reporter: jruderman, Assigned: birtles)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(5 files, 1 obsolete file)

Attached image testcase
###!!! ASSERTION: Sample time should not precede current interval: 'aContainerTime >= beginTime', file content/smil/nsSMILTimedElement.cpp, line 609

###!!! ASSERTION: Expecting non-negative active time: 'aActiveTime >= 0', file content/smil/nsSMILTimedElement.cpp, line 1763
Attached file stack traces
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Just a little refactoring to remove unneeded mTimedDocumentRoot that was confusing this part of the code.
Attachment #496055 - Flags: review?(dholbert)
Main patch. This requires some explanation.

Firstly, we were previously doing these local samples inside nsSMILTimedElement to try and ensure we kept up-to-date. This is fine most of the time, but when a backwards seek is required we expect to get told to do a Rewind first and in this case we weren't since we were bypassing the time container which normally detects this sort of thing.

Now it's possible to detect that case in the timed element and handle it there but basically we'd end up duplicating the functionality of the time container. Better still is just to remove this kind of logic from the timed element altogether.

That's what this patch does. A note on how it works: Generally whenever we change via the DOM we don't resample immediately. Instead we just set the resample needed flag in the time container. When we go to do anything that requires our model to be up-to-date (such as fetching an animated value) we flush the resample requests at that time.

For Begin/EndElement we can do likewise PROVIDED we take care to flush resample requests before we perform subsequent Begin/EndElement requests (or calls to SetCurrentTime etc.)

For SetCurrentTime we need to force the resample there and then in order to mark off the end of the seek interval correctly.

Two changes in the patch that might raise eyebrows:

a) changing the number of expected assertions for anim-use-href-01.svg

This is because now we're doing an extra FlushAnimations() at the start of SetCurrentTime().

If we really want the old behaviour then we could remove this extra FlushAnimations() and make Begin/EndElement force a resample (by adding a FlushAnimations() at the end of each of those methods) but I think it makes more sense to have SetCurrentTime and the like be responsible for ensuring the model is up to date before they do anything.

Note that having Begin/EndElement NOT force a resample shouldn't cause problems for TimeEvents since these events are dispatched asynchronously anyway and will simply be dispatched on the next sample (or next FlushAnimations).

I suppose one noticeable difference would be if we had:

 BeginElement();
 SomeotherDOMCallThatPutsAnEventOnTheQueue();
 var x = circle.cx.animVal.value;

In this case we won't dispatch the event until the call to get the animated value (when we'll flush resample requests) meaning it might appear to be out of order. Is that ok?

b) changes to the test_smilKeySplines.xhtml test. Just fixing the test case. The previous test case was wrong. I'm not exactly sure why it worked before, it probably shouldn't have. I can only guess that now we're actually updating correctly.
Attachment #496056 - Flags: review?(dholbert)
Whiteboard: [awaiting review]
Attachment #496055 - Flags: review?(dholbert) → review+
Comment on attachment 496056 [details] [diff] [review]
Part 2: Remove local samples from nsSMILTimedElement::Begin/EndElementAt

>diff --git a/content/smil/nsSMILTimedElement.cpp b/content/smil/nsSMILTimedElement.cpp
>--- a/content/smil/nsSMILTimedElement.cpp
>+++ b/content/smil/nsSMILTimedElement.cpp
>@@ -259,64 +259,28 @@ nsSMILTimedElement::BeginElementAt(doubl
> {
>-  if (mElementState != STATE_STARTUP) {
>-    DoSampleAt(currentTime, PR_FALSE); // Regular sample, not end sample
>-  }

Per irc discussion, we should flush animations here & in EndElementAt so that we get an up-to-date repaint once this completes.

>+      AnimationNeedsResample();
>+      // While seeking we don't fire events, but as soon as seeking finishes we
>+      // should start firing events. However, the seek state is reset within
>+      // a sample so we want to force a sample now to ensure that from here
>+      // onwards events will be fired.
>+      FlushAnimations();

This comment confused me a little bit -- maybe simplify (and expand the scope of why we need the synchronous sample) to say something like:
   // Trigger synchronous sample now, to:
   //  - Make sure we get an up-to-date paint after this method
   //  - re-enable event firing (it got disabled during seeking, and it doesn't get
   //    re-enabled until the first sample after the seek -- so let's make that happen now.)

r=dholbert with that. :)  Sorry for the wait on this one.
Attachment #496056 - Flags: review?(dholbert) → review+
Whiteboard: [awaiting review]
(In reply to comment #4)
> Per irc discussion, we should flush animations here & in EndElementAt so that
> we get an up-to-date repaint once this completes.
Fixed.

> >+      AnimationNeedsResample();
> >+      // While seeking we don't fire events, but as soon as seeking finishes we
...
> This comment confused me a little bit -- maybe simplify (and expand the scope
> of why we need the synchronous sample) to say something like:
>    // Trigger synchronous sample now, to:
>    //  - Make sure we get an up-to-date paint after this method
>    //  - re-enable event firing (it got disabled during seeking, and it doesn't
> get
>    //    re-enabled until the first sample after the seek -- so let's make that
> happen now.)
Fixed.

> r=dholbert with that. :)  Sorry for the wait on this one.
(Not at all, the [awaiting review] thing is just so I can remember where I'm up to--I wasn't intending to hurry you along :)
Attachment #496056 - Attachment is obsolete: true
Attachment #497719 - Flags: review+
Requesting approval to land. Fixes assertion. Removes and simplifies code. Includes test.
Attachment #497721 - Flags: approval2.0?
Comment on attachment 497721 [details] [diff] [review]
Roll-up patch for landing approval

Looks like you forgot to include the test in the patch?
Attachment #497721 - Flags: approval2.0? → approval2.0+
(In reply to comment #7)
> Looks like you forgot to include the test in the patch?

Oh, cheers, I didn't notice that. hg had a fit when I tried to qfold a bitrotted patch and left the repo in a mess. I'll be sure to push attachment 497719 [details] [diff] [review] which includes the test.
Pushed:
http://hg.mozilla.org/mozilla-central/rev/819a2310b235
http://hg.mozilla.org/mozilla-central/rev/7f2832603edb
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Flags: in-testsuite+
OS: Linux → All
Hardware: x86_64 → All
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b9
You need to log in before you can comment on or make changes to this bug.