Some SMIL reftests take too long to stop animating

RESOLVED FIXED in mozilla17

Status

()

Core
SVG
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mattwoodrow, Unassigned)

Tracking

unspecified
mozilla17
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 650336 [details] [diff] [review]
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.
Attachment #650336 - Flags: review?(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.
Attachment #650336 - Flags: review?(dholbert) → review+
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
(Reporter)

Comment 2

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1302dc0adcd5
OS: All → Mac OS X
Hardware: All → x86
Version: Trunk → unspecified
Just so I understand, the problem here is that the timeline starts before the MozReftestInvalidate event is fired? Why does that matter?
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.
(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.)

Updated

5 years ago
Blocks: 539356
(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

5 years ago
https://hg.mozilla.org/mozilla-central/rev/1302dc0adcd5
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Reporter)

Comment 8

5 years ago
(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.
You need to log in before you can comment on or make changes to this bug.