Support multiple composite operations in a StackingContext #802

eqrion · 2017-01-29T09:05:14Z

A StackingContext holds a Vec and a MixBlendMode, but currently only one operation is applied. This change modifies the render task building code to respect multiple composite operations. It's also careful to apply filters first and then the mix-blend-mode according to the spec.

This change is

eqrion · 2017-01-29T09:07:23Z

I've tested this on some basic filter and mix-blend-mode combinations and it seems to work. I used wrench plus the changes in #801.

glennw · 2017-01-30T06:06:26Z

Reviewed 2 of 3 files at r1.
Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

webrender/src/tiling.rs, line 1880 at r1 (raw file):

                      pipeline_id: PipelineId,
                      scroll_layer_id: ScrollLayerId,
                      composite_ops: &CompositeOps) {

Let's remove the & and move the CompositeOps here - that will avoid the clone() below.

Comments from Reviewable

glennw · 2017-01-30T06:06:45Z

This looks great! Just one minor issue above.

eqrion · 2017-01-30T20:55:29Z

Review status: 1 of 3 files reviewed at latest revision, 1 unresolved discussion.

webrender/src/tiling.rs, line 1880 at r1 (raw file):

Previously, glennw (Glenn Watson) wrote…

Let's remove the & and move the CompositeOps here - that will avoid the clone() below.

The &[CompositionOp] was passed into push_layer twice if the stacking context was root with a background color. But I don't think that extra layer needs CompositeOps, so I've removed the & and give that layer CompositeOps::empty().

Comments from Reviewable

kvark

It's nice to see the removal of CompositionOp, which makes things a bit more straightforward. I got a single concern here, and it's not critical.

kvark · 2017-01-30T21:31:55Z

webrender/src/tiling.rs

-                            let prev_task = mem::replace(&mut current_task, new_task);
-                            alpha_task_stack.push(prev_task);
-                        }
+                    let composite_count = layer.composite_ops.count();


why do you need to separate task creation from task initialization (which is happening in PopStackingContext)? Could we have all the logic in one place (in the pop code) instead?

kvark · 2017-01-30T21:33:03Z

webrender/src/tiling.rs

+                    for filter in &layer.composite_ops.filters {
+                        let mut prev_task = alpha_task_stack.pop().unwrap();
+                        let item = AlphaRenderItem::Blend(sc_index, current_task.id, *filter, next_z);
+                        next_z += 1;


I don't think changing the Z makes sense here, given that we are rendering into the task cache and that's not supposed to use the depth test anyway.

The Z is used here when drawing the blend item into the main scene.

glennw · 2017-01-30T22:19:11Z

@rlhunt Yup, you're right about the extra layer - your change seems correct to me. r=me once the question from @kvark is resolved.

glennw · 2017-01-31T05:29:41Z

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.

webrender/src/tiling.rs, line 2595 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why do you need to separate task creation from task initialization (which is happening in PopStackingContext)? Could we have all the logic in one place (in the pop code) instead?

The tasks are created here and pushed onto a stack, so that the items in the primitive runs get pushed on to the right task. Or am I completely misunderstanding your comment? :)

Comments from Reviewable

bors-servo · 2017-01-31T07:13:22Z

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

eqrion · 2017-01-31T16:07:01Z

So yes, basically what Glenn said. Tasks need to be created before the following primitive run so the primitives are added to them. And I'm not familiar with how the Z order code is used, but Glenn's comment makes sense.

I tried to preserve the original task creation logic. I only extended it so multiple filters on a single stacking context would act like multiple identical stacking contexts with a single filter.

eqrion · 2017-01-31T18:04:47Z

Hmm, so I rebased my branch on the latest master. Not sure why that isn't showing up here.

glennw · 2017-01-31T19:51:36Z

I'm not sure why appveyor is confused either. Could you try rebasing again, or if it's already up to date, try do just run git commit --amend which will modify the timestamp and commit SHA, and then force push?

kvark

ok, sounds good, thanks for the answers!

eqrion · 2017-01-31T19:57:27Z

Okay, that did the trick!

glennw · 2017-01-31T19:59:50Z

@bors-servo r+

bors-servo · 2017-01-31T19:59:51Z

📌 Commit 4dadd6c has been approved by glennw

bors-servo · 2017-02-01T19:43:24Z

⌛ Testing commit 4dadd6c with merge b8fd811...

Support multiple composite operations in a StackingContext A StackingContext holds a Vec<FilterOp> and a MixBlendMode, but currently only one operation is applied. This change modifies the render task building code to respect multiple composite operations. It's also careful to apply filters first and then the mix-blend-mode according to the spec.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/802)

glennw · 2017-02-01T19:44:22Z

@bors-servo r+

bors-servo · 2017-02-01T19:44:22Z

💡 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 · 2017-02-01T19:44:23Z

📌 Commit 4dadd6c has been approved by glennw

glennw · 2017-02-02T09:10:09Z

Merging manually - homu is not listening, but travis has passed on both platforms.

kvark reviewed Jan 30, 2017

View reviewed changes

kvark approved these changes Jan 31, 2017

View reviewed changes

Support multiple composite operations in a StackingContext

4dadd6c

glennw closed this Feb 1, 2017

glennw reopened this Feb 1, 2017

bors-servo mentioned this pull request Feb 1, 2017

handle external raw buffer #776

Merged

glennw merged commit 1f767cf into servo:master Feb 2, 2017

bors-servo mentioned this pull request Feb 2, 2017

Clarify the difference between packed layers and stacking contexts #824

Merged

eqrion deleted the composite branch April 6, 2017 21:39

Support multiple composite operations in a StackingContext #802

Support multiple composite operations in a StackingContext #802

Uh oh!

Conversation

eqrion commented Jan 29, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eqrion commented Jan 29, 2017

Uh oh!

glennw commented Jan 30, 2017

Uh oh!

glennw commented Jan 30, 2017

Uh oh!

eqrion commented Jan 30, 2017

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

kvark Jan 30, 2017

Choose a reason for hiding this comment

Uh oh!

kvark Jan 30, 2017

Choose a reason for hiding this comment

Uh oh!

glennw Jan 30, 2017

Choose a reason for hiding this comment

Uh oh!

glennw commented Jan 30, 2017

Uh oh!

glennw commented Jan 31, 2017

Uh oh!

bors-servo commented Jan 31, 2017

Uh oh!

eqrion commented Jan 31, 2017

Uh oh!

eqrion commented Jan 31, 2017

Uh oh!

glennw commented Jan 31, 2017

Uh oh!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

eqrion commented Jan 31, 2017

Uh oh!

glennw commented Jan 31, 2017

Uh oh!

bors-servo commented Jan 31, 2017

Uh oh!

bors-servo commented Feb 1, 2017

Uh oh!

glennw commented Feb 1, 2017

Uh oh!

bors-servo commented Feb 1, 2017

Uh oh!

bors-servo commented Feb 1, 2017

Uh oh!

glennw commented Feb 2, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eqrion commented Jan 29, 2017 •

edited by larsbergstrom

Loading