658189 - SMIL transform animation is frozen at intermediate state when switching to another tab & back, at Marek's "Cuba" demo

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

STEPS TO REPRODUCE:
 1. Load attached testcase.

 2. Click document to trigger 'squish' animation

 3. Immediately (within a second), switch tabs or open new tab (Ctrl+PageUp/T)

 4. Wait 2 seconds to be sure 'squish' animation duration is complete, and then switch back to the tab with the testcase.

 4. If you don't get ACTUAL RESULTS yet, return to step 2.

ACTUAL RESULTS: When you return to the tab, some dominos are at intermediate states of the 'squish' animation.

EXPECTED RESULTS: Dominos should all be returned to their normal size.

Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110518 Firefox/6.0a1

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  Do they then snap into position?  Or stay there?

If you disable the exponential backoff in background refresh drivers, does the problem go away?

Comment 2

[Daniel Holbert [:dholbert]]

Note: this testcase resists attempts at reducing. :) While it's not minimal, the current testcase is much simpler than the original demo, after some work by Marek & myself.

Comment 3

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
> Hmm.  Do they then snap into position?  Or stay there?

They stay there.

> If you disable the exponential backoff in background refresh drivers, does the
> problem go away?

I'll give that a try, but maybe this answers that question:

Using a much less reduced testcase, I got the following regression range earlier today:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=75cc425bbaf0&tochange=e1d55bbd1d1d
.. in which bug 586201 ("Throttle refresh drivers in background tabs") looks like the most likely candidate.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot of bug

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm.  That was the fixed-duration throttling, before we switched to exponential backoff.

In that case, you should see the problem in Firefox 4.  If so, what values of "layout.throttled_frame_rate" show the problem?  I assume 60 does not but 1 does?

Would SMIL have problems with the refresh timer firing "too late" for some reason?

Comment 6

[Boris Zbarsky [:bzbarsky]]

To be clear, that's a pref.  And it's live, so should be easy to test.

Comment 7

[Boris Zbarsky [:bzbarsky]]

fwiw, I would assume we're hitting the SMIL sleep detection here, right?  Does that matter?

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to comment #5)
> In that case, you should see the problem in Firefox 4.  If so, what values
> of "layout.throttled_frame_rate" show the problem?  I assume 60 does not but
> 1 does?

Correct. (in Firefox 4 -- not in nightlies, because as bz tells me on IRC, the pref means something completely different in nighlties.)

> Would SMIL have problems with the refresh timer firing "too late" for some
> reason?

By |firing "too late"|, do you mean "longer-than-normal time between samples", or "refresh driver lied about how much time had elapsed"?

It seems feasible that these could cause problems, but I'm not sure how, at the moment.

