Refactor shadow flattening to do work in pop_all_shadows. #3167

gw3583 · 2018-10-04T23:28:24Z

This simplifies how shadow contexts are handled, by deferring the
work of adding shadows and primitives until pop_all_shadows.

Longer explanation below:

Instead of adding a picture primitive, and then adding primitives
to it, the picture API will be modified to take an immutable list
of primitives during construction. This will be part of the changes
to support interning of primitives for picture caching. This patch
allows all the primitives for a shadow to be created before the
shadow picture itself is added / created, which is required to work
with these planned changes.

Additionally, we can now detect if the shadow ended up having no
primitives in it, and skip adding the picture in this case. This
is probably a small win in some cases.

This change is

gw3583 · 2018-10-04T23:29:25Z

r? @gankro

Pending try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c94d12d7017379f652427b658247b959a57fa59c

I think this makes the shadow handling code a bit conceptually simpler too, in addition to being useful for the reasons above.

Gankra

Actual details seem fine, but I'd prefer we remove the contortions to support the unused pattern I discuss in the review.

Gankra · 2018-10-05T00:26:56Z

webrender/src/display_list_flattener.rs

@@ -1207,7 +1183,7 @@ impl<'a> DisplayListFlattener<'a> {
        }

        assert!(
-            self.shadow_stack.is_empty(),
+            self.pending_shadow_items.is_empty(),
            "Found unpopped text shadows when popping stacking context!"


The text of this assertion is out of date

Gankra · 2018-10-05T00:26:56Z

webrender/src/picture.rs

+    pub fn is_empty(&self) -> bool {
+        self.prim_instances.is_empty()
+    }
+


An idle reminder that this sort of thing can go awry if item culling is too aggressive -- e.g. transparent text can be dropped from rendering, but still casts a visible shadow.

(We have to explicitly prevent this optimization in gecko when building the wr DL)

I think that's not relevant in this case - because it's just checking if any primitives are present, and if not, they can't cast a shadow (e.g. if the alpha of the primitives was all zero so they weren't added).

yeah we have several tests that incidentally check that invisible things cast shadows anyway

Gankra · 2018-10-05T00:26:56Z

webrender/src/display_list_flattener.rs

+        //      - Add *any* primitives that remain in the item list to this shadow.
+        // If the item is a primitive:
+        //      - Add that primitive as a normal item (if alpha > 0)
+        //


This all seems a bit heavy-handed to explicitly support the (never emitted, and quite frankly low-value) pattern the display-list format incidentally implies of [shadow, item, shadow, item, pop-all]. I would prefer that we just write if off and do:

shadows: Vec<Shadow>, shadow_prims: Vec<PendingItem>,

fn add_shadow(..) { assert!(shadow_prims.is_empty(), "shadows and items being interleaved isn't supported"); ... }

(it's possible I checked in a test that checks this pattern works but i'm fully convinced that it's not worth our time anymore)

I'm fine with doing that, although hesitant to change that now without testing servo as well. Would you be fine with landing it like this and I'll file an issue to simplify this API in the future?

I'm fairly certain servo wouldn't emit it, but yeah I guess sure, I can do the followup to unblock you.

Gankra · 2018-10-05T00:26:56Z

webrender/src/display_list_flattener.rs

        let pipeline_id = self.sc_stack.last().unwrap().pipeline_id;
        let max_clip = LayoutRect::max_rect();
+        let mut items = mem::replace(&mut self.pending_shadow_items, VecDeque::new());


Just ~18 more weeks until NLL!!!

Gankra · 2018-10-05T00:26:56Z

webrender/src/display_list_flattener.rs

+                            // Add the new primitive to the shadow picture.
+                            shadow_pic.add_primitive(shadow_prim_index);
+                        }
+                    }


blindly assuming this block is a cut-paste from add_primitive

Yep, it is.


        Refactor shadow flattening to do work in pop_all_shadows.

This simplifies how shadow contexts are handled, by deferring the work of adding shadows and primitives until pop_all_shadows. Longer explanation below: Instead of adding a picture primitive, and then adding primitives to it, the picture API will be modified to take an immutable list of primitives during construction. This will be part of the changes to support interning of primitives for picture caching. This patch allows all the primitives for a shadow to be created before the shadow picture itself is added / created, which is required to work with these planned changes. Additionally, we can now detect if the shadow ended up having no primitives in it, and skip adding the picture in this case. This is probably a small win in some cases.

gw3583 · 2018-10-05T01:52:11Z

OK, try run looks good. There is one failure, but that's due to the revert of the pre/post-translate patch by @staktrace .

Gankra · 2018-10-05T02:05:39Z

@bors-servo r+

bors-servo · 2018-10-05T02:05:39Z

📌 Commit 90f3d14 has been approved by Gankro

bors-servo · 2018-10-05T02:05:42Z

⌛ Testing commit 90f3d14 with merge fb1705b...


        Auto merge of #3167 - gw3583:shadow-collect, r=Gankro

Refactor shadow flattening to do work in pop_all_shadows. This simplifies how shadow contexts are handled, by deferring the work of adding shadows and primitives until pop_all_shadows. Longer explanation below: Instead of adding a picture primitive, and then adding primitives to it, the picture API will be modified to take an immutable list of primitives during construction. This will be part of the changes to support interning of primitives for picture caching. This patch allows all the primitives for a shadow to be created before the shadow picture itself is added / created, which is required to work with these planned changes. Additionally, we can now detect if the shadow ended up having no primitives in it, and skip adding the picture in this case. This is probably a small win in some cases.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3167)

gw3583 · 2018-10-05T02:16:51Z

@gankro Sounds good, thanks!

bors-servo · 2018-10-05T02:59:05Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: Gankro
Pushing fb1705b to master...

Gankra suggested changes Oct 5, 2018

View changes

gw3583 force-pushed the gw3583:shadow-collect branch from c81a684 to 90f3d14 Oct 5, 2018

bors-servo merged commit 90f3d14 into servo:master Oct 5, 2018
3 checks passed

3 checks passed

Taskcluster (pull_request) TaskGroup: success
Details

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

homu Test successful
Details

bors-servo mentioned this pull request Oct 5, 2018

Revised backface visibility and 3D plane ordering #3131

Merged

gw3583 mentioned this pull request Oct 5, 2018

Simplify shadow APIs #3168

Open

servo / webrender

Refactor shadow flattening to do work in pop_all_shadows. #3167

Refactor shadow flattening to do work in pop_all_shadows. #3167

gw3583 commented Oct 4, 2018 •

edited by larsbergstrom

gw3583 commented Oct 4, 2018

Gankra left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gw3583 commented Oct 5, 2018

Gankra commented Oct 5, 2018

bors-servo commented Oct 5, 2018

bors-servo commented Oct 5, 2018

gw3583 commented Oct 5, 2018

bors-servo commented Oct 5, 2018

servo / webrender

Join GitHub today

Refactor shadow flattening to do work in pop_all_shadows. #3167

Refactor shadow flattening to do work in pop_all_shadows. #3167

Conversation

gw3583 commented Oct 4, 2018 • edited by larsbergstrom

gw3583 commented Oct 4, 2018

Gankra left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gw3583 commented Oct 5, 2018

Gankra commented Oct 5, 2018

bors-servo commented Oct 5, 2018

bors-servo commented Oct 5, 2018

gw3583 commented Oct 5, 2018

bors-servo commented Oct 5, 2018

gw3583 commented Oct 4, 2018 •

edited by larsbergstrom