Closed Bug 1390840 Opened 3 years ago Closed 3 years ago

Expose texture cache and intermediate targets debug utilities

Categories

(Core :: Graphics: WebRender, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: nical, Assigned: nical)

Details

Attachments

(1 file, 3 obsolete files)

WebRender has debugging tools to show its embedded frame profiler, but also the content of the texture cache and the intermediate render targets. The latter two are not yet usable in gecko.
This adds the prefs:
- gfx.webrender.debug.texture-cache
- gfx.webrender.debug.render-targets
- gfx.webrender.debug.profiler (instead of gfx.webrender.profiler.enabled)

To interact with webrender's builtin debugging tools. In the future, adding support for more debugging tools will just be a matter of setting new bits in the DebugFlags bit-field (instead of adding code all over gfx/layers to pass the information from prefs to webrender with ipc in the middle).
Assignee: nobody → nical.bugzilla
Attachment #8897828 - Flags: review?(jmuizelaar)
Comment on attachment 8897828 [details] [diff] [review]
Add prefs, refactor the way debug options are passed to webrender.

Review of attachment 8897828 [details] [diff] [review]:
-----------------------------------------------------------------

Drive-by comments: I'm a little confused why you are sending this over IPDL instead of just handling it directly on the parent side. The prefs are already synced across processes so the parent side has the same pref information that the child side does. And because there's one WebRenderBridgeChild per tab (each with its own copy of mDebugFlags) any time one of these prefs is changed you'll be sending a message per-tab which seems inefficient because you only need it per-renderer.

::: gfx/thebes/gfxPrefs.h
@@ +487,5 @@
>    DECL_GFX_PREF(Live, "gfx.webrender.highlight-painted-layers",WebRenderHighlightPaintedLayers, bool, false);
>    DECL_GFX_PREF(Live, "gfx.webrender.layers-free",             WebRenderLayersFree, bool, false);
> +  DECL_GFX_PREF(Live, "gfx.webrender.debug.profiler",          WebRenderDebugProfiler, bool, false);
> +  DECL_GFX_PREF(Live, "gfx.webrender.debug.texture-cache",     WebRenderDebugTextureCache, bool, false);
> +  DECL_GFX_PREF(Live, "gfx.webrender.debug.render-targets",    WebRenderDebugRenderTarget, bool, false);

Alphabetical order by pref name, please

::: gfx/webrender_bindings/RenderThread.h
@@ +130,1 @@
>    RenderTextureHost* GetRenderTexture(WrExternalImageId aExternalImageId);

This drops the thread comment on GetRenderTexture
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> Comment on attachment 8897828 [details] [diff] [review]
> Add prefs, refactor the way debug options are passed to webrender.
> 
> Review of attachment 8897828 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm a little confused why you are sending this over IPDL
> instead of just handling it directly on the parent side.

My understanding is that you can't access libpref from the GPU process (if this assumption is wrong then I can happily cut down by half the size of this patch).

> And because there's one
> WebRenderBridgeChild per tab (each with its own copy of mDebugFlags) any
> time one of these prefs is changed you'll be sending a message per-tab which
> seems inefficient because you only need it per-renderer.

I meant to check that the current process is the parent process before sending the message to avoid doing it once per child process, and then I forgot about it. I don't think it will make a measurable difference anyway, because the pref rarely changes and also we should have a WebRenderBridgeChild per process, not tab (I think?).
gfxVars should be used for getting prefs to the gpu process.
APZ uses gfxPrefs all over the place, and lives in the GPU process. I'm pretty sure it works. There's some code to send pref changes over the PGPU channel at http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/gfx/thebes/gfxPrefs.cpp#81
(In reply to Nicolas Silva [:nical] from comment #3)
> we should have a
> WebRenderBridgeChild per process, not tab (I think?).

No, WebRenderBridgeChild is similar to LayerTransactionChild - there is one per layer manager. It is created at http://searchfox.org/mozilla-central/rev/13148faaa91a1c823a7d68563d9995480e714979/gfx/layers/wr/WebRenderLayerManager.cpp#62. So it will be one per tab.
Attachment #8897828 - Flags: review?(jmuizelaar) → review-
This version is simpler. It doesn't use gfxPrefs and use a pref change callback instead and pushes the change through gfxVars.
Attachment #8897828 - Attachment is obsolete: true
Attachment #8898328 - Flags: review?(jmuizelaar)
Comment on attachment 8898328 [details] [diff] [review]
Add prefs, refactor the way debug options are passed to webrender (v2)

Review of attachment 8898328 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/thebes/gfxPlatform.cpp
@@ +613,5 @@
> +  }
> +  if (Preferences::GetBool(WR_DEBUG_PREF".render-targets", false)) {
> +    flags |= (1 << 2);
> +  }
> +  printf(" pref change: %s  -> %i \n", aPrefName, (int)flags);

