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

Correctness and optimizations for clips in different coord systems. #2980

Merged
merged 2 commits into from Aug 24, 2018

Conversation

@gw3583
Copy link
Collaborator

commented Aug 22, 2018

For clips that are in a different coordinate system, it's not
generally feasible to get a correct mapping into a different
coordinate system - so handle these clips in world space. For
now, the world space is actually screen space. However, it is
trivial to modify this so that the world space is the coordinate
system that we are rasterizing this picture in.

As an optimization, if we encounter a primitive that is large,
and has clips from a different coord system, but no local clips,
then segment it into a uniform grid. This allows large primitives
that would not otherwise be segmented to opt in to segmenting,
which can significantly reduce the size of the allocated clip
masks for that primitive.

As a further optimization, we are now able to generate a custom
clip chain instance for each segment, rather than using the
clip chain from the primitive. This allows us to build a more
optimized clip chain, based on the segment rectangle, which can
often remove redundant clips from the mask for each segment, or
even determine that this segment is not affected by any clips
at all.


This change is Reviewable

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2018

r? @kvark

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

Fixes: https://bugzilla.mozilla.org/show_bug.cgi?id=1483610

Some explanation / double checking is warranted for the try run:

[2] [wpt8] [wpt6] - unrelated intermittent

[clipping-4-canvas.html] [R2] max difference 8/1 - marked fuzzy on several other backends - should fuzz for WR too.

[1081185-1.html] [R8 + R2] This was fixed by #2974, and then marked as fixed during the WR update. However, that fix was actually incorrect and just masking the existing bug that causes the failure. Then, #2975 merged overnight, which fixes the bug in #2974 but re-exposes the bug which causes this test to fail. This patch doesn't actually alter the result of this test - it occurs on master due to the above reason. We should mark this test as FAIL again during the next update - it will be fixed by some of the follow up work to these current patch sets.

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2018

No idea what all those purple items are on the try or why it seems to be triggering dozens of re-runs by itself?

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2018

Well that was weird - the try job looked like it was in some weird loop where it just kept automatically respawning dozens of jobs. I cancelled the try run - I think all the important tests had completed?

@sotaroikeda

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2018

Sorry, I added a lot of talos tasks to check if talos performance is improved. To check performance, we need to run multiple talos tasks. The purple items often happens when many linux talos tasks are added.

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2018

@sotaroikeda Ah, that makes sense, thanks for the explanation! 👍

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 22, 2018

Added another try job which has the same results but I didn't cancel the run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=741d3c12cd7cb29e864fda2b4837a57fdde407d5

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 23, 2018

Added a follow up commit that has a few isolated parts from the patch I'm working on locally. The extra commit shouldn't have any noticeable effect, but I pushed a try run just in case:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91b0112bfa8957e12d9522a3612d3e4128f44a06

Copy link
Member

left a comment

Reviewed 9 of 12 files at r1, 5 of 5 files at r2.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gw3583)


webrender/src/clip.rs, line 203 at r2 (raw file):

    Local,
    Offset(LayoutVector2D),
    Transform(LayoutToWorldTransform),

Any reason to keep this separate, as opposed to a generalized CoordinateSpaceMapping<T> (where T is the destination space)?


