Use segments when drawing clip masks, decouple from tiles. #696

glennw · 2017-01-10T05:57:23Z

This change is

glennw · 2017-01-10T06:02:51Z

@kvark This implements the clip mask changes we discussed earlier. Instead of applying the more general optimization, it just handles the common case of a single complex rect, and draws the corners only in this case. But this PR is easily extendable to the general case, either as a follow up or before merging.

This code passes the tests, and is fast, but I'm not super happy with it. Subjectively, it feels much more complicated than the other method to me. It is also less efficient in the common case, since the rectangle clip shader has to evaluate all four clip corners, whereas with the other method it only handles one clip corner at a time.

But anyway, I'm interested to hear your thoughts, and specifically (a) if you agree that this code solution is more complicated than the other option and (b) if you can think of a nicer / cleaner way to implement this functionality.

glennw · 2017-01-10T07:13:49Z

@kvark Hmmm, it probably has to be this way actually, since I think the other method can suffer from double AA issues at the edges of clip corners. Need to think about that a bit more, but it may rule out the other method anyway.

kvark · 2017-01-10T18:13:01Z

I'm not completely following you on AA issues, but you are making a good point for me on this part. If we are talking about MSAA, then in your approach we'd only need to super-sample the corner draws, while the main draw call of the primitive would use regular multi-sample. That is going to be much faster than my approach, which would need to super-sample the whole mask. This concern only applies to MSAA though, which I'm not sure is what you were referring to.

kvark

@glennw thanks for putting this together!

(a) if you agree that this code solution is more complicated than the other option

I don't see where this would be more complicated than the other option, because at the current state it hasn't even diverged from that other option. The approaches work differently for complex and/or transformed masks, while this case you optimized here should be treated equally well by both.

(note: I don't assume you mean #695 here since it is incomplete and cuts down more stuff than we can afford)

(b) if you can think of a nicer / cleaner way to implement this functionality.

I think we'd need to dig deeper before looking for a way to refactor it. So far it looks fine, but the complexity is expected to come from us actually stopping allocating that inner rectangle.

P.S.: the "request changes" status here is not for actual changes as it is to get some answers

kvark · 2017-01-10T19:17:17Z

webrender/src/tiling.rs

+        let mut geometry_kind = MaskGeometryKind::Default;
+
+        if inner_rect.is_some() && clips.len() == 1 {
+            let &(ref sc_index, ref clip_info) = clips.first().unwrap();


nit: clips[0] would do

kvark · 2017-01-10T19:17:17Z

webrender/src/tiling.rs

+                                    match mask_opt {
+                                        MaskResult::Outside => {
+                                            // Primitive is completely clipped out.
+                                            prim_metadata.clip_task = None;


doesn't this mean that there is no clipping for this primitive?
where do you actually cull this primitive?

kvark · 2017-01-10T19:17:17Z

webrender/src/tiling.rs

                        }
                    }
                }
                &PrimitiveRunCmd::PopStackingContext => {
+                    let sc_index = *layer_stack.last().unwrap();
+                    let layer = &mut self.layer_store[sc_index.0];
+                    if layer.is_visible() {


the push stuff is using a different check - can_contribute_to_scene(), so there might be a bug here

kvark · 2017-01-10T19:17:17Z

webrender_traits/src/display_item.rs

@@ -129,17 +129,16 @@ impl ComplexClipRegion {
    }

    //TODO: move to `util` module?
-    /// Return a maximum aligned rectangle that is fully inside the clip region.
+    /// Return an aligned rectangle that is fully inside the clip region.


why is k removed from here?

I use the inner rect to generate the vertices for the corners in the vertex shader - so it needs to extend to the radius for this technique. I think that is fine now, for the way inner_rect is now used. But perhaps in the future we could add a mode to get_inner_rect to optionally get the maximal inner rect, if that's useful elsewhere?

Makes sense, yeah. I was just shocked by the removal of my carefully computed mathematical constant that was supposed to optimize everything :)


        Use segments when drawing clip masks, decouple from tiles.

glennw · 2017-01-10T20:48:17Z

@kvark OK, comments addressed and [wip] removed. Thanks!

kvark · 2017-01-10T22:27:35Z

Thanks @glennw , :)
@bors-servo r+

bors-servo · 2017-01-10T22:27:36Z

📌 Commit 422862a has been approved by kvark

bors-servo · 2017-01-10T23:01:04Z

⚡ Test exempted - status


        Auto merge of #696 - glennw:remove-clip-tiles2, r=kvark

Use segments when drawing clip masks, decouple from tiles.  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/696)

kvark requested changes Jan 10, 2017

View changes

Use segments when drawing clip masks, decouple from tiles.

Loading status checks…

422862a

glennw force-pushed the glennw:remove-clip-tiles2 branch from e4e17a3 to 422862a Jan 10, 2017

glennw changed the title ~~[wip] Use segments when drawing clip masks, decouple from tiles.~~ Use segments when drawing clip masks, decouple from tiles. Jan 10, 2017

glennw mentioned this pull request Jan 10, 2017

Removes the tiling and render task stack from clipping, draws clips in regions of interest. #685

Closed

kvark approved these changes Jan 10, 2017

View changes

bors-servo merged commit 422862a into servo:master Jan 10, 2017
2 checks passed

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

servo / webrender

Use segments when drawing clip masks, decouple from tiles. #696

Use segments when drawing clip masks, decouple from tiles. #696

glennw commented Jan 10, 2017 •

edited by larsbergstrom

glennw commented Jan 10, 2017

glennw commented Jan 10, 2017

kvark commented Jan 10, 2017 •

edited

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

glennw commented Jan 10, 2017

kvark commented Jan 10, 2017

bors-servo commented Jan 10, 2017

bors-servo commented Jan 10, 2017

servo / webrender

Join GitHub today

Use segments when drawing clip masks, decouple from tiles. #696

Use segments when drawing clip masks, decouple from tiles. #696

Conversation

glennw commented Jan 10, 2017 • edited by larsbergstrom

glennw commented Jan 10, 2017

glennw commented Jan 10, 2017

kvark commented Jan 10, 2017 • edited

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

glennw commented Jan 10, 2017

kvark commented Jan 10, 2017

bors-servo commented Jan 10, 2017

bors-servo commented Jan 10, 2017

glennw commented Jan 10, 2017 •

edited by larsbergstrom

kvark commented Jan 10, 2017 •

edited