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

RESOLVED FIXED in Firefox 69

Status

()

defect
P2
normal
RESOLVED FIXED
Last year
4 days ago

People

(Reporter: acupsa, Assigned: kvark)

Tracking

(Blocks 1 bug)

63 Branch
mozilla69
Unspecified
All
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox-esr60 disabled, firefox-esr68 disabled, firefox63 disabled, firefox64 disabled, firefox67 wontfix, firefox68 wontfix, firefox69 fixed)

Details

Attachments

(7 attachments)

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:
Priority: -- → P1
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.
Priority: P1 → P2
Priority: P2 → P3
Can this still be reproduced?
Flags: needinfo?(andreea.cupsa)
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.
Flags: needinfo?(andreea.cupsa)
OS: Windows 10 → All
Summary: [WebRender Shield Study] Specific images entirely coded in HTML & CSS are not correctly rendered with WebRender enabled → (francine's sister) [WebRender Shield Study] Specific images entirely coded in HTML & CSS are not correctly rendered with WebRender enabled
Summary: (francine's sister) [WebRender Shield Study] Specific images entirely coded in HTML & CSS are not correctly rendered with WebRender enabled → (francine's sister correctness) [WebRender Shield Study] Specific images entirely coded in HTML & CSS are not correctly rendered with WebRender enabled
Looks like the problem here is a mix of perspective transforms and box shadow.
Summary: (francine's sister correctness) [WebRender Shield Study] Specific images entirely coded in HTML & CSS are not correctly rendered with WebRender enabled → perspective transforms and box shadow from css orange doesn't draw correctly
Assignee: nobody → emilio
Much likely this is the same as bug 1512537, which I said I was going to look at :-)
Depends on: 1512537
Emilio, bug 1512537 is fixed but this still shows up. Thoughts?
Flags: needinfo?(emilio)
Will take a look when I'm back from PTO on monday.

Also looking at this...

stealing since :emilio is busy with more important stuff atm

Assignee: emilio → dmalyshau
Flags: needinfo?(emilio)

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

Priority: P3 → P4

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
Blocks: wr-67
No longer blocks: stage-wr-trains
Priority: P4 → P2
Blocks: wr-68
No longer blocks: wr-67
Assignee: dmalyshau → a.beingessner
Posted patch test.yamlSplinter Review

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

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.

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.

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.

Assignee: a.beingessner → dmalyshau
Flags: needinfo?(gwatson)

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

Flags: needinfo?(gwatson)

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

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

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.

Flags: needinfo?(a.beingessner)
Attachment #9070370 - Attachment description: [WIP] Box shadow interpolation Wrench test by Gankro → [WIP] Perspective clip interpolation fix

updated

Flags: needinfo?(a.beingessner)
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b50cc2768579
Add reftest for a box-shadow in perspective transform without preserve-3d. r=kvark
Attachment #9070370 - Attachment description: [WIP] Perspective clip interpolation fix → Perspective clip interpolation fix
Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/24f1d034783e
Perspective clip interpolation fix r=gw

a follow-up to the actual fix and the reftest

Pushed by dmalyshau@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05f479965d04
Enable WR perspective box shadow reftest r=Gankro
Status: NEW → RESOLVED
Closed: 27 days ago
Resolution: --- → FIXED

Is this something we should uplift?

Target Milestone: --- → mozilla69

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

Is this something we should uplift?

Flags: needinfo?(dmalyshau)

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

Flags: needinfo?(dmalyshau)
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.