webrender/src/clip.rs, line 871 at r2 (raw file):

        match *self {
            ClipItem::Rectangle(ref clip_rect, ClipMode::Clip) => {
                if let Some(inner_clip_rect) = project_inner_rect(transform, clip_rect) {

are we sure that the transformation here is going from a node to some of the parent node? because otherwise the projection needs to work entirely differently (remember inverse footprint routine?)


webrender/src/clip.rs, line 897 at r2 (raw file):

                    });

                if let Some(inner_clip_rect) = inner_clip_rect {

from here to the end of the match arm we have the same code as the previous arm
can we re-use it?


webrender/src/clip.rs, line 1098 at r2 (raw file):

        // we just checked for all the points to be in positive hemisphere, so `unwrap` is valid
        let points = [
            homogens[0].to_point2d().unwrap(),

you can just do transform.transform_point2d(xx)? here instead of getting the homogeneous coordinates and branching out on any of them being behind the near plane


webrender/src/gpu_types.rs, line 397 at r2 (raw file):

impl TransformMetadata {
    fn invalid() -> Self {

uh, this is tricky (the AxisAligned is very well valid)


webrender/src/prim_store.rs, line 2006 at r2 (raw file):

        }

        local_clip_count += 1;

what is this one for?


webrender/src/prim_store.rs, line 2053 at r2 (raw file):

    if is_large || rect_clips_only {
        // If there we no local clips, then we will subdivide the primitive into

"there we no"


webrender/src/prim_store.rs, line 2163 at r2 (raw file):

                Some(segment_clip_chain) => {
                    if segment_clip_chain.clips_range.count == 0 {
                        segment.clip_task_id = BrushSegmentTaskId::Opaque;

if non of the clip items reached this segment, why would it be Opaque as opposed to Empty??


webrender/src/util.rs, line 295 at r2 (raw file):

}

impl MaxRect for WorldRect {

let's just implement it for generic TypedRect<f32, T>

Copy link
Collaborator Author

left a comment

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @gw3583 and @kvark)


webrender/src/clip.rs, line 203 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Any reason to keep this separate, as opposed to a generalized CoordinateSpaceMapping<T> (where T is the destination space)?

Not specifically, but I'm not sure there's any benefit of it being generic in this case? I do think once this stuff is complete (and settles) we should introduce a new coordinate system though - a concept of a raster space (which will often be the same as world space, but not always). Does that sound reasonable?


webrender/src/clip.rs, line 871 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

are we sure that the transformation here is going from a node to some of the parent node? because otherwise the projection needs to work entirely differently (remember inverse footprint routine?)

I believe so - get_relative_transform assumes this and panics if it's not the case.


webrender/src/clip.rs, line 897 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

from here to the end of the match arm we have the same code as the previous arm
can we re-use it?

Done.


webrender/src/clip.rs, line 1098 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

you can just do transform.transform_point2d(xx)? here instead of getting the homogeneous coordinates and branching out on any of them being behind the near plane

Done.


webrender/src/gpu_types.rs, line 397 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

uh, this is tricky (the AxisAligned is very well valid)

Yea, it's not ideal. Is there something specific we should change it to for now? Or perhaps just a todo comment?


webrender/src/prim_store.rs, line 2006 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

what is this one for?

It's described below where it's used - we only segment into a uniform grid if there's no other segment inputs (which are the local clips).


webrender/src/prim_store.rs, line 2053 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

"there we no"

Done.


webrender/src/prim_store.rs, line 2163 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

if non of the clip items reached this segment, why would it be Opaque as opposed to Empty??

It means that it is a valid segment in the primitive, but has no clip masks on it. Thus, we can draw this segment in the opaque pass.


webrender/src/util.rs, line 295 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's just implement it for generic TypedRect<f32, T>

Done.

@gw3583 gw3583 force-pushed the gw3583:clip-opt-3 branch from 6fcb77d to 2fad514 Aug 23, 2018
@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 23, 2018

@kvark Thanks, addressed those comments, I think. Once you're happy with the changes, I'll do another try run before merging, just to make sure I didn't break anything with those changes.

For clips that are in a different coordinate system, it's not
generally feasible to get a correct mapping into a different
coordinate system - so handle these clips in world space. For
now, the world space is actually screen space. However, it is
trivial to modify this so that the world space is the coordinate
system that we are rasterizing this picture in.

As an optimization, if we encounter a primitive that is large,
and has clips from a different coord system, but no local clips,
then segment it into a uniform grid. This allows large primitives
that would not otherwise be segmented to opt in to segmenting,
which can significantly reduce the size of the allocated clip
masks for that primitive.

As a further optimization, we are now able to generate a custom
clip chain instance for each segment, rather than using the
clip chain from the primitive. This allows us to build a more
optimized clip chain, based on the segment rectangle, which can
often remove redundant clips from the mask for each segment, or
even determine that this segment is not affected by any clips
at all.
@gw3583 gw3583 force-pushed the gw3583:clip-opt-3 branch from 2fad514 to 08dc816 Aug 23, 2018
@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 23, 2018

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 23, 2018

Latest try run looks good.

@kvark

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

Those R8 and R2s are expected failures?

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 24, 2018

@kvark Yep - there's some details on why here #2980 (comment)

@kvark
kvark approved these changes Aug 24, 2018
Copy link
Member

left a comment

:lgtm:

Reviewed 3 of 5 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kvark and @gw3583)


webrender/src/clip.rs, line 1065 at r3 (raw file):

}

fn project_rect(

is it much different now from calculate_screen_bounding_rect?


webrender/src/clip.rs, line 1117 at r3 (raw file):

) -> Option<WorldRect> {
    let points = [
        transform.transform_point2d(&rect.origin),

you can just add ? at the end of these lines and avoid a bunch of code that follows


webrender/src/gpu_types.rs, line 397 at r2 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Yea, it's not ideal. Is there something specific we should change it to for now? Or perhaps just a todo comment?

TODO is fine

@gw3583 gw3583 force-pushed the gw3583:clip-opt-3 branch from 08dc816 to fc4b78f Aug 24, 2018
Copy link
Collaborator Author

left a comment

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kvark and @gw3583)


webrender/src/clip.rs, line 1065 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

is it much different now from calculate_screen_bounding_rect?

Only in that it directly returns world coordinates without scaling to device space and casting to ints. In the upcoming patches, we'll start using this directly, and only convert to integer device coordinates when we actually need to (which is a small fraction of the total primitives).


webrender/src/clip.rs, line 1117 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

you can just add ? at the end of these lines and avoid a bunch of code that follows

Nice, done!


webrender/src/gpu_types.rs, line 397 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

TODO is fine

Done.

@kvark

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

📌 Commit fc4b78f has been approved by kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

⌛️ Testing commit fc4b78f with merge 78aa14b...

bors-servo added a commit that referenced this pull request Aug 24, 2018
Correctness and optimizations for clips in different coord systems.

For clips that are in a different coordinate system, it's not
generally feasible to get a correct mapping into a different
coordinate system - so handle these clips in world space. For
now, the world space is actually screen space. However, it is
trivial to modify this so that the world space is the coordinate
system that we are rasterizing this picture in.

As an optimization, if we encounter a primitive that is large,
and has clips from a different coord system, but no local clips,
then segment it into a uniform grid. This allows large primitives
that would not otherwise be segmented to opt in to segmenting,
which can significantly reduce the size of the allocated clip
masks for that primitive.

As a further optimization, we are now able to generate a custom
clip chain instance for each segment, rather than using the
clip chain from the primitive. This allows us to build a more
optimized clip chain, based on the segment rectangle, which can
often remove redundant clips from the mask for each segment, or
even determine that this segment is not affected by any clips
at all.

<!-- 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/2980)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

💔 Test failed - status-appveyor

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 24, 2018

Looks like a network error on the windows CI machine, hopefully intermittent.

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

⌛️ Testing commit fc4b78f with merge eae33f7...

bors-servo added a commit that referenced this pull request Aug 24, 2018
Correctness and optimizations for clips in different coord systems.

For clips that are in a different coordinate system, it's not
generally feasible to get a correct mapping into a different
coordinate system - so handle these clips in world space. For
now, the world space is actually screen space. However, it is
trivial to modify this so that the world space is the coordinate
system that we are rasterizing this picture in.

As an optimization, if we encounter a primitive that is large,
and has clips from a different coord system, but no local clips,
then segment it into a uniform grid. This allows large primitives
that would not otherwise be segmented to opt in to segmenting,
which can significantly reduce the size of the allocated clip
masks for that primitive.

As a further optimization, we are now able to generate a custom
clip chain instance for each segment, rather than using the
clip chain from the primitive. This allows us to build a more
optimized clip chain, based on the segment rectangle, which can
often remove redundant clips from the mask for each segment, or
even determine that this segment is not affected by any clips
at all.

<!-- 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/2980)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

💔 Test failed - status-appveyor

@kvark

This comment has been minimized.

Copy link
Member

commented Aug 24, 2018

AppVeyor has been cranky all day, unfortunately (see gfx-rs/gfx#2336), even though the status page is all green :(

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 24, 2018

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

⌛️ Testing commit fc4b78f with merge ce4569c...

bors-servo added a commit that referenced this pull request Aug 24, 2018
Correctness and optimizations for clips in different coord systems.

For clips that are in a different coordinate system, it's not
generally feasible to get a correct mapping into a different
coordinate system - so handle these clips in world space. For
now, the world space is actually screen space. However, it is
trivial to modify this so that the world space is the coordinate
system that we are rasterizing this picture in.

As an optimization, if we encounter a primitive that is large,
and has clips from a different coord system, but no local clips,
then segment it into a uniform grid. This allows large primitives
that would not otherwise be segmented to opt in to segmenting,
which can significantly reduce the size of the allocated clip
masks for that primitive.

As a further optimization, we are now able to generate a custom
clip chain instance for each segment, rather than using the
clip chain from the primitive. This allows us to build a more
optimized clip chain, based on the segment rectangle, which can
often remove redundant clips from the mask for each segment, or
even determine that this segment is not affected by any clips
at all.

<!-- 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/2980)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

💔 Test failed - status-appveyor

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 24, 2018

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

⌛️ Testing commit fc4b78f with merge 0ad9b62...

bors-servo added a commit that referenced this pull request Aug 24, 2018
Correctness and optimizations for clips in different coord systems.

For clips that are in a different coordinate system, it's not
generally feasible to get a correct mapping into a different
coordinate system - so handle these clips in world space. For
now, the world space is actually screen space. However, it is
trivial to modify this so that the world space is the coordinate
system that we are rasterizing this picture in.

As an optimization, if we encounter a primitive that is large,
and has clips from a different coord system, but no local clips,
then segment it into a uniform grid. This allows large primitives
that would not otherwise be segmented to opt in to segmenting,
which can significantly reduce the size of the allocated clip
masks for that primitive.

As a further optimization, we are now able to generate a custom
clip chain instance for each segment, rather than using the
clip chain from the primitive. This allows us to build a more
optimized clip chain, based on the segment rectangle, which can
often remove redundant clips from the mask for each segment, or
even determine that this segment is not affected by any clips
at all.

<!-- 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/2980)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

💔 Test failed - status-appveyor

@jrmuizel

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

CRL or OCSP is broken?

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

⌛️ Testing commit fc4b78f with merge ea7afd0...

bors-servo added a commit that referenced this pull request Aug 24, 2018
Correctness and optimizations for clips in different coord systems.

For clips that are in a different coordinate system, it's not
generally feasible to get a correct mapping into a different
coordinate system - so handle these clips in world space. For
now, the world space is actually screen space. However, it is
trivial to modify this so that the world space is the coordinate
system that we are rasterizing this picture in.

As an optimization, if we encounter a primitive that is large,
and has clips from a different coord system, but no local clips,
then segment it into a uniform grid. This allows large primitives
that would not otherwise be segmented to opt in to segmenting,
which can significantly reduce the size of the allocated clip
masks for that primitive.

As a further optimization, we are now able to generate a custom
clip chain instance for each segment, rather than using the
clip chain from the primitive. This allows us to build a more
optimized clip chain, based on the segment rectangle, which can
often remove redundant clips from the mask for each segment, or
even determine that this segment is not affected by any clips
at all.

<!-- 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/2980)
<!-- Reviewable:end -->
@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 24, 2018

@jrmuizel Yea, seems to be affecting other projects using appveyor too.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

💔 Test failed - status-appveyor

@jrmuizel

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

rust-lang/cargo#2464 seems like an old related issue.

@gw3583

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 24, 2018

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

⌛️ Testing commit fc4b78f with merge a63602f...

bors-servo added a commit that referenced this pull request Aug 24, 2018
Correctness and optimizations for clips in different coord systems.

For clips that are in a different coordinate system, it's not
generally feasible to get a correct mapping into a different
coordinate system - so handle these clips in world space. For
now, the world space is actually screen space. However, it is
trivial to modify this so that the world space is the coordinate
system that we are rasterizing this picture in.

As an optimization, if we encounter a primitive that is large,
and has clips from a different coord system, but no local clips,
then segment it into a uniform grid. This allows large primitives
that would not otherwise be segmented to opt in to segmenting,
which can significantly reduce the size of the allocated clip
masks for that primitive.

As a further optimization, we are now able to generate a custom
clip chain instance for each segment, rather than using the
clip chain from the primitive. This allows us to build a more
optimized clip chain, based on the segment rectangle, which can
often remove redundant clips from the mask for each segment, or
even determine that this segment is not affected by any clips
at all.

<!-- 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/2980)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2018

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

@bors-servo bors-servo merged commit fc4b78f into servo:master Aug 24, 2018
2 of 4 checks passed
2 of 4 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
code-review/reviewable 3 files, 3 discussions left (kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.