Tweak blur filter so the result is close to gecko. #1920

mephisto41 · 2017-10-24T08:44:41Z

We actually pass the std deviation into cs_blur shader for gaussian
blur. So, correct some naming which causing confusion. Also, according
to the spec, the std deviation of box and text shadow is half of blur
radius.

Here are some gecko try for comparison.
With this patch

Without this patch

r? @glennw

This change is

glennw · 2017-10-24T09:00:48Z

Nice work!

There is a CI failure in the shader validation - you can test this locally by running cargo test in the root webrender directory.

We should also do a gecko try run with this to see what effect it has on all the existing tests.

mephisto41 · 2017-10-24T09:43:18Z

Thanks. I have fixed the shader validation.

gecko try is here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9311537999c72f68f92dc4912ce9f576776a5abf&selectedJob=139079239

Only fuzzy changes required I think.

kvark · 2017-10-24T18:56:48Z

Travis seems OK:

No output has been received in the last 10m0s

Taskcluster is showing one of the reftest failures: https://tools.taskcluster.net/groups/EtuwFR_uQd-l9S7d7QDiow/tasks/EtuwFR_uQd-l9S7d7QDiow/runs/0/logs/public%2Flogs%2Flive.log#L1152

Also, I suppose this fixes #1827

glennw · 2017-10-24T21:16:21Z

@mephisto41 For some reason, the TaskCluster CI often seems to require slightly more fuzziness than the Travis CI. It might just be a case of looking at the TC logs and updating the fuzziness on that one wrench test that is failing.

glennw · 2017-10-24T21:17:31Z

@mephisto41 It does look like quite a lot of unexpected fails on the try run - could you detail the ones that have changed and what the differences are?

mephisto41 · 2017-10-25T02:54:40Z

@mephisto41 It does look like quite a lot of unexpected fails on the try run - could you detail the ones that have changed and what the differences are?

Yap, we have R3, R4 and R7 reftests failure. R3 has some box-shadow tests, R4 has both box-shadow and css blur filter tests and R7 has css blur filter tests.

We only need change the fuzzy value for those tests since we change the blur shader.

glennw · 2017-10-25T06:30:45Z

Most of them do appear to be fairly small differences. There's one (maybe in R7?) where one of the reftests has no blur but the reference has a blur.

