Use segments when drawing clip masks, decouple from tiles. #696
Conversation
|
@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. |
|
@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. |
|
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. |
There was a problem hiding this comment.
@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
| let mut geometry_kind = MaskGeometryKind::Default; | ||
|
|
||
| if inner_rect.is_some() && clips.len() == 1 { | ||
| let &(ref sc_index, ref clip_info) = clips.first().unwrap(); |
| match mask_opt { | ||
| MaskResult::Outside => { | ||
| // Primitive is completely clipped out. | ||
| prim_metadata.clip_task = None; |
There was a problem hiding this comment.
doesn't this mean that there is no clipping for this primitive?
where do you actually cull this primitive?
| } | ||
| } | ||
| } | ||
| &PrimitiveRunCmd::PopStackingContext => { | ||
| let sc_index = *layer_stack.last().unwrap(); | ||
| let layer = &mut self.layer_store[sc_index.0]; | ||
| if layer.is_visible() { |
There was a problem hiding this comment.
the push stuff is using a different check - can_contribute_to_scene(), so there might be a bug here
| @@ -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. | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Makes sense, yeah. I was just shocked by the removal of my carefully computed mathematical constant that was supposed to optimize everything :)
glennw
force-pushed
the
remove-clip-tiles2
branch
from
e4e17a3 to
422862a
Compare
|
@kvark OK, comments addressed and [wip] removed. Thanks! |
glennw
changed the title
|
Thanks @glennw , |
:)
|
📌 Commit 422862a has been approved by |
|
⚡ Test exempted - status |
Use segments when drawing clip masks, decouple from tiles. <!-- Reviewable:start --> This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/696) <!-- Reviewable:end -->
This change is