541588 - Hook up nsSMILAnimationController to "nsRefreshDriver" class for scheduling samples

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

As dbaron suggested in bug 537139 comment 0, we should probably be using the generalized animation sample-timer-callback class "nsRefreshDriver" for timing of SMIL animation samples.

See nsSMILAnimationController::StartTimer(), as well as:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.h
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsRefreshDriver.cpp

Comment 1

[Daniel Holbert [:dholbert]]

Taking; hope to work on this in next day or two.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: patch A v1: just use nsRefreshDriver for scheduling

This patch has us schedule our samples using the refresh driver, instead of using a fixed-interval nsITimer.

This patch ignores the timestamp passed in by the refresh driver -- we still calculate the current time on our own, when necessary.  Ultimately, we'll probably want to use the passed-in time, but I don't think that's particularly important (and we probably need to switch our SMIL code over to using mozilla:TimeStamp and mozilla:TimeDuration before doing that).

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: patch A v2: just use nsRefreshDriver for scheduling

Updated version of patch A -- removes some duplicated code & cleans up some XXX comments.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to comment #2)
> (and we probably need to switch our SMIL code over to using
> mozilla:TimeStamp and mozilla:TimeDuration before doing that).

I filed Bug 550071 on using TimeStamp/TimeDuration.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 430159 [details] [diff] [review]
patch A v2: just use nsRefreshDriver for scheduling

Looks good, but use NS_IMPL_ADDREF/RELEASE instead of your own stuff?
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#310

Comment 6

[Daniel Holbert [:dholbert]]

Ok, thanks!

