870192 - Prevent rounding issues in gfxMatrix::ScaleFactors from combining with timing issues to cause sporadic invalidation of transforming elements

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

Attachment: testcase

With the latest patch from bug 869611 (just landed on inbound) we're performing vastly better on bug 852588 (Santa's Workshop IE testdrive demo). The one thing than seems to be holding us back from equal performance with IE/Chrome is the continuous invalidation of the gently rotating (and translating down) snowflakes.

I've tried to reduce the issue. Normally we don't invalidate transforming elements, and the active layer just composites them. Great. But if the transform involves a rotation, and if another element in the document invalidates, it seems to trigger an invalidation of the transforming element for some reason. This is the case even when the invalidating element is very far from the transforming element.

In this testcase, turn on paint flashing, let the jiggling square stop invalidating, then mouse over the smaller square below it to apply a :hover rule to the smaller square that will change its fill color. This will invalidate the smaller square as expected, but there also seems to be a very strong correlation with the transforming square then suddenly starting to invalidate for no apparent reason.

Comment 1

[Jonathan Watt [:jwatt]]

The issue here is that the FrameLayerBuilder::ContainerParameters for the layer created for the nsDisplayTransform usually have mXScale and mYScale set to 4. For some reason though, when the small, untransformed rect changes color, the ContainerParameters for the transformed rect changes to 3 temporarily.  That causes the FuzzyEqual comparison in ContainerState::CreateOrRecycleThebesLayer to return false, resulting in InvalidateEntireThebesLayer being called.

Comment 2

[Jonathan Watt [:jwatt]]

The relationship of the invalidation of the small untransformed rect to the invalidation of the larger transforming rect is purely that of changing the outcome of a race condition.

The code that's going wrong is in ChooseScaleAndSetTransform here:

http://mxr.mozilla.org/mozilla-central/source/layout/base/FrameLayerBuilder.cpp?rev=1d7c615b4b62&mark=2707-2708#2701

What's happening is that |oldScale == scale| evaluates to false as long as we're not dropping any frames, since each frame we toggle to the "other" transform matrix. However, when the small rect is invalidated, we frequently seem to skip a frame (or an odd number of frames) and end up with exactly the same transform, causing |oldScale == scale| to evaluate to true. As explained in the previous comment, that results in different mXScale and mYScale values, which in turn causes the entire layer to be invalidated.

