Rewrite the blur shader to remove the variable-length loop and to use the texture filtering hardware more effectively. #3028

pcwalton · 2018-09-05T22:42:24Z

This new shader performs Gaussian resampling instead of regular convolution.
It samples in between texels to reduce the number of taps.

The speed is about the same as the existing technique. It is nevertheless the
fastest blur method that I could come up with. In particular, it exceeds the
performance of the Kawase and dual Kawase blur techniques. The speed comes from
working at lower resolution and incurring fewer downsampling and upsampling
passes. As is often the case, ALU performance does not really seem to be the
limiting factor; it's mostly memory bandwidth, which is why downsampling is so
important. Further improvements should come from not doing the downsampling at
all and instead rendering the blurred content at low resolution to begin with.

Closes #2821.

Some reftests are currently failing; do not merge yet. I just wanted to get this here to discuss. cc @gw3583

This change is

pcwalton · 2018-09-06T03:26:08Z

Just one reftest is failing and needs investigation. (It may just be a harmless minor blur algorithm difference, but I want to be sure.)

kvark · 2018-09-06T18:18:42Z

Is there anything special going on during upsampling in order to preserve the missing detail?

pcwalton · 2018-09-06T18:20:35Z

There's no (well, negligible) missing detail because Gaussian blur is a low-pass filter. (The Fourier transform of a Gaussian convolution is itself a Gaussian with the characteristic bell curve shape.) That's why downsampling works in the first place. Because the output of a Gaussian blur is effectively bandlimited, the Nyquist-Shannon sampling theorem says that we can preserve fewer samples in order to perfectly reconstruct the signal.


        Add a new benchmark, `large-blur-image`.

This benchmark tests the performance of large blur radii combined with large images.


        Allow the output path to be specified for the `wrench png` subtool


        Fix a reftest that relied on blurring out-of-bounds image data


        Rewrite the blur shader to remove the variable-length loop and to use…

… the texture filtering hardware more effectively. This new shader performs Gaussian *resampling* instead of regular convolution. It samples in between texels to reduce the number of taps. The speed is about the same as the existing technique. It is nevertheless the fastest blur method that I could come up with. In particular, it exceeds the performance of the Kawase and dual Kawase blur techniques. The speed comes from working at lower resolution and incurring fewer downsampling and upsampling passes. As is often the case, ALU performance does not really seem to be the limiting factor; it's mostly memory bandwidth, which is why downsampling is so important. Further improvements should come from not doing the downsampling at all and instead rendering the blurred content at low resolution to begin with. Closes #2821.

pcwalton · 2018-09-06T22:14:47Z

OK, this should be ready for review, though we should do a Gecko try run as well. r? @gw3583

kvark · 2018-09-07T00:54:38Z

Launched a try in https://treeherder.mozilla.org/#/jobs?repo=try&revision=94b30b8a56e652bf8e653d1165f33b7e7acc6a3f

kvark · 2018-09-07T01:03:44Z

There's no (well, negligible) missing detail because Gaussian blur is a low-pass filter. (The Fourier transform of a Gaussian convolution is itself a Gaussian with the characteristic bell curve shape.) That's why downsampling works in the first place. Because the output of a Gaussian blur is effectively bandlimited, the Nyquist-Shannon sampling theorem says that we can preserve fewer samples in order to perfectly reconstruct the signal.

We aren't perfectly reconstructing the signal, if I understand correctly, it's still an approximation.
Some light reading with pictures for those interested - http://chemaguerra.com/downsampling-and-gaussian-filters/

kvark · 2018-09-07T01:05:40Z

Weirdly Github doesn't show the CI icon for your latest commit. Could happen if you changed the date and/or committed from a different time zone?

kvark

Looks sensible! A few concerns below:

kvark · 2018-09-07T01:27:48Z

webrender/res/cs_blur.glsl

 };

