Fix drop-shadow render incorrectly on viewport's edge. #2243

mephisto41 · 2017-12-20T03:28:21Z

Initially, I apply the drop-shadow's offset before render the blur
result. This has problem if the blur is close to edge of
viewport because the blur result might be crop by the viewport.
This patch apply the offset after the blur result generated.

Fix #2197.

This change is

mephisto41 · 2017-12-20T03:29:35Z

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec23089146b038ce93f144037bed510eeacdea3e&selectedJob=152518179

Gecko try here, the long-chain.html get fixed after apply this patch. We can replace the fails-if in long-chain.html with fuzzy value.

kvark

Nice fix!
There is a few things we can do nicer, otherwise is good to go.

kvark · 2017-12-20T18:36:13Z

webrender/src/picture.rs

@@ -232,7 +234,7 @@ impl PicturePrimitive {
        let local_content_rect = prim_run_rect.local_rect_in_actual_parent_space;

        match self.kind {
-            PictureKind::Image { composite_mode, ref mut real_local_rect, .. } => {
+            PictureKind::Image { composite_mode, ref mut real_local_rect, ref mut content_rect, .. } => {


would it (possibly, in the future) make sense outside of FilterOp::DropShadow? If not, we may move it inside the DropShadow variant for clarity.

kvark · 2017-12-20T18:36:13Z

webrender/src/picture.rs

@@ -350,13 +353,20 @@ impl PicturePrimitive {
                        let blur_render_task_id = render_tasks.add(blur_render_task);
                        self.render_task_id = Some(blur_render_task_id);
                    }
-                    Some(PictureCompositeMode::Filter(FilterOp::DropShadow(offset, blur_radius, color))) => {
+                    Some(PictureCompositeMode::Filter(FilterOp::DropShadow(_, blur_radius, color))) => {
+                        let content_width =


perhaps, we can just do this one liner instead?

let rect = (content_rect * TypedScale::<_, DevicePixel>::new(prim_context.device_pixel_ratio).round().to_i32();

@glennw btw, I think it would make sense to just store the device_pixel_ratio as that TypedScale in the first place

kvark · 2017-12-20T18:36:13Z

webrender/src/picture.rs

-                        local_content_rect.inflate(inflate_size, inflate_size)
-                                          .translate(&offset)
+                        *content_rect = local_content_rect.inflate(inflate_size, inflate_size);
+                        content_rect.translate(&offset)


Note that technically you can apply the translation later, at the last moment.
This would allow you to merge those match arms, since you only need the blur_radius out of both to inflate the rect

kvark · 2017-12-20T18:36:13Z

webrender/src/tiling.rs

@@ -775,17 +776,19 @@ fn add_to_batch(
                                                secondary_textures,
                                            );
                                            let batch = batch_list.get_suitable_batch(key, &item_bounding_rect);
-                                            let device_offset_x = device_length(offset.x, ctx.device_pixel_ratio);
-                                            let device_offset_y = device_length(offset.y, ctx.device_pixel_ratio);
+                                            let content_origin_x = device_length(content_rect.origin.x, ctx.device_pixel_ratio);


same one liner trick can be done here

glennw · 2017-12-21T04:59:50Z

Looks good to me once @kvark 's concerns are addressed.

mephisto41 · 2017-12-21T09:31:58Z

Review status: 0 of 9 files reviewed at latest revision, 4 unresolved discussions.

webrender/src/picture.rs, line 237 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would it (possibly, in the future) make sense outside of FilterOp::DropShadow? If not, we may move it inside the DropShadow variant for clarity.

Done.

webrender/src/picture.rs, line 248 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Note that technically you can apply the translation later, at the last moment.
This would allow you to merge those match arms, since you only need the blur_radius out of both to inflate the rect

Done.

webrender/src/picture.rs, line 357 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

perhaps, we can just do this one liner instead?
let rect = (content_rect * TypedScale::<_, DevicePixel>::new(prim_context.device_pixel_ratio).round().to_i32();
@glennw btw, I think it would make sense to just store the device_pixel_ratio as that TypedScale in the first place

Done.

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

Previously, kvark (Dzmitry Malyshau) wrote…

same one liner trick can be done here

Done.

Comments from Reviewable

kvark · 2017-12-21T15:05:11Z

Reviewed 3 of 5 files at r1, 6 of 6 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

webrender/src/picture.rs, line 248 at r1 (raw file):

Previously, mephisto41 (Morris Tseng) wrote…

Done.

Sorry for being unclear, this is not exactly what I meant. By "later" I mean at the time where it's used (in prepare_for_render), not right here after the match.

webrender/src/picture.rs, line 357 at r1 (raw file):

Previously, mephisto41 (Morris Tseng) wrote…

Done.

Please note that I went and refactored most of the device/pixel ratio logic to have some type safety in #2246. Sorry about the need for rebase on that! If this PR was ready, I'd gladly yield and rebase on top.

Comments from Reviewable

bors-servo · 2017-12-21T15:37:26Z

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

glennw · 2018-01-02T21:28:39Z

I think this is ready to merge once rebased @kvark @mephisto41 ?

mephisto41 · 2018-01-03T02:30:43Z

It has 1 nit should be fixed. I'll fix it today.

kvark · 2018-01-03T17:41:02Z

Reviewed 4 of 4 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

webrender/src/prim_store.rs, line 1750 at r3 (raw file):

            );

            match pic.kind {

can this be done inside update_local_rect?

Comments from Reviewable

mephisto41 · 2018-01-04T08:40:10Z

I made some significant change here. Since I realized that the content_rect can be reconstructed by prim_metadata.local_rect - drop-shadow-offset. So I can no longer store content_rect in FilterOp::DropShadow. This change makes the patch cleaner.

bors-servo · 2018-01-04T10:01:46Z

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

kvark · 2018-01-04T20:50:15Z

needs rebase though

Reviewed 3 of 7 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

Comments from Reviewable

kvark · 2018-01-05T17:18:14Z

Thank you!
@bors-servo r+

bors-servo · 2018-01-05T17:18:14Z

📌 Commit 176f2f5 has been approved by kvark

bors-servo · 2018-01-05T17:34:47Z

⌛ Testing commit 176f2f5 with merge 87c43db...


        Auto merge of #2243 - mephisto41:drop-shadow-edge, r=kvark

Fix drop-shadow render incorrectly on viewport's edge. Initially, I apply the drop-shadow's offset before render the blur result. This has problem if the blur is close to edge of viewport because the blur result might be crop by the viewport. This patch apply the offset after the blur result generated. Fix #2197.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2243)

bors-servo · 2018-01-05T18:47:03Z

💔 Test failed - status-travis

kvark · 2018-01-05T19:11:37Z

Ah, since the PR got largely rewritten, could you fire up another try push please?

bors-servo · 2018-01-08T03:42:48Z

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


        Fix drop-shadow render incorrectly on viewport's edge.

Initially, I apply the drop-shadow's offset before render the blur result. This has problem if the blur is close to edge of viewport because the blur result might be crop by the viewport. This patch apply the offset after the blur result generated.

mephisto41 · 2018-01-08T08:11:05Z

Yes, latest try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99ce5ccd151694845a486b3692dbd8e490eed5a5

kvark · 2018-01-08T15:11:06Z

There is an R7 in long-chain.html failing there.

mephisto41 · 2018-01-08T23:15:13Z

See the comment 1 for explanation. Before this patch, the long-chain test is completely wrong so we set fails-if for it. After this patch, you can see that I removed the fails-if annotation in the try and the long-chain result is correct although we need tweak some fuzzy value for it. So the long-chain failure is expected. Just some fuzzy value change is fine.

kvark · 2018-01-09T01:47:16Z

Thanks!
@bors-servo r+

bors-servo · 2018-01-09T01:47:16Z

📌 Commit f0cf25e has been approved by kvark

bors-servo · 2018-01-09T02:43:06Z

⌛ Testing commit f0cf25e with merge f242fa1...


        Auto merge of #2243 - mephisto41:drop-shadow-edge, r=kvark

Fix drop-shadow render incorrectly on viewport's edge. Initially, I apply the drop-shadow's offset before render the blur result. This has problem if the blur is close to edge of viewport because the blur result might be crop by the viewport. This patch apply the offset after the blur result generated. Fix #2197.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2243)

bors-servo · 2018-01-09T03:47:03Z

💔 Test failed - status-travis

glennw · 2018-01-09T04:29:00Z

@bors-servo retry


        Auto merge of #2243 - mephisto41:drop-shadow-edge, r=kvark

Fix drop-shadow render incorrectly on viewport's edge. Initially, I apply the drop-shadow's offset before render the blur result. This has problem if the blur is close to edge of viewport because the blur result might be crop by the viewport. This patch apply the offset after the blur result generated. Fix #2197.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2243)

bors-servo · 2018-01-09T04:29:04Z

⌛ Testing commit f0cf25e with merge 5001837...

bors-servo · 2018-01-09T05:20:04Z

💔 Test failed - status-travis

glennw · 2018-01-09T06:12:18Z

@bors-servo retry
sigh

bors-servo · 2018-01-09T06:12:21Z

⌛ Testing commit f0cf25e with merge 68dcbc4...


        Auto merge of #2243 - mephisto41:drop-shadow-edge, r=kvark

Fix drop-shadow render incorrectly on viewport's edge. Initially, I apply the drop-shadow's offset before render the blur result. This has problem if the blur is close to edge of viewport because the blur result might be crop by the viewport. This patch apply the offset after the blur result generated. Fix #2197.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2243)

bors-servo · 2018-01-09T06:57:25Z

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

kvark reviewed Dec 20, 2017

View changes

mephisto41 force-pushed the mephisto41:drop-shadow-edge branch from 17e618e to b3d39e6 Dec 21, 2017

mephisto41 force-pushed the mephisto41:drop-shadow-edge branch 2 times, most recently from 233becd to 5cc0b8a Jan 3, 2018

mephisto41 force-pushed the mephisto41:drop-shadow-edge branch from 5cc0b8a to 6be4a71 Jan 4, 2018

mephisto41 force-pushed the mephisto41:drop-shadow-edge branch from 6be4a71 to 176f2f5 Jan 5, 2018

mephisto41 force-pushed the mephisto41:drop-shadow-edge branch from 176f2f5 to f0cf25e Jan 8, 2018

Darkspirit mentioned this pull request Jan 24, 2018

regression: broken images on https://testpilot.firefox.com/experiments #2334

Closed

servo / webrender

Fix drop-shadow render incorrectly on viewport's edge. #2243

Fix drop-shadow render incorrectly on viewport's edge. #2243

mephisto41 commented Dec 20, 2017 •

edited by larsbergstrom

mephisto41 commented Dec 20, 2017

kvark left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

glennw commented Dec 21, 2017

mephisto41 commented Dec 21, 2017

kvark commented Dec 21, 2017

bors-servo commented Dec 21, 2017

glennw commented Jan 2, 2018

mephisto41 commented Jan 3, 2018

kvark commented Jan 3, 2018

mephisto41 commented Jan 4, 2018

bors-servo commented Jan 4, 2018

kvark commented Jan 4, 2018

kvark commented Jan 5, 2018

bors-servo commented Jan 5, 2018

bors-servo commented Jan 5, 2018

bors-servo commented Jan 5, 2018

kvark commented Jan 5, 2018

bors-servo commented Jan 8, 2018

mephisto41 commented Jan 8, 2018

kvark commented Jan 8, 2018

mephisto41 commented Jan 8, 2018

kvark commented Jan 9, 2018

bors-servo commented Jan 9, 2018

bors-servo commented Jan 9, 2018

bors-servo commented Jan 9, 2018

glennw commented Jan 9, 2018

bors-servo commented Jan 9, 2018

bors-servo commented Jan 9, 2018

glennw commented Jan 9, 2018

bors-servo commented Jan 9, 2018

bors-servo commented Jan 9, 2018

servo / webrender

Join GitHub today

Fix drop-shadow render incorrectly on viewport's edge. #2243

Fix drop-shadow render incorrectly on viewport's edge. #2243

Conversation

mephisto41 commented Dec 20, 2017 • edited by larsbergstrom

mephisto41 commented Dec 20, 2017

kvark left a comment

This comment has been minimized.

kvark Dec 20, 2017

This comment has been minimized.

kvark Dec 20, 2017

This comment has been minimized.

kvark Dec 20, 2017

This comment has been minimized.

kvark Dec 20, 2017 • edited

glennw commented Dec 21, 2017

mephisto41 commented Dec 21, 2017

kvark commented Dec 21, 2017

bors-servo commented Dec 21, 2017

glennw commented Jan 2, 2018

mephisto41 commented Jan 3, 2018

kvark commented Jan 3, 2018

mephisto41 commented Jan 4, 2018

bors-servo commented Jan 4, 2018

kvark commented Jan 4, 2018

kvark commented Jan 5, 2018

bors-servo commented Jan 5, 2018

bors-servo commented Jan 5, 2018

bors-servo commented Jan 5, 2018

kvark commented Jan 5, 2018

bors-servo commented Jan 8, 2018

mephisto41 commented Jan 8, 2018

kvark commented Jan 8, 2018

mephisto41 commented Jan 8, 2018

kvark commented Jan 9, 2018

bors-servo commented Jan 9, 2018

bors-servo commented Jan 9, 2018

bors-servo commented Jan 9, 2018

glennw commented Jan 9, 2018

bors-servo commented Jan 9, 2018

bors-servo commented Jan 9, 2018

glennw commented Jan 9, 2018

bors-servo commented Jan 9, 2018

bors-servo commented Jan 9, 2018

mephisto41 commented Dec 20, 2017 •

edited by larsbergstrom

kvark Dec 20, 2017 •

edited