Note that for |oldScale == scale| to evaluate to true the old and new matrices do not need to be the same (as evidenced by the continuously invalidating snowflakes in the Santa's Workshop demo).

This is a regression introduced by the patch for bug 706690.

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: this doesn't work

Rather than try to detect panning by comparing the result of ScaleFactors() on the old and new matrices, I was thinking that this could be solved by more specifically checking whether all the matrix components except the translation components are the same. That would stop rotations falling afoul of the panning detection block.

Unfortunately, it turns out that another very common cause of |oldScale == scale| evaluating to true is getting two consecutive paints between transform changes. The result is that my initial idea fails, since when that happens the non-translation components of the old and new matrices will obviously compare equal, and the code will take the "we're panning" path.

One way to solve this would be to check if the matrices are the same, and in that case reuse the old mXScale/mYScale, but as far as I can tell those old scales (the actual clamped or not clamped values) are not saved or available in ChooseScaleAndSetTransform. Perhaps we could cache them on aLayer (add mXScale/mYScale members)? That should be easy and doable before the uplift next week.

Another approach would perhaps be to have the frame pass on a "don't clamp me" flag that could be set by the fennec code in some way. That sounds like it might require a fair bit of digging to figure out what that should look like.

Comment 4

[Jonathan Watt [:jwatt]]

roc, any preference on how to solve this?

Comment 5

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

So you're saying that the snowflakes are just rotating, but that rotation causes the computed scale factors to change? That seems wrong.

Comment 6

[Jonathan Watt [:jwatt]]

Yeah, the above is a bit confused since I was thinking that normally the old and new scale factors should be different, and it's broken when they're the same. Actually, of course, they should always be the same (at least in the rotating snowflake case), and the problem is that they are sometimes different.

The reason that the old and new scale factors are different for different angles of rotation is because there is a slight (unchanging) scale in addition to the rotation. That scale is enough to cause rounding issues in gfxMatrix::ScaleFactors that cause it to return different scale factors for different angles of rotation.

For example, say that we have transform="scale(1.1) rotate(<angle>, <cx>, <cy>)". For different values of <angle>, gfxMatrix::ScaleFactors().width will be:

 0 deg: 1.10000001493379
 3 deg: 1.10000002829671
 6 deg: 1.10000007650886
 9 deg: 1.10000006170794
12 deg: 1.10000006885501
15 deg: 1.10000007862061
18 deg: 1.09999997885643
21 deg: 1.09999998058213
24 deg: 1.10000001458119
27 deg: 1.10000001696274
30 deg: 1.10000001937242
33 deg: 1.0999999874684
36 deg: 1.10000005289285
39 deg: 1.10000004386417
42 deg: 1.10000004017989
45 deg: 1.10000004386417
48 deg: 1.10000005289285
51 deg: 1.0999999874684
54 deg: 1.10000001937242
57 deg: 1.10000001696274
60 deg: 1.10000001458119
63 deg: 1.09999998058213
66 deg: 1.09999997885643
69 deg: 1.10000007862061
72 deg: 1.10000006885501
75 deg: 1.10000006170794
78 deg: 1.10000007650886
81 deg: 1.10000002829671
84 deg: 1.10000001493379
87 deg: 1.10000002384186
90 deg: 1.10000001493379

Now consider the case where script is updating the rotation on an element 30 times per second, but other invalidations in the document are causing us to paint at 60fps. We will enter ChooseScaleAndSetTransform 60 times per second. Every second paint we will find that the old and new transforms are the same, giving us the same scale factors, and causing us to take the "clamp" path. Every other paint we will find that the old and new transforms are different, ScaleFactors will return a different gfxSize due to rounding error (as shown by the table above), and we will _not_ take the "clamp" path. As a result the rotating element will invalidate 30 times per second.

Of course this is a nice tidy example where the timing is neatly aligned to consistently reproduce the unwanted invalidation. In in-the-wild examples the timing and values of the changing transforms will usually result in very erratic and unpredictable unwanted invalidations with no apparent rhyme or reason.

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: testcase showing rounding issue - click to toggle animation of the color of the small rect

Here's a testcase showing that 30fps invalidation of the transforming rect whenever the color of the other rect is animating.

Comment 8

[Jonathan Watt [:jwatt]]

Attachment: patch to avoid bad rounding

Comment 9

[Jonathan Watt [:jwatt]]

(FWIW, for anyone looking, this doesn't currently make much difference to the Santa's Workshop demo specifically. That's only because bug 871172 means that the snowflakes and bodies of the elves don't actually render because the gradient references don't resolve. In a modified version of Santa's Workshop where the gradient references do resolve it makes a very significant difference.)

Comment 10

[Jonathan Watt [:jwatt]]

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

Comment 11

[Jonathan Watt [:jwatt]]

Comment on attachment 748439 [details] [diff] [review]
patch to avoid bad rounding

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: bad perf on certain transform animations - b2g wants this
Testing completed (on m-c, etc.): landed on m-i before uplift
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 748439 [details] [diff] [review]
patch to avoid bad rounding

If this is only useful for B2G, there's no point in taking on Firefox 22/23.

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 748439 [details] [diff] [review]
patch to avoid bad rounding

It's useful on all platforms. I only mentioned B2G because my manager told me that B2G people have been complaining a lot about SVG performance recently, and I heard that 1.1 _may_ end up being based on v23. At any rate, this is a significant win everywhere, and we only just branched.

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 748439 [details] [diff] [review]
patch to avoid bad rounding

OK - low risk fix in support of cross-platform svg perf. Approving for Aurora 23. We won't take on Beta 22 though without the regressing bug listed.

Comment 16

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0016395fd426

Comment 17

[Paul Silaghi, QA [:pauly]]

Reproduced the issue on Nightly 2013-05-09 using the testcases.
Verified fixed FF 23b6 Win 7 x64.

Comment 18

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 24b4 Win 7 x64.