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

Refactor flattener for mix-blend-mode optimizations. #3042

Merged
merged 1 commit into from Sep 11, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Contributor

gw3583 commented Sep 11, 2018

This patch doesn't contain any user visible changes. It does
change the display list flattener to append pictures to their
parent picture during pop_stacking_context instead of during
push_stacking_context.

This not only makes the code clearer, it also makes it possible
to introduce some optimizations to mix-blend-mode in the
future. Specifically, we want to detect mix blend mode, and
insert a new picture node in the tree which re-parents the
existing stacking context, in some cases.


This change is Reviewable

@gw3583

This comment has been minimized.

Show comment
Hide comment
Contributor

gw3583 commented Sep 11, 2018

@kvark

Looks quite a bit more comprehensible than before!

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


webrender/src/display_list_flattener.rs, line 950 at r1 (raw file):

        let mut composite_mode = None;

        // If this stacking context if the root of a pipeline, and the caller

"if the" -> "is the"


webrender/src/display_list_flattener.rs, line 951 at r1 (raw file):

        // If this stacking context if the root of a pipeline, and the caller
        // has requested it as an output frame, create a render task to isolate it.

should we isolate it only if it's a root of pipeline? Do we just ignore output_pipelines otherwise?


webrender/src/display_list_flattener.rs, line 958 at r1 (raw file):

        }

        // Force an intermediate surface if the stacking context

why should we spawn an intermediate surface if the clip node ends up in the same coordinate system as the primitives?


webrender/src/display_list_flattener.rs, line 1102 at r1 (raw file):

        // to an off-screen surface.
        let pic_count = sc.root_prim_index.0 + 1 - sc.leaf_prim_index.0;
        for i in 0 .. pic_count {

can we do sc.leaf_prim_index.0 .. sc.root_prim_index.0 + 1 right here?


webrender/src/display_list_flattener.rs, line 1121 at r1 (raw file):

                .iter()
                .rev()
                .find(|sc| sc.establishes_3d_context)

love those iterators :)

Refactor flattener for mix-blend-mode optimizations.
This patch doesn't contain any user visible changes. It does
change the display list flattener to append pictures to their
parent picture during pop_stacking_context instead of during
push_stacking_context.

This not only makes the code clearer, it also makes it possible
to introduce some optimizations to mix-blend-mode in the
future. Specifically, we want to detect mix blend mode, and
insert a new picture node in the tree which re-parents the
existing stacking context, in some cases.
@gw3583

Reviewable status: 0 of 1 files reviewed, 5 unresolved discussions (waiting on @kvark and @gw3583)


webrender/src/display_list_flattener.rs, line 950 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

"if the" -> "is the"

Done.


webrender/src/display_list_flattener.rs, line 951 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

should we isolate it only if it's a root of pipeline? Do we just ignore output_pipelines otherwise?

Yep - the semantics right now of output pipelines is that you can select an iframe (pipeline) to be output only.


webrender/src/display_list_flattener.rs, line 958 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why should we spawn an intermediate surface if the clip node ends up in the same coordinate system as the primitives?

Long term, we shouldn't (necessarily). The initial implementation is conservative (e.g. we need an intermediate surface even in same coord system if the clip contains an image mask and/or rounded rect etc).


webrender/src/display_list_flattener.rs, line 1102 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we do sc.leaf_prim_index.0 .. sc.root_prim_index.0 + 1 right here?

Done.


webrender/src/display_list_flattener.rs, line 1121 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

love those iterators :)

Done.

@gw3583

This comment has been minimized.

Show comment
Hide comment
@gw3583

gw3583 Sep 11, 2018

Contributor

@kvark Thanks! Comments addressed and pushed an updated commit.

Contributor

gw3583 commented Sep 11, 2018

@kvark Thanks! Comments addressed and pushed an updated commit.

@kvark

kvark approved these changes Sep 11, 2018

:lgtm:

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@kvark

This comment has been minimized.

Show comment
Hide comment
@kvark
Member

kvark commented Sep 11, 2018

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 11, 2018

Contributor

📌 Commit 85b5a3f has been approved by kvark

Contributor

bors-servo commented Sep 11, 2018

📌 Commit 85b5a3f has been approved by kvark

@bors-servo

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 11, 2018

Contributor

⌛️ Testing commit 85b5a3f with merge 703a3fd...

Contributor

bors-servo commented Sep 11, 2018

⌛️ Testing commit 85b5a3f with merge 703a3fd...

bors-servo added a commit that referenced this pull request Sep 11, 2018

Auto merge of #3042 - gw3583:checkpoint-mix-blend-mode-flatten, r=kvark
Refactor flattener for mix-blend-mode optimizations.

This patch doesn't contain any user visible changes. It does
change the display list flattener to append pictures to their
parent picture during pop_stacking_context instead of during
push_stacking_context.

This not only makes the code clearer, it also makes it possible
to introduce some optimizations to mix-blend-mode in the
future. Specifically, we want to detect mix blend mode, and
insert a new picture node in the tree which re-parents the
existing stacking context, in some cases.

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

This comment has been minimized.

Show comment
Hide comment
@bors-servo

bors-servo Sep 11, 2018

Contributor

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

Contributor

bors-servo commented Sep 11, 2018

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

@bors-servo bors-servo merged commit 85b5a3f into servo:master Sep 11, 2018

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
code-review/reviewable 1 file reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment