1465948 - crates.io download graphs are broken on WebRender

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Dirkjan Ochtman (:djc)]

Attachment: actual.png

See the attachments. This is on macOS, latest Nightly.

Comment 1

[Dirkjan Ochtman (:djc)]

Attachment: expected.png

Comment 2

[Dirkjan Ochtman (:djc)]

I expect the regression range to be fairly recent, in the past few weeks or so.

Comment 3

[Darkspirit]

https://crates.io/crates/rustls
Only the top quarter or top half is fine until I zoom in. Then it looks good.

Comment 4

[Darkspirit]

mozregression --good 2018-05-15 --bad 2018-05-31 --pref gfx.webrender.all:true startup.homepage_welcome_url:'https://crates.io/crates/rustls'
> 9:02.10 INFO: Last good revision: a25b2c7238f46770d612f2a2cb2f8731e31261ee
> 9:02.10 INFO: First bad revision: 133a13c44abedac2e448d315a32068ce1a5568f4
> 9:02.10 INFO: Pushlog:
> https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a25b2c7238f46770d612f2a2cb2f8731e31261ee&tochange=133a13c44abedac2e448d315a32068ce1a5568f4

> 133a13c44abe	Jeff Muizelaar — Bug 1458968. Adjust fuzz for webrender tests r=mstange
> 0f55d0ffe494	Markus Stange — Bug 1458968 - Create the nsDisplaySVGWrapper item in nsSVGOuterSVGAnonChildFrame, not in nsSVGOuterSVGFrame. r=mattwoodrow
> 6629dc2614ed	Markus Stange — Bug 1458968 - Disable blend-difference-stacking.html because it fails now. r=mattwoodrow
> 30d54bb4cc27	Markus Stange — Bug 1458968 - Make the nsSVGOuterSVGAnonChildFrame a reference frame by always returning true from IsSVGTransformed. r=mattwoodrow
> 88d41ddd11be	Markus Stange — Bug 1165185 - Add a test for not invalidating transformed elements inside SVG during scrolling. r=roc

Comment 5

[Markus Stange [:mstange]]

Over to Alexis since it's a blob invalidation bug.

Comment 6

[Aria Beingessner [:Gankra]]

confirmed only happens with blob-invalidation

Comment 7

[Aria Beingessner [:Gankra]]

notes from discussion yesterday:

It seems like the way we're building the blob for a group (recording a DT using BasicLayerManager) is clipped to the viewable area, which the current blob-invalidation code doesn't expect, so it doesn't properly invalidate when the rest of the group gets scrolled in-bounds.

Either need to force the whole blob to be recorded regardless of how much is in-bounds, or otherwise properly invalidate when it's scrolled in-bounds.

Comment 8

[Dirkjan Ochtman (:djc)]

Not sure if the same thing, but this page also has interesting WebRender-only breakage:

https://dev.gentoo.org/~mgorny/active-devs.html (hover around a bit, resting on the colored bars)

Comment 9

[Aria Beingessner [:Gankra]]

the gentoo page seems fixed now for me; confirm?

Comment 10

[Dirkjan Ochtman (:djc)]

Confirmed.

Comment 11

[Markus Stange [:mstange]]

(In reply to Alexis Beingessner [:Gankro] from comment #7)
> notes from discussion yesterday:
> 
> It seems like the way we're building the blob for a group (recording a DT
> using BasicLayerManager) is clipped to the viewable area, which the current
> blob-invalidation code doesn't expect, so it doesn't properly invalidate
> when the rest of the group gets scrolled in-bounds.

The current code does attempt to invalidate properly in that case. When the new items enter the display port, there will be a paint. In that paint, the new items will be present in the display list. And then we should hit this code for those new items:
https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/gfx/layers/wr/WebRenderCommandBuilder.cpp#386-398
Which attempts to add the items to the invalid rect.

Comment 12

[Aria Beingessner [:Gankra]]

Attachment: Bug 1465948 - add test for scrolling svg groups.

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

Comment 13

[Aria Beingessner [:Gankra]]

PR is WIP, need to do some more testing to confirm it works, but the basic idea is, as explained to me by matt woodrow: The change to use reference frames means coordinates no longer change while scrolling, so we no longer invalidate things properly as they scroll into view. To do that we must factor in a visibility rect, which is apparently called the "builder rect". Just intersect that with the item and we get the "true" bounds rect.

