Fix some calculation errors for ps_split_composite and make_polygon. #1787

mephisto41 · 2017-10-02T04:00:04Z

There are some bugs when we have a transform-style:flat in the middle of transform-style:preserve-3d. So I fixed it by following changes.

ps_split_composite shader did not take position of dest_task into
account.
make_polygon didn't work if isolated_items_bounds is empty.

r? kvark

This change is

kvark · 2017-10-02T17:35:19Z

webrender/src/frame_builder.rs

 ) -> Polygon<f32, WorldPixel> {
-    //TODO: only work with `isolated_items_bounds.size` worth of space


why is this comment removed?

I removed this accidentally. I'll add it back.

kvark · 2017-10-02T17:35:19Z

webrender/res/ps_split_composite.glsl

@@ -38,11 +38,15 @@ void main(void) {
    CompositeInstance ci = fetch_composite_instance();
    SplitGeometry geometry = fetch_split_geometry(ci.user_data0);
    AlphaBatchTask src_task = fetch_alpha_batch_task(ci.src_task_index);
+    AlphaBatchTask dest_task = fetch_alpha_batch_task(ci.render_task_index);
+
+    vec2 dest_origin = dest_task.render_target_origin -


kvark · 2017-10-02T17:35:19Z

webrender/src/frame_builder.rs

+        let rect = stacking_context.screen_bounds.to_f32() / device_pixel_ratio;
+        let bounds = LayerRect::new(LayerPoint::new(rect.origin.x, rect.origin.y),
+                                    LayerSize::new(rect.size.width, rect.size.height));
+        return Polygon::from_transformed_rect(bounds, LayerToWorldTransform::identity(), anchor);


hmm, I'm not seeing the point here. If a stacking context has no items to isolate, why would we even want it to participate in the plane splitting (and hence, generate a polygon here)?

In the reftest we have following display list.

sc1 (preserve3d) sc2 (preserve3d) rect1 sc3 (flat) sc4 (preserve3d) sc5 (preserve3d) rect2 sc6 (preserve3d) rect3

We'll create plane splitting for rect 2 and rect 3, then render it in sc3. Next, we'll create plane splitting for sc3 and render it on the screen. Thus, we have a sc with no item bound but need a plane splitting. Does it makes sense?

I believe the isolated_items_bounds for SC3 would then need to include the bounds of SC4. Given that SC4 is transformed, its bounds would need to be projected onto SC3 (transformed).

mephisto41 · 2017-10-05T08:43:46Z

@kvark , Now if the SC doesn't have any primitive I'll use the union of all children's bounds as isolated_items_bounds. Can you review it again? Thanks.

kvark · 2017-10-05T12:42:52Z

@mephisto41 looks good to me now, thank you!
Do you mind launching a try push to see if any Gecko issues appear before we proceed?

mephisto41 · 2017-10-05T13:47:38Z

Sure, I'll submit a try.

mephisto41 · 2017-10-05T13:50:30Z

Here is try. https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f31a21048f83634bed711b15e2a6b558e894805

bors-servo · 2017-10-05T16:55:33Z

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

kvark · 2017-10-05T17:00:03Z

@mephisto41 looks good, thank you!
Please rebase and r=me

kvark · 2017-10-05T17:00:39Z

@bors-servo delegate=mephisto41

bors-servo · 2017-10-05T17:00:40Z

✌️ @mephisto41 can now approve this pull request

bors-servo · 2017-10-11T16:59:36Z

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

mephisto41 · 2017-10-12T03:30:19Z

@kvark, I found the original solution did not resolved the issue completely. If the SC is not isolation and it contains primitives and preserve-3d SCs. The bounds for make_polygon should include primitives bounds and all SC children bounds. My original solution only works with the non-isolation SC without any primitives. So I add a new test case for this problem and add a condition in the handle_pop_stacking_context. I think this modification need your review.

else if stacking_context.isolation != ContextIsolation::Items {
    stacking_context.isolated_items_bounds = stacking_context
        .isolated_items_bounds
        .union(&stacking_context.children_sc_bounds);
}


        Fix some calculation errors for ps_split_composite and make_polygon.

1. ps_split_composite shader did not take position of dest_task into account. 2. make_polygon didn't work if isolated_items_bounds is empty.


        Add reftest.

kvark · 2017-10-12T12:55:36Z

If the SC is not isolation and it contains primitives and preserve-3d SCs

Could you have a test case for that? AFAIS, both your reftests have all the contexts being isolated.

mephisto41 · 2017-10-12T13:38:41Z

I think the SC in line 19 of intermediate-2.yaml is non-isolated SC which has a primitive and a preserve-3d SC.

kvark · 2017-10-12T18:02:17Z

@mephisto41 ok, I think we are good to go then, thank you!
@bors-servo r+

bors-servo · 2017-10-12T18:02:18Z

📌 Commit 1b97c1f has been approved by kvark

bors-servo · 2017-10-12T18:17:51Z

⌛ Testing commit 1b97c1f with merge cd4415a...


        Auto merge of #1787 - mephisto41:fix-split-composite, r=kvark

Fix some calculation errors for ps_split_composite and make_polygon. There are some bugs when we have a transform-style:flat in the middle of transform-style:preserve-3d. So I fixed it by following changes. 1. ps_split_composite shader did not take position of dest_task into account. 2. make_polygon didn't work if isolated_items_bounds is empty. r? kvark  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1787)

bors-servo · 2017-10-12T21:55:08Z

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

kvark reviewed Oct 2, 2017

View changes

mephisto41 force-pushed the mephisto41:fix-split-composite branch from 69f04bd to 556ee36 Oct 5, 2017

mephisto41 force-pushed the mephisto41:fix-split-composite branch from 556ee36 to 7c1ffa0 Oct 11, 2017

mephisto41 force-pushed the mephisto41:fix-split-composite branch from 7c1ffa0 to 12f5b3b Oct 12, 2017

mephisto41 added 2 commits Sep 29, 2017

Fix some calculation errors for ps_split_composite and make_polygon.

504004e

1. ps_split_composite shader did not take position of dest_task into account. 2. make_polygon didn't work if isolated_items_bounds is empty.

Add reftest.

Loading status checks…

1b97c1f

mephisto41 force-pushed the mephisto41:fix-split-composite branch from 12f5b3b to 1b97c1f Oct 12, 2017

servo / webrender

Fix some calculation errors for ps_split_composite and make_polygon. #1787

Fix some calculation errors for ps_split_composite and make_polygon. #1787

mephisto41 commented Oct 2, 2017 •

edited by larsbergstrom

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mephisto41 commented Oct 5, 2017

kvark commented Oct 5, 2017

mephisto41 commented Oct 5, 2017

mephisto41 commented Oct 5, 2017

bors-servo commented Oct 5, 2017

kvark commented Oct 5, 2017

kvark commented Oct 5, 2017

bors-servo commented Oct 5, 2017

bors-servo commented Oct 11, 2017

mephisto41 commented Oct 12, 2017

kvark commented Oct 12, 2017

mephisto41 commented Oct 12, 2017

kvark commented Oct 12, 2017

bors-servo commented Oct 12, 2017

bors-servo commented Oct 12, 2017

bors-servo commented Oct 12, 2017

		) -> Polygon<f32, WorldPixel> {
		//TODO: only work with `isolated_items_bounds.size` worth of space

servo / webrender

Join GitHub today

Fix some calculation errors for ps_split_composite and make_polygon. #1787

Fix some calculation errors for ps_split_composite and make_polygon. #1787

Conversation

mephisto41 commented Oct 2, 2017 • edited by larsbergstrom

This comment has been minimized.

kvark Oct 2, 2017

This comment has been minimized.

mephisto41 Oct 3, 2017

This comment has been minimized.

kvark Oct 2, 2017

This comment has been minimized.

kvark Oct 2, 2017

This comment has been minimized.

mephisto41 Oct 3, 2017

This comment has been minimized.

kvark Oct 3, 2017

mephisto41 commented Oct 5, 2017

kvark commented Oct 5, 2017

mephisto41 commented Oct 5, 2017

mephisto41 commented Oct 5, 2017

bors-servo commented Oct 5, 2017

kvark commented Oct 5, 2017

kvark commented Oct 5, 2017

bors-servo commented Oct 5, 2017

bors-servo commented Oct 11, 2017

mephisto41 commented Oct 12, 2017

kvark commented Oct 12, 2017

mephisto41 commented Oct 12, 2017

kvark commented Oct 12, 2017

bors-servo commented Oct 12, 2017

bors-servo commented Oct 12, 2017

bors-servo commented Oct 12, 2017

mephisto41 commented Oct 2, 2017 •

edited by larsbergstrom