1529992 - Text shadow is improperly clipped

Status: RESOLVED FIXED

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

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: test.html

Compare attached testcase with and without WR

Comment 1

[Alice0775 White]

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4b17b7e5bd5a582ddb9d1b9e1bb1ed56d41d3cc2&tochange=63049c1db4fc9be3bfd673920dfa1a801da7a56d

Regressed by: 63049c1db4fc Glenn Watson — Bug 1522395 - Fix double inflation of text shadow bounds. r=kvark

Comment 2

[Glenn Watson [:gw]]

This change relies on Gecko already expanding the bounding rect of text runs in a shadow context.

It seems possible that Gecko is not expanding the local clip rect in the same way? Needs more investigation to see if that's the case or something else going on.

Comment 3

[Glenn Watson [:gw]]

Attachment: blur.png

Comment 4

[Glenn Watson [:gw]]

Attachment: scene-1-0.ron

Comment 5

[Glenn Watson [:gw]]

I attached a reduced RON capture file showing the problem, and a screenshot of the minimized case loaded in wrench, adding a highlight where the bounding rect of the shadow element is.

I think it looks like a bug in Gecko, but posting here for someone else to take a look at and see if they reach the same conclusion.

In the PushShadow item, the bounds of the shadow are ((-3, -3), (384, 309)). The clip rect is approximately the same, and the text run display item has the same bounds and clip.

Since should_inflate is false, WR assumes that the bounding rect has been expanded enough to include the content after blurring.

The first glyph is at (-3, -3) + (0, 97). Since the blur_radius is 75 I would expect the bounding rect of the shadow/text item to be significantly larger, since should_inflate is false. I confirmed that setting should_inflate to true, and letting WR expand the bounds does result in the correct rendering (but is less efficient since the bounding box is bigger now).

kats, does that sound right to you?

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I looked at the RON file and it seems to me that the rect/clip on both the PushShadow and Text items should include both the text and the shadow. I'm assuming the (0, 97) position for the first text character refers to the bottom-left corner of the 'h' glyph relative to the content area. If that's the case, then the character extends on the y-axis down to y=97; the shadow is offset by an additional y=75, and has a radius also of 75px. 97+75+75 = 247, which is less than the -3 + 309 bottom of the rect. Similarly, the (-3, -3) top-left corner of the rect should include everything in the top-left corner of the content area, where the shadow is currently being clipped.

Based on this RON file I would guess that there is a semantics mismatch on the offset property of the PushShadow item, in that Gecko is providing a rect/clip that already accounts for the offset, whereas WR is applying the offset again to the rect/clip. I'll try and verify my theory.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

This code looks like it's doing what I described. Maybe removing that (or disabling it for Gecko) would fix the problem. Alternatively, we can conform to the implied semantics there on the Gecko side, by adjusting the clip/rect of the shadow element to take this into account.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)

Maybe removing that (or disabling it for Gecko) would fix the problem.

Tried this; it just made the text shadow appear directly behind the text and ignored the offset entirely. Which I guess is not totally surprising since that's the only place the offset seems to be used.

Alternatively, we can conform to the implied semantics there on the Gecko side, by adjusting the clip/rect of the shadow element to take this into account.

After reading the code a bit more, I don't think that would work either. I suspect the problem is that the ShadowItem::TextRun item's clip/rect is what's wrong, but I'm not sure what the correct value would be. It almost seems like the shadow is drawn using the top-left corner of the translated rect regardless of the blur radius. In which case it might be better to shrink the rects on the Gecko side and let WR do the inflation by setting should_inflate to true.

Comment 9

[Aria Beingessner [:Gankra]]

I know these parts of the code reasonably well, hopefully can finish this up.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Thanks. For posterity here's a link to the IRC discussion that happened between comment 8 and comment 9: https://mozilla.logbot.info/gfx/20190408#c16186450

Comment 11

[Aria Beingessner [:Gankra]]

