1572197 - Egencia itinerary doesn't paint properly when scrolling

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Emilio Cobos Álvarez [:emilio]]

Doesn't happen without WR enabled.

I have a page saved locally which reproduces the issue, but obviously has a bunch of personal info, so I'll try to come up with a reduced test-case and/or find a regression range first.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

68 is good.

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Regression from bug 1566901.

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: Screenshot.

Looks pretty busted.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: reasonably reduced test-case

If your viewport is too big for your change you can zoom and it still repros.

I stripped all text and scripts and such, so this should be fine to upload.

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Glenn, any chance you can take a look at this regression from bug 1566901? If you need the full site I can send it to you.

Comment 6

[Glenn Watson [:gw]]

Yea, I'll take a look at it today.

Comment 7

[Glenn Watson [:gw]]

I can repro this locally if I set layout.css.devPixelsPerPx to 2.

Comment 8

[Glenn Watson [:gw]]

Attachment: Bug 1572197 - Fix world clip region for preserve-3d items with picture caching.

When adding planes to the plane splitter, we supply a world clip
rect to the polygon clipper. Generally this is used to help with
float accuracy issues, but it also clips polygons to the visible
region.

The previous code supplied the visible world rect, but this is
not always correct. When drawing picture cache tiles, we may
be rendering to a tile that is partially off-screen. In this case
we need to pass the combined world dirty rect, which is inflated
to include the off-screen tile parts that are being drawn. This
ensures that preserve-3d items are correctly clipped to the tile
boundaries rather than the currently visible screen rect.

Comment 9

[Glenn Watson [:gw]]

Seems to cause a windows-only (?) failure in one of the apz perspective scrolling tests, investigating now.

Comment 10

[Glenn Watson [:gw]]

The test failure seems strange.

Although the patch above fixes the reported bug, it causes a test failure in layout/reftests/async-scrolling/perspective-scrolling-1.html (seemingly on Windows only, according to CI).

The problem is occurring in add_split_plane.

Before the fix above, inside add_split_plane, we have:
world_rect = 816x1039 at 0,0
transform = Transform([1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, -400.0, -500.0, 1.0, -1.0, 100.0, 100.0, 0.0, 1.0])

And the clipper produces a single output polygon, that renders correctly.

However, with the fix above, we get:
world_rect = 1024x1536 at 0,0
(same transform)

The polygon clipper returns Ok but has zero polygons returned, so nothing renders.

In this case, the new inflated world rect is larger (and contains) the world rect being used before the fix was made. I would have expected that the polygon clipper would produce the same output in such a case. Does this sound like a bug inside the clipper / plane-splitting code? Or am I missing something simple here?

Comment 11

[Glenn Watson [:gw]]

I confirmed that the trace output of this test before the inflated world rect is:

Outside of the plane
Outside of the plane
Splitting with normal
Reached complex case
  ...
  outputs a polygon

Whereas with the enlarged world rect, we get:

Outside of the plane
Outside of the plane
Coplanar
<no poly produced>

Which seems likely to be a plane-split bug, I think? I haven't tested this on Linux, but it seems like the same bug should occur - not sure why I wasn't seeing a failure on CI, but you might be able to repro on Linux locally.

Comment 12

[Glenn Watson [:gw]]

This patch [1] in the plane-split crate resolves the test failure with this fix. Once that lands in Gecko, we can land this (and request uplift for both of them).

[1] https://github.com/servo/plane-split/pull/33

Comment 13

[Dzmitry Malyshau [:kvark]]

Attachment: Plane split dependency update

updates for https://github.com/servo/plane-split/pull/33

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65d774a64533
Plane split dependency update

Comment 15

[Dzmitry Malyshau [:kvark]]

Both PRs are on their way now, removing leave-open. Will request uplift once the dust settles.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6b8cdf8a722
Fix world clip region for preserve-3d items with picture caching. r=emilio

Comment 17

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/65d774a64533
https://hg.mozilla.org/mozilla-central/rev/e6b8cdf8a722

Comment 18

[Glenn Watson [:gw]]

Comment on attachment 9083850 [details]
Bug 1572197 - Fix world clip region for preserve-3d items with picture caching.

Beta/Release Uplift Approval Request

• User impact if declined: Users with WebRender will see corruption on some pages that use preserve-3d elements.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Follow steps in bug with the reduced test case.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's a small patch, and only affects users enrolled in WebRender.
• String changes made/needed:

Comment 19

[Daniel Bodea [:danibodea]]

I have reproduced this issue in Beta v69.0b12 and in Nightly v70.0a1 from 2019-08-08 and I have verified the fix in Nightly v70.0a1 from 2019-08-12.
Leaving bug general status as FIXED and a "qe-verify+" tag until it is verified in Beta, also.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9083850 [details]
Bug 1572197 - Fix world clip region for preserve-3d items with picture caching.

Fixes rendering corruption with preserve-3d when WebRender is enabled. Approved for 69.0b14.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

There are conflicts for the uplift of "Plane split dependency update". Please provide a patch for beta.

Comment 22

[Dzmitry Malyshau [:kvark]]

Problem is that Beta uses the older euclid and plane-splitter. I'm working on a backport of the fix, see https://github.com/servo/plane-split/pull/35

Comment 23

[Dzmitry Malyshau [:kvark]]

Attachment: beta-plane-split-update.patch

This is the same patch applied to Beta, shouldn't need to be reviewed, shouldn't pose any risk.
I launched a try just in case - https://treeherder.mozilla.org/#/jobs?repo=try&revision=daad6f4459ae9bf87262fab240dcf1b64c0e16c2

Comment 24

[Darkspirit]

Uplift of comment 23 is also needed for bug 1557875.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f2b104ade7ce
https://hg.mozilla.org/releases/mozilla-beta/rev/6d6c73623b32

Comment 26

[Daniel Bodea [:danibodea]]

I have also verified this issue on Beta v69.0b14 on Ubuntu 18.04. Thank you.