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

[WIP] Redesigned clip and scroll API #3251

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@kvark
Member

kvark commented Oct 30, 2018

Fixes https://bugzilla.mozilla.org/show_bug.cgi?id=1503447
Note: this description is edited to contain the relevant up-to-date information about the code.

General idea

This is something I wanted for a long time: the spatial hierarchy that isn't mixed up with the clip hierarchy. The change dragged a bunch of other API fixes that were accumulated in our technical debt, so it grew rather big/scary.

The motivation is - removing the complexity of the API that we inherited from the past. Both Servo and Gecko are in a better position to explicitly tell us what the clip/spatial properties of items are, and they don't need to mix them up in the same type. The old API had more complex logic, more state, and caused the Gecko integration to be painful and non-solid (there are edge cases not handled properly).

API changes:

  • separate SpatialId from ClipId 🎉
  • remove the clip/scroll stack from the API, all the SpatialId and ClipId are passed explicitly for display items in the form of SpaceAndClipInfo, which roughly corresponds to the old ClipAndScrollInfo.
  • reference frame are no longer pushed/popped and instead are "defined", similar to scroll/sticky frames.
  • stacking contexts no longer have an internal ClipId passed in (and the standard one being ignored), instead they use the standard one from the associated SpaceAndClipInfo.
  • define_clip_with_parent and define_scroll_frame_with_parent are removed.

TODO:

  • reviews
  • integrate with Gecko and provide a good try push
  • integrate with Servo:

This change is Reviewable

@kvark kvark requested a review from gw3583 Oct 30, 2018

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 30, 2018

It would be good to also provide a patch for Servo as part of this PR, or some detailed instructions on what will be required to change in the next WR update in Servo.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 30, 2018

(I haven't reviewed this yet, btw - just an initial comment above)

@mrobinson

It's great to finally see this change! I did a quick review of most of this patch and I'm happy to take a closer look later.

@@ -63,6 +63,12 @@ impl<T> ItemRange<T> {
}
}
#[derive(Clone, Debug)]
pub enum ClipParent {
Inherit,

This comment has been minimized.

@mrobinson

mrobinson Oct 31, 2018

Member

Maybe this should be something like FromStack? I mention this because the Clip isn't really inherited from a parent in a real sense. It's just taken from the ClipStack itself, which is a convenience.

ClipId::Spatial(..) => {
self.spatial_node_map[&id]
}
ClipId::ClipChain(_) => panic!("Tried to use ClipChain as scroll node."),

This comment has been minimized.

@mrobinson

mrobinson Oct 31, 2018

Member

It's great to see this go!

let root_scroll_node = ClipId::root_scroll_node(pipeline_id);
let reference_frame_info = self.simple_scroll_and_clip_chain(&root_scroll_node);

This comment has been minimized.

@mrobinson

mrobinson Oct 31, 2018

Member

Why are you adding the background color to the scroll frame here? Won't that mean it will scroll with the contents?

This comment has been minimized.

@kvark

kvark Oct 31, 2018

Member

nicely spotted! This is a mistake

@@ -332,7 +315,7 @@ impl<'a> DisplayListFlattener<'a> {
item: &DisplayItemRef,
info: &StickyFrameDisplayItem,
clip_and_scroll: &ScrollNodeAndClipChain,
parent_id: &ClipId,
//parent_id: &ClipId,

This comment has been minimized.

@mrobinson

mrobinson Oct 31, 2018

Member

This should probably be deleted.

new_node_id: ClipId,
parent_id: ClipId,
new_node_id: SpatialId,
cs_info: &ClipAndScrollInfo,

This comment has been minimized.

@mrobinson

mrobinson Oct 31, 2018

Member

Why not just pass parent_spatial_id here since you don't need the ClipId?

@@ -396,12 +376,12 @@ impl<'a> DisplayListFlattener<'a> {
pipeline_id: PipelineId,
item: &DisplayItemRef,
reference_frame: &ReferenceFrame,
scroll_node_id: ClipId,
cs_info: &ClipAndScrollInfo,

This comment has been minimized.

@mrobinson

mrobinson Oct 31, 2018

Member

It looks like you can just pass the SpatialId here.

@@ -417,7 +397,7 @@ impl<'a> DisplayListFlattener<'a> {
pipeline_id: PipelineId,
item: &DisplayItemRef,
stacking_context: &StackingContext,
scroll_node_id: ClipId,
scroll_node_id: SpatialId,

This comment has been minimized.

@mrobinson

mrobinson Oct 31, 2018

Member

Maybe a good time to rename this to spatial_node_id? since the scroll_node_id thing is no longer accurate?

@@ -1310,8 +1283,10 @@ impl<'a> DisplayListFlattener<'a> {
self.prim_store.chase_id = Some(prim_index);
}
let root_space_id = SpatialId::root_reference_frame(pipeline_id);

This comment has been minimized.

@mrobinson

mrobinson Oct 31, 2018

Member

Maybe this could be root_reference_frame_id or something like that, since root_space_id is a little ambiguous.

let spatial_node = self.id_to_index_mapper.get_spatial_node_index(parent_id);
let mut parent_clip_chain_index = match cs_info.clip_node_id {
Some(clip_id) => self.id_to_index_mapper.get_clip_chain_id(clip_id),
None => ClipChainId::NONE,

This comment has been minimized.

@mrobinson

mrobinson Oct 31, 2018

Member

This looks like it might cause a lot of code changes in Servo and Gecko? Currently, when this value is None, it effectively uses whatever the "implied" parent clip chain is. Now Servo and Gecko will need to track that value themselves and set it explicitly. Won't this cause a "no clip" situation instead?

This comment has been minimized.

@kvark

kvark Oct 31, 2018

Member

This piece is not a part of the API, it's the DL processing. At this level, we just strictly respect the parent clip chain specified. It's up to the webrender_api/src/display_list.rs to ensure that the "implied" parent semantics is preserved.

I'm going to test Gecko shortly and see what the implications are. I'd expect wrench reftests to fail if this semantics was ported incorrectly.

pub clip_node_id: Option<ClipId>,
}
impl ClipAndScrollInfo {
pub fn simple(node_id: ClipId) -> ClipAndScrollInfo {
pub fn simple(scroll_node_id: SpatialId) -> Self {

This comment has been minimized.

@mrobinson

mrobinson Oct 31, 2018

Member

Now that these are separate ids, I'm not sure it is meaningful to have clip_node_id an Option<...>. Previously None here meant "use the implied ancestor of my spatial node. It doesn't look like that is possible any longer. Perhaps "no clip" should just be represented by the root clip.

This comment has been minimized.

@kvark

kvark Oct 31, 2018

Member

I removed ClipAndScrollInfo altogether now. No need to group together loosely related things.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 31, 2018

@mrobinson Thanks for jumping in to help with reviewing this! 👍

@kvark

This comment has been minimized.

Member

kvark commented Oct 31, 2018

@mrobinson it's a pleasant surprise to see you, thanks for taking a look!

@kvark

This comment has been minimized.

Member

kvark commented Nov 2, 2018

Asking for a real review now, while I'm finishing testing Gecko side.

@gw3583

Reviewed 4 of 14 files at r2, 11 of 11 files at r3.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @mrobinson and @kvark)


wrench/reftests/performance/no-clip-mask.png, line 0 at r3 (raw file):
I'm somewhat surprised that this patch causes a difference here. Do we know why?

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Nov 4, 2018

Just one question above, since it's not clear to me that this should have affected any reftest outputs (and if that has any implications for Gecko / Servo).

Otherwise, it seems like we need:

Then this should be ready to merge.

@kvark

This comment has been minimized.

Member

kvark commented Nov 6, 2018

@gw3583 I'm working on even more changes now that remove the clip-scroll stack completely from the API. More updates to come... and I'll double check the test. I assume there is a slight change in YAML reading semantics (e.g. parsing the "clip-and-scroll" field), since it diverges further from the actual WR API, and I'm trying to preserve compatibility with the existing reftests.

@kvark kvark changed the title from [WIP] Separate clipping and scrolling for good to [WIP] Redesigned clip and scroll API Nov 7, 2018

@bors-servo

This comment has been minimized.

Contributor

bors-servo commented Nov 12, 2018

☔️ The latest upstream changes (presumably #3300) made this pull request unmergeable. Please resolve the merge conflicts.

@kvark

This comment has been minimized.

Member

kvark commented Nov 15, 2018

I've got to document this somewhere: we have a few places in DL building where the clip is literally smuggled through the scroll ID, while the clip ID is silently dropped:

  • push_reference_frame
  • define_clip
  • possibly other

What happens is scroll_node_id gets through, and the DL flattener associates the produced clip (for define_clip) or scroll (for push_reference_frame) with the clip chain of the parent, hence the clipping survives even though the original clip_node_id is ignored.

But the worst thing is that there is code in Gecko that relies on that behavior...

bors-servo added a commit that referenced this pull request Nov 16, 2018

Auto merge of #3315 - kvark:clip-respect, r=gw3583
Don't ignore the clip on the stack for reference frames and clips

Similar to #3311, this is a prelude to #3251, with the main purpose of getting rid of Gecko's hack in https://searchfox.org/mozilla-central/rev/d850d799a0009f851b5535580e0a8b4bb2c591d7/gfx/layers/wr/ClipManager.cpp#224

It also makes sense for WR in isolation (for the current clip/stack model, at least): in those spots addressed, we used to ignore the clip ID on the stack, only to re-interpret (smuggle!) the scroll ID as a clip. Now we are actually using the clip ID more consistently. See also - #3251 (comment)

Gecko try (that disables the hack in addition to having the WR change):
https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=212038686&revision=c10e5d839e024ba451a6622fa611c3525457f3ff
(Looks green! 🎉 )

<!-- 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/3315)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment