Closed Bug 1572197 Opened Last month Closed Last month

Egencia itinerary doesn't paint properly when scrolling

Categories

(Core :: Graphics: WebRender, defect)

Desktop
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla70
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- verified
firefox70 --- verified

People

(Reporter: emilio, Assigned: gw)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: correctness, regression)

Attachments

(5 files)

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.

Flags: needinfo?(emilio)

68 is good.

Keywords: regression

Regression from bug 1566901.

Regressed by: 1566901
Attached image Screenshot.

Looks pretty busted.

Blocks: wr-69
Has Regression Range: --- → yes
Keywords: correctness
OS: Unspecified → Linux
Hardware: Unspecified → Desktop

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.

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.

Flags: needinfo?(emilio) → needinfo?(gwatson)

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

Assignee: nobody → gwatson
Flags: needinfo?(gwatson)
OS: Linux → All

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

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.

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

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?

Flags: needinfo?(dmalyshau)

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.

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

Flags: needinfo?(dmalyshau)

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

Keywords: leave-open
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e6b8cdf8a722
Fix world clip region for preserve-3d items with picture caching. r=emilio
Status: NEW → RESOLVED
Closed: Last month
Resolution: --- → FIXED
Target Milestone: --- → mozilla70

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:
Attachment #9083850 - Flags: approval-mozilla-beta?
Attachment #9084319 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Flags: qe-verify+

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.

Attachment #9083850 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9084319 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Flags: needinfo?(dmalyshau)

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

Flags: needinfo?(dmalyshau)

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

Uplift of comment 23 is also needed for bug 1557875.

Flags: needinfo?(aryx.bugmail)
Flags: needinfo?(aryx.bugmail)

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.