Introduce ClipChain #1833

mrobinson · 2017-10-09T17:45:05Z

Clip chains are a linked list of ClipWorkItems that are used to avoid
having to recalculate the stack of clips necessary to render items when
switching between different ClipScrollNodes. The ClipChains are
calculated once on a per node basis during update_transform.

This change is

mrobinson · 2017-10-09T17:48:02Z

I did a bit of performance testing for this change. For nasty situations (a thousand deep nested clip), ClipScrollNode::update_all_transforms now takes about as long as ClipScrollNode::update_all_transforms and FrameBuilder::recalculate_clip_scroll_nodes together (as one would expect). Meanwhile a per-clip node cost has been eliminated, meaning that rendering should be slightly faster overall.

glennw · 2017-10-10T06:06:03Z

I didn't get to this today, sorry. I'll look at it first thing tomorrow.

The CI seems to be failing on mac - it might be a float accuracy issue with the clip changes that requires re-generating the reference images?

mrobinson · 2017-10-10T22:44:12Z

@glennw I'm pretty sure the failure here is due to your recent fix for the behavior of inner_rect. To keep this fix, I'll need to implement the thing we discussed which was the id assignment for each axis-aligned group of coordinate spaces. I've almost got that finished, but I need to fix one more issue. Hopefully I can have an updated PR for this tomorrow.

glennw · 2017-10-11T04:22:43Z

OK, I'll hold off on reviewing until the updated patch. @kvark pointed out to me that one of my assumptions in that fix was incorrect - so if there's a better way to fix it that still passes the new reftest, go for it.

mrobinson · 2017-10-11T13:03:37Z

@glennw Okay. I think the branch is ready now. The good thing about the change is that now we can start using the coordinate_system_id for more kinds of optimizations.

kvark

Looks fine to me, minus one moment.

kvark · 2017-10-11T15:48:52Z

webrender/src/render_task.rs

+
+                // We cannot apply any kind of optimization based on inner rectangles when the
+                // clips exists in another axis-aligned coordinate system.
+                if node.work_item.has_compatible_coordinate_system(current_coordinate_system_id) {


I don't think this is correct. Inner rects are valid regardless of the coordinate systems.

This is just a replacement for the workaround in #1820. Would it be better for me to add a FIXME and open an issue?

yes, please

glennw · 2017-10-12T03:08:35Z

This looks good to me - but we should do a gecko/servo try run before merging too.

bors-servo · 2017-10-12T11:17:26Z

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

mrobinson · 2017-10-12T15:54:19Z

Here's the try run for this PR: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e48d88f50d99977ba93d86b7a36dd9c58c9d5cd&selectedJob=136489766

There are a few tests failing, but it seems like the differences are subpixel. Yet, this PR should change rendering at all. I will investigate a bit more.

glennw · 2017-10-12T23:12:22Z

@mrobinson Looking at the reference image that was updated here. There appears to be AA being applied along the axis-aligned edges of the rectangle. That shouldn't be occurring - those edges should be pixel perfect. It's quite possible that fixing that will fix the gecko reftest errors, I think.

I wonder if an extra clip / mask is being applied that wasn't previously?

glennw · 2017-10-12T23:13:12Z

It's probably worth fixing that ref test in wrench before looking into the gecko failures, since they are probably the same issue.

mrobinson · 2017-10-13T13:38:31Z

Okay. After looking at this a bit I found two bugs in my original PR. The first was that the condition for the inner_rect workaround was reversed (!). Additionally the outer bounds of the clip were not being passed properly down the ClipScrollTree hierarchy. I have fixed these issues and have done the rebase.

Here is the try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=604e9e610ea14c65039f2a5b9e258ce23585b982&selectedJob=136791157

Of the unexpected results:

Some are due sticky fixes I made recently
Two unexpected passes appear to be intermittent tests deviations
One is an accessibility failure that looks completely unrelated

kvark · 2017-10-13T13:41:19Z

@mrobinson awesome, thank you!
@bors-servo r+

bors-servo · 2017-10-13T13:41:20Z

📌 Commit fed7c88 has been approved by kvark

bors-servo · 2017-10-13T13:41:22Z

⌛ Testing commit fed7c88 with merge 7b5553d...

Introduce ClipChain Clip chains are a linked list of ClipWorkItems that are used to avoid having to recalculate the stack of clips necessary to render items when switching between different ClipScrollNodes. The ClipChains are calculated once on a per node basis during update_transform.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1833)

bors-servo · 2017-10-13T14:53:56Z

💔 Test failed - status-travis

kvark · 2017-10-13T15:06:32Z

Downloading euclid v0.15.2
warning: spurious network error (2 tries remaining): failed to get 200 response from https://crates.io/api/v1/crates/euclid/0.15.2/download, got 503
warning: spurious network error (1 tries remaining): failed to get 200 response from https://crates.io/api/v1/crates/euclid/0.15.2/download, got 503
error: unable to get packages from source

@bors-servo retry