Was it intentional to leave the printf here?

::: gfx/webrender_bindings/RendererOGL.cpp
@@ +131,5 @@
>  {
> +  uint32_t flags = gfx::gfxVars::WebRenderDebugFlags();
> +  
> +  if (mDebugFlags.mBits != flags) {
> +    printf("wr flags changed: %i\n", (int)flags);

Same with this printf

::: gfx/webrender_bindings/webrender_ffi_generated.h
@@ +6,1 @@
>  

It's probably better to just use cbindgen 0.1.20 so this doesn't get a bunch of unrelated changes.
Attachment #8898328 - Flags: review?(jmuizelaar) → review+
Attached patch Update to cbindgen 0.1.21 (obsolete) — Splinter Review
Github caught a cold and I am currently unable to download cbindgen 0.1.20 So I just ran cbindgen again with version 0.1.21 in a separate patch to avoid mixing the two changes.
Attachment #8899470 - Flags: review?(jmuizelaar)
Attachment #8899470 - Flags: review?(jmuizelaar) → review+
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/483991689217
Use cbindgen 0.1.21 in webrender's binding. r=jrmuizel
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c53ad8893b
Add prefs to display the debugging view of webrender's texture cache and intermediate targets. r=jrmuizel
Backed out for bustage at dist/include/mozilla/webrender/webrender_ffi_generated.h:1199: expected ',' before ')' token:

https://hg.mozilla.org/integration/mozilla-inbound/rev/14544a690e384aa516dafc550e4f6761aa9306ad
https://hg.mozilla.org/integration/mozilla-inbound/rev/226958859036e7562eead0fa8e321d238f38e9c8

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=b9c53ad8893b177521d194a10140eae14da49297&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=124600540&repo=mozilla-inbound

[task 2017-08-21T16:33:38.980313Z] 16:33:38     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/webrender/webrender_ffi_generated.h:1199:33: error: expected ',' before ')' token
[task 2017-08-21T16:33:38.980408Z] 16:33:38     INFO -   static_assert(sizeof(float) == 4);
[task 2017-08-21T16:33:38.980454Z] 16:33:38     INFO -                                   ^
[task 2017-08-21T16:33:38.980568Z] 16:33:38     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/webrender/webrender_ffi_generated.h:1199:33: error: expected string-literal before ')' token
[task 2017-08-21T16:33:38.980648Z] 16:33:38     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/webrender/webrender_ffi_generated.h:1200:34: error: expected ',' before ')' token
[task 2017-08-21T16:33:38.980721Z] 16:33:38     INFO -   static_assert(sizeof(double) == 8);
[task 2017-08-21T16:33:38.980759Z] 16:33:38     INFO -                                    ^
[task 2017-08-21T16:33:38.981286Z] 16:33:38     INFO -  /home/worker/workspace/build/src/obj-firefox/dist/include/mozilla/webrender/webrender_ffi_generated.h:1200:34: error: expected string-literal before ')' token
[task 2017-08-21T16:33:38.981841Z] 16:33:38     INFO -  /home/worker/workspace/build/src/config/rules.mk:1064: recipe for target 'Unified_cpp_xpcom_build0.o' failed
[task 2017-08-21T16:33:38.982355Z] 16:33:38     INFO -  gmake[5]: *** [Unified_cpp_xpcom_build0.o] Error 1
Flags: needinfo?(nical.bugzilla)
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0451006e9f
Add prefs to display the debugging view of webrender's texture cache and intermediate targets. r=jrmuizel
Flags: needinfo?(nical.bugzilla)
Attachment #8899470 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/ba0451006e9f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
You need to log in before you can comment on or make changes to this bug.