-BlurTask fetch_blur_task(int address) {
+BlurTask fetchBlurTask(int address) {


please stick to the existing naming convention in the shaders

I made that change to improve consistency. Do function names have snake_case and variable names have CamelCase? I never was able to figure it out.

Look at prim_shared.glsl for examples. It's pretty much the Rust naming convention:

variables and functions are snake_case

type names are CamelCase

kvark · 2018-09-07T01:27:48Z

webrender/res/cs_blur.glsl

 #endif

 #ifdef WR_FRAGMENT_SHADER

-#if defined WR_FEATURE_COLOR_TARGET
+#define SUPPORT     4


it's not clear what SUPPORT here means

kvark · 2018-09-07T01:27:48Z

webrender/res/cs_blur.glsl

-    gauss_coefficient.xy *= gauss_coefficient.yz;
+    bool vertical = vVertical != 0;
+    vec2 axisCoord = vertical ? vUv.yx : vUv.xy;
+    float start = floor(axisCoord.x - float(SUPPORT)) + 0.5;


if the flooring changes our start position, wouldn't the computed Gaussian factors no longer match?

The computed Gaussian factors are based on offset, which is relative to the un-floored axisCoord.x.

kvark · 2018-09-07T01:27:48Z

webrender/src/clip.rs

@@ -274,7 +274,7 @@ impl ClipNode {
                // Quote from https://drafts.csswg.org/css-backgrounds-3/#shadow-blur
                // "the image that would be generated by applying to the shadow a
                // Gaussian blur with a standard deviation equal to half the blur radius."
-                let blur_radius_dp = (info.blur_radius * 0.5 * device_pixel_scale.0).round();
+                let blur_radius_dp = (info.blur_radius * device_pixel_scale.0).round();


comment above needs to be addressed as well

kvark · 2018-09-07T01:27:48Z

webrender/src/render_task.rs

+        let uv_rect_kind = render_tasks[src_task_id].uv_rect_kind();
+
+        // Determine the number of downsampling passes that we'll use.
+        let downsampling_passes = (30 as u32).saturating_sub((radius as u32).leading_zeros());


number of downsample passes = 30 - (leading 0 bits of radius)?
where does 30 come from?

It's log₂(r) - 1 == log₂(r/2). (Note the leading_zeros call.) I'll add this to a comment :)

kvark · 2018-09-07T01:27:48Z

webrender/src/renderer.rs

            let _timer = self.gpu_profile.start_timer(GPU_TAG_BLUR);

            self.set_blend(false, framebuffer_kind);
            self.shaders.cs_blur_rgba8
                .bind(&mut self.device, projection, &mut self.renderer_errors);

-            if !target.vertical_blurs.is_empty() {
+            if !target.blurs.is_empty() {


we already check for this condition in an enclosing block

pcwalton · 2018-09-07T14:50:52Z

I should mention that we shouldn't be downsampling any more than the old code did.

I noticed a bunch of test failures on try (thanks for kicking off the try run!)—I'll investigate those.

pcwalton · 2018-09-13T01:43:44Z

Given all the test failures on try, I'm going to try separating the new shader from the downscaling changes to try to isolate the source of the problems.

bors-servo · 2018-09-25T17:59:46Z

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


        Round blur support up to the nearest even number of pixels.

I'm pretty confident this is correct--it's either just as accurate or more accurate than the current implementation--so it'll let me sort out which test failures, if any, from servo#3028 and servo#3122 are true regressions.


        Round blur support up to the nearest even number of pixels.

I'm pretty confident this is correct--it's either just as accurate or more accurate than the current implementation--so it'll let me sort out which test failures, if any, from servo#3028 and servo#3122 are true regressions.


        Auto merge of #3126 - pcwalton:blur-even-support, r=kvark

Round blur support up to the nearest even number of pixels. I'm pretty confident this is correct--it's either just as accurate or more accurate than the current implementation--so it'll let me sort out which test failures, if any, from #3028 and #3122 are true regressions.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3126)

gw3583 · 2018-10-05T04:55:09Z

@pcwalton Do we still want this kept open?

pcwalton · 2018-10-05T18:25:00Z

@gw3583 Once #3122 lands I want to update this to be just about removing the variable-length loop (as much as possible anyway).

gw3583 · 2018-12-18T04:41:57Z

@pcwalton #3122 has landed now. Should we keep this open and rebase on top of those changes, or would it be best to close this and open a new PR when ready?

pcwalton force-pushed the pcwalton:blur branch from bd0244d to 3b20924 Sep 6, 2018

pcwalton added 4 commits Sep 4, 2018

Add a new benchmark, `large-blur-image`.

b9ae5a3

This benchmark tests the performance of large blur radii combined with large images.

Allow the output path to be specified for the `wrench png` subtool

28f977c

Fix a reftest that relied on blurring out-of-bounds image data

e509c87

pcwalton force-pushed the pcwalton:blur branch from 3b20924 to 4684f99 Sep 6, 2018

pcwalton changed the title ~~(DO NOT MERGE YET) Rewrite the blur shader to remove the variable-length loop and to use the texture filtering hardware more effectively.~~ Rewrite the blur shader to remove the variable-length loop and to use the texture filtering hardware more effectively. Sep 6, 2018

pcwalton requested a review from gw3583 Sep 6, 2018

kvark self-requested a review Sep 7, 2018

kvark reviewed Sep 7, 2018

View changes

pcwalton mentioned this pull request Sep 26, 2018

Round blur support up to the nearest even number of pixels. #3126

Merged

pcwalton closed this Dec 18, 2018

servo / webrender

Rewrite the blur shader to remove the variable-length loop and to use the texture filtering hardware more effectively. #3028

Rewrite the blur shader to remove the variable-length loop and to use the texture filtering hardware more effectively. #3028

pcwalton commented Sep 5, 2018 •

edited by larsbergstrom

pcwalton commented Sep 6, 2018

kvark commented Sep 6, 2018

pcwalton commented Sep 6, 2018 •

edited

pcwalton commented Sep 6, 2018

kvark commented Sep 7, 2018

kvark commented Sep 7, 2018

kvark commented Sep 7, 2018

kvark left a comment

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pcwalton commented Sep 7, 2018

pcwalton commented Sep 13, 2018

bors-servo commented Sep 25, 2018

gw3583 commented Oct 5, 2018

pcwalton commented Oct 5, 2018

gw3583 commented Dec 18, 2018

servo / webrender

Join GitHub today

Rewrite the blur shader to remove the variable-length loop and to use the texture filtering hardware more effectively. #3028

Rewrite the blur shader to remove the variable-length loop and to use the texture filtering hardware more effectively. #3028

Conversation

pcwalton commented Sep 5, 2018 • edited by larsbergstrom

pcwalton commented Sep 6, 2018

kvark commented Sep 6, 2018

pcwalton commented Sep 6, 2018 • edited

pcwalton commented Sep 6, 2018

kvark commented Sep 7, 2018

kvark commented Sep 7, 2018

kvark commented Sep 7, 2018

kvark left a comment

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pcwalton Sep 7, 2018 • edited

This comment has been minimized.

pcwalton commented Sep 7, 2018

pcwalton commented Sep 13, 2018

bors-servo commented Sep 25, 2018

gw3583 commented Oct 5, 2018

pcwalton commented Oct 5, 2018

gw3583 commented Dec 18, 2018

pcwalton commented Sep 5, 2018 •

edited by larsbergstrom

pcwalton commented Sep 6, 2018 •

edited

pcwalton Sep 7, 2018 •

edited