Skip to content
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

Support snapping on clip masks, and fast path rectangle + clip out mode. #2618

Merged
merged 2 commits into from Apr 6, 2018

Conversation

@glennw
Copy link
Member

glennw commented Apr 5, 2018

This change is Reviewable

@glennw
Copy link
Member Author

glennw commented Apr 5, 2018

This supersedes #2608 (it includes that commit).

This PR achieves two things:

  • Support snapping of axis-aligned clip masks (mostly reviewed in #2608).
  • Native Rectangle + ClipOut mode (an optimization, and accuracy win).

This allows primitives that only contain clip rects to avoid generating
a clip mask, and also fixes a number of reftests.

Fixes #2595.
Fixes #2501.

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=89310dbe1725653477a524a69804cd34dcb084e1

Of the relevant reftest differences:

R4 - 1 new PASS
R6 - 1 new PASS
R7 - 1 new PASS, 1 new FAIL (agreed to fuzz #2608 (comment))

r? @mrobinson or @kvark

@kvark
Copy link
Member

kvark commented Apr 5, 2018

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


webrender/res/clip_shared.glsl, line 85 at r1 (raw file):

    // compute the point position inside the scroll node, in CSS space
    vec2 vertex_pos = device_pos +

I don't understand this part. When we snap, we want the final VS results to be snapped. But this code uses the pre-snapped device pos


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

        }

        if is_large || rect_clips_only {

why are we segmenting a small but rect-only primitive?


Comments from Reviewable

@glennw
Copy link
Member Author

glennw commented Apr 5, 2018

Review status: all files reviewed at latest revision, 2 unresolved discussions.


webrender/res/clip_shared.glsl, line 85 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I don't understand this part. When we snap, we want the final VS results to be snapped. But this code uses the pre-snapped device pos

Because in the clip shaders, the vertices are the flat coordinates of the corners of the 2d bounding rect of the clip task. So I don't see how / why we'd be able to apply snapping here? What this PR does is keep the original vertex positions the same as the task rect, but adjust the interpolants for the local position at each corner vertex such that the snapping is accounted for. At least, that's the intent :) Does that sound reasonable?


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

Previously, kvark (Dzmitry Malyshau) wrote…

why are we segmenting a small but rect-only primitive?

Because rect only primitives can be efficiently segmented into CPU regions and thus completely avoid any clip-mask at all. For example, I intend to draw simple borders as a rectangle with a clip + clip-out rectangle (got a local patch nearly ready for this). This means that for the most common border types we don't invoke the border shader at all, and don't allocate / render any clip masks in the case of no rounded corners.


Comments from Reviewable

@bors-servo
Copy link
Contributor

bors-servo commented Apr 6, 2018

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

gw3583 added 2 commits Apr 4, 2018
Previously, all ClipOut items would result in a clip mask being
generated. As we start to use ClipOut mode more often, it makes
sense to handle this natively in the segment builder and avoid
generating a clip-mask at all, where possible.

Fixes #2595.
Fixes #2501.
@glennw glennw force-pushed the glennw:clip-mask-snap-clip-out branch from 400f99e to 076b443 Apr 6, 2018
@glennw
Copy link
Member Author

glennw commented Apr 6, 2018

Rebased.

@kvark
Copy link
Member

kvark commented Apr 6, 2018

:lgtm:


Reviewed 4 of 4 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@kvark
Copy link
Member

kvark commented Apr 6, 2018

Thank you!
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Apr 6, 2018

📌 Commit 076b443 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Apr 6, 2018

Testing commit 076b443 with merge fe24137...

bors-servo added a commit that referenced this pull request Apr 6, 2018
Support snapping on clip masks, and fast path rectangle + clip out mode.

<!-- 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/2618)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 6, 2018

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

@bors-servo bors-servo merged commit 076b443 into servo:master Apr 6, 2018
4 checks passed
4 checks passed
Taskcluster (pull_request) TaskGroup: success
Details
code-review/reviewable 13 files reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@glennw glennw deleted the glennw:clip-mask-snap-clip-out branch Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.