846861 - nsSMILAnimationController::WillRefresh should always update the moving average

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Seth Fowler [:seth] [:s2h]]

Right now we have an intermittent (but frequent) orange in test_AnimSVGImage.html. Investigation in bug 835658 shows that the problem is that the nsSMILAnimationController::WillRefresh gets an initial update with a very low elapsed time (typically 1) and thereafter the elapsed time is much higher (in these test runs on try, typically ~300). Because the first update is 1, mAvgTimeBetweenSamples gets set to 1 after the initial update, and even though the elapsed time is consistent after that point, it's large enough to trigger the "detected really long sample" code in WillRefresh every time. That code only advances the animation by mAvgTimeBetweenSamples (which has a value of 1) and never updates mAvgTimeBetweenSamples.

All of this means that we always only update the animation by the smallest possible step size, making it run incredibly slowly, and there's no way to recover unless we are lucky enough to get a call to WillRefresh with a much shorter elapsed time at some point. The solution is pretty simple: we always update the moving average, even if we hit the "really long sample" code. That means that if we keep getting long samples, the moving average will recover extremely fast to the point where we start handling them normally as we should.

Comment 1

[Seth Fowler [:seth] [:s2h]]

Attachment: Always update the moving average in nsSMILAnimationController::WillRefresh.

Proposed patch.

Comment 2

[Seth Fowler [:seth] [:s2h]]

Attachment: Always update the moving average in nsSMILAnimationController::WillRefresh.

Whoops, left a printf in! Also, Daniel recommended kicking this review over to you, Brian, since you're the original author of the code.

Comment 3

[Seth Fowler [:seth] [:s2h]]

Up to now this has been tested mostly against Mochitest-oth. I just created a full try job for it here:

https://tbpl.mozilla.org/?tree=Try&rev=b59250f78ead

Comment 4

[Seth Fowler [:seth] [:s2h]]

That try run was pretty unlucky (there was some major bustage today that got pulled into that try run, hence all the failed X runs, and tests now fail if the number of assertions produced isn't exactly as expected, which is causing more oranges) but I really don't think any of the failures there are caused by this patch. And look at all that green 'oth'! This looks like this is the fix we've been looking for.

Comment 5

[Robert Longson [:longsonr]]

I wonder if this patch also fixes bug 658189

Comment 6

[Daniel Holbert [:dholbert]]

I doubt it.  That bug is about individual animations in an SVG document appearing to "freeze" while others keep going.  This bug here addresses an issue where the document's global SVG-timeline slows down (not just for individual animations within it).

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 720063 [details] [diff] [review]
Always update the moving average in nsSMILAnimationController::WillRefresh.

While I still would like birtles' feedback on this, I'll go ahead and r+ it since it's pretty small, it makes sense to me, and it'll fix one of our current spammiest randomoranges.  So -- let's get this landed, and if birtles has any suggestions/objections afterwards, we can address them at that point.

Just one comment-related nit:

>   } else {
>+    // Usual case, update moving average.
>+    if (elapsedTime > SAMPLE_DEV_THRESHOLD * mAvgTimeBetweenSamples) {

This comment is now a little confusing. It used to be pretty much right above the code that updates the moving average, but with this patch, it's not really near that code at all anymore.

Maybe change this to say...
  // Usual case (not the first sample)

...since I think that's really what it's trying to say (it's a heading for the "if first sample... else" clause), and then add "Update the moving average" lower down to the beginning of this comment:
>     // Due to truncation here the average will normally be a little less than
>     // it should be but that's probably ok
>     mAvgTimeBetweenSamples =

r=me wit that comment-tweak.

Comment 8

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

Comment on attachment 720063 [details] [diff] [review]
Always update the moving average in nsSMILAnimationController::WillRefresh.

Looks good to me! Thanks Seth and Daniel!

Comment 9

[Ed Morley [:emorley]]

Other than the comment nit in comment 7, this is good to land now yeah?

Thank you for looking at this Seth :-)

Comment 10

[Seth Fowler [:seth] [:s2h]]

Super, thanks Daniel and Brian for taking a look at the patch. Will post an updated version shortly. I'll just go ahead and push that version to inbound since the only changes involve comments.

Comment 11

[Seth Fowler [:seth] [:s2h]]

Attachment: Always update the moving average in nsSMILAnimationController::WillRefresh.

Updated patch based on Daniel's review comments. Pushed to inbound:

https://hg.mozilla.org/integration/mozilla-inbound/rev/a18af07069de

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a18af07069de