Draw text shadows in screen space. #2540

glennw · 2018-03-19T06:49:51Z

With box shadows drawn via a clip source, the only picture type
that was drawing in local space was text shadows. Switching them
to draw in screen-space simplifies a lot of code, and also gives
a better quality result.

Specifically:

Remove ContentOrigin enum.
Remove cs_text_run shader (use standard text shader).
Remove cache text run hash maps.
Remove local-space raster mode from brush primitive.
Don't fetch color from picture for text runs.
Allow primitive culling on all Picture types.
Optimize allocated region for text-shadows.

There's one performance regression with this change - text shadows
which have large rotations will blur more pixels than previously.
This can be fixed in a follow up, by drawing the blur as a normal
primitive instead of a 2D rect. This will also optimize other blurs,
such as CSS filters.

This is preparation work for caching text shadows, and also some
other optimizations (such as drawing text decorations as clip
sources).

This change is

glennw · 2018-03-19T06:52:32Z

This is not quite ready for merge yet - it can be reviewed if you have time, or can wait until the remaining work below is complete:

One failure in try run to fix (https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa2a7084b1a32acb6edaff20eb8f19531d007022).
Update wrench reftest images once the try failure above is fixed.
Add a couple of wrench reftests with rotated and perspective text-shadow elements.

r? @kvark

kvark · 2018-03-19T19:11:09Z

Holy crab, this is indeed simplifying quite a bit of code!
I do worry though, why did you decide to spin 180 w.r.t. local-space render tasks? Didn't you put a ton of effort in a series of PRs during the past 2-3 months to support those properly? My understanding was that we want to have an ability to do this trade-off, based on how frequent are the local versus global space changes.

Reviewed 11 of 11 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

glennw · 2018-03-20T01:20:28Z

I fixed the failure in R1 (disabling the local clip rect inside the text-shadow picture context).

Latest try is here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c46f56f830590a4e87bdc608138dcaee883333b&selectedJob=169088199

There's a couple of intermittent failures I haven't seen before, but I don't think any of them are related to this change. It'd be good to get @staktrace to confirm though. The Windows tests are still pending.

Apart from that, this should be ready for review / merge now, r? @kvark

glennw · 2018-03-20T02:19:55Z

I re-triggered each of the orange results, and they passed - so they do appear to be intermittent failures. I'm fairly confident they are all unrelated to this PR. The Windows tests also look green. 🎉

staktrace · 2018-03-20T13:02:15Z

Try push looks good to me!

glennw · 2018-03-20T20:34:44Z

r? @mrobinson

mrobinson

Looks good to me. This seems like a nice simplification.

mrobinson · 2018-03-21T08:28:29Z

webrender/src/display_list_flattener.rs

@@ -1552,13 +1559,13 @@ impl<'a> DisplayListFlattener<'a> {
            self.shadow_prim_stack[idx].1.push((prim_index, clip_and_scroll));
        }

-        let prim_index = self.create_primitive(


Hrm. Out of curiosity, does this change allow for making this optimization or is it an unrelated change?

This change allows it - previously we would reference the same primitive in the slow shadow path below. Now we build a separate primitive there. There's a slight memory overhead to this for slow text shadows, which we might be able to improve on in the future, but it should be minor.

glennw · 2018-03-21T20:13:10Z

@bors-servo r=mrobinson

bors-servo · 2018-03-21T20:13:11Z

📌 Commit 660f871 has been approved by mrobinson

bors-servo · 2018-03-21T20:13:14Z

⌛ Testing commit 660f871 with merge 9747cd0...

Draw text shadows in screen space. With box shadows drawn via a clip source, the only picture type that was drawing in local space was text shadows. Switching them to draw in screen-space simplifies a lot of code, and also gives a better quality result. Specifically: * Remove ContentOrigin enum. * Remove cs_text_run shader (use standard text shader). * Remove cache text run hash maps. * Remove local-space raster mode from brush primitive. * Don't fetch color from picture for text runs. * Allow primitive culling on all Picture types. * Optimize allocated region for text-shadows. There's one performance regression with this change - text shadows which have large rotations will blur more pixels than previously. This can be fixed in a follow up, by drawing the blur as a normal primitive instead of a 2D rect. This will also optimize other blurs, such as CSS filters. This is preparation work for caching text shadows, and also some other optimizations (such as drawing text decorations as clip sources).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2540)