I think this is probably OK to merge, if it makes the blur look closer to Gecko (I haven't checked that myself). But I'd like to get opinion from @staktrace first on whether those failures are reasonable to fuzz (for now).

mephisto41 · 2017-10-25T06:56:47Z

Ah, I saw it. it is in R4 (mix-blend-mode-and-filter.html). I'll check it again to see if it is regressed by this patch. Thanks for reminder.

bors-servo · 2017-10-25T07:20:27Z

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

mephisto41 · 2017-10-25T09:29:00Z

mix-blend-mode-and-filter.html is regressed by gecko bug (https://bugzilla.mozilla.org/show_bug.cgi?id=1370564). Not this bug, I think this bug is ready to go. I'll create a gecko bug for fixing mix-blend-mode-and-filter.html.

staktrace · 2017-10-25T13:25:49Z

gecko try is here: https://treeherder.mozilla.org/#/jobs?repo=try&
revision=9311537999c72f68f92dc4912ce9f576776a5abf&selectedJob=139079239

Yeah it looked like the only non-fuzzy change here was mix-blend-mode-and-filter.html which already got annotated in m-c via bug 1370564. The rest of these changes look fine to me.

glennw · 2017-10-25T20:13:04Z

@staktrace @mephisto41 OK, thanks!

@bors-servo r+

bors-servo · 2017-10-25T20:13:05Z

📌 Commit 3fa3c2b has been approved by glennw

bors-servo · 2017-10-25T22:27:17Z

⌛ Testing commit 3fa3c2b with merge b04278f...

@glennw

Tweak blur filter so the result is close to gecko. We actually pass the std deviation into cs_blur shader for gaussian blur. So, correct some naming which causing confusion. Also, according to the spec, the std deviation of box and text shadow is half of blur radius. Here are some gecko try for comparison. [With this patch](https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/TTHMc9DqR-m6i0qnZo5oCw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1) [Without this patch](https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/JureAaHrT2qty-tYG0BOHg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1) r? @glennw  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1920)

bors-servo · 2017-10-25T22:33:35Z

💔 Test failed - status-appveyor

glennw · 2017-10-25T22:44:25Z

@mephisto41 Looks like this has compile errors as a result of some other PRs that landed in the meantime, so will need to be updated, sorry!

We actually pass the std deviation into cs_blur shader for gaussian blur. So, correct some naming which causing confusion. Also, according to the spec, the std deviation of box and text shadow is half of blur radius.

mephisto41 · 2017-10-26T02:17:17Z

I've updated the changesets.

glennw · 2017-10-26T02:49:49Z

webrender/src/frame_builder.rs

@@ -1861,10 +1862,12 @@ impl FrameBuilder {
                        match *filter {
                            FilterOp::Blur(blur_radius) => {
                                let blur_radius = device_length(blur_radius, device_pixel_ratio);
+                                let blur_std_deviation = blur_radius.0 as f32;


This doesn't seem right - the std deviation shouldn't be the same as the blur radius?

I know this is confusing. But for css blur filter, the blur radius is the std deviation. See the spec https://drafts.fxtf.org/filter-effects/#FilterProperty

"Applies a Gaussian blur to the input image. The passed parameter (the radius) defines the value of the standard deviation to the Gaussian function."

glennw · 2017-10-26T03:06:14Z

Ah, right. Thanks!

@bors-servo r+

bors-servo · 2017-10-26T03:06:15Z

📌 Commit e1f0c80 has been approved by glennw

bors-servo · 2017-10-26T04:10:51Z

⌛ Testing commit e1f0c80 with merge c47d106...

@glennw

Tweak blur filter so the result is close to gecko. We actually pass the std deviation into cs_blur shader for gaussian blur. So, correct some naming which causing confusion. Also, according to the spec, the std deviation of box and text shadow is half of blur radius. Here are some gecko try for comparison. [With this patch](https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/TTHMc9DqR-m6i0qnZo5oCw/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1) [Without this patch](https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/JureAaHrT2qty-tYG0BOHg/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1) r? @glennw  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/1920)

bors-servo · 2017-10-26T05:54:20Z

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

mephisto41 force-pushed the improve-blur branch from 85dd6b7 to 8a92e5f Compare October 24, 2017 09:41

mephisto41 force-pushed the improve-blur branch from 8a92e5f to d56edf5 Compare October 25, 2017 02:50

mephisto41 force-pushed the improve-blur branch from d56edf5 to 3fa3c2b Compare October 25, 2017 09:27

glennw mentioned this pull request Oct 25, 2017

WebRender's blur is much uglier #1946

Closed

Tweak blur filter.

e1f0c80

We actually pass the std deviation into cs_blur shader for gaussian blur. So, correct some naming which causing confusion. Also, according to the spec, the std deviation of box and text shadow is half of blur radius.

mephisto41 force-pushed the improve-blur branch from 3fa3c2b to e1f0c80 Compare October 26, 2017 02:16

glennw requested changes Oct 26, 2017

View reviewed changes

glennw approved these changes Oct 26, 2017

View reviewed changes

bors-servo merged commit e1f0c80 into servo:master Oct 26, 2017

bors-servo mentioned this pull request Oct 26, 2017

Tidy up picture/brush code in preparation for box shadow optimizations. #1947

Merged

staktrace mentioned this pull request Oct 26, 2017

Followup fix to update_image_template #1948

Merged

glennw mentioned this pull request Nov 8, 2017

Divide shadow blur_radius by 2? #1827

Closed

Tweak blur filter so the result is close to gecko. #1920

Tweak blur filter so the result is close to gecko. #1920

Uh oh!

Conversation

mephisto41 commented Oct 24, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennw commented Oct 24, 2017

Uh oh!

mephisto41 commented Oct 24, 2017

Uh oh!

kvark commented Oct 24, 2017

Uh oh!

glennw commented Oct 24, 2017

Uh oh!

glennw commented Oct 24, 2017

Uh oh!

mephisto41 commented Oct 25, 2017

Uh oh!

glennw commented Oct 25, 2017

Uh oh!

mephisto41 commented Oct 25, 2017

Uh oh!

bors-servo commented Oct 25, 2017

Uh oh!

mephisto41 commented Oct 25, 2017

Uh oh!

staktrace commented Oct 25, 2017

Uh oh!

glennw commented Oct 25, 2017

Uh oh!

bors-servo commented Oct 25, 2017

Uh oh!

bors-servo commented Oct 25, 2017

Uh oh!

bors-servo commented Oct 25, 2017

Uh oh!

glennw commented Oct 25, 2017

Uh oh!

mephisto41 commented Oct 26, 2017

Uh oh!

glennw Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

mephisto41 Oct 26, 2017

Choose a reason for hiding this comment

Uh oh!

glennw commented Oct 26, 2017

Uh oh!

bors-servo commented Oct 26, 2017

Uh oh!

bors-servo commented Oct 26, 2017

Uh oh!

bors-servo commented Oct 26, 2017

Uh oh!

Uh oh!

mephisto41 commented Oct 24, 2017 •

edited by larsbergstrom

Loading