Closed Bug 579828 Opened 14 years ago Closed 14 years ago

SVG SMIL: Trim, don't prune invalid active intervals

Categories

(Core :: SVG, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: birtles, Assigned: birtles)

Details

Attachments

(2 files, 1 obsolete file)

Currently when the active interval becomes invalid we prune the interval--behaving as if it never existed. This appears to be wrong according to SMIL which says that the active interval is never pruned.

See SMIL 3.0, section 5.4.5, "Evaluation of begin and end lists", 'Principles for building and pruning intervals' (http://www.w3.org/TR/SMIL/smil-timing.html#q89) which gives:

  Do not prune an interval that has already begun.

Yet that is pretty much what we do:

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

Unfortunately the correct behaviour isn't clear. I've posted to www-smil seeking clarification on this:

  http://lists.w3.org/Archives/Public/www-smil/2010JulSep/0003.html

At very least I think we need to:
* Post an appropriate event
* Review what notifications we send to dependent elements -- it probably doesn't make sense to tell begin dependents that the interval has been deleted. But what do we tell end dependents?

For now, I'll wait to see what response we get from www-smil.
Assignee: nobody → birtles
Attached patch Patch v1a (obsolete) — Splinter Review
This patch makes us trim active but invalid intervals so that the end time is coincident with the begin time. Under this arrangement we will also be sure to fire end events for such cases rather than just silently vanishing.
Attachment #482434 - Flags: review?(dholbert)
Comment on attachment 482434 [details] [diff] [review]
Patch v1a

>diff --git a/content/smil/nsSMILTimedElement.cpp b/content/smil/nsSMILTimedElement.cpp
>   } else {
[...]
>+    if (mElementState == STATE_ACTIVE) {
>+      // The interval is active so we can't just delete it, instead trim it so
>+      // that begin==end.
>+      mCurrentInterval->SetEnd(*mCurrentInterval->Begin());

This "else" just before this chunk (surrounding this block of code) is about 35 lines from its "if", and it took me a second to establish the context of what's going on here.

Could you insert a comment saying something like "// GetNextInterval failed" on the line after } else {, just to make that clearer?

>+      // The transition to the postactive state will take place on the next
>+      // sample (along with firing end events, clearing intervals etc.)
>+      RegisterMilestone();

So, what's the implication of this comment for our current sample?  Do we already know at this point that some things will be out of date when we paint this sample?
Attached patch Patch v1bSplinter Review
(In reply to comment #2)
> Could you insert a comment saying something like "// GetNextInterval failed" on
> the line after } else {, just to make that clearer?

Done.

In this updated patch I've also added a check so we don't updated the interval end needlessly and send redundant change notices.

> >+      // The transition to the postactive state will take place on the next
> >+      // sample (along with firing end events, clearing intervals etc.)
> >+      RegisterMilestone();
> 
> So, what's the implication of this comment for our current sample?  Do we
> already know at this point that some things will be out of date when we paint
> this sample?

I think this is safe. Most of the places we update the interval aren't within a sample. The couple of places we do update within a sample it's following a call to Reset. The first instance, within DoSampleAt, is fine since we'll process the state transition immediately. The other instance, withn DoPostSeek, will mean that, yes, potentially we'll go ahead and paint a sample that shows us as active but upon the next sample we'll transition to postactive and clear the result.

That second case is pretty obscure. In fact, I'm not sure it's even possible. You'd have to do a backwards seek that somehow landed you in a state whereupon performing a reset caused the current interval to disappear.

Some alternatives:
a) Force a sample. I think this would be really bad. It's something we try to avoid at all costs. It would be particularly problematic if we end up forcing a sample within a sample and unnecessary in most other cases.
b) Force a sample only in DoPostSeek. This feels hacky so I'd want to have a test case that showed this was actually problematic before doing this.
c) Reproduce much of the active-postactive transition behaviour in DoSampleAt in UpdateCurrentInterval. I don't really like the idea of firing events and the like from UpdateCurrentInterval. This is something which I would prefer was isolated to sampling.

The current behaviour has the side effect of allowing us to potentially redeem a trimmed interval if a sequence of script calls make the interval go from being valid to invalid to valid again inbetween samples.

That said, I want to revisit our reset behaviour some time. I think SMIL's definition can lead to instances where you create an interval only to immediately clear it.
Attachment #482434 - Attachment is obsolete: true
Attachment #483362 - Flags: review?(dholbert)
Attachment #482434 - Flags: review?(dholbert)
(In reply to comment #3)
> The current behaviour has the side effect of allowing us to potentially redeem
> a trimmed interval if a sequence of script calls make the interval go from
> being valid to invalid to valid again inbetween samples.

Ok -- so, this means we've got some indeterminacy in JS+SMIL, stemming from precisely when our samples happen to fire.  For example, if we have two short setTimeout()s with the first changing an interval to "invalid" and the second changing it back to "valid", then we'll potentially have a different outcome based on whether a SMIL sample fires between those callbacks.

I guess that's not too bad, though...  That does seem like a pretty obscure situation.
Attachment #483362 - Flags: review?(dholbert) → review+
(In reply to comment #4)
> (In reply to comment #3)
> > The current behaviour has the side effect of allowing us to potentially redeem
> > a trimmed interval if a sequence of script calls make the interval go from
> > being valid to invalid to valid again inbetween samples.
> 
> Ok -- so, this means we've got some indeterminacy in JS+SMIL, stemming from
> precisely when our samples happen to fire.  For example, if we have two short
> setTimeout()s with the first changing an interval to "invalid" and the second
> changing it back to "valid", then we'll potentially have a different outcome
> based on whether a SMIL sample fires between those callbacks.

Right. I think it generally will. At any rate, I think this is in keeping with the spirit of SVG's error handling:

  Because of situations where a block of scripting changes might cause a given SVG document fragment to go into and out of error, error processing shall occur only at times when document presentation (e.g., rendering to the display device) is updated.[1]

[1] http://www.w3.org/TR/SVG/implnote.html#ErrorProcessing

Then again, it's not clear if this is really an error situation.
Status: NEW → ASSIGNED
Comment on attachment 483362 [details] [diff] [review]
Patch v1b

Requesting approval to land. This fixes currently broken behaviour. Includes tests.
(Sorry for bug spam, still haven't worked out how to add someone to CC list and nominate for landing approval at the same time :)
Attachment #483362 - Flags: approval2.0?
Pushed: http://hg.mozilla.org/mozilla-central/rev/8c07f7bd6d3a
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
This will hopefully fix the test that seemed to fail on Linux opt on mc. Currently trying to repro the problem on Try and then I'll see if this fixes it.
Looks good on Try. Running it through once more to be sure and if it's ok I'll push to m-c later when I have a chance to watch it.
Pushed test case fix: http://hg.mozilla.org/mozilla-central/rev/e79b950e6ea5
Waiting to see how it goes this time before closing this.
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: