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

Smarter opaque/transparent splits and transformed AA #2016

Merged
merged 4 commits into from Nov 9, 2017

Conversation

@kvark
Copy link
Member

kvark commented Nov 8, 2017

Related to #1384, #1797, #1297
Also TODO: Gecko try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4e8ff793bcf938c2cb9ca414368f4bc808a66ad

Currently blocked on the rotated-image.yaml breakage, investigation is ongoing - solved!


This change is Reviewable

@kvark kvark force-pushed the kvark:rect-split branch from 1d9c786 to 2a1b689 Nov 8, 2017
@kvark kvark changed the title [WIP] Smarter opaque/transparent splits and transformed AA Smarter opaque/transparent splits and transformed AA Nov 8, 2017
@glennw
Copy link
Member

glennw commented Nov 9, 2017

Reviewed 21 of 21 files at r1.
Review status: all files reviewed at latest revision, 5 unresolved discussions.


webrender/res/prim_shared.glsl, line 772 at r1 (raw file):

Rectangle fetch_rectangle(int address) {
    vec4 data[2] = fetch_from_resource_cache_2(address);
    vec4 mask = vec4((int(data[1].x) & ivec4(1,2,4,8)) != ivec4(0));

Neat!


webrender/src/frame.rs, line 46 at r1 (raw file):

    pipeline_epochs: Vec<(PipelineId, Epoch)>,
    replacements: Vec<(ClipId, ClipId)>,
    opaque_parts: Vec<LayoutRect>,

Perhaps add a comment here saying that these are part of the context to avoid reallocations?


webrender/src/frame.rs, line 638 at r1 (raw file):

    /// the rounded parts of the clip. This is pretty simple now, so has a lot of
    /// potential for further optimizations.
    fn try_to_add_rectangle_splitting_on_clip(

It might be worth only calling this function if the size of the rectangle is over some arbitrary threshold (maybe 256?).


webrender/src/util.rs, line 305 at r1 (raw file):

    }

    fn split_rectangles(

This seems like it would still result in t-junctions if the radii are not the same at each corner? I might just be not reading this carefully enough...


webrender_api/src/display_item.rs, line 42 at r1 (raw file):

}

bitflags! {

NOTE: Once we make the primitive / clip segments work internally, this shouldn't need to be in the public API, and we can move it from the api crate to the main crate. But we need it here for now since we're actually creating new primitives when splitting via the public API.


Comments from Reviewable

@kvark
Copy link
Member Author

kvark commented Nov 9, 2017

Review points are addressed now, and the try push looks green.


Review status: 20 of 21 files reviewed at latest revision, 5 unresolved discussions.


webrender/src/frame.rs, line 46 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Perhaps add a comment here saying that these are part of the context to avoid reallocations?

fixed


webrender/src/frame.rs, line 638 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

It might be worth only calling this function if the size of the rectangle is over some arbitrary threshold (maybe 256?).

good idea, added now


webrender/src/util.rs, line 305 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

This seems like it would still result in t-junctions if the radii are not the same at each corner? I might just be not reading this carefully enough...

this implementation is sub-optimal (still, even if much better than the old one) for the fact it works off the inner rectangle that is conservative: it takes the biggest of the radii affecting each side of the produced rectangle, so we just waste some space if one of the corners have lesser radii than the others.
My plan was to leave this for further optimization, given that the framework is set (framework of producing the list of opaque/transparent parts).


webrender_api/src/display_item.rs, line 42 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

NOTE: Once we make the primitive / clip segments work internally, this shouldn't need to be in the public API, and we can move it from the api crate to the main crate. But we need it here for now since we're actually creating new primitives when splitting via the public API.

good point!


Comments from Reviewable

@glennw glennw self-requested a review Nov 9, 2017
@glennw
glennw approved these changes Nov 9, 2017
Copy link
Member

glennw left a comment

Yep, it's definitely an improvement. We can look into the t-junction with non uniform radii issue as a follow up. r=me once we have a green gecko try run.

@kvark
Copy link
Member Author

kvark commented Nov 9, 2017

Thanks, it is all green.
@bors-servo r+

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2017

📌 Commit 98b0991 has been approved by kvark

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2017

Testing commit 98b0991 with merge 6de1b7c...

bors-servo added a commit that referenced this pull request Nov 9, 2017
Smarter opaque/transparent splits and transformed AA

Related to #1384, #1797, #1297
~~Also TODO:~~ Gecko try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4e8ff793bcf938c2cb9ca414368f4bc808a66ad

~~Currently blocked on the `rotated-image.yaml` breakage, investigation is ongoing~~ - solved!

<!-- 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/2016)
<!-- Reviewable:end -->
@kvark
Copy link
Member Author

kvark commented Nov 9, 2017

@bors-servo r-
@bors-servo r=glennw

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2017

💡 This pull request was already approved, no need to approve it again.

  • This pull request is currently being tested. If there's no response from the continuous integration service, you may use retry to trigger a build again.
@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2017

📌 Commit 98b0991 has been approved by glennw

@bors-servo
Copy link
Contributor

bors-servo commented Nov 9, 2017

Testing commit 98b0991 with merge 8e4f5c1...

bors-servo added a commit that referenced this pull request Nov 9, 2017
Smarter opaque/transparent splits and transformed AA

Related to #1384, #1797, #1297
~~Also TODO:~~ Gecko try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=e4e8ff793bcf938c2cb9ca414368f4bc808a66ad

~~Currently blocked on the `rotated-image.yaml` breakage, investigation is ongoing~~ - solved!

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

bors-servo commented Nov 9, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: glennw
Pushing 8e4f5c1 to master...

@bors-servo bors-servo merged commit 98b0991 into servo:master Nov 9, 2017
4 of 5 checks passed
4 of 5 checks passed
code-review/reviewable 1 file, 5 discussions left (glennw, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@kvark kvark deleted the kvark:rect-split branch Nov 9, 2017
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

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