bors-servo · 2018-03-21T20:34:05Z

💔 Test failed - status-taskcluster

glennw · 2018-03-21T20:49:10Z

@bors-servo r=mrobinson

bors-servo · 2018-03-21T20:49:10Z

📌 Commit e508f86 has been approved by mrobinson

bors-servo · 2018-03-21T20:49:14Z

⌛ Testing commit e508f86 with merge 14bb805...

Draw text shadows in screen space. With box shadows drawn via a clip source, the only picture type that was drawing in local space was text shadows. Switching them to draw in screen-space simplifies a lot of code, and also gives a better quality result. Specifically: * Remove ContentOrigin enum. * Remove cs_text_run shader (use standard text shader). * Remove cache text run hash maps. * Remove local-space raster mode from brush primitive. * Don't fetch color from picture for text runs. * Allow primitive culling on all Picture types. * Optimize allocated region for text-shadows. There's one performance regression with this change - text shadows which have large rotations will blur more pixels than previously. This can be fixed in a follow up, by drawing the blur as a normal primitive instead of a 2D rect. This will also optimize other blurs, such as CSS filters. This is preparation work for caching text shadows, and also some other optimizations (such as drawing text decorations as clip sources).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2540)

bors-servo · 2018-03-21T21:32:12Z

💔 Test failed - status-taskcluster

glennw · 2018-03-22T00:21:29Z

Three tests fail on mac taskcluster.

I confirmed that one of them is a max difference of 1, so added fuzziness for that.

I confirmed that the other two are fixed when #2554 is applied locally. In the interests of getting this landed, I disabled those two tests on mac (they still run on Linux) for now. Once #2554 lands, we can re-enable those tests on mac.

@bors-servo r=mrobinson

bors-servo · 2018-03-22T00:21:29Z

📌 Commit 785d1af has been approved by mrobinson

bors-servo · 2018-03-22T00:21:33Z

⌛ Testing commit 785d1af with merge 769acd6...

Draw text shadows in screen space. With box shadows drawn via a clip source, the only picture type that was drawing in local space was text shadows. Switching them to draw in screen-space simplifies a lot of code, and also gives a better quality result. Specifically: * Remove ContentOrigin enum. * Remove cs_text_run shader (use standard text shader). * Remove cache text run hash maps. * Remove local-space raster mode from brush primitive. * Don't fetch color from picture for text runs. * Allow primitive culling on all Picture types. * Optimize allocated region for text-shadows. There's one performance regression with this change - text shadows which have large rotations will blur more pixels than previously. This can be fixed in a follow up, by drawing the blur as a normal primitive instead of a 2D rect. This will also optimize other blurs, such as CSS filters. This is preparation work for caching text shadows, and also some other optimizations (such as drawing text decorations as clip sources).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2540)

bors-servo · 2018-03-22T00:58:42Z

💔 Test failed - status-taskcluster

With box shadows drawn via a clip source, the only picture type that was drawing in local space was text shadows. Switching them to draw in screen-space simplifies a lot of code, and also gives a better quality result. Specifically: * Remove ContentOrigin enum. * Remove cs_text_run shader (use standard text shader). * Remove cache text run hash maps. * Remove local-space raster mode from brush primitive. * Don't fetch color from picture for text runs. * Allow primitive culling on all Picture types. * Optimize allocated region for text-shadows. There's one performance regression with this change - text shadows which have large rotations will blur more pixels than previously. This can be fixed in a follow up, by drawing the blur as a normal primitive instead of a 2D rect. This will also optimize other blurs, such as CSS filters. This is preparation work for caching text shadows, and also some other optimizations (such as drawing text decorations as clip sources).

glennw · 2018-03-22T01:10:13Z

@bors-servo r=mrobinson

bors-servo · 2018-03-22T01:10:14Z

📌 Commit 1d65037 has been approved by mrobinson

bors-servo · 2018-03-22T01:10:17Z

⌛ Testing commit 1d65037 with merge b954383...

