nsSMILAnimationController::WillRefresh should always update the moving average

RESOLVED FIXED in mozilla22

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

Trunk
mozilla22
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 720061 [details] [diff] [review]
Always update the moving average in nsSMILAnimationController::WillRefresh.

Proposed patch.
Attachment #720061 - Flags: review?(dholbert)
(Assignee)

Comment 2

6 years ago
Created attachment 720063 [details] [diff] [review]
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.
Attachment #720063 - Flags: review?(birtles)
(Assignee)

Updated

6 years ago
Attachment #720061 - Attachment is obsolete: true
Attachment #720061 - Flags: review?(dholbert)
(Assignee)

Comment 3

6 years ago
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
(Assignee)

Comment 4

6 years ago
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.
I wonder if this patch also fixes bug 658189
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 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.
Attachment #720063 - Flags: review?(birtles)
Attachment #720063 - Flags: review+
Attachment #720063 - Flags: feedback?(birtles)
Comment on attachment 720063 [details] [diff] [review]
Always update the moving average in nsSMILAnimationController::WillRefresh.

Looks good to me! Thanks Seth and Daniel!
Attachment #720063 - Flags: feedback?(birtles) → feedback+

Comment 9

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

Thank you for looking at this Seth :-)
(Assignee)

Comment 10

6 years ago
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.
(Assignee)

Comment 11

6 years ago
Created attachment 720887 [details] [diff] [review]
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
(Assignee)

Updated

6 years ago
Attachment #720063 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/a18af07069de
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.