Remove ClipScrollGroup and PackedLayer. #1981

glennw · 2017-11-02T07:46:27Z

Instead, we provide access to both the clip node and scroll
node for a given primitive in the shader. The shader then
calculated the appropriate local clip rect.

There's a few hacks in here - for example, the way that
I pack the clip/scroll IDs into a single u32 for the
shaders. In the future, we'll be switching the vertex formats
to work on u16 size data fields, so this will become a lot
simpler then.

This change is

glennw · 2017-11-02T07:47:25Z

r? @mrobinson

This is not quite ready for merge yet - there are a couple of failures in the Gecko try and and a couple of failures in the Servo WPT suite I need to fix. But I think those should be fairly minor fixes, so this is probably ready for review.

Gecko try:

https://hg.mozilla.org/try/rev/933c76557503d46ff73501b31ebbda5be1eefe30
https://treeherder.mozilla.org/#/jobs?repo=try&revision=933c76557503d46ff73501b31ebbda5be1eefe30

glennw · 2017-11-02T07:48:32Z

This simplifies a lot of the CPU code that deals with primitives, and makes it feasible to work on the changes proposed in #1774.

mrobinson · 2017-11-02T10:29:19Z

@gw I absolutely love this change. I've taken a quick look and don't see any obvious problems, but I'll do a more thorough review today.

mrobinson · 2017-11-02T11:14:12Z

Okay. I have one question about the ClipScrollNodeData. It seems that it is associated to primitives via their assigned scroll node, yet it contains information about the local clip rect. The local clip rect is a property of the primitive's assigned clip node. Perhaps that is why you are seeing failures on Gecko.

glennw · 2017-11-03T00:46:34Z

@mrobinson OK, there are three small fix-up commits added. With those changes, all WR / Servo / Gecko tests pass.

Gecko try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=844bc16b48eea4096461c405e96547755590517b

mrobinson

This looks great. I left a couple nit comments, but I still love this PR.

mrobinson · 2017-11-03T12:04:12Z

webrender/src/clip.rs

-        // All the coordinates we receive are relative to the stacking context, but we want
-        // to convert them to something relative to the origin of the clip.
-        let negative_origin = -rect.origin.to_vector();
+        rect = rect.translate(reference_frame_relative_offset);


This could use reassignment and you will be able to avoid making this variable mutable.

mrobinson · 2017-11-03T12:04:12Z

webrender/src/clip.rs

        let complex_clips = match local_clip {
            &LocalClip::Rect(_) => Vec::new(),
            &LocalClip::RoundedRect(_, ref region) => vec![region.clone()],
        };
-        ClipRegion::create_for_clip_node(*local_clip.clip_rect(), complex_clips, None)
+        ClipRegion::create_for_clip_node(*local_clip.clip_rect(), complex_clips, None, reference_frame_relative_offset)


Nit: This line is longer than 100 characters.

mrobinson · 2017-11-03T12:04:12Z

webrender/src/gpu_types.rs

+
+#[derive(Copy, Debug, Clone)]
+#[repr(C)]
+pub struct ClipScrollNodeId(pub u32);


Maybe this should be called ClipScrollNodeIndex to avoid confusing it with the ClipId and also to express that it is an index into the array of ClipScrollNodeData.

mrobinson · 2017-11-03T12:04:12Z

webrender/src/clip_scroll_node.rs

