Use z-buffer for rejecting opaque fragments. by glennw · Pull Request #648 · servo/webrender

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

    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

    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

+        }
+    }
+
+    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

 #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


 enum ShaderKind {
    Primitive,
-    Clear,


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

        self.profile_counters.draw_calls.inc();
    }

+    fn submit_batch(&mut self,


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

-            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

        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

        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 [

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

Conversation

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

Uh oh!

Uh oh!

glennw commented Dec 15, 2016

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 Dec 21, 2016

Uh oh!

pcwalton left a comment

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!

kvark left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvark commented Dec 23, 2016 via email

Uh oh!

kvark commented Dec 23, 2016

Uh oh!

bors-servo commented Dec 23, 2016

Uh oh!

bors-servo commented Dec 23, 2016

Uh oh!

bors-servo commented Dec 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

glennw commented Dec 15, 2016 •

edited by larsbergstrom

Loading