Remove the per-item complex clipping and masking from the API #1412

mrobinson · 2017-06-21T12:16:34Z

This is the first step to getting all masking operations into the
ClipScrollTree. We don't completely this feature, as it's still sued
for the extra clips. These will be moved into the ClipScrollTree in a
later change. This change allows us to fully eliminate the
DisplayListBuilder::push_clip_region API and push its functionality
into the define_clip API.

Most of the changes here are:

Changes to the examples, wrench, and reftests to match the new API.
Adapting the "split rectangle clipped by rounded rect"
optimization to use the ClipScrollTree. This is unfortunately a
bit more complicated now, but now includes any rounded rect clip
instead of per-item ones.

This change is

mrobinson · 2017-06-21T12:17:12Z

Sorry for the large patch, but most of the changes are due to the API change as well as switching two of the examples to use the example boilerplate.

kvark

Why did wrench/reftests/aa/rounded-rects-ref.png change?

Also, perhaps you forgot to remove PrimitiveMetadata::clip_cache_info?

kvark · 2017-06-21T16:03:29Z

webrender/src/clip_scroll_node.rs

@@ -412,6 +411,38 @@ impl ClipScrollNode {
            _ => false,
        }
    }
+
+    pub fn find_unclipped_rectangle(&self,


It appears to be similar to the MashBounds::inner. Perhaps, you could just use clip_info.mask_cache_info.bounds.inner instead of having a separate method?

It looks like MaskBounds::inner is using inner_rect_safe() while your optimization is using inner_rect_full(). I confirmed locally that these two methods are producing different rectangles. I'm not sure what is correct here, so would it make sense to try to combine these calculations in a later patch?

bors-servo · 2017-06-22T16:27:41Z

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

kvark · 2017-06-22T18:46:01Z

@bors-servo r+

bors-servo · 2017-06-22T18:46:02Z

📌 Commit e6228a7 has been approved by kvark

bors-servo · 2017-06-22T20:34:42Z

⌛ Testing commit e6228a7 with merge ee59a40...


        Auto merge of #1412 - mrobinson:remove-per-clip-mask, r=kvark

Remove the per-item complex clipping and masking from the API This is the first step to getting all masking operations into the ClipScrollTree. We don't completely this feature, as it's still sued for the extra clips. These will be moved into the ClipScrollTree in a later change. This change allows us to fully eliminate the `DisplayListBuilder::push_clip_region` API and push its functionality into the `define_clip` API. Most of the changes here are: 1. Changes to the examples, wrench, and reftests to match the new API. 2. Adapting the "split rectangle clipped by rounded rect" optimization to use the ClipScrollTree. This is unfortunately a bit more complicated now, but now includes any rounded rect clip instead of per-item ones.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1412)

bors-servo · 2017-06-22T21:58:13Z

💔 Test failed - status-travis

kvark · 2017-06-22T22:04:13Z

Something scary is going on:

REFTEST reftests/aa/rounded-rects.yaml == reftests/aa/rounded-rects-ref.png
thread 'RenderBackend' has overflowed its stack
fatal runtime error: stack overflow


        Remove the per-item complex clipping and masking from the API

This is the first step to getting all masking operations into the ClipScrollTree. We don't completely this feature, as it's still sued for the extra clips. These will be moved into the ClipScrollTree in a later change. This change allows us to fully eliminate the DisplayListBuilder::push_clip_region API and push its functionality into the define_clip API. Most of the changes here are: 1. Changes to the examples, wrench, and reftests to match the new API. 2. Adapting the "split rectangle clipped by rounded rect" optimization to use the ClipScrollTree. This is unfortunately a bit more complicated now, but now includes any rounded rect clip instead of per-item ones.

mrobinson · 2017-06-23T07:04:45Z

@kvark This was an issue with the rebase on top of the rust upgrade. It should be fixed now. Sorry for the churn!

kvark · 2017-06-23T13:15:00Z

@mrobinson thanks!
@bors-servo r+

bors-servo · 2017-06-23T13:15:00Z

📌 Commit 0bf6655 has been approved by kvark

bors-servo · 2017-06-23T13:15:04Z

⌛ Testing commit 0bf6655 with merge df8c508...


        Auto merge of #1412 - mrobinson:remove-per-clip-mask, r=kvark

Remove the per-item complex clipping and masking from the API This is the first step to getting all masking operations into the ClipScrollTree. We don't completely this feature, as it's still sued for the extra clips. These will be moved into the ClipScrollTree in a later change. This change allows us to fully eliminate the `DisplayListBuilder::push_clip_region` API and push its functionality into the `define_clip` API. Most of the changes here are: 1. Changes to the examples, wrench, and reftests to match the new API. 2. Adapting the "split rectangle clipped by rounded rect" optimization to use the ClipScrollTree. This is unfortunately a bit more complicated now, but now includes any rounded rect clip instead of per-item ones.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1412)