Draw text shadows in screen space. With box shadows drawn via a clip source, the only picture type that was drawing in local space was text shadows. Switching them to draw in screen-space simplifies a lot of code, and also gives a better quality result. Specifically: * Remove ContentOrigin enum. * Remove cs_text_run shader (use standard text shader). * Remove cache text run hash maps. * Remove local-space raster mode from brush primitive. * Don't fetch color from picture for text runs. * Allow primitive culling on all Picture types. * Optimize allocated region for text-shadows. There's one performance regression with this change - text shadows which have large rotations will blur more pixels than previously. This can be fixed in a follow up, by drawing the blur as a normal primitive instead of a 2D rect. This will also optimize other blurs, such as CSS filters. This is preparation work for caching text shadows, and also some other optimizations (such as drawing text decorations as clip sources).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2540)

bors-servo · 2018-03-22T01:48:25Z

☀️ Test successful - status-appveyor, status-taskcluster, status-travis
Approved by: mrobinson
Pushing b954383 to master...

glennw changed the title ~~Draw text shadows in screen space.~~ [WIP] Draw text shadows in screen space. Mar 19, 2018

glennw force-pushed the text-shadow-screen2 branch 5 times, most recently from 154bcc0 to 660f871 Compare March 20, 2018 00:48

glennw changed the title ~~[WIP] Draw text shadows in screen space.~~ Draw text shadows in screen space. Mar 20, 2018

glennw mentioned this pull request Mar 21, 2018

Remove brush_line, convert line decorations to a clip source. #2552

Merged

mrobinson approved these changes Mar 21, 2018

View reviewed changes

glennw force-pushed the text-shadow-screen2 branch from 660f871 to e508f86 Compare March 21, 2018 20:48

glennw force-pushed the text-shadow-screen2 branch from e508f86 to 785d1af Compare March 22, 2018 00:19

glennw force-pushed the text-shadow-screen2 branch from 785d1af to 1d65037 Compare March 22, 2018 01:09

bors-servo merged commit 1d65037 into servo:master Mar 22, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draw text shadows in screen space. #2540

Draw text shadows in screen space. #2540

glennw commented Mar 19, 2018 •

edited by larsbergstrom

Loading

glennw commented Mar 19, 2018 •

edited

Loading

kvark commented Mar 19, 2018

glennw commented Mar 20, 2018

glennw commented Mar 20, 2018

staktrace commented Mar 20, 2018

glennw commented Mar 20, 2018

mrobinson left a comment

mrobinson Mar 21, 2018

glennw Mar 21, 2018

glennw commented Mar 21, 2018

bors-servo commented Mar 21, 2018

bors-servo commented Mar 21, 2018

bors-servo commented Mar 21, 2018

glennw commented Mar 21, 2018

bors-servo commented Mar 21, 2018

bors-servo commented Mar 21, 2018

bors-servo commented Mar 21, 2018

glennw commented Mar 22, 2018

bors-servo commented Mar 22, 2018

bors-servo commented Mar 22, 2018

bors-servo commented Mar 22, 2018

glennw commented Mar 22, 2018

bors-servo commented Mar 22, 2018

bors-servo commented Mar 22, 2018

bors-servo commented Mar 22, 2018

Draw text shadows in screen space. #2540

Draw text shadows in screen space. #2540

Conversation

glennw commented Mar 19, 2018 • edited by larsbergstrom Loading

glennw commented Mar 19, 2018 • edited Loading

kvark commented Mar 19, 2018

glennw commented Mar 20, 2018

glennw commented Mar 20, 2018

staktrace commented Mar 20, 2018

glennw commented Mar 20, 2018

mrobinson left a comment

Choose a reason for hiding this comment

mrobinson Mar 21, 2018

Choose a reason for hiding this comment

glennw Mar 21, 2018

Choose a reason for hiding this comment

glennw commented Mar 21, 2018

bors-servo commented Mar 21, 2018

bors-servo commented Mar 21, 2018

bors-servo commented Mar 21, 2018

glennw commented Mar 21, 2018

bors-servo commented Mar 21, 2018

bors-servo commented Mar 21, 2018

bors-servo commented Mar 21, 2018

glennw commented Mar 22, 2018

bors-servo commented Mar 22, 2018

bors-servo commented Mar 22, 2018

bors-servo commented Mar 22, 2018

glennw commented Mar 22, 2018

bors-servo commented Mar 22, 2018

bors-servo commented Mar 22, 2018

bors-servo commented Mar 22, 2018

glennw commented Mar 19, 2018 •

edited by larsbergstrom

Loading

glennw commented Mar 19, 2018 •

edited

Loading