Use z-buffer for rejecting opaque fragments. #648

glennw · 2016-12-15T05:31:51Z

This change is

glennw · 2016-12-15T05:32:39Z

This still needs a little bit of cleanup - but it implements the functionality and passes all tests. So it should be fairly close to the final version.

kvark · 2016-12-15T18:55:00Z

webrender/res/prim_shared.glsl

@@ -286,6 +287,7 @@ struct PrimitiveInstance {
    int clip_task_index;
    int layer_index;
    int sub_index;
+    int z;


we could probably reduce the user_data to a single int in order to keep the 32 byte size

Yep, I think it's fine to do this as a follow up though.

kvark · 2016-12-15T18:55:43Z

webrender/res/prim_shared.glsl

@@ -352,6 +356,7 @@ Primitive load_primitive_custom(PrimitiveInstance pi) {
    prim.prim_index = pi.specific_prim_index;
    prim.sub_index = pi.sub_index;
    prim.user_data = pi.user_data;
+    prim.z = pi.z;


I think some compilers may require an explicit cast here.
Moreover, what are you going to do with this Z value? We need to scale it down into [-1, 1] region before passing into gl_Position, and I don't see this happening anywhere.

Fixed the cast. The vertices are transformed by uTransform, which is an orthographic projection matrix at the end of the function. This converts the z values to NDC.

kvark · 2016-12-15T18:58:29Z

webrender/src/device.rs

+        }
+    }
+
+    pub fn enable_depth(&self) {


we could merge those enable_*/disable_* for tinier interface with the device

Yep, perhaps as a follow up we should tidy up the whole device interface?

kvark · 2016-12-15T18:59:18Z

webrender/res/ps_gradient.vs.glsl

@@ -39,6 +39,7 @@ void main(void) {
 #ifdef WR_FEATURE_TRANSFORM
    TransformVertexInfo vi = write_transform_vertex(segment_rect,
                                                    prim.local_clip_rect,
+                                                    prim.z,


at this point, we might as well want to just pass prim there directly

Some of the shaders generate the local rect separately (as in the case above), which is why I wasn't passing in the prim directly - to avoid bugs where that code accesses the prim.local_rect accidentally. But we could probably tidy up those structure definitions in the shader code.

kvark · 2016-12-15T19:01:33Z

webrender/src/renderer.rs

@@ -123,7 +122,6 @@ const CLIP_FEATURE: &'static str = "CLIP";

 enum ShaderKind {
    Primitive,
-    Clear,


kvark · 2016-12-15T19:02:16Z

webrender/src/renderer.rs

@@ -990,12 +961,87 @@ impl Renderer {
        self.profile_counters.draw_calls.inc();
    }

+    fn submit_batch(&mut self,


kvark · 2016-12-15T19:05:21Z

webrender/src/renderer.rs

-            let needs_clipping = batch.key.flags.needs_clipping();
-            debug_assert!(!needs_clipping || batch.key.blend_mode == BlendMode::Alpha);
+        self.device.enable_depth();
+        self.device.enable_depth_write();


do we set the depth function anywhere?

glennw · 2016-12-21T21:57:23Z

@kvark @pcwalton I think this is ready for review now.

pcwalton

Looks good, just had one question

pcwalton · 2016-12-21T22:41:09Z

replay/src/main.rs

@@ -120,7 +120,6 @@ fn main() {
        debug: false,
        enable_subpixel_aa: false,
        clear_framebuffer: true,
-        clear_empty_tiles: false,


Why do we not clear empty tiles anymore?

I think it's already guaranteed to be cleared in draw_target. Not sure though, at what point it became redundant/useless.

Since we have to clear the z-buffer now, I figure we might as well just do a clear of the color buffer as well. This also makes it perform much better on mobile / tiled GPUs which rely on a clear due to the way tiling works. We could definitely re-instate this later as an optimization if it makes sense to.

kvark

@glennw I agree with all the "let's do X as follow up" requests, the PR looks good!

kvark · 2016-12-22T21:54:49Z

replay/src/main.rs

@@ -120,7 +120,6 @@ fn main() {
        debug: false,
        enable_subpixel_aa: false,
        clear_framebuffer: true,
-        clear_empty_tiles: false,


I think it's already guaranteed to be cleared in draw_target. Not sure though, at what point it became redundant/useless.

kvark · 2016-12-23T00:53:40Z

AMD GCN architecture, covering probably most of the AMD cards on the market, has meta-data for clears, fragment masks, and depth/stencil bounds - all per internal tile. So clearing on them is dirt cheap, you just pay a little before sampling from those targets for the first time, since the driver needs to fill up the untouched tiles. It basically does what is being removed from WR now, just more efficiently ;)

…

On Dec 22, 2016, at 18:10, Glenn Watson ***@***.***> wrote: @glennw commented on this pull request. In replay/src/main.rs: > @@ -120,7 +120,6 @@ fn main() { debug: false, enable_subpixel_aa: false, clear_framebuffer: true, - clear_empty_tiles: false, Since we have to clear the z-buffer now, I figure we might as well just do a clear of the color buffer as well. This also makes it perform much better on mobile / tiled GPUs which rely on a clear due to the way tiling works. We could definitely re-instate this later as an optimization if it makes sense to. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

kvark · 2016-12-23T02:06:02Z

@bors-servo r+

bors-servo · 2016-12-23T02:06:03Z

📌 Commit 7eb458c has been approved by kvark

bors-servo · 2016-12-23T02:06:07Z

⌛ Testing commit 7eb458c with merge 56131c9...

Use z-buffer for rejecting opaque fragments.  This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/648)

bors-servo · 2016-12-23T02:15:49Z

☀️ Test successful - status-travis

gw3583 added 4 commits December 15, 2016 14:27

Remove clear tiles.

3ea1645

Clear color support

0ffcb66

Remove old occlusion culling

2285e4f

Make use of the z-buffer for rejecting opaque fragments.

5d24bb8

kvark suggested changes Dec 15, 2016

View reviewed changes

glennw mentioned this pull request Dec 21, 2016

add ability to set weather the background should be cleared on RendererOptions #655

Closed

Address some review comments.

7eb458c

glennw changed the title ~~[WIP] Use z-buffer for rejecting opaque fragments.~~ Use z-buffer for rejecting opaque fragments. Dec 21, 2016

pcwalton reviewed Dec 21, 2016

View reviewed changes

kvark approved these changes Dec 22, 2016

View reviewed changes

bors-servo merged commit 7eb458c into servo:master Dec 23, 2016

bors-servo mentioned this pull request Dec 23, 2016

Handle external image life time in api.delete_image() call #636

Merged

glennw deleted the zb branch December 23, 2016 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use z-buffer for rejecting opaque fragments. #648

Use z-buffer for rejecting opaque fragments. #648

glennw commented Dec 15, 2016 •

edited by larsbergstrom

Loading

glennw commented Dec 15, 2016

kvark Dec 15, 2016

glennw Dec 21, 2016

kvark Dec 15, 2016

glennw Dec 21, 2016

kvark Dec 15, 2016

glennw Dec 21, 2016

kvark Dec 15, 2016

glennw Dec 21, 2016

kvark Dec 15, 2016

kvark Dec 15, 2016

kvark Dec 15, 2016

glennw Dec 21, 2016

glennw commented Dec 21, 2016

pcwalton left a comment

pcwalton Dec 21, 2016

kvark Dec 22, 2016

glennw Dec 22, 2016

kvark left a comment

kvark Dec 22, 2016

kvark commented Dec 23, 2016 via email

kvark commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

Use z-buffer for rejecting opaque fragments. #648

Use z-buffer for rejecting opaque fragments. #648

Conversation

glennw commented Dec 15, 2016 • edited by larsbergstrom Loading

glennw commented Dec 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glennw commented Dec 21, 2016

pcwalton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kvark commented Dec 23, 2016 via email

kvark commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

glennw commented Dec 15, 2016 •

edited by larsbergstrom

Loading