1474294 - perspective transforms and box shadow from css orange doesn't draw correctly

Status: VERIFIED FIXED

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

Description

[Andreea Cupsa [:acupsa], Experiments QA]

Attachment: Recording of the issue.gif

Version: 63.0a1
Build ID: 20180708220048
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

[Affected Platforms]:
- Windows 10 64bit

[Prerequisites]
- Have WebRender enabled (gfx.webrender.all.qualified preference with true value).

[Steps to reproduce]:
1. Open Firefox Nightly (63.0a1) and navigate to cyanharlow.github.io/purecss-vignes 
2. Observe the rendered image.

[Expected result]:
- The image is correctly rendered.

[Actual result]:
- A part of the image is not correctly rendered.

[Notes]:
- Also reproducible with http://diana-adrianne.com/purecss-zigario/
- While resizing browser window or performing any action in the tab, creates browser lag.
- This issue is not reproducible with WebRender disabled.
- Attached a screenshot of the issue:

Comment 1

[Jeff Muizelaar [:jrmuizel]]

It would be nice to have a reduced test case of this brokenness. I've filed https://github.com/servo/webrender/issues/2881 about the broken rendering. https://github.com/servo/webrender/issues/2715 tracks the performance problem.

The brokenness here shouldn't be common enough for us to block the shield study.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Can this still be reproduced?

Comment 3

[Darkspirit]

The issue from attachment 8990701 [details] has likely been fixed by https://github.com/servo/webrender/pull/3073.

https://hg.mozilla.org/integration/mozilla-inbound/log/14c6b338e32c

last bad: mozregression --repo mozilla-inbound --launch a1a0f861a0ae --pref gfx.webrender.all:true -a http://diana-adrianne.com/purecss-vignes/

first good: mozregression --repo mozilla-inbound --launch 14c6b338e32c --pref gfx.webrender.all:true -a http://diana-adrianne.com/purecss-vignes/


There is one issue left: Half of the orange is a solid color at most zoom levels.

Comment 4

[Darkspirit]

Attachment: 2018-09-28_20-50-41.mp4

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Attachment: Reduced test case of some of it

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Looks like the problem here is a mix of perspective transforms and box shadow.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Much likely this is the same as bug 1512537, which I said I was going to look at :-)

Comment 8

[Jeff Muizelaar [:jrmuizel]]

Emilio, bug 1512537 is fixed but this still shows up. Thoughts?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Will take a look when I'm back from PTO on monday.

Comment 10

[Dzmitry Malyshau [:kvark]]

Also looking at this...

Comment 11

[Dzmitry Malyshau [:kvark]]

stealing since :emilio is busy with more important stuff atm

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Dzmitry has debugged this enough to understand that it is rare. Demoting to P4

Comment 13

[Dzmitry Malyshau [:kvark]]

I think the issue comes from the way we interpolate clip UV coordinates for the case where the clip space is perspective-transformed. Our current approach involves special code for 2D un-projection of a local point into the clip plane ("get_node_pos" in shaders), and one of the following may be happening:

1. the un-projection code doesn't work with perspective: it assumes the space is just a rotated/scaled plane, which isn't exactly the case here
2. these UV coordinates aren't interpolated with perspective correction

Comment 14

[Aria Beingessner [:Gankra]]

Attachment: test.yaml

Attaching a simple yaml demonstrating how we mess up box-shadows in perspective so I can easily run this on different machines.

Comment 15

[Aria Beingessner [:Gankra]]

Attachment: Bug 1474294 - Add reftest for a box-shadow in perspective transform without preserve-3d.

Comment 16

[Aria Beingessner [:Gankra]]

Ok so dug through this a bit and we seem to have figured this out. Basically, we are currently overly relying on "preserve-3d" to force us to render things in local space / intermediates. Notably, the "CS" family of shaders aren't supposed to be run with perspective -- they're supposed to be drawn in local space and then have the perspective applied with brush_image.

The case in question draws a box shadow inside a perspective transform without any preserve_3ds. The pipeline then concludes nothing needs to be composited and we get messed up shadows. The attached test-case demonstrates that just adding a preserve-3d node fixes everything. But we can't rely on users adding these, so we need to add some more robust automatic detection.

kvark has suggested this should be done when we're allocating render tasks.

Comment 17

[Dzmitry Malyshau [:kvark]]

To clarify on my suggestion, in the render task chain Primitive -> BoxShadow -> Output, when there is perspective transformation involved we are currently doing the transform in Primitive -> BoxShadow step, which is done via a "cs_" shader that doesn't support perspective, while BoxShadow -> Output is done via an image (or blend) brush without transformation. Instead, we should be doing Primitive -> BoxShadow without transformation and then apply the perspective to the image/blend shader in the second step.

Comment 18

[Aria Beingessner [:Gankra]]

handing back to kvark since the fix is too subtle for my understanding of these subsystems.

Just to add extra information that has led to some confusion:

:gw asserted that the cs_clip family of shaders should be handling this case correctly. The shader does seem to be partially perspective aware, just not perspective-interpolation aware, in the BRUSH_FLAG_PERSPECTIVE_INTERPOLATION sense.

So this can be either regarded as a bug that we're letting perspective get all the way to this shader, or that the shader is failing to handle perspective interpolation.

ni?gw just to get their two cents on which is the "right" fix.

Comment 19

[Dzmitry Malyshau [:kvark]]

lemme look at it on my (magical) machine first before ni? gw

Comment 20

[Dzmitry Malyshau [:kvark]]

Attachment: Perspective clip interpolation fix

Set up position W of clip rendering,
such that everything is perspective-interpolated in clip space.

Comment 21

[Aria Beingessner [:Gankra]]

Did you mean to only attach the reftest with no code changes?

Comment 22

[Dzmitry Malyshau [:kvark]]

I actually wanted to attach the code only, but thought it could cause issues if we land the test case separately. It would be great if you could land that reftest disabled, so that I can just flip the switch in my PR.

Comment 23

[Aria Beingessner [:Gankra]]

updated

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b50cc2768579
Add reftest for a box-shadow in perspective transform without preserve-3d. r=kvark

Comment 25

[Andreea Pavel [:apavel]]

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

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24f1d034783e
Perspective clip interpolation fix r=gw

Comment 27

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/24f1d034783e

Comment 28

[Dzmitry Malyshau [:kvark]]

Attachment: Enable WR perspective box shadow reftest

a follow-up to the actual fix and the reftest

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/05f479965d04
Enable WR perspective box shadow reftest r=Gankro

Comment 30

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/05f479965d04

Comment 31

[Julien Cristau [:jcristau]]

Is this something we should uplift?

Comment 32

[Julien Cristau [:jcristau]]

(In reply to Julien Cristau [:jcristau] from comment #31)

Is this something we should uplift?

Comment 33

[Dzmitry Malyshau [:kvark]]

Julien,
It could be, but I don't think it manifests a lot on real pages, so there is no urgency to ship it.

Comment 34

[Julien Cristau [:jcristau]]

Thanks!

Comment 35

[Alexandru Trif, Desktop QA [:atrif]]

Hello!
Reproduced the issue using Firefox 63.0a1 (20180709221247) on Windows 10x64.
The issue is verified fixed with Firefox 69.0b8 (20190723212625) on Windows 10.x64, macOS 10.14 and Ubuntu 18.04, the image is rendered as expected while using WebRender.