Comment 14

[Aria Beingessner [:Gankra]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4d66f29b11cc9c0dd353f3ae701a2c60c3a5d032

talos: https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&newProject=try&newRevision=4d66f29b11cc9c0dd353f3ae701a2c60c3a5d032&framework=1&selectedTimeRange=172800

Comment 15

[Aria Beingessner [:Gankra]]

nice, a huge pile of unexpected passes

a bit confused by the talos results, which seem great? Is factoring in the visible rect here making us avoid work..?

Comment 16

[Markus Stange [:mstange]]

(In reply to Alexis Beingessner [:Gankro] from comment #13)
> The change to use reference
> frames means coordinates no longer change while scrolling, so we no longer
> invalidate things properly as they scroll into view.

The coordinates no longer change during scrolling, that's true, but this should have made invalidation *easier*, not harder. And the invalidation should be happening in response to new display items *appearing* in the display list, not *moving*, so the coordinates shouldn't matter that much for this case.

> To do that we must
> factor in a visibility rect, which is apparently called the "builder rect".
> Just intersect that with the item and we get the "true" bounds rect.

If we use that intersected rect as the blob image rect, that's going to make it hard to keep around existing rasterized content as the display port moves during scrolling. I think this change would make us invalidate the full blob any time new content gets added to one side or removed on the other side due to scrolling.

The idea for handling this was the following (and was supposed to work, but obviously that's not currently the case):
 (1) Always size the blob image to unclipped bounds of the SVG wrapper display item.
 (2) Make sure that WebRender only rasterizes and composites the parts of it that are within the display port, by forwarding the SVG wrapper item's "builder rect" or the "paint rect" to WebRender. This part is currently not really working, but shouldn't matter much for correct invalidation anyway. Nical's work in bug 1455422 is probably going to cover this part.
 (3) During scrolling, any rasterized content that is not explicitly invalidated will be kept around by WebRender. If new items appear on one side, or items disappear on the other side, Gecko should detect this and tell WebRender to repaint those parts of the blob by setting an appropriate invalid rect.
 (4) Once this system is working, we can make use of (2) to optimize the case where the fact that we only have a single invalid rect bites us: If scrolling causes new items to be added on the bottom *and also* existing items to be removed at the top, we'll have an invalid rect that goes across the content in the middle, and we'll repaint that content even though it hasn't changed. This case is what the comment at https://searchfox.org/mozilla-central/rev/97d488a17a848ce3bebbfc83dc916cf20b88451c/gfx/layers/wr/WebRenderCommandBuilder.cpp#56-58 is about. Once we have the SetImageVisibleArea API that's being added in bug 1455422, I think we can use that to solve this problem: WebRender should discard any pixels that leave the visible area, and should treat any pixels that have just been added to the visible area as invalid. Then we no longer have to include those areas in the blob's invalid rect, and can use the invalid rect just for invalidations in the "center part" of the image. (By "center part" I mean the intersection of the old visible area and the new visible area.)

The large number of unexpected passes is surprising to me, and I don't understand why intersecting the image's bounds with the "builder rect" affected those tests.

Comment 17

[Markus Stange [:mstange]]

(In reply to Alexis Beingessner [:Gankro] from comment #15)
> a bit confused by the talos results, which seem great? Is factoring in the
> visible rect here making us avoid work..?

This is probably due to (2) not really working at the moment. I think jrmuizel and were hoping that WebRender would using tiling for all blobs automatically, and only rasterize the pieces that intersect the screen, but from what I've seen, it looks like we're not using tiling for most of these blobs and just rasterize everything. Your current patch is one way to work around this bug, but I think nical's work in bug 1455422 is the better fix for this.

Comment 18

[Aria Beingessner [:Gankra]]

Results of a bunch of discussion with stakeholders in blobs/invalidation/apz:

It seems like this solution is a reasonable one while we try to figure out the "ideal" solution. Clearly it improves performance and fixes a ton of correctness issues. nical is fine with this making some of his work "useless", as that part isn't the most important part.

I turned on paint flashing (hardcoded with an ifdef for this path) and didn't see any extra paints once the content is visibly on screen, which is good/promising.

Comment 19

[Markus Stange [:mstange]]

I'd also be interested in the results of paint flashing on a longer SVG, where there's a visible middle piece that's not supposed to be invalidated during scrolling. For example in the testcase from bug 1470219.

Comment 20

[Aria Beingessner [:Gankra]]

Yes, that case constantly fully repaints during scrolling.

Comment 21

[Markus Stange [:mstange]]

Ok. I'd actually expect that testcase to fully repaint even without this patch because of the problem mentioned in (4)... so here's a testcase where I wouldn't expect that:
data:text/html,<svg height=20000><rect fill=none x=50 y=50 width=200 height=19900 stroke=blue stroke-width=5 stroke-dasharray="10 10"/>

I'd expect this one to not invalidate without your patch, but fully invalidate with your patch. Can you check that too?

Comment 22

[Aria Beingessner [:Gankra]]

you are correct that it it constantly invalidates with my patch. Without my patch it seems to draw correctly, although the paint-flashing area is broken so I can't tell if it's repainting (my guess is it doesn't)

Comment 23

[Aria Beingessner [:Gankra]]

Attachment: Bug 1465948 - factor in the BuildingRect for filters and masks.

new impl, based on an insight from jrmuizel: this is seemingly only a problem with BasicLayerManager, which we only use with Filters/Masks, so, only apply this hack to those items.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8991083 [details] [diff] [review]
Bug 1465948 - factor in the BuildingRect for filters and masks.

Review of attachment 8991083 [details] [diff] [review]:
-----------------------------------------------------------------

This looks reasonable to me.

Comment 25

[Aria Beingessner [:Gankra]]

still needs a test but i want to land this since this is an annoying bug on nightly

Comment 26

[Aria Beingessner [:Gankra]]

Attachment: Bug 1465948 - factor in the BuildingRect for filters and masks.

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

Comment 27

[Aria Beingessner [:Gankra]]

Comment on attachment 8989549 [details]
Bug 1465948 - add test for scrolling svg groups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/254574/diff/1-2/

Comment 28

[Aria Beingessner [:Gankra]]

test added and uploaded with mozreview since my splinter patch was corrupt

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7b32e48fadc5
factor in the BuildingRect for filters and masks. r=jrmuizel

Comment 30

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/7b32e48fadc5

Comment 31

[Andreea Pavel [:apavel]]

:Gankro this has been landed, looks pretty green: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=7b32e48fadc594b0df36ba4c47cbb42bef3e177c&group_state=expanded

:jrmuizel the patch has been merged to mozilla-central (https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=04dd259d71db60341016eccf53ced43742319631), no evident issues so far.

Comment 32

[Markus Stange [:mstange]]

Comment on attachment 8989549 [details]
Bug 1465948 - add test for scrolling svg groups.

https://reviewboard.mozilla.org/r/254574/#review263894

Comment 33

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8991988 [details]
Bug 1465948 - factor in the BuildingRect for filters and masks.

This patch landed already, dropping review flag.

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/793a1a3ba372
add test for scrolling svg groups. r=mstange

Comment 35

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/793a1a3ba372

Comment 36

[Dirkjan Ochtman (:djc)]

This is still/again broken for me. IIRC it worked correctly for a while there, but now it fails in pretty much the same way -- with the only difference that the breakage doesn't get fixed by clicking on the chart.

Comment 37

[Darkspirit]

(In reply to Bogdan Tara[:bogdan_tara] from comment #35)
> https://hg.mozilla.org/mozilla-central/rev/793a1a3ba372

mozregression --launch 793a1a3ba372 --pref gfx.webrender.all:true -a https://crates.io/crates/rustls
This has become an intermittent bug and can be fixed by clicking on the green background.

Comment 38

[Aria Beingessner [:Gankra]]

i can't seem to reproduce this at all anymore, any repro details?

Comment 39

[Darkspirit]

I scrolled a bit down by mousewheel, moved my mouse over the "Fork me on Github" banner to the scrollbar and scrolled down with it. I see the issue once in three to five attempts (after reloading the page).

Comment 40

[Kartikaya Gupta (email:[e-mail])]

I'm not able to reproduce this either. From a process point of view it would be better to leave this bug closed and clone it for the new issue.

Comment 41

[Jeff Muizelaar [:jrmuizel]]

Closing. :darkspirit can you file a new bug for what you're seeing.