537852 - SVG SMIL: Add tests for syncbase timing

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

I'm filing this bug to cover just the tests for SMIL syncbase timing.

I've got some minor patches to improve a few things in birtles' final tests-patch from bug 474743.  I don't want to clutter up that bug with more attachments & more discussion, so I'm separating out the tests into this bug.

(The tests patch(es) from this bug will still need to land at roughly the same time as bug 474743, though.)

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: main tests patch (birtles' latest from bug 474743 - "Patch E" v4)

This patch is identical to attachment 419657 [details] [diff] [review] on bug 474743.
It's the latest "Test cases" patch posted on that bug.  I'm obsoleting it there and reposting here.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: Followup A: Take out unneeded 'fill="remove"'

This followup patch removes all the 'fill="remove"' attributes from animations in the reftests.  These attribute settings aren't necessary, since 'remove' is the default value of fill.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: Followup B: Pause before seeking

A lot of the reftests had onload handlers of the form:
> <svg [...] onload="
>       document.documentElement.setCurrentTime(0);
>       // Do something that only works if we're still close to the time we just seeked to.
>      setTimeAndSnapshot(2, true)">

In low-resource conditions, though, we've got no guarantee on the amount of document-time that passes between the setCurrentTime() call and the line of script that follows it.

So, we need to pause animations before seeking, in order to be sure that the document time won't have changed between the seek & the next line.  This followup adds a pause in all the tests that seek in their onload handler.  (There might be one or two that don't actulaly need this fix, but I think they mostly all do.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Followup C: Add whitespace to improve readibility in sandwich-priority-*

This patch just adds whitespace to improve readibility in the sandwich-priority-* tests, e.g.
-    <set attributeName="fill" to="yellow" begin="1s" id="b"/>
-    <set attributeName="fill" to="green" begin="b.begin" id="c"/>
-    <set attributeName="fill" to="red" begin="1s" id="a"/>
+    <set attributeName="fill" to="yellow" begin="1s"      id="b"/>
+    <set attributeName="fill" to="green"  begin="b.begin" id="c"/>
+    <set attributeName="fill" to="red"    begin="1s"      id="a"/>

It also moves the 'id' attribute for <set> to the end of the line in a few cases, to make attributes line up between nodes with & without 'id' attrs. (and to match the convention used in most of the other syncbase reftests)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Followup D: misc fixes

While the previous followups all were "mass patches" of a certain type, this one's just a conglomeration of miscellaneous fixes.  Most of the changes are comment-tweaks, with these exceptions:

> test_smilFillMode.xhtml
- Shift two lines into the utility "createAnim()" function, to reduce duplicated code.

> test_smilUpdatedInterval.xhtml
- Clarify the warning message in the exception-thrown case -- old message said 'Simple duration...was updated' even though the simple duration almost certainly wouldn't have changed.

> changed-interval-resolved-1.svg
> changed-interval-resolved-2.svg
- remove unneeded 'fill=freeze' on syncbase

> cycle-recursion-1.svg
- remove unneeded 'additive="replace"' (default value of that attr)

> new-interval-early-end-3.svg
> new-interval-freeze-begin-1.svg
- These animations had "orange; green; [somethingelse]", while their sibling testcases all had "orange; green; orange".  Changed these to "orange; green; orange" to match.

Requesting r=birtles -- Brian, can you let me know if any of these changes (in this patch or any of the other followup patches) are undesirable for any reason?

Comment 6

[Daniel Holbert [:dholbert]]

(I'm not bothering to request review on "Followup C", since it's basically just whitespace readability changes.)

Brian, I've got two questions on the tests patch that aren't covered by the above followups patches:

>diff --git a/layout/reftests/svg/smil/syncbase/new-interval-early-end-4.svg b/layout/reftests/svg/smil/syncbase/new-interval-early-end-4.svg
>+  <!--
>+    Test an early end where two times could be chosen for the following
>+    interval.
>+    (As before but with a negative offset and restart="always". The earlier time
>+    should be rejected because it overlaps with the previous interval as in
>+    new-interval-end-negative-1,2.svg.)
>+   -->
>+   -->
>+  <rect width="100" height="100" fill="red">
>+    <animate attributeName="y" attributeType="XML" from="0" to="0" id="a"
>+      begin="0s; 1.5s" dur="1s"/>
>+    <animate attributeName="fill" attributeType="CSS"
>+      values="orange; green; orange" fill="remove"
>+      begin="a.begin-0.25s; a.begin" dur="1s"/>
>+  </rect>
>+</svg>

I don't understand the comment for this test.  Which time should be rejected, and which interval does it overlap with?  The only overlap I see is a trivial one between |a.begin-0.25| and |a.begin|, and my understanding is that the latter of those will just trigger a restart.  What am I missing?

>+++ b/layout/reftests/svg/smil/syncbase/new-interval-end-negative-1.svg
>+    REALLY IMPORTANT: If an interval ends and a new instance time is generated
>+    before the end of the last interval it is IGNORED. However, if the instance
>+    time is generated BEFORE the interval ends, and restart="always", a new
>+    interval will be generated and the animation will restart.

I'm unclear on the two sentences above.  Here are simplified versions of them:
> If an interval ends and a new instance time is generated... before the end...it is IGNORED.
> However, if the instance time is generated BEFORE the interval ends...a new interval will be generated
That "However" condition there doesn't seem to be any different from the sentence before it, which makes this confusing... Can you clarify what you meant there?

(I'll tweak both comments to make them clearer in the final version of "Followup D" -- I just need to understand them first. :) )

Comment 7

[Daniel Holbert [:dholbert]]

Taking this bug, so that it's got an assignee. (Though when this bug's stack-o'-patches lands, the first one -- "main tests patch" -- should be landed with birtles as the patch-writer).

Comment 8

[Brian Birtles (:birtles)]

Comment on attachment 420012 [details] [diff] [review]
Followup A: Take out unneeded 'fill="remove"'

> This followup patch removes all the 'fill="remove"' attributes from animations
> in the reftests.  These attribute settings aren't necessary, since 'remove' is
> the default value of fill.

Thanks for going through these and tidying the tests up. That's really good.

This paricular patch is fine but I had thought it would be good just to make the fill mode explicit for those who aren't so familiar with SMIL (i.e. can't remember the defaults for each attribute) and who are trying to understand the expected output (i.e. why the rectangle is coloured ret). But I think what you've done is good anyway since probably most people assume an animation stops taking effect after it's finished unless stated otherwise. Thanks Daniel!

Comment 9

[Brian Birtles (:birtles)]

Comment on attachment 420013 [details] [diff] [review]
Followup B: Pause before seeking

> So, we need to pause animations before seeking, in order to be sure that the
> document time won't have changed between the seek & the next line.  This
> followup adds a pause in all the tests that seek in their onload handler. 
> (There might be one or two that don't actulaly need this fix, but I think they
> mostly all do.)

That's great Daniel--thank you! I had meant to do this actually--it's definitely needed.

Comment 10

[Brian Birtles (:birtles)]

Comment on attachment 420015 [details] [diff] [review]
Followup D: misc fixes

(In reply to comment #5)
> > cycle-recursion-1.svg
> - remove unneeded 'additive="replace"' (default value of that attr)
Once again, this is just making explicit what is expected here. Without is fine as well.

> > new-interval-early-end-3.svg
> > new-interval-freeze-begin-1.svg
> - These animations had "orange; green; [somethingelse]", while their sibling
> testcases all had "orange; green; orange".  Changed these to "orange; green;
> orange" to match.
I think I prefer the "orange; green; [somethingelse]" arrangement. I started with "orange; green; orange" but later changed to using "orange; green; purple" and the like instead because then when it fails you can more easily tell in which direction it failed (too early or too late).

> Requesting r=birtles -- Brian, can you let me know if any of these changes (in
> this patch or any of the other followup patches) are undesirable for any
> reason?
Looks great. Especially all the updates to the comments. Thanks for that.

Comment 11

[Brian Birtles (:birtles)]

(In reply to comment #6)
> Brian, I've got two questions on the tests patch that aren't covered by the
> above followups patches:
> 
> >diff --git a/layout/reftests/svg/smil/syncbase/new-interval-early-end-4.svg b/layout/reftests/svg/smil/syncbase/new-interval-early-end-4.svg
> >+  <!--
> >+    Test an early end where two times could be chosen for the following
> >+    interval.
> >+    (As before but with a negative offset and restart="always". The earlier time
> >+    should be rejected because it overlaps with the previous interval as in
> >+    new-interval-end-negative-1,2.svg.)
> >+   -->
> >+   -->
> >+  <rect width="100" height="100" fill="red">
> >+    <animate attributeName="y" attributeType="XML" from="0" to="0" id="a"
> >+      begin="0s; 1.5s" dur="1s"/>
> >+    <animate attributeName="fill" attributeType="CSS"
> >+      values="orange; green; orange" fill="remove"
> >+      begin="a.begin-0.25s; a.begin" dur="1s"/>
> >+  </rect>
> >+</svg>
> 
> I don't understand the comment for this test.  Which time should be rejected,
> and which interval does it overlap with?  The only overlap I see is a trivial
> one between |a.begin-0.25| and |a.begin|, and my understanding is that the
> latter of those will just trigger a restart.  What am I missing?

Nothing, it's wrong. There's no overlap. I think what might be a useful test is something like this:

<rect width="100" height="100" fill="green">
  <animate attributeName="y" attributeType="XML" from="0" to="0" id="a"
    begin="0s; 1.75s" dur="4s"/>
  <animate attributeName="fill" attributeType="CSS"
    values="red"
    begin="a.begin+0.4s; a.begin-0.4s" dur="1s" restart="whenNotActive"/>
</rect>

You might want to check my working here but I think this should give us the following:

Initially we have
a: 0s -> 4s

Leading to b having begin instance times -0.4s and 0.4s giving us the following:
-0.4s -> 0.6s

t=0.4s: early end for second element
=> 0.4s -> 1.4s

t=1.4s: second element goes to postactive state, no more begin instances

t=1.75s: early end for 'a' => new interval 1.75s -> 5.75s
=> leads to second element getting instance times 1.35s, 2.15s
but since 1.35s overlaps with previous interval (0.4s -> 1.4s) it should be ignored, therefore the new interval will be 2.15s -> 3.15s and at t=2s the animation should not be active (I've switched the colours in the test case so that we'll get green when it's inactive).

Does that sound right? (Sorry I'm not near the work laptop at the moment so I can't check.)

> >+++ b/layout/reftests/svg/smil/syncbase/new-interval-end-negative-1.svg
> >+    REALLY IMPORTANT: If an interval ends and a new instance time is generated
> >+    before the end of the last interval it is IGNORED. However, if the instance
> >+    time is generated BEFORE the interval ends, and restart="always", a new
> >+    interval will be generated and the animation will restart.
> 
> I'm unclear on the two sentences above.  Here are simplified versions of them:
> > If an interval ends and a new instance time is generated... before the end...it is IGNORED.
> > However, if the instance time is generated BEFORE the interval ends...a new interval will be generated
> That "However" condition there doesn't seem to be any different from the
> sentence before it, which makes this confusing... Can you clarify what you
> meant there?

The confusion has to do with the fact that we are thinking in terms of two timelines: document time, i.e. the actual values assigned to intervals, and "real" time, i.e. when the times are generated.

So we have two situations:

Case A: "If an interval ends and a new instance time is generated before the end of the last interval it is IGNORED."

   +-----+
      ^       *
 New instance  \
 time goes here \
            But we're already up
            to here

Result: instance time is ignored (as per SMIL3 section 5.4.5, 'Evaluation of begin and end time lists': "Nevertheless, once a time has happened, it is fixed." That is, we can no longer early-end the interval since it has finished. Furthermore, the pseudocode for getNextInterval shows us getting the first begin instance >= the end of the last interval). So the result is simply:

   +-----+


Case B: "However, if the instance time is generated BEFORE the interval ends, and restart="always", a new interval will be generated and the animation will restart."

   +-----+
      ^ *
     /   \
 New      \
 instance  \ We're still up to here, i.e. the interval hasn't ended
 time
 goes
 here

Result: since, restart="always", we get an early end, i.e.

   +--|
      +-----+


So the difference between the two is when the the instance time is added. In the first case it is added after the interval has ended ("If an interval ends...") whilst in the second case it is added before the interval has ended ("if the instance time is generated BEFORE the interval ends").

Feel free to adjust it as you see fit to make it more understandable.

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: Followup D v2: misc fixes

(In reply to comment #10)
> I think I prefer the "orange; green; [somethingelse]" arrangement. I started
> with "orange; green; orange" but later changed to using "orange; green; purple"
> and the like instead because then when it fails you can more easily tell in
> which direction it failed (too early or too late).

Yeah, I agree.  I'll attach a followup to standardize on the "orange; green; [somethingelse]" arrangement.  For now, I've removed that chunk from Patch D. (the one that changed a few tests to "orange; green; orange")

(In reply to comment #11)
Thanks for the clear explanations of those! That makes sense to me now.  I've edited new-interval-early-end-4.svg as you suggested, except without the  |restart="whenNotActive"| chunk, because I think that part would prevent the "early end for second element" that you've got marked in your timeline above.  I also clarified the "REALLY IMPORTANT" comment in the other test a bit, I think.

Carrying forward r+ (but do let me know if anything about those changes looks off).

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: Followup E: s/orange;green;orange/orange;green;purple/

This patch changes all syncbase tests with "orange; green; orange" (most of them) to instead use "orange; green; purple".  As Brian pointed out in comment 10, this will help us diagnose what's going wrong, in the (unexpected) event that one of these tests starts failing.

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 420009 [details] [diff] [review]
main tests patch (birtles' latest from bug 474743 - "Patch E" v4)

Marking the base "main tests patch" r=dholbert, since all of my concerns have been addressed in followup patches posted here.

Comment 15

[Daniel Holbert [:dholbert]]

I think this all is ready to land, once bug 474743 is ready.

(Brian: I'd be happy to address bsmedberg's /**/-vs-// comment on your behalf and land that bug's patches for you, if you like.  From your last comment there, though, it sounds like maybe you've got a few other things you want to look into, though, so I'll hold off unless I hear otherwise from you.)

Comment 16

[Brian Birtles (:birtles)]

(In reply to comment #15)
> (Brian: I'd be happy to address bsmedberg's /**/-vs-// comment on your behalf
> and land that bug's patches for you, if you like.  From your last comment
> there, though, it sounds like maybe you've got a few other things you want to
> look into, though, so I'll hold off unless I hear otherwise from you.)

That'd be great, thanks Daniel. Sorry, I don't even have IRC access at the moment. The only outstanding work was the /**/ issue you mentioned. As for landing I was planning on perhaps landing patch B (nsTPriorityQueue) first. It should be atomic. Patches C & D are NOT atomic--i.e. shouldn't be landed separately. Patch A should be safe to land on its own but I was just thinking of landing A, C and D together. Also, Patch B has a different sr so it might make more sense to land it separately.

I think they need another go on TryServer too.

Thanks Daniel!

Comment 17

[Daniel Holbert [:dholbert]]

Addressed bsmedberg's suggestion & passed this through tryserver last night. So, landed!  (all together in one push, so as not to trigger extra tinderbox cycles on an already-somewhat-overloaded tree)

base patch: http://hg.mozilla.org/mozilla-central/rev/c7caff27720c
followup A: http://hg.mozilla.org/mozilla-central/rev/32f00b9392d3
followup B: http://hg.mozilla.org/mozilla-central/rev/ed6cdd68e924
followup C: http://hg.mozilla.org/mozilla-central/rev/27c396a5ddda
followup D: http://hg.mozilla.org/mozilla-central/rev/19807a1e5012
followup E: http://hg.mozilla.org/mozilla-central/rev/c519fb35ea64