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 basic picture caching debug overlay. #3493

Merged
merged 1 commit into from Jan 9, 2019

Conversation

Projects
None yet
4 participants
@gw3583
Copy link
Collaborator

gw3583 commented Jan 9, 2019

Add support for frame building to push debug rects and strings
that are displayed by the debug renderer.

Add a debug flag for toggling the picture caching debug overlay.

Add some basic information to the picture caching overlay:

  • The red rectangle is the current dirty rect.
  • The green rectangles show tile boundaries with per-tile stats.

In future, this will be expanded to include more information about
each cached tile, such as invalidation reasons.


This change is Reviewable

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Jan 9, 2019

r? @kvark or anyone

The current debug display (much more information yet to be added):

dirty_rect

@emilio

emilio approved these changes Jan 9, 2019

Copy link
Member

emilio left a comment

Looks straight-forward :)

@@ -1061,6 +1066,29 @@ impl TileCache {
// Decide how to handle this tile when drawing this frame.
if tile.is_valid {
self.tiles_to_draw.push(TileIndex(i));

#[cfg(feature = "debug_renderer")]

This comment has been minimized.

@emilio

emilio Jan 9, 2019

Member

I think you can put the #[cfg()] just on top of the if and deindent it, here and below.

This comment has been minimized.

@gw3583

gw3583 Jan 9, 2019

Collaborator

Unfortunately that gives error: attributes are not yet allowed on `if` expressions

@kvark
Copy link
Member

kvark left a comment

Wonderful feature! A few comments below

Reviewed 9 of 9 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @emilio and @gw3583)


webrender/src/frame_builder.rs, line 30 at r1 (raw file):

use std::sync::Arc;
use tiling::{Frame, RenderPass, RenderPassKind, RenderTargetContext, RenderTarget};

nit: I liked those empty 2 lines after the use statements (sort of a "header" for the module)


webrender/src/picture.rs, line 988 at r1 (raw file):

        gpu_cache: &mut GpuCache,
        frame_context: &FrameBuildingContext,
        _scratch: &mut PrimitiveScratchBuffer,

can we have it under #cfg instead of marking as unused?


webrender_api/src/api.rs, line 988 at r1 (raw file):

        const TEXTURE_CACHE_DBG_CLEAR_EVICTED = 1 << 14;
        /// Show picture caching debug overlay
        const PICTURE_CACHING_DBG   = 1 << 14;

let's not collide with other flags for clarity, we have space to expand

Add basic picture caching debug overlay.
Add support for frame building to push debug rects and strings
that are displayed by the debug renderer.

Add a debug flag for toggling the picture caching debug overlay.

Add some basic information to the picture caching overlay:
 - The red rectangle is the current dirty rect.
 - The green rectangles show tile boundaries with per-tile stats.

In future, this will be expanded to include more information about
each cached tile, such as invalidation reasons.

@gw3583 gw3583 force-pushed the gw3583:frame-debug branch from 25d5b29 to c5a9ed0 Jan 9, 2019

@gw3583
Copy link
Collaborator

gw3583 left a comment

Reviewable status: 7 of 9 files reviewed, 4 unresolved discussions (waiting on @kvark and @emilio)


webrender/src/frame_builder.rs, line 30 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: I liked those empty 2 lines after the use statements (sort of a "header" for the module)

Fixed


webrender/src/picture.rs, line 988 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we have it under #cfg instead of marking as unused?

You can't place attributes on fn parameters (rustc gives attributes are not allowed here). Or did you mean something else?


webrender_api/src/api.rs, line 988 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

let's not collide with other flags for clarity, we have space to expand

Ooops! Fixed

@kvark

kvark approved these changes Jan 9, 2019

Copy link
Member

kvark left a comment

:lgtm:

Reviewed 2 of 2 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @emilio and @gw3583)


webrender/src/picture.rs, line 988 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

You can't place attributes on fn parameters (rustc gives attributes are not allowed here). Or did you mean something else?

oh indeed

@kvark

This comment has been minimized.

Copy link
Member

kvark commented Jan 9, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

📌 Commit c5a9ed0 has been approved by kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

⌛️ Testing commit c5a9ed0 with merge d3edc30...

bors-servo added a commit that referenced this pull request Jan 9, 2019

Auto merge of #3493 - gw3583:frame-debug, r=kvark
Add basic picture caching debug overlay.

Add support for frame building to push debug rects and strings
that are displayed by the debug renderer.

Add a debug flag for toggling the picture caching debug overlay.

Add some basic information to the picture caching overlay:
 - The red rectangle is the current dirty rect.
 - The green rectangles show tile boundaries with per-tile stats.

In future, this will be expanded to include more information about
each cached tile, such as invalidation reasons.

<!-- 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/3493)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

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

@bors-servo bors-servo merged commit c5a9ed0 into servo:master Jan 9, 2019

3 of 4 checks passed

code-review/reviewable 2 discussions left (emilio, gw3583)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jan 10, 2019

Bug 1518899 - Update webrender to commit d3edc30cf95d3c96fd8308969b22…
…062698a0f6ce (WR PR #3493). r=kats

servo/webrender#3493

Differential Revision: https://phabricator.services.mozilla.com/D16102

--HG--
extra : moz-landing-system : lando

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Jan 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment