Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a debugging feature to visualize overdraw. #2963

Merged
merged 2 commits into from Aug 11, 2018

Conversation

@pcwalton
Copy link
Collaborator

commented Aug 10, 2018

Lighter shades of orange indicate more overdraw.

This has already helped to diagnose a few issues (e.g. #2958).

r? @gw3583


This change is Reviewable

@pcwalton pcwalton requested a review from gw3583 Aug 10, 2018
@gw3583

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2018

Looks like there are some shader validation failures on CI:

https://ci.appveyor.com/project/servo/webrender/build/1.0.10141#L304

It might just be related to needing to pass the right defines to the validation code, if they've changed.

Copy link
Collaborator

left a comment

Reviewed 19 of 19 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @pcwalton)


webrender/res/brush_debug_overdraw.glsl, line 1 at r1 (raw file):

/* This Source Code Form is subject to the terms of the Mozilla Public

Instead of having a separate shader, we could perhaps have just had an #ifdef in the fragment shader skeleton for brush.glsl? Doesn't really matter either way though.

@gw3583

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2018

We'll need to resolve the shader validation errors on CI above. If we instead added an #ifdef in brush.glsl we could probably select the debug variant of each normal brush shader. This might simplify some of the complexity involved with the vecs_per_brush code?

We should do a try run too, to make sure that nothing breaks with any of the weird shader combinations. I can do that on Monday if you're not sure how to do a gecko try.

@gw3583

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2018

(if you think it's simpler to leave as is, that's fine too - the above is just what came to mind when looking through the code).

Copy link
Collaborator

left a comment

Left comments in reviewable.

@gw3583

This comment has been minimized.

Copy link
Collaborator

commented Aug 10, 2018

This is a really helpful feature to have, btw! 👍

@kvark

This comment has been minimized.

Copy link
Member

commented Aug 10, 2018

A screnshot would be handy here

@pcwalton pcwalton force-pushed the pcwalton:show-overdraw branch from 65a47f4 to 50a1293 Aug 10, 2018
@pcwalton

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 10, 2018

@gw3583 Updated patch per your suggestion. This is way simpler :)

@kvark

Here's a before and after:

screen shot 2018-08-10 at 12 01 47 pm

screen shot 2018-08-10 at 12 01 51 pm

// Sets the blend mode. Blend is unconditionally set if the "show overdraw" debugging mode is
// enabled.
fn set_blend(&self, mut blend: bool) {
if !self.debug_flags.contains(DebugFlags::SHOW_OVERDRAW) {

This comment has been minimized.

Copy link
@kvark

kvark Aug 10, 2018

Member

shouldn't this be the other way around? if self.debug_flags.contains(DebugFlags::SHOW_OVERDRAW) { blend = true; }

This comment has been minimized.

Copy link
@pcwalton

pcwalton Aug 10, 2018

Author Collaborator

Doh!

@@ -4013,6 +4020,47 @@ impl Renderer {
}
self.device.end_frame();
}

// Sets the blend mode. Blend is unconditionally set if the "show overdraw" debugging mode is

This comment has been minimized.

Copy link
@kvark

kvark Aug 10, 2018

Member

shouldn't it only affect the final (screen) pass?

This comment has been minimized.

Copy link
@pcwalton

pcwalton Aug 10, 2018

Author Collaborator

Could you elaborate?

This comment has been minimized.

Copy link
@kvark

kvark Aug 10, 2018

Member

We render a frame in multiple passes. All but the last one go into off-screen targets. Since we are only displaying the screen overdraw, we don't want the blending overrides to affect rendering in any other passes than the last one.

This comment has been minimized.

Copy link
@pcwalton

pcwalton Aug 10, 2018

Author Collaborator

OK, changed it.

brush_radial_gradient: BrushShader,
brush_linear_gradient: BrushShader,
brush: BrushShaders,
brush_debug_overdraw: BrushShaders,

This comment has been minimized.

Copy link
@kvark

kvark Aug 10, 2018

Member

a brush shader struct contains 3 versions:

struct BrushShader {
    opaque: LazilyCompiledShader,
    alpha: LazilyCompiledShader,
    dual_source: Option<LazilyCompiledShader>,
}

Would it make sense to add a 4th for the debug overdraw instead of duplicating the whole thing?

@@ -0,0 +1,29 @@
/* This Source Code Form is subject to the terms of the Mozilla Public

This comment has been minimized.

Copy link
@kvark

kvark Aug 10, 2018

Member

is this shader still used?

@pcwalton pcwalton force-pushed the pcwalton:show-overdraw branch from 50a1293 to 6037640 Aug 10, 2018
@pcwalton

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 10, 2018

Comments addressed.

@pcwalton pcwalton force-pushed the pcwalton:show-overdraw branch from 6037640 to a03ab4b Aug 10, 2018
@pcwalton

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 10, 2018

Comment addressed.

@kvark
kvark approved these changes Aug 11, 2018
Copy link
Member

left a comment

Thank you! I got one last nit, otherwise shipit.

@@ -2304,7 +2308,7 @@ impl Renderer {

self.device.disable_scissor();
self.device.disable_depth();
self.device.set_blend(false);
self.set_blend(false, true);

This comment has been minimized.

Copy link
@kvark

kvark Aug 11, 2018

Member

uh, that's a bit confusing when reading the code.
can have an enum TargetKind or something like this?

pcwalton added 2 commits Aug 10, 2018
…ple counts

as percentages to make them clearer
Lighter shades of orange indicate more overdraw.

This has already helped to diagnose a few issues (e.g. #2958).
@pcwalton pcwalton force-pushed the pcwalton:show-overdraw branch from a03ab4b to a80d7e1 Aug 11, 2018
@pcwalton

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 11, 2018

Comment addressed. r? @kvark

@kvark

This comment has been minimized.

Copy link
Member

commented Aug 11, 2018

Wonderful, thank you!
@bors-servo r+

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2018

📌 Commit a80d7e1 has been approved by kvark

bors-servo added a commit that referenced this pull request Aug 11, 2018
Add a debugging feature to visualize overdraw.

Lighter shades of orange indicate more overdraw.

This has already helped to diagnose a few issues (e.g. #2958).

r? @gw3583

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2963)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2018

⌛️ Testing commit a80d7e1 with merge 002c7f6...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Aug 11, 2018

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing 002c7f6 to master...

@bors-servo bors-servo merged commit a80d7e1 into servo:master Aug 11, 2018
3 of 4 checks passed
3 of 4 checks passed
code-review/reviewable 4 files, 6 discussions left (gw3583, kvark, pcwalton)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.