Make stacking context clipping explicit

[mrobinson]

Instead of relying on the combination of a stacking contexts which
render to intermediate surfaces and the PushStackingContext's clip node,
add an explicit clipping node parameter for stacking contexts. This is
necessary, because currently stacking context clipping is done by always
rendering to an intermediate surface. Once we support merging stacking
context and contained item clip chains we can remove this and the extra
parameter.

---

This change is 

[mrobinson]

The Gecko try job for this change is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=653eda9276eb3ecaede897f2f98e6263a69ca4b5&selectedJob=171426186

Looks like there are some intermittent failures and one newly passing test.

[glennw]

Reviewed 24 of 24 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

webrender/src/batch.rs, line 680 at r1 (raw file):

                                let source_id = cache_task_id;

                                match picture.composite_mode.expect("bug: only composites here") {

This change will mean the catch-all _ at the bottom of this match executes if a bug occurs and we get here without a composite mode. Is there a specific reason to remove the expect here?

---

webrender/src/display_list_flattener.rs, line 1201 at r1 (raw file):

            frame_output_pipeline_id,
            true,
            clipping_node.is_some(),

We should perhaps test here (well, later probably) if the clipping node actually applies any useful clips? This would avoid doing redundant render work if the client supplies a clip node that has no effect.

If we did something like that, we'd probably check during the picture prepare each frame if the clip node applies to this Picture, and select at that point whether to use an intermediate surface.

We can do that as a follow up optimization later though - this will definitely let us do some testing in Gecko and make sure it works as we want the API to.

---

webrender/src/picture.rs, line 204 at r1 (raw file):

        if self.can_draw_directly_to_parent_surface() {
            if !self.force_intermediate_surface {

Should this bool check just be added to the can_draw_directly_to_parent_surface method?

---

webrender/src/picture.rs, line 373 at r1 (raw file):

                prim_screen_rect.clipped
            }
            None =>unreachable!("None case not handled by can_draw_directly_to_parent_surface."),

nit: space

---

webrender/src/prim_store.rs, line 1050 at r1 (raw file):

            apply_local_clip_rect,
        );
        picture.force_intermediate_surface = force_intermediate_surface;

Should we just push this through the new_image method rather than making it a mut variable here and setting it?

---

wrench/reftests/filters/blend-clipped.png, line 0 at r1 (raw file):
Is this just a change to the crop dimensions or was the content affected too?

---

Comments from Reviewable

[glennw]

Generally this looks great! I added some comments in reviewable.

[bors-servo]

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

[mrobinson]

Thanks for the review! I've answered you comments below and updated the PR.

---

Review status: all files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

webrender/src/batch.rs, line 680 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

This change will mean the catch-all _ at the bottom of this match executes if a bug occurs and we get here without a composite mode. Is there a specific reason to remove the expect here?

The issue is that now even Pictures without a composition mode might have a surface assigned. This code was expecting that to never happen. Unless there is a better way to handle this, I can maintain the runtime check is by adding an assertion like:

                                assert!(
                                    picture.composite_mode.is_some() ||
                                    picture.force_intermediate_surface
                                );

---

webrender/src/display_list_flattener.rs, line 1201 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

We should perhaps test here (well, later probably) if the clipping node actually applies any useful clips? This would avoid doing redundant render work if the client supplies a clip node that has no effect.

If we did something like that, we'd probably check during the picture prepare each frame if the clip node applies to this Picture, and select at that point whether to use an intermediate surface.

We can do that as a follow up optimization later though - this will definitely let us do some testing in Gecko and make sure it works as we want the API to.

That's correct. Right now the clips are applied twice, which in our rendering model could even affect the output. Eventually I think we will need to append the two clip chains on all items in the stacking context (while perhaps removing duplicate clips). In the end, this should remove the need to force the use of an intermediate surface.

This may require us to revisit how clips are applied in WebRender, in general.

---

webrender/src/picture.rs, line 204 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Should this bool check just be added to the can_draw_directly_to_parent_surface method?

Yes, this made the code quite a bit simpler!

---

webrender/src/picture.rs, line 373 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

nit: space

This should be fixed with the previous change,

---

webrender/src/prim_store.rs, line 1050 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Should we just push this through the new_image method rather than making it a mut variable here and setting it?

Done.

---

wrench/reftests/filters/blend-clipped.png, line at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Is this just a change to the crop dimensions or was the content affected too?

The content was affected here as well.

---

Comments from Reviewable

[glennw]

Review status: 20 of 24 files reviewed at latest revision, 2 unresolved discussions.

---

webrender/src/batch.rs, line 680 at r1 (raw file):

Previously, mrobinson (Martin Robinson) wrote…

The issue is that now even Pictures without a composition mode might have a surface assigned. This code was expecting that to never happen. Unless there is a better way to handle this, I can maintain the runtime check is by adding an assertion like:

                                assert!(
                                    picture.composite_mode.is_some() ||
                                    picture.force_intermediate_surface
                                );

Ah, I see. I inverted that logic a bit in 6b63b20, which is merging now. That will unfortunately conflict with your change, but perhaps the logic there might work a bit better with your change now (it inverts the composite mode / surface check). Or does that complicate things further?

---

webrender/src/display_list_flattener.rs, line 1201 at r1 (raw file):

Previously, mrobinson (Martin Robinson) wrote…

That's correct. Right now the clips are applied twice, which in our rendering model could even affect the output. Eventually I think we will need to append the two clip chains on all items in the stacking context (while perhaps removing duplicate clips). In the end, this should remove the need to force the use of an intermediate surface.

This may require us to revisit how clips are applied in WebRender, in general.

In theory, it's possible to avoid having clips applied twice, by having a different clip chain applied to the stacking context from the one that is set on primitives within the stacking context, isn't it? I thought the idea was that the caller could then choose whether to specify a clip-chain for the stacking context AND/OR the primitive as they want to. Or is my conceptual model of how this works not right?

---

Comments from Reviewable

[bors-servo]

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

[glennw]

r=me btw, once rebased and the questions above are covered.

[bors-servo]

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

[mrobinson]

Review status: 17 of 24 files reviewed at latest revision, 2 unresolved discussions.

---

webrender/src/batch.rs, line 680 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Ah, I see. I inverted that logic a bit in 6b63b20, which is merging now. That will unfortunately conflict with your change, but perhaps the logic there might work a bit better with your change now (it inverts the composite mode / surface check). Or does that complicate things further?

I think it works a bit better with your change from 6b63b20. Hopefully we can remove this completely in the future

---

webrender/src/display_list_flattener.rs, line 1201 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

In theory, it's possible to avoid having clips applied twice, by having a different clip chain applied to the stacking context from the one that is set on primitives within the stacking context, isn't it? I thought the idea was that the caller could then choose whether to specify a clip-chain for the stacking context AND/OR the primitive as they want to. Or is my conceptual model of how this works not right?

Oh I definitely want to preserve the functionality where when the stacking context and primitives are assigned different clip chains they both clip the primitive. This is how things work after this change.

What I mean by avoiding double clipping is to handle the situation where, for instance, the stacking context clip chain is a subset of the primitive clip chain. In this example, the stacking context clip chain would be ignored completely. It would be great to have this de-duplication done on a per-clip basis while processing primitive runs.

---

Comments from Reviewable

[mrobinson]

@bors-servo r=glennw

[bors-servo]

📌 Commit 5e42243 has been approved by glennw

[bors-servo]

⌛ Testing commit 5e42243 with merge 5922781...

[bors-servo]

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: glennw
Pushing 5922781 to master...

[glennw]

@staktrace Heads up for public API change. Passing None for the new parameter to push_stacking_context will retain current behaviour.