Ok so this is a weird thing I noticed while refactoring the display list, and seems to be a source of some confusion here: the clip_rect and rect on a PushShadow are currently immediately thrown away. I convinced myself that this is "fine" because any necessary clipping can be fed through the clip node and the backend could figure out the size some other way. Evidently I was mistaken.

On their own, in the semantics I see, nothing but the clip_node should be able to clip the blur of the shadow. The content that is-to-be-blurred gets clipped by an offset clip_rect because we agreed to have clip_rects-in-shadows be treated as part of the item's attempt to draw itself (because we needed some way to handle the way gecko currently uses local clips to draw things like partial ligatures and aligned decorations).

Inflating these rects before the blur isn't correct, because it would reveal content that shouldn't have been drawn (the item itself intentionally clipped that part out). But inflating them to compute the area covered by the shadow might be reasonable? For that I need to check how gecko is doing its inflation.

Comment 12

[Aria Beingessner [:Gankra]]

Attachment: test2.html

ok so after some reading and testing I've determined that shadow expansion is a red herring here. The real issue is that overflow:hidden things like the edge of the screen are ~reasonably incorporated in the calculation of the text's bounds (or perhaps clip_rect?), but this breaks down when we offset those bounds by the shadow's offset. See here an unblurred "fast shadow" which is badly cutoff because it's just translating the edge of the window.

Comment 13

[Aria Beingessner [:Gankra]]

Attachment: Bug 1529992 - don't apply shadow adjustment to text bounds in gecko with WR

Comment 14

[Aria Beingessner [:Gankra]]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c052f8896a3e85b7c4b6ebc679f8e239eb90b3a

Comment 15

[Aria Beingessner [:Gankra]]

Hmm, upon reflection my justification for the applying the blur to mBounds doesn't really make sense to me, but also without it we get bad artifacts on the edges. [sleeps on it]

Comment 16

[Aria Beingessner [:Gankra]]

Argh, this is extra complex for ::selection shadows because they are in a separate path that needs a bunch of initialization but also those shadows should get clipped by the text's "selection box".

Increasingly inclined to think webrender should be accepting bounds on shadows.

Comment 17

[Aria Beingessner [:Gankra]]

Attachment: mBounds.png

Ok so here's where I'm at now.

While messing around with solutions to this bug (shadows getting over-clipped by Webrender) I wrote this reftest:

== https://gankro.github.io/blah/webtests/1529992.html https://gankro.github.io/blah/webtests/1529992-ref.html

Using it, I discovered some issues with mBounds on nsDisplayText, which until now we had been forwarding to webrender directly, but which isn't actually correct for our usecase:

(attached image: the mBounds rect on two pieces of text with highly translated and blurred shadows -- note that the bounds are tight on two sides, and the shadow blurs are badly clipped on exactly those sides!)

mBounds represents the total area covered by the text and its shadows including the shadow offsets. This makes perfect sense for how gecko/invalidation works, but isn't what webrender wants. It just wants the area covered by the text. Webrender has two modes: assume the bounds are inflated for the shadows already, or don't. Previously we ran in the latter mode, we are currently running in the former mode. However, in either case webrender then applies the shadow offsets to those bounds. This has mostly been accidentally working since if there are low offsets or low blurs, it all works out well enough.

However with large shadow offsets and large blurs, we develop artifacts on the sides the shadow isn't on, because those sides are still ~tight against the glyphs, but webrender is translating them to where the shadow is, meaning we are missing the blur expansion on that side. In this regard the "don't assume the bounds are inflated" mode was more correct, because indeed the bounds aren't inflated in the way webrender wants.

But then we also have massively over-inflated bounds on the other side, which leads to extra work for the blur shader, and additional overdraw, because we think actual content might be in there. So we either get really bad overdraw (slow), or incorrect extreme blurs (wrong).

What we really want is mBounds without any acknowledgement of shadows. Then webrender can apply the inflations and offsets on its own terms and get everything nice and tidy.

However even without shadows, mBounds is sometimes extremely large, even if the text is obscured by something (such as an overflow:hidden ancestor or the screen's edge). To early reject some things, mattwoodrow introduced an intersection with the PaintRect of the nsDisplayText. For the non-shadows case this is fine, but with shadows we run into a problem: shadows can offset otherwise hidden glyphs into view. So these intersected bounds need to be inflated to include shadow offsets so hidden glyphs will be drawn (or alternatively: completely abandon the intersection with PaintRect if any shadows are present).

Finally, what I thought was an mBounds bug but wasn't: I have discovered that DisplayListBuilder::PushText is using the MergeClipLeaf optimization on text with ::selection shadows, which isn't supposed to happen for any shadows (understandable: everyone forgets ::selection shadows :)).

Comment 18

[Aria Beingessner [:Gankra]]

kats: on the matter of issue 3 in the above comment: I can't seem to find where we make sure not to apply the MergeClipLeaf when shadows are around. Do you recall?

Context in case you don't recall this issue: clip_rects are translated inside of shadows (to handle painting hacks gecko uses), so smashing clips that exist "outside" the shadow into clip_rects inside of a shadow is incorrect.

Comment 19

[Kartikaya Gupta (email:[e-mail])]

https://searchfox.org/mozilla-central/rev/69ace9da347adcc4a33c6fa3d8e074759b91068c/gfx/layers/wr/ClipManager.cpp#159-161 is the bit that's supposed to skip the MergeClipLeaf thing for text items with shadows. It could be that we're missing additional conditions there.

Comment 20

[Aria Beingessner [:Gankra]]

Attachment: Bug 1529992 - disable the MergeClipLeaf optimization for all shadows properly.

Comment 21

[Aria Beingessner [:Gankra]]

try (need to farm fuzzy params from it) https://treeherder.mozilla.org/#/jobs?repo=try&revision=44f925faec679d9549868b1d2d44667a7b20e62b

Comment 22

[Aria Beingessner [:Gankra]]

oops: https://treeherder.mozilla.org/#/jobs?repo=try&revision=14b8c95a94de4fc89d8f7495459998d86ab4a6e8

Comment 23

[Narcis Beleuzu [:NarcisB]]

:Gankro , patches failed to land :
"We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. (255, 'applying /tmp/tmprBzT73\npatching file layout/generic/nsTextFrame.cpp\nHunk #1 FAILED at 5019\n1 out of 10 hunks FAILED -- saving rejects to file layout/generic/nsTextFrame.cpp.rej\nabort: patch failed to apply', '') "

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddf29d68efb2
don't apply shadow adjustment to text bounds in gecko with WR r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/87b64e169b1b
disable the MergeClipLeaf optimization for all shadows properly. r=kats

Comment 25

[Daniel Varga [:dvarga]]

Backed out 2 changesets (Bug 1529992) for reftest failure at http://10.0.2.2:8854/tests/layout/reftests/bugs/1529992-1.html == http://10.0.2.2:8854/tests/layout/reftests/bugs/1529992-1-ref.html. On a CLOSED TREE

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=240426374&revision=87b64e169b1b4f0962e550b2c5854f5252de7181

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=240422129&repo=autoland&lineNumber=2996

Backout link: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=240426374&revision=81be11ad9dfbd8f137a0db436884f23d6463c0fc

[task 2019-04-15T20:16:26.113Z] 20:16:26 INFO - REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/reftests/bugs/1529992-1.html == http://10.0.2.2:8854/tests/layout/reftests/bugs/1529992-1-ref.html | image comparison, max difference: 153, number of differing pixels: 22558

Comment 26

[Aria Beingessner [:Gankra]]

tests adjusted.

Comment 27

[Cosmin Sabou [:CosminS]]

Alexis, because these diffs have landed and have been closed you need to reopen revisions from the Add action button in the bottom left corner in Lando. Then they will be landable again.

Comment 28

[Aria Beingessner [:Gankra]]

done..?

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/472d0da9b5a6
don't apply shadow adjustment to text bounds in gecko with WR r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/951d2c0a477d
disable the MergeClipLeaf optimization for all shadows properly. r=kats

Comment 30

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/472d0da9b5a6
https://hg.mozilla.org/mozilla-central/rev/951d2c0a477d