781362 - Some SMIL reftests take too long to stop animating

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Matt Woodrow (:mattwoodrow)]

Attachment: Delay the animation start time

We should delay the start time of the animations so we don't have to wait for them.

I *think* this patch also fixes bug 547801.

Comment 1

[Daniel Holbert [:dholbert]]

Comment on attachment 650336 [details] [diff] [review]
Delay the animation start time

Yup -- in particular, we don't want there to be any immediately-animating stuff, because any initial animations will delay the firing of MozReftestInvalidate.

Comment 2

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1302dc0adcd5

Comment 3

[Jonathan Watt [:jwatt]]

Just so I understand, the problem here is that the timeline starts before the MozReftestInvalidate event is fired? Why does that matter?

Comment 4

[Daniel Holbert [:dholbert]]

The problem arises from these facts:
 * These tests have animations that start immediately, at time=0

 * The immediate animations trigger continuous paints which (with DLBI) delay the firing of MozReftestInvalidate.  This is because DLBI tickles something about the way we determine "all painting has stopped" for firing that event.

 * This means the MozReftestInvalidate-triggered scripting (including the "reftest-wait" removal) doesn't run until the animations have completely played through, which takes on the order of seconds.

So: at a minimum, this means the reftests take longer than we'd like to run.  In some cases it can makes them time out before they fire MozReftestInvalidate, if we continue invalidating forever due to e.g. some fill="freeze" by-animations that we don't trust to be the same from sample to sample since the underlying value might change.

Comment 5

[Jonathan Watt [:jwatt]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
>  * The immediate animations trigger continuous paints which (with DLBI)
> delay the firing of MozReftestInvalidate.  This is because DLBI tickles
> something about the way we determine "all painting has stopped" for firing
> that event.

If anyone (Matt?) knows more about this part, I'd be interested to hear the details.

>  * This means the MozReftestInvalidate-triggered scripting (including the
> "reftest-wait" removal) doesn't run until the animations have completely
> played through

Wow, okay. Good to know. CC'ing some other people who should be aware of this when writing future SMIL animation tests.

FWIW, to me it seems like it would be a good idea to be calling pauseAnimations() before the load events fire to make tests a bit cleaner. (Doing the pause under setTimeAndSnapshot() in onload seems less great in the face of this bug.)

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Jonathan Watt [:jwatt] from comment #5)
> (In reply to Daniel Holbert [:dholbert] from comment #4)
> >  * The immediate animations trigger continuous paints which (with DLBI)
> > delay the firing of MozReftestInvalidate.
[...]
> If anyone (Matt?) knows more about this part, I'd be interested to hear the
> details.

(For a bit more discussion on this, see bug 758505 comment 7 and 10-12.)

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1302dc0adcd5

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Jonathan Watt [:jwatt] from comment #5)
> (In reply to Daniel Holbert [:dholbert] from comment #4)
> >  * The immediate animations trigger continuous paints which (with DLBI)
> > delay the firing of MozReftestInvalidate.  This is because DLBI tickles
> > something about the way we determine "all painting has stopped" for firing
> > that event.
> 
> If anyone (Matt?) knows more about this part, I'd be interested to hear the
> details.

Both with and without DLBI, we always wait until there are no pending paints to be processed before we fire the MozReftestInvalidate event and finish the test.

It appeared with the existing code that we'd often manage to paint the last set of changes and then finish the test before the SMIL animation code scheduled another paint.

All my patches do is change some of the paint timings, which make this no longer true and we have to wait out the entire animation.