bors-servo · 2017-10-13T17:53:45Z

⌛ Testing commit fed7c88 with merge ce81770...

Introduce ClipChain Clip chains are a linked list of ClipWorkItems that are used to avoid having to recalculate the stack of clips necessary to render items when switching between different ClipScrollNodes. The ClipChains are calculated once on a per node basis during update_transform.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1833)

bors-servo · 2017-10-14T00:41:01Z

💔 Test failed - status-travis

bors-servo · 2017-10-14T02:53:13Z

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

Clip chains are a linked list of ClipWorkItems that are used to avoid having to recalculate the stack of clips necessary to render items when switching between different ClipScrollNodes. The ClipChains are calculated once on a per node basis during update_transform. In order to make this happen we need to start tracking when ClipScrollNodes share axis-aligned coordinate spaces. We can use these ids to render rectangular clips into masks when necessary.

mrobinson · 2017-10-14T11:26:39Z

The failure looks like it was related to the merge conflict. I rebased the branch.

mrobinson · 2017-10-14T17:52:03Z

@bors-servo r=kvark

bors-servo · 2017-10-14T17:52:03Z

📌 Commit 9822f69 has been approved by kvark

bors-servo · 2017-10-14T17:52:06Z

⌛ Testing commit 9822f69 with merge d97dc8e...

Introduce ClipChain Clip chains are a linked list of ClipWorkItems that are used to avoid having to recalculate the stack of clips necessary to render items when switching between different ClipScrollNodes. The ClipChains are calculated once on a per node basis during update_transform.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1833)

bors-servo · 2017-10-14T18:42:12Z

💔 Test failed - status-travis

mrobinson · 2017-10-14T20:44:48Z

@bors-servo retry

"The job exceeded the maximum time limit for jobs, and has been terminated." Seems that this was an issue with the build infrastructure.

bors-servo · 2017-10-14T21:07:22Z

⌛ Testing commit 9822f69 with merge 0b52dd7...

Introduce ClipChain Clip chains are a linked list of ClipWorkItems that are used to avoid having to recalculate the stack of clips necessary to render items when switching between different ClipScrollNodes. The ClipChains are calculated once on a per node basis during update_transform.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1833)

bors-servo · 2017-10-14T22:15:07Z

☀️ Test successful - status-appveyor, status-travis
Approved by: kvark
Pushing 0b52dd7 to master...

mrobinson requested a review from glennw October 9, 2017 17:45

mrobinson force-pushed the clip-chain branch from 400aa47 to 26c4c7f Compare October 11, 2017 11:50

kvark approved these changes Oct 11, 2017

View reviewed changes

mrobinson force-pushed the clip-chain branch from 26c4c7f to fed7c88 Compare October 13, 2017 11:40

mrobinson force-pushed the clip-chain branch from fed7c88 to 9822f69 Compare October 14, 2017 11:15

bors-servo merged commit 9822f69 into servo:master Oct 14, 2017

mrobinson deleted the clip-chain branch December 8, 2017 13:18

Introduce ClipChain #1833

Introduce ClipChain #1833

Uh oh!

Conversation

mrobinson commented Oct 9, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrobinson commented Oct 9, 2017

Uh oh!

glennw commented Oct 10, 2017

Uh oh!

mrobinson commented Oct 10, 2017

Uh oh!

glennw commented Oct 11, 2017

Uh oh!

mrobinson commented Oct 11, 2017

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kvark Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

mrobinson Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

kvark Oct 11, 2017

Choose a reason for hiding this comment

Uh oh!

glennw commented Oct 12, 2017

Uh oh!

bors-servo commented Oct 12, 2017

Uh oh!

mrobinson commented Oct 12, 2017

Uh oh!

glennw commented Oct 12, 2017

Uh oh!

glennw commented Oct 12, 2017

Uh oh!

mrobinson commented Oct 13, 2017

Uh oh!

kvark commented Oct 13, 2017

Uh oh!

bors-servo commented Oct 13, 2017

Uh oh!

bors-servo commented Oct 13, 2017

Uh oh!

bors-servo commented Oct 13, 2017

Uh oh!

kvark commented Oct 13, 2017

Uh oh!

bors-servo commented Oct 13, 2017

Uh oh!

bors-servo commented Oct 14, 2017

Uh oh!

bors-servo commented Oct 14, 2017

Uh oh!

mrobinson commented Oct 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrobinson commented Oct 14, 2017

Uh oh!

bors-servo commented Oct 14, 2017

Uh oh!

bors-servo commented Oct 14, 2017

Uh oh!

bors-servo commented Oct 14, 2017

Uh oh!

mrobinson commented Oct 14, 2017

Uh oh!

bors-servo commented Oct 14, 2017

Uh oh!

bors-servo commented Oct 14, 2017

Uh oh!

Uh oh!

mrobinson commented Oct 9, 2017 •

edited by larsbergstrom

Loading

mrobinson commented Oct 14, 2017 •

edited

Loading