bors-servo · 2017-06-23T14:01:26Z

☀️ Test successful - status-travis
Approved by: kvark
Pushing df8c508 to master...

staktrace · 2017-06-25T20:05:43Z

@mrobinson FYI the change to only treat non-zero-origin or clip-smaller-than-content clips as scroll frames resulted in a bunch of async-scrolling reftest regressions in gecko (link). Presumably once the API allows us to more explicitly create scroll frames vs non-scrolling clips and we start using that this will not be a problem any more. The patch I applied to verify this can be found here.

glennw · 2017-06-27T14:20:37Z

@mrobinson We should discuss the per-item clip stuff in SF - I have a few questions / concerns about possible performance impacts of not supporting extra_clips on items.


        Treat all clips as scroll frames (partial revert of servo#1412)

PR servo#1412 introduced a small semantic change mixed in with the removal of per-item complex clipping and masking. The semantic change resulted in numerous Gecko reftest failures as many scroll frames ended up being treated as clips. Eventually WR will have an explicit API to allow callers to distinguish scroll frames from clips, but until that is in place we should maintain the old behaviour of just treating all clips as potential scroll frames.

staktrace · 2017-07-07T12:32:45Z

Presumably once the API allows us to more explicitly create scroll frames vs non-scrolling clips and we start using that this will not be a problem any more.

This is happening in #1428.


        Auto merge of #2225 - kvark:yaml, r=glennw

Make image mask path optional in YAML reader Otherwise we crash during "show" command on some frames. Appears to be broken as of #1412  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2225)

mrobinson requested review from glennw and kvark Jun 21, 2017

mrobinson force-pushed the mrobinson:remove-per-clip-mask branch from e6bea06 to f5cfc51 Jun 21, 2017

kvark reviewed Jun 21, 2017

View changes

kvark approved these changes Jun 22, 2017

View changes

mrobinson force-pushed the mrobinson:remove-per-clip-mask branch from f5cfc51 to e6228a7 Jun 22, 2017

mrobinson force-pushed the mrobinson:remove-per-clip-mask branch from e6228a7 to 0bf6655 Jun 23, 2017

bors-servo merged commit 0bf6655 into servo:master Jun 23, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

mrobinson deleted the mrobinson:remove-per-clip-mask branch Jun 23, 2017

mrobinson mentioned this pull request Jun 24, 2017

Remove the per-item mask and complex clip feature #1377

Closed

staktrace mentioned this pull request Jun 25, 2017

Improved Local Combined Clip Rectangle #1374

Merged

staktrace mentioned this pull request Jul 7, 2017

Treat all clips as scroll frames (partial revert of #1412) #1459

Closed

kvark mentioned this pull request Dec 14, 2017

Make image mask path optional in YAML reader #2225

Merged

servo / webrender

Remove the per-item complex clipping and masking from the API #1412

Remove the per-item complex clipping and masking from the API #1412

mrobinson commented Jun 21, 2017 •

edited by larsbergstrom

mrobinson commented Jun 21, 2017

kvark left a comment

This comment has been minimized.

This comment has been minimized.

bors-servo commented Jun 22, 2017

kvark commented Jun 22, 2017

bors-servo commented Jun 22, 2017

bors-servo commented Jun 22, 2017

bors-servo commented Jun 22, 2017

kvark commented Jun 22, 2017

mrobinson commented Jun 23, 2017

kvark commented Jun 23, 2017

bors-servo commented Jun 23, 2017

bors-servo commented Jun 23, 2017

bors-servo commented Jun 23, 2017

staktrace commented Jun 25, 2017

glennw commented Jun 27, 2017

staktrace commented Jul 7, 2017

servo / webrender

Join GitHub today

Remove the per-item complex clipping and masking from the API #1412

Remove the per-item complex clipping and masking from the API #1412

Conversation

mrobinson commented Jun 21, 2017 • edited by larsbergstrom

mrobinson commented Jun 21, 2017

kvark left a comment

This comment has been minimized.

kvark Jun 21, 2017

This comment has been minimized.

mrobinson Jun 22, 2017 • edited

bors-servo commented Jun 22, 2017

kvark commented Jun 22, 2017

bors-servo commented Jun 22, 2017

bors-servo commented Jun 22, 2017

bors-servo commented Jun 22, 2017

kvark commented Jun 22, 2017

mrobinson commented Jun 23, 2017

kvark commented Jun 23, 2017

bors-servo commented Jun 23, 2017

bors-servo commented Jun 23, 2017

bors-servo commented Jun 23, 2017

staktrace commented Jun 25, 2017

glennw commented Jun 27, 2017

staktrace commented Jul 7, 2017

mrobinson commented Jun 21, 2017 •

edited by larsbergstrom

mrobinson Jun 22, 2017 •

edited