(In reply to comment #7)
> fwiw, I would assume we're hitting the SMIL sleep detection here, right?

I don't think we are, actually.  I don't see any instances of this warning being spammed, at least:
>  // Unexpectedly long delay between samples:
>  } else if (elapsedTime > SAMPLE_DEV_THRESHOLD * mAvgTimeBetweenSamples) {
>    NS_WARNING("Detected really long delay between samples, continuing from "
>               "previous sample");
>    mParentOffset += elapsedTime - mAvgTimeBetweenSamples;

...and I tried raising SAMPLE_DEV_THRESHOLD by 100x, too, and it didn't affect anything.

Note also that this sleep detection code was added in Nov 2010 (with Bug 606932), 3 months after the regression pushlog from comment 3.

Comment 9

[Daniel Holbert [:dholbert]]

Alternate STEPS TO REPRODUCE:
 1. Set about:config pref layout.frame_rate = 1 (or 2 or 3, if you like)
 2. Load testcase.
 3. Click document.  Watch to see if dominos return to their original state.

ACTUAL RESULTS: Most of the time, the dominos don't make it all the way back to their original state.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> By |firing "too late"|, do you mean "longer-than-normal time between samples", > or "refresh driver lied about how much time had elapsed"?

The former.

> Note also that this sleep detection code was added in Nov 2010 

OK, then.  It's not the problem.

Certainly given comment 9 it's not the problem.  Comment 9 just sounds like SMIL can't deal with very rare refresh driver notifications for some reason...

Comment 11

[Daniel Holbert [:dholbert]]

Yeah, agreed -- I think Comment 9 is the real bug here, and the regression range in Comment 3 just introduced a new way to hit it.

Comment 12

[(mostly gone) XtC4UaLL [:xtc4uall]]

Confirmed against Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110519 Firefox/6.0a1 ID:20110519030627

Comment 13

[Daniel Holbert [:dholbert]]

So from doing a targeted old build with a low-framerate code tweak...
> content/smil/nsSMILAnimationController.cpp
> -const PRUint32 nsSMILAnimationController::kTimerInterval = 22;
> +const PRUint32 nsSMILAnimationController::kTimerInterval = 1000;
...I hit this bug after bug 533291's csets landed (revision 9cdfc9ee6754) but not before (revision 5dd3f323fa37).

Comment 14

[Brian Birtles (:birtles, Feb 26~ parental leave)]

(In reply to Boris Zbarsky (:bz) from comment #10)
> Certainly given comment 9 it's not the problem.  Comment 9 just sounds like
> SMIL can't deal with very rare refresh driver notifications for some
> reason...

SMIL (and our implementation of it) should be totally independent of sample rate. If it's not, it's a bug.

My suspicion is we're ignoring the sample or filtering it out for some reason.

Comment 15

[Daniel Holbert [:dholbert]]

> My suspicion is we're ignoring the sample or filtering it out for some reason.

Per comment 13, bug 533291 appears to have regressed this -- so it looks like we're incorrectly thinking that our animations haven't changed when actually they have.

Comment 16

[Daniel Holbert [:dholbert]]

To clarify: I think the issue is something to do with the switch from active to inactive -- we (incorrectly) think that nothing's changed on that first inactive sample, & we don't bother recompositing.

The low frame rate only matters insomuch as it makes that mistake more obvious, because it means that our final composited frame is from somewhere in the middle of the simple duration rather than somewhere very close to the end.

Comment 18

[Robert Longson [:longsonr]]

Let's consider the case where we have two animations one the same element, animating the same property. Those animations will use the same compositor instance so we can properly add them together if we need to.

Let's also assume that one of the animations is either frozen or active but unchanging. It won't cause SMILAnimationFunction::mHasChanged to be set and therefore won't cause SMILCompositor::mForceCompositing to be true and will enable compositing to be skipped. This is what bug 533291 implemented.

But we have a second animation, one that is active so it does force compositing until it ends. At the moment it ends though, it's no longer added to the compositor and so we fall back to the other animation and we skip compositing for that element. If we have sufficiently fast resource driver ticks then the old animation may be close to its final state and we won't notice it didn't quite finish but if the ticks aren't very fast then its last active state may have been some way from the end and it's then removed so it can no longer force compositing.

To fix this we'd also want to force compositing for an element if any animation functions are removed from one pass to the next.

Comment 19

[Robert Longson [:longsonr]]

Attachment: Bug 658189 Part 1 - non-functional cleanup r=emilio

• Use range based loops where we can
• Use IsEmpty() instead of comparing Count() to 0
• Use LastElement() where we can
• Don't flush if we're not going to use an animation function at all

Comment 20

[Robert Longson [:longsonr]]

Attachment: Bug 658189 Part 2 - force compositing if the number of animation functions changes r=birtles

Depends on D195218

Comment 21

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/jobs?repo=try&revision=d999f5f8bf6b1709a14a76fced2a0b4a2fa3c1c6

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df63f8d34d41
Part 1 - non-functional cleanup r=emilio
https://hg.mozilla.org/integration/autoland/rev/67f8b12335fd
Part 2 - force compositing if the number of animation functions changes r=birtles

Comment 23

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/df63f8d34d41
https://hg.mozilla.org/mozilla-central/rev/67f8b12335fd