Change text-shadow to blur entire text run, and clean up texture cache. #546

glennw · 2016-11-09T04:24:02Z

Add support for text-shadow to be drawn via render tasks on text runs.
Add support for blur render tasks. Uses standard 2 pass separable technique.
Remove a large amount of code that was used only for old glyph blur support.
Remove blur_radius from GlyphKey, resource cache, texture cache etc.
Add gaussian blur shader (cs_blur).
Add text run cache shader (cs_text_run).
Add generic primitive shader for drawing cache items (ps_cache_image).

This change is

* Add support for text-shadow to be drawn via render tasks on text runs. * Add support for blur render tasks. Uses standard 2 pass separable technique. * Remove a large amount of code that was used only for old glyph blur support. * Remove blur_radius from GlyphKey, resource cache, texture cache etc. * Add gaussian blur shader (cs_blur). * Add text run cache shader (cs_text_run). * Add generic primitive shader for drawing cache items (ps_cache_image).

glennw · 2016-11-09T04:24:46Z

r? @pcwalton

I'm sorry that this ended up as one commit. I can separate them into a couple of commits if you would prefer?

kvark

I really appreciate the amount of deleted code in this PR. Way to go! Just a few nits I'd prefer addressed before merging.

kvark · 2016-11-09T18:50:23Z

webrender/res/cs_blur.fs.glsl

+    }
+
+    // Unpremultiply the alpha.
+    color = vec4(color.rgb / color.a, color.a);


seems like a longer variant of just color.rgb /= color.a

kvark · 2016-11-09T18:51:46Z

webrender/res/cs_blur.fs.glsl

+}
+
+void main(void) {
+    vec4 color = texture(sCache, vUv) * gauss(0.0, vSigma);


this sample is missing alpha pre-multiplication

kvark · 2016-11-09T18:52:13Z

webrender/res/cs_blur.vs.glsl

+    vec4 local_rect = task.data0;
+
+    vec2 pos = mix(local_rect.xy,
+    			   local_rect.xy + local_rect.zw,


indentation is broken

kvark · 2016-11-09T18:52:56Z

webrender/res/cs_blur.vs.glsl

+
+    vec2 pos = mix(local_rect.xy,
+    			   local_rect.xy + local_rect.zw,
+    			   aPosition.xy);


I wonder if there is any reason not to write this as "aPosition.xy * local_rect.zw + local_rect.xy"

kvark · 2016-11-09T18:54:49Z

webrender/res/cs_blur.vs.glsl

+    vBlurRadius = int(task.data1.y);
+    vSigma = task.data1.y * 0.5;
+
+    vec4 src_rect;


seems unused?

kvark · 2016-11-09T18:55:27Z

webrender/res/cs_blur.vs.glsl

+    switch (cmd.dir) {
+        case DIR_HORIZONTAL:
+            vOffsetScale = vec2(1.0 / texture_size.x, 0.0);
+            vUvRect = vec4(src_task.data0.xy, src_task.data0.xy + src_task.data0.zw);


I don't see any difference in vUvRect, uv0, or uv1 computation between the switch arms

kvark · 2016-11-09T18:56:59Z

webrender/res/cs_text_run.vs.glsl

+    vec2 st1 = glyph.uv_rect.zw / texture_size;
+
+    vec2 pos = mix(local_rect.xy,
+    			   local_rect.xy + local_rect.zw,


indentation broken

kvark · 2016-11-09T19:02:33Z

webrender/src/prim_store.rs

+    // text-shadow, this creates a render task chain
+    // that implements a 2-pass separable blur on a
+    // text run.
+    pub render_task: Option<RenderTask>,


I wonder if we should turn it into Vec<RenderTask>. Do we ever need more than one task?..

We might do in the future - it's an easy change if/when we do need it.

kvark · 2016-11-09T19:11:37Z

webrender/src/tiling.rs

+            id: RenderTaskId::Dynamic(RenderTaskKey::VerticalBlur(blur_radius.0, prim_index)),
+            children: vec![prim_cache_task],
+            location: RenderTaskLocation::Dynamic(None, blur_target_size),
+            kind: RenderTaskKind::VerticalBlur(blur_radius, prim_index),


this seems slightly redundant, as the same info is already stored with the RenderTaskKey

Yea - it makes it a bit easier to extract the info (since you don't have to match on all the key kinds). But I do agree it's a bit redundant and we might want to consider changing how this works in the future.

glennw · 2016-11-09T19:51:03Z

@kvark Thanks for the review, comments addressed.

pcwalton · 2016-11-09T19:54:45Z

@bors-servo: r+

bors-servo · 2016-11-09T19:54:47Z

📌 Commit fc999d0 has been approved by pcwalton

bors-servo · 2016-11-09T19:54:48Z

⌛ Testing commit fc999d0 with merge 106c2c8...

Change text-shadow to blur entire text run, and clean up texture cache. * Add support for text-shadow to be drawn via render tasks on text runs. * Add support for blur render tasks. Uses standard 2 pass separable technique. * Remove a large amount of code that was used only for old glyph blur support. * Remove blur_radius from GlyphKey, resource cache, texture cache etc. * Add gaussian blur shader (cs_blur). * Add text run cache shader (cs_text_run). * Add generic primitive shader for drawing cache items (ps_cache_image).  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/546)

bors-servo · 2016-11-09T20:43:05Z

☀️ Test successful - status-travis

kvark has make several important contributions to webrender, both as a reviewer and committer. He also has significant Rust experience previously as the author of gfx-rs. Some examples of recent reviews: servo/webrender#546 servo/webrender#523 servo/webrender#561 servo/webrender#554

@kvark

[Proposal] Give @kvark reviewer privileges. kvark has make several important contributions to webrender, both as a reviewer and committer. He also has significant Rust experience previously as the author of gfx-rs. Some examples of recent reviews: servo/webrender#546 servo/webrender#523 servo/webrender#561 servo/webrender#554  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/saltfs/551)

kvark has make several important contributions to webrender, both as a reviewer and committer. He also has significant Rust experience previously as the author of gfx-rs. Some examples of recent reviews: servo/webrender#546 servo/webrender#523 servo/webrender#561 servo/webrender#554

kvark reviewed Nov 9, 2016

View reviewed changes

Address review comments

fc999d0

bors-servo merged commit fc999d0 into servo:master Nov 9, 2016

This was referenced Nov 9, 2016

[WIP] Add support for externally allocated images in the render backend #548

Closed

[WIP] Simplify texture names and support more than two textures in the shaders. #534

Merged

glennw deleted the text-shadow branch December 12, 2016 20:43

glennw mentioned this pull request Dec 13, 2016

[Proposal] Give @kvark reviewer privileges. servo/saltfs#551

Merged

Change text-shadow to blur entire text run, and clean up texture cache. #546

Change text-shadow to blur entire text run, and clean up texture cache. #546

Uh oh!

Conversation

glennw commented Nov 9, 2016 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glennw commented Nov 9, 2016

Uh oh!

kvark left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glennw commented Nov 9, 2016

Uh oh!

pcwalton commented Nov 9, 2016

Uh oh!

bors-servo commented Nov 9, 2016

Uh oh!

bors-servo commented Nov 9, 2016

Uh oh!

bors-servo commented Nov 9, 2016

Uh oh!

Uh oh!

glennw commented Nov 9, 2016 •

edited by larsbergstrom

Loading

kvark left a comment •

edited

Loading