(I hadn't used those macros because I'd had the impression that they only worked for nsISupports subclasses, from back in bug 216462 comment 78.  Looking back there, it was just that we lacked an owningThread member variable, and I've got one of those here, so the macros should be fine in this case.)

(FWIW, the AddRef impl I'd used was taken from nsSMILInstanceTime.h, which in turn was based on implementations in both imgLoader.h and nsStyleStruct.h.  We should probably change all of those to use the NS_IMPL_ macros too, except for nsStyleStruct.h since it lacks an owningThread. That one (and the rest of these too?) should probably use the owningThread-less Thebes version mentioned in bug 551298).

Comment 7

[Daniel Holbert [:dholbert]]

pushed (using NS_IF_ADDREF/RELEASE): http://hg.mozilla.org/mozilla-central/rev/76abe26bf57c

Comment 8

[Daniel Holbert [:dholbert]]

Backed out due to tinderbox orange, w/ this assertion:
###!!! ABORT: observers should have unregistered: 'ObserverCount() == 0', file e:/builds/moz2_slave/mozilla-central-win32-debug/build/layout/base/nsRefreshDriver.cpp, line 70
http://hg.mozilla.org/mozilla-central/rev/e92899b72067

We are unregistering ourselves inside nsSMILAnimationController::StopTimer, which gets called by the nsSMILAnimationController destructor, but I guess the RefreshDriver is getting destroyed before that (and before we make any other calls to StopTimer).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  The refresh driver lifetime is that of the presentation.  What's the lifetime of the animation controller?  Who's expected to call StopTimer?  Should presshell teardown just do it?

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to comment #9)
> What's the
> lifetime of the animation controller?

It's stored in a nsAutoPtr owned by nsIDocument, so it dies when the document dies.

> Who's expected to call StopTimer?

I think it normally happens via nsDocument::OnPageHide, which calls nsSMILAnimationController::OnPageHide, which calls Pause, which calls StopTimer.

> Should presshell teardown just do it?

Maybe... Looking into it.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I think it normally happens via nsDocument::OnPageHide

Ah, yes.  That can happen much much later than presentation teardown.  Do we need to run SMIL animations in subframes that have display:none?

Comment 12

[Daniel Holbert [:dholbert]]

> Do we
> need to run SMIL animations in subframes that have display:none?

Well, we have been so far, and we have tests that assert that we do.

Having said that, maybe we could get away with not scheduling any samples there, and only sampling when we need to (i.e. from FlushAnimations() calls in DOM accessors).  SMIL doesn't provide any guarantees of frame rate, so it's perfectly acceptable to take samples as infrequently as we want in display:none content.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Well, we have been so far, and we have tests that assert that we do.

How do they assert that?

> Having said that, maybe we could get away with not scheduling any samples
> there, and only sampling when we need to (i.e. from FlushAnimations() calls in
> DOM accessors)

Right.  Is there any other way for anyone to tell that we're not animating, or is this completely a SMIL falling in a forest situation?

Comment 14

[Daniel Holbert [:dholbert]]

> How do they assert that?

Create a SMIL animation, use setCurrentTime to seek to its beginning/middle/end and check presentation value. See e.g. test_smilCSSFromTo.xhtml [1] which uses a helper-method "hideSubtree" [2] to recursively set display:none.

[1] http://mxr.mozilla.org/mozilla-central/source/content/smil/test/test_smilCSSFromTo.xhtml#56
[2] http://mxr.mozilla.org/mozilla-central/source/content/smil/test/smilTestUtils.js#137

Actually though, those aren't cases with a _subframe_ that's display:none -- just a subset of our SVG content.

> Right.  Is there any other way for anyone to tell that we're not animating

Yes, actually... An author can check the animated value via getComputedStyle & e.g. foo.x.animVal.value.  **If** we've explicitly requested a resample, that will use FlushAnimations() to bring us up-to-date -- but otherwise, FlushAnimations is a no-op.  And we'll only explicitly request a resample when the animation parameters or base values are changed.  So: if nothing changes (aside from the passage of time), we won't update animated values, and that's visible to the page author.

Note: if the author uses "SetCurrentTime", though, that forces an immediate resample [3] which brings them up-to-date.

[3] http://mxr.mozilla.org/mozilla-central/source/content/svg/content/src/nsSVGSVGElement.cpp#545

So, anyway: Yes, an author could easily tell that we're not animating -- they'd just need to periodically check the animated values and notice that they're not changing.  However, I think that behavior is reasonable, since we (& SMIL) don't guarantee any particular sampling frequency.  If it ends up being an issue, we can use some alternate method to schedule infrequent automatic samples in display:none subframes. (But I think it'd be premature to make a special exception for that now.)

Comment 15

[Daniel Holbert [:dholbert]]

Note to self: I can reproduce the NS_ABORT_IF_FALSE failure by loading the file
   content/test/reftest/xml-stylesheet/embedded_dtd_id.svg
in a debug build (with this bug's patch applied) and then quitting.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Actually though, those aren't cases with a _subframe_ that's display:none --
> just a subset of our SVG content.

Right; that's a totally different situation.

> However, I think that behavior is reasonable, since we (& SMIL)
> don't guarantee any particular sampling frequency.

So in particular SMIL doesn't guarantee that if you examine the state at time T then the styles will match the state at time T?  That's kinda odd, but ok.  I guess it prevents animations from jumping when people inspect the DOM....

Sounds like we should also remove the animation controller from the refresh driver at presshell teardown, and readd it at presshell setup if we've already seen the pageshow, right?

Comment 17

[Daniel Holbert [:dholbert]]

Attachment: patch A v3

This patch version adds these changes:
 - Renames "StartTimer" / "StopTimer" to "StartSampling" / "StopSampling"...
 - ...makes them public methods...
 - ...makes them take a nsRefreshDriver* as an argument (because at PresShell teardown, we can't look up the refresh driver from the animationController, since the document's presshell link has been cleared)
 - ...makes calls to them from PresShell Init/Destroy methods.

Note that Init only calls StartSampling if the document is unpaused (which includes situations where we haven't called OnPageShow yet).  If it's paused, we know we'll get a StartSampling call if/when it becomes unpaused.

Changes to patch since previously-posted version are:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/7bff09fe7fde
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/adf2223bc35d

(plus the NS_IMPL_ADDREF change that I made before first landing-attempt:
http://hg.mozilla.org/users/dholbert_mozilla.com/smil-patches/rev/5bf991a153be
)

Comment 18

[Daniel Holbert [:dholbert]]

I also moved the manual first sample out of StartTimer (now "StartSampling") up to its two pre-existing callers, since I'm not sure whether it's safe to do a synchronous sample (and e.g. trigger frame construction) from within PresShell::Init.

Comment 19

[Daniel Holbert [:dholbert]]

Landed: http://hg.mozilla.org/mozilla-central/rev/68363c7a7f06

Comment 20

[Daniel Holbert [:dholbert]]

D'oh... This patch was fine on tryserver (which is opt-only) and was fine on mochitest-chrome with my local debug build, but it still has issues on some subset of debug mochitest-plain set #1 (at least):

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268376723.1268377350.22338.gz
OS X 10.5.2 mozilla-central debug test mochitests-1/5 on 2010/03/11 22:52:03
{
15231 INFO TEST-PASS | /tests/content/base/test/test_bug416317-2.html | Element.querySelector: .plus .t1:hover + .unitTest + .unitTest
###!!! ABORT: Starting sampling with wrong refresh driver: 'aRefreshDriver == GetRefreshDriverForDoc(mDocument)', file /builds/moz2_slave/mozilla-central-macosx-debug/build/content/smil/nsSMILAnimationController.cpp, line 234
}

Backed out: http://hg.mozilla.org/mozilla-central/rev/2656ae66fcda

Comment 21

[Daniel Holbert [:dholbert]]

Attachment: patch A v4

The abort in comment 20 is from us tripping over the following situation:
 - We have SMIL animation in a "display:none" iframe
 - We make that iframe visible by toggling its "display" property
 - This gives us this call stack:
     nsDocument::doCreateShell
      --> PresShell::Init
       --> nsSMILAnimationController::StartSampling
 - doCreateShell hasn't set the document's PresShell pointer yet, so the document still has a null PresShell (until just after Init completes).  So when Init calls StartSampling, we get confused because our document still lacks a PresShell (and hence a PresContext and a RefreshDriver).

We just need to extend this failing nsRefreshDriver-mismatch ABORT_IF_FALSE to allow for our document to lack a refresh-driver. (fixed in attached patch)  Note that this matches the ABORT_IF_FALSE that we're already using in StopSampling.

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So in that situation, how does sampling eventually start?

Comment 23

[Daniel Holbert [:dholbert]]

The "StartSampling" call that made us abort in comment 20 will no longer abort, and instead will proceed in hooking us up to the passed-in refresh driver (given to us by the caller, PresShell::Init).  We'll get a callback from that refresh driver after a period of time, and that will be the first sample.

(Note that SVGLoad has already fired, so our timeline is already running -- we're not 'paused' in any sense.  We just haven't been sampling.)

Comment 24

[Daniel Holbert [:dholbert]]

Pushed: http://hg.mozilla.org/mozilla-central/rev/59f507847beb

Comment 25

[Jeff Muizelaar [:jrmuizel]]

It looks like this caused tSVG regressions on multiple operating systems.

Comment 26

[Daniel Holbert [:dholbert]]

Hm, yeah -- so far, the tree-management bot reports an SVG increase of 0.10% on MacOSX 10.6.2 and 0.94% on XP Firefox, and an Opacity increase of 5.63% on XP (with all three pointing to this push).

Last I checked, these test-suites don't contain any animation, but their SVG documents *will* get nsSMILAnimationControllers (because we still need to record the document start-time as "time=0", in case animations get added later).  So this regression is presumably from something that changed for the basic animation infrastructure code that gets run in SVG documents that lack animations.

I'm happy to back this out, but I'd like to leave it in a bit longer to give us more data points to definitively compare in vs out (particularly since the regression was under 1% on the SVG suite, so far),

Comment 27

[Daniel Holbert [:dholbert]]

Perhaps the perf regression is due to the Refresh Driver's repeated calls to the virtual method "WillRefresh" (which replaces a timer-based callback to the *non-virtual* method "Notify")?   Or maybe nsRefreshDriver just has more overhead than a timer-based callback? (but only if it's got any listeners)

Assuming it's related to callbacks to WillRefresh, we could alleviate the problem by **not** calling StartSampling until animations are added to the document.

This would have the slightly bogus effect of making document.documentElement.getCurrentTime() always return the same value (0) until animations were added (or until the user does a manual setCurrentTime), at which point the current-time would jump up-to-date.  I think that's legal (if unexpected) behavior, though, with the whole "SMIL doesn't guarantee any particular framerate" thing.  And we could work around it, too, by making getCurrentTime update our time-counter if we have zero animations.

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Sounds good.

Comment 29

[Daniel Holbert [:dholbert]]

(In reply to comment #27)
> Assuming it's related to callbacks to WillRefresh, we could alleviate the
> problem by **not** calling StartSampling until animations are added to the
> document.

Filed Bug 553075 on this.

Comment 30

[Daniel Holbert [:dholbert]]

(In reply to comment #26)
> Hm, yeah -- so far, the tree-management bot reports an SVG increase of 0.10% on
> MacOSX 10.6.2 and 0.94% on XP Firefox, and an Opacity increase of 5.63% on XP
> (with all three pointing to this push).

FWIW, the Opacity increase seems to have been a random spike -- it came back down in the next point on the talos graph, though not quite all the way.

So, I'm not absolutely convinced that the perf regressions (SVG & Opacity) are real, but I'm backing this out anyway, to see if the backout gets us an improvement:  http://hg.mozilla.org/mozilla-central/rev/33925fdc2807

Comment 31

[Jeff Muizelaar [:jrmuizel]]

Looks like backing out had an effect.

Improvement: SVG decrease 0.12% on MacOSX 10.6.2 Firefox

Previous results: 3312.82 from build 20100318074635 of revision af2a60e9e64e at 2010-03-18 10:57:00 on talos-r3-snow-008 run # 0

New results: 3309.0 from build 20100318080332 of revision 60088b59cb41 at 2010-03-18 09:32:00 on talos-r3-snow-016 run # 0

Suspected checkin range: from revision af2a60e9e64e to revision 60088b59cb41

Comment 32

[Daniel Holbert [:dholbert]]

Ok, cool -- in that case, I'll fix Bug 553075 (which should get us a perf win for animationless SVG) before re-landing this.  And once Bug 553075 is fixed, this should no longer have any impact on animationless SVG.

Comment 33

[Daniel Holbert [:dholbert]]

Attachment: Subpatch 1: Change method names & subclass nsRefreshDriverObserver, but don't register with nsRefreshDriver yet

So -- to make bug 553075's patch simpler, and to make it easier to isolate any perf regression that might remain, I'm splitting this bug's patch into two parts.
 - Subpatch 1: Change method names & signatures, but don't change behavior yet. (still use nsITimer for sampling callback)
 - Subpatch 2: Flip the switch -- change StartSampling / StopSampling to use nsRefreshDriver for triggering sample-callbacks.

When combined, Subpatch 1 & Subpatch 2 exactly match the previously-posted patch here, with two minor exceptions.[1]

I intend to land patches in this order:
- Subpatch 1 (shouldn't affect perf)
- Bug 553075's patch (should improve perf, if anything)
- Subpatch 2 (shouldn't affect perf, when landed after Bug 553075's patch)

[1] There are two chunks in Subpatch 2 that are new w.r.t. the previously-posted patch here:
 - I added one comment ("// nsRefreshDriver Callback function" above WillRefresh impl)
 - Along with removing the nsSMILAnimationController::Notify() implementation, I now also remove the declaration of that static method from the header file. (I accidentally left the declaration in before, but it didn't cause any harm since it wasn't used anymore.)

Comment 34

[Daniel Holbert [:dholbert]]

Attachment: Subpatch 2: Switch to actually use nsRefreshDriver for callbacks

Comment 35

[Daniel Holbert [:dholbert]]

Attachment: Subpatch 2: Switch to actually use nsRefreshDriver for callbacks

Comment 36

[Daniel Holbert [:dholbert]]

Landed subpatch 1: http://hg.mozilla.org/mozilla-central/rev/c6e4f04278cd

Comment 37

[Daniel Holbert [:dholbert]]

Landed subpatch 2: http://hg.mozilla.org/mozilla-central/rev/984bd6092ba3