+            None => {
+                state.combined_outer_clip_bounds = DeviceIntRect::zero();
+
+                ClipScrollNodeData {


I think it would be good to add an empty() factory on ClipScrollNodeData to make this code a bit clearer.

glennw · 2017-11-05T21:07:22Z

@mrobinson Thanks, fixed those comments up in the last commit. Once you review that change, I'll squash before merge.

mrobinson

Looks good to me!


        Remove ClipScrollGroup and PackedLayer.

Instead, we provide access to both the clip node and scroll node for a given primitive in the shader. The shader then calculated the appropriate local clip rect. There's a few hacks in here - for example, the way that I pack the clip/scroll IDs into a single u32 for the shaders. In the future, we'll be switching the vertex formats to work on u16 size data fields, so this will become a lot simpler then.

glennw · 2017-11-05T21:48:13Z

Squashed fixup commits.

@bors-servo r=mrobinson

bors-servo · 2017-11-05T21:48:13Z

📌 Commit 5bdcb95 has been approved by mrobinson


        Auto merge of #1981 - glennw:remove-clip-scroll, r=mrobinson

Remove ClipScrollGroup and PackedLayer. Instead, we provide access to both the clip node and scroll node for a given primitive in the shader. The shader then calculated the appropriate local clip rect. There's a few hacks in here - for example, the way that I pack the clip/scroll IDs into a single u32 for the shaders. In the future, we'll be switching the vertex formats to work on u16 size data fields, so this will become a lot simpler then.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1981)

bors-servo · 2017-11-05T21:48:17Z

⌛ Testing commit 5bdcb95 with merge bb73014...

bors-servo · 2017-11-05T22:56:58Z

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

kvark

This is one long awaited and heck of an awesome change 👍
One thing that's missing is a bit of measurement of the performance impact.

kvark · 2017-11-06T18:56:46Z

webrender/res/prim_shared.glsl

+};
+
+Layer fetch_layer(int clip_node_id, int scroll_node_id) {
+    ClipScrollNode clip_node = fetch_clip_scroll_node(clip_node_id);


we should have a fast path for clip_node_id == scroll_node_id instead of fetching the same data again

kvark · 2017-11-06T18:56:46Z

webrender/res/prim_shared.glsl

+
+struct Layer {
+    mat4 transform;
+    mat4 inv_transform;


we should gate the inv_transform fetches with the WR_FEATURE_TRANSFORM to save on fetches for a more popular (aligned) case

kvark · 2017-11-06T18:56:46Z

webrender/res/prim_shared.glsl

+};
+
+Layer fetch_layer(int clip_node_id, int scroll_node_id) {
+    ClipScrollNode clip_node = fetch_clip_scroll_node(clip_node_id);


also note: the clip_note.inv_transform is not used here at all, so we shouldn't fetch it

kvark · 2017-11-06T18:56:46Z

webrender/src/clip_scroll_node.rs

+
+        // Store coord system ID, and also the ID used for shaders to reference this node.
+        self.coordinate_system_id = state.current_coordinate_system_id;
+        self.id = ClipScrollNodeIndex(node_data.len() as u32);


might be useful to have the name of this type reflecting the fact it's for GPU/shader consumption only

kvark · 2017-11-06T18:56:46Z

webrender/src/frame_builder.rs

@@ -113,11 +113,6 @@ pub struct FrameBuilder {
    pub config: FrameBuilderConfig,

    stacking_context_store: Vec<StackingContext>,
-    clip_scroll_group_store: Vec<ClipScrollGroup>,


glennw force-pushed the glennw:remove-clip-scroll branch from fd552ea to 9f43586 Nov 2, 2017

glennw force-pushed the glennw:remove-clip-scroll branch from b46ba8c to b5f4a7b Nov 3, 2017

mrobinson approved these changes Nov 3, 2017

View changes

glennw force-pushed the glennw:remove-clip-scroll branch from b5f4a7b to c106ab0 Nov 5, 2017

mrobinson approved these changes Nov 5, 2017

View changes

glennw force-pushed the glennw:remove-clip-scroll branch from c106ab0 to 5bdcb95 Nov 5, 2017

bors-servo mentioned this pull request Nov 5, 2017

Allow gecko to provide the already-applied sticky offset to WR #1977

Merged

glennw deleted the glennw:remove-clip-scroll branch Nov 5, 2017

kvark reviewed Nov 6, 2017

View changes

servo / webrender

Remove ClipScrollGroup and PackedLayer. #1981

Remove ClipScrollGroup and PackedLayer. #1981

glennw commented Nov 2, 2017 •

edited by larsbergstrom

glennw commented Nov 2, 2017

glennw commented Nov 2, 2017

mrobinson commented Nov 2, 2017

mrobinson commented Nov 2, 2017

glennw commented Nov 3, 2017

mrobinson left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

glennw commented Nov 5, 2017

mrobinson left a comment

glennw commented Nov 5, 2017

bors-servo commented Nov 5, 2017

bors-servo commented Nov 5, 2017

bors-servo commented Nov 5, 2017

kvark 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.

servo / webrender

Join GitHub today

Remove ClipScrollGroup and PackedLayer. #1981

Remove ClipScrollGroup and PackedLayer. #1981

Conversation

glennw commented Nov 2, 2017 • edited by larsbergstrom

glennw commented Nov 2, 2017

glennw commented Nov 2, 2017

mrobinson commented Nov 2, 2017

mrobinson commented Nov 2, 2017

glennw commented Nov 3, 2017

mrobinson left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

glennw commented Nov 5, 2017

mrobinson left a comment

glennw commented Nov 5, 2017

bors-servo commented Nov 5, 2017

bors-servo commented Nov 5, 2017

bors-servo commented Nov 5, 2017

kvark 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.

glennw commented Nov 2, 2017 •

edited by larsbergstrom