Closed Bug 1474294 Opened 3 years ago Closed 2 years ago

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

Categories

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

63 Branch
Unspecified
All
defect

Tracking

()

VERIFIED FIXED
mozilla69
Tracking Status
firefox-esr60 --- disabled
firefox-esr68 --- disabled
firefox63 --- disabled
firefox64 --- disabled
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- verified

People

(Reporter: acupsa, Assigned: kvark)

References

Details

Attachments

(7 files)

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:
See Also: → 1459187
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
Blocks: stage-wr-trains
No longer blocks: stage-wr-nightly
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.
Attached video 2018-09-28_20-50-41.mp4
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
Attached 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: 2 years 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+

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.

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