Major refactoring of how clips are processed down the pipeline #512

kvark · 2016-11-01T22:07:10Z

This moves the clipping hack down the road (from Frame to FrameBuilder), allowing to eventually implement multiple transformed clips properly (#498), as discussed on IRC with @glennw .

This change is

kvark · 2016-11-01T22:26:48Z

Now that I think of it, this might be incorrect. I'm borrowing the scene.pipeline_auxiliary_lists during both the frame building and rendering (where it used to clone() them). Bad things will happen if these lists are changed in-between.

kvark · 2016-11-02T13:17:35Z

I rewrote the code to solve the concern by cloning the aux lists into the FrameBuilder. Should be ready now.

glennw · 2016-11-02T19:32:10Z

At the moment, I try do avoid passing the auxiliary lists around to many APIs, and instead resolve them all at one part of the frame. This has the added advantage that we only resolve items from the auxiliary lists when they first come on screen, avoiding a large upfront cost for a new scene. Is it possible do just make clips work the same way as all the other primitives, or does something prevent that?

bors-servo · 2016-11-03T00:00:42Z

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

kvark · 2016-11-03T14:41:06Z

Redesigned the whole thing after talking to @glennw . The auxiliary lists are no longer accessed at all during the primitive population, and are only passed down (by reference, no cloning) during the frame building. This is also an optimization, since we only resolve the aux lists for visible primitives now.
There is a lot of code simplification here as well.

glennw · 2016-11-03T19:49:54Z

webrender/src/prim_store.rs

@@ -58,15 +58,38 @@ pub struct PrimitiveCacheInfo {
    pub key: PrimitiveCacheKey,
 }

+#[derive(Debug)]
+pub enum PrimitiveClipInfo {


Is the idea here that it gets resolved from Complex / Region to GpuAddress? Would it make more sense to split into a "source" and "resolved" field perhaps?

Yes, that's the idea. The reason I decided not to have separate fields for source/resolved is because they are mutually exclusive (or at least, the source make no sense once the resolved is set), which is reflected here by the use of enum. I agree this is probably the most controversial aspect of this PR, just not seeing a better solution at the moment.

OK, I think that's fine - we might like to make some of the conditions marked as unreachable!() or something like that (e.g. if the non-resolved type gets encountered after it should always have been resolved). But no need to do that now.

glennw · 2016-11-03T19:50:42Z

webrender/src/prim_store.rs

+}
+
+impl PrimitiveClipInfo {
+    pub fn is_clipped(&self) -> bool {


Will this return true even for a rectangular clip only? (If so, that will prevent occlusion culling where it is being used).

You are right, this part doesn't seem right. If it's called after after the prepare_* body (and I think it does), the only variant here would be GpuAddress. Will be fixed in a minute.

glennw · 2016-11-03T19:52:30Z

webrender/src/prim_store.rs

                                   auxiliary_lists: &AuxiliaryLists) -> bool {
        let metadata = &mut self.cpu_metadata[prim_index.0];
        debug_assert!(metadata.need_to_build_cache);
        metadata.need_to_build_cache = false;

-        if let Some(mask_key) = metadata.mask_image {
-            let tex_cache = resource_cache.get_image(mask_key, ImageRendering::Auto, frame_id);
+        let clip_data_mask = match metadata.clip_info.as_ref() {


Does this mean that we'll end up with a complex clip region enabled when the clip region is a simple rect with zero radius? If so, that will introduce a lot more expensive clip shaders than needed.

I don't think we handled the zero radius anywhere except this piece:

if scrollbar_prim.border_radius == 0.0 { - self.prim_store.set_complex_clip(scrollbar_prim.prim_index, None); + self.prim_store.unset_complex_clip(scrollbar_prim.prim_index) } else {

The general case of no ComplexClipRegion given is handled below, here:

let clip_data = match clips.len() { 0 if clip_region.image_mask.is_none() => None,

So I don't think this PR changes the logic in a way that makes the shaders more expensive.

Yup, that should handle it.

glennw · 2016-11-03T21:09:05Z

@kvark I ran the mozilla css tests subset with this patch - border_radius_clip_a.html fails but everything else passes. Could you take a look at that failure when you have time?

kvark · 2016-11-03T21:14:00Z

@glennw I was also checking the tests, saw that one, and confirmed it happens without this PR equally.
Edit: upon closer inspection, this does appear to be my issue. Looking...

glennw · 2016-11-03T22:41:55Z

Generally looks good to me - this fits in quite well with the rest of the items that use ItemRange. Just need to get the test failures sorted and then this should be good to go.

bors-servo · 2016-11-04T01:45:00Z

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

…eparation to multiple transformed clip support. Removed MaskImageSource, introduced PrimitiveClipSource to the metadata. Avoided cloning the auxiliary lists map.

glennw · 2016-11-04T03:35:30Z

@bors-servo r+

bors-servo · 2016-11-04T03:35:31Z

📌 Commit 4ba3493 has been approved by glennw

bors-servo · 2016-11-04T03:35:33Z

⚡ Test exempted - status

@glennw

Major refactoring of how clips are processed down the pipeline This moves the clipping hack down the road (from `Frame` to `FrameBuilder`), allowing to eventually implement multiple transformed clips properly (#498), as discussed on IRC with @glennw .  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/512)

glennw · 2016-11-04T03:35:38Z

Looks good, thanks!

kvark force-pushed the region branch from eeb19c4 to 8fd781f Compare November 2, 2016 13:16

kvark changed the title ~~Refactored passing down ClipRegion and auxiliary context lists~~ Refactoring of ClipRegion handling by the FrameBuilder with auxiliary context lists Nov 2, 2016

kvark force-pushed the region branch from 8fd781f to 3047fe2 Compare November 3, 2016 14:38

kvark changed the title ~~Refactoring of ClipRegion handling by the FrameBuilder with auxiliary context lists~~ Major refactoring of how clips are processed down the pipeline Nov 3, 2016

glennw mentioned this pull request Nov 3, 2016

Remove ResourceList structure and usage. #523

Merged

glennw reviewed Nov 3, 2016

View reviewed changes

kvark force-pushed the region branch from 6d9c492 to 0ed7eef Compare November 3, 2016 21:41

kvark force-pushed the region branch 2 times, most recently from 9a44521 to c553d77 Compare November 4, 2016 02:40

Major refactoring of how clips are processed down the pipeline, in pr…

01517a2

…eparation to multiple transformed clip support. Removed MaskImageSource, introduced PrimitiveClipSource to the metadata. Avoided cloning the auxiliary lists map.

kvark force-pushed the region branch from c553d77 to 01517a2 Compare November 4, 2016 02:55

Avoid resolving the clip every frame

4ba3493

bors-servo merged commit 4ba3493 into servo:master Nov 4, 2016

kvark deleted the region branch November 4, 2016 14:21

kvark mentioned this pull request Nov 11, 2016

Support for transformed clip regions #498

Closed

4 tasks

Major refactoring of how clips are processed down the pipeline #512

Major refactoring of how clips are processed down the pipeline #512

Uh oh!

Conversation

kvark commented Nov 1, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark commented Nov 1, 2016

Uh oh!

kvark commented Nov 2, 2016

Uh oh!

glennw commented Nov 2, 2016

Uh oh!

bors-servo commented Nov 3, 2016

Uh oh!

kvark commented Nov 3, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennw commented Nov 3, 2016

Uh oh!

kvark commented Nov 3, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennw commented Nov 3, 2016

Uh oh!

bors-servo commented Nov 4, 2016

Uh oh!

glennw commented Nov 4, 2016

Uh oh!

bors-servo commented Nov 4, 2016

Uh oh!

bors-servo commented Nov 4, 2016

Uh oh!

glennw commented Nov 4, 2016

Uh oh!

Uh oh!

kvark commented Nov 1, 2016 •

edited by larsbergstrom

Loading

kvark commented Nov 3, 2016 •

edited

Loading