1458968 - Make every nsSVGOuterSVGAnonChildFrame a reference frame

Status: RESOLVED FIXED

(Core :: SVG, enhancement, P3)

Description

[Markus Stange [:mstange]]

The frame tree for an outer <svg> element looks like this:
                                   
> SVGOuterSVG(svg) ... [sc=11e210408]<
>   SVGOuterSVGAnonChild(svg) ... [sc=12d880c08:-moz-svg-outer-svg-anon-child]<
>     [contents]
>   >
> >

In other words, every nsSVGOuterSVGFrame has a single child frame which is an nsSVGOuterSVGAnonChildFrame which wraps the contents of the SVG.

If there is a viewbox transform, the nsSVGOuterSVGAnonChildFrame becomes a reference frame and display list building creates an nsDisplayTransform that wraps its contents.

I propose that we make this the case *even if there is no viewbox transform*: Make the nsSVGOuterSVGAnonChildFrame always a reference frame and always create a nsDisplayTransform for it.
This will have the effect that the offset between this <svg> element and the parent reference frame is completely absorbed by that nsDisplayTransform, and that this offset does not affect the SVG contents' transforms. Consequently, if the <svg> element moves, e.g. during scrolling, the transform matrices of the contents are unaffected.

This simplifies invalidation in two ways:
 - It fixes bug 1165185, which is caused by floating point inaccuracies when comparing transform matrices that move during scrolling. The shift is applied to the two to-be-compared matrices in different orders, and this gives slightly different results.
 - It means that WebRender's special path for SVG painting doesn't need to do any shifting during invalidation - it can just compare everything in the reference frame space and call it a day.

Comment 1

[Markus Stange [:mstange]]

Attachment: Bug 1458968 - Make the nsSVGOuterSVGAnonChildFrame a reference frame by always returning true from IsSVGTransformed.

This causes transforms of the <svg> contents to be unaffected by scrolling / offset changes of the <svg> element.

Review commit: https://reviewboard.mozilla.org/r/241568/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241568/

Comment 2

[Markus Stange [:mstange]]

Attachment: Bug 1458968 - Disable blend-difference-stacking.html because it fails now.

This is caused by an existing bug (bug 1458353) about the fact that we treat 2d transforms in
SVG as isolated groups. In this test, that bug didn't manifest because the viewbox transform
happened to be the identity matrix. Now that we no longer treat identity viewbox transforms as
special, bug 1458353 starts affecting this test.

Review commit: https://reviewboard.mozilla.org/r/241570/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241570/

Comment 3

[Markus Stange [:mstange]]

Attachment: Bug 1458968 - Create the nsDisplaySVGWrapper item in nsSVGOuterSVGAnonChildFrame, not in nsSVGOuterSVGFrame.

This ensures that the nsDisplaySVGWrapper's mFrame and its reference frame are the same frame: the nsSVGOuterSVGAnonChildFrame.
It'll also cause the nsDisplaySVGWrapper to be *inside* the nsDisplayTransform for the <svg> element's viewbox transform.

This patch reverts nsSVGOuterSVGFrame::BuildDisplayList to its pre-bug 1407938 form.
(That's the bug that introduced nsDisplaySVGWrapper.)

Review commit: https://reviewboard.mozilla.org/r/241572/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/241572/

Comment 4

[Markus Stange [:mstange]]

Moving the nsDisplaySVGWrapper inside the viewbox transform caused some interesting reftest failures. Some of them seem to be snapping-related, one seems fuzzable, viewport-percent-graphic-user-01.svg looks entirely wrong and there are a few unexpected passes.

Comment 5

[Markus Stange [:mstange]]

(Those reftest failures only happen with WebRender on.)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8609dac42b9528499dc3827565e7fc25013cfc8

Comment 6

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8973038 [details]
Bug 1458968 - Make the nsSVGOuterSVGAnonChildFrame a reference frame by always returning true from IsSVGTransformed.

https://reviewboard.mozilla.org/r/241568/#review247538

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8973039 [details]
Bug 1458968 - Disable blend-difference-stacking.html because it fails now.

https://reviewboard.mozilla.org/r/241570/#review247540

Comment 8

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8973040 [details]
Bug 1458968 - Create the nsDisplaySVGWrapper item in nsSVGOuterSVGAnonChildFrame, not in nsSVGOuterSVGFrame.

https://reviewboard.mozilla.org/r/241572/#review247542

Comment 9

[Jeff Muizelaar [:jrmuizel]]

The test crash is caused by WebRender not handling very large blob images. We basically end up OOMing as we try to create 42 million primitives. This should be helped by https://github.com/servo/webrender/pull/2704

Comment 10

[Jeff Muizelaar [:jrmuizel]]

layout/reftests/invalidation/fractional-transform-2.html is failing on Windows with this patch

Comment 11

[Jeff Muizelaar [:jrmuizel]]

Any ideas mstange why this test would fail and only on Window 10?

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Here's the whole try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d16be4b34b098c4dfb83d953b42e6b23f5c35db&selectedJob=179271964

Comment 13

[Markus Stange [:mstange]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #11)
> Any ideas mstange why this test would fail and only on Window 10?

No, no idea. If you can make a Windows build with --enable-dump-painting (or manually remove the #ifdef MOZ_DUMP_PAINTING lines around the nsLayoutUtils::InvalidationDebuggingIsEnabled() checks), we should be able to find out what causes the invalidations.

Alternatively, since these failures are in new tests, we could land the tests disabled on Windows. But it would still be nice to know why we invalidate.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30d54bb4cc27
Make the nsSVGOuterSVGAnonChildFrame a reference frame by always returning true from IsSVGTransformed. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/6629dc2614ed
Disable blend-difference-stacking.html because it fails now. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f55d0ffe494
Create the nsDisplaySVGWrapper item in nsSVGOuterSVGAnonChildFrame, not in nsSVGOuterSVGFrame. r=mattwoodrow
https://hg.mozilla.org/integration/mozilla-inbound/rev/133a13c44abe
Adjust fuzz for webrender tests r=mstange

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/30d54bb4cc27
https://hg.mozilla.org/mozilla-central/rev/6629dc2614ed
https://hg.mozilla.org/mozilla-central/rev/0f55d0ffe494
https://hg.mozilla.org/mozilla-central/rev/133a13c44abe