Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Simplifying the clipping API. #2551

Closed
glennw opened this issue Mar 20, 2018 · 4 comments
Closed

[RFC] Simplifying the clipping API. #2551

glennw opened this issue Mar 20, 2018 · 4 comments

Comments

@glennw
Copy link
Member

@glennw glennw commented Mar 20, 2018

This is a writeup of a conversation I had with @mstange. I'll try to summarize what we discussed, and list what I think would change in WR code.

One of the things that complicates drawing a Picture in local space is that clips that affect the Picture can be defined in other coordinate spaces, due to the way ancestor clip-scroll nodes can be referenced. See #2533 for details.

Gecko doesn't actually require this. The Gecko requirements for clipping are that:

  • A primitive can have clips defined that are in the same reference frame (stacking context from public API point of view).
  • Any clip from outside that reference frame is applied to the stacking context. This is done by drawing the stacking context to an intermediate surface, and then applying the ancestor clips to the stacking context itself, rather than the primitives within the stacking context.

Previously, this was not really feasible in WR, because we didn't support clip masks on stacking contexts. Now that stacking context blits are drawn as brush primitives, we do (mostly) support this, and expanding it to support in all cases should be trivial (more details below).

If we were to change WR such that it supports clipping the way Gecko uses (that is, local clips on primitives, and ancestor clips on stacking contexts), that has the potential to:

  • Drastically simplify implementing local-space rendering (#2533) should we decide to.
  • Make the existing clipping code simpler (I think) and significantly faster. The clip mask for an ancestor clip-chain would only need to be drawn once, and since Picture drawing supports segments just like normal primitives, we would render only minimal clip masks in this case.

These are the steps that I envisage we'd take:

  • Complete the WR support for clipping stacking contexts. Effectively, this should work now for stacking contexts that already draw to an intermediate surface. All we should need to do here is add a piece of code that marks a Picture as needing an intermediate surface if it contains a complex clip chain that affects the stacking context rect (the code to do the intermediate rendering / clipping is already in place and works).
  • Update Gecko to make use of that API (not sure how complex this is?).
  • Deprecate and remove support in the WR display list builder for adding clips to primitives that are outside its reference frame.

@mstange @mrobinson @staktrace @kvark Does that sound like a correct summary? Any corrections / thoughts / questions?

@glennw
Copy link
Member Author

@glennw glennw commented Mar 20, 2018

cc @jrmuizel too

@staktrace
Copy link
Contributor

@staktrace staktrace commented Mar 21, 2018

This sounds good to me, at least in theory. It should be much more in line with the display list representation that gecko produces. I have a clipping rewrite already basically done that uses the ClipChain API to basically do a 1:1 mapping from gecko clips to WR clips and is much cleaner. However the last time I rebased it it was showing some unexpected failures on a try push which I haven't had time to debug. I suspect now it might be because of the partially implemented support WR has for applying clips to stacking contexts. At any rate, I'm happy to pick up that work again once I'm done with the current APZ/RenderBackend changes.

@mrobinson
Copy link
Member

@mrobinson mrobinson commented Mar 22, 2018

It seems like a good first step to allow clipping stacking context contents and then see how it affects Gecko. Adding constraints to how you can apply clips might make the API quite a bit more complicated, but if it leads to important optimizations, it's probably worth it.

@gw3583
Copy link
Collaborator

@gw3583 gw3583 commented Aug 2, 2018

This happened.

@gw3583 gw3583 closed this Aug 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.