1352151 - Improve the compositor FPS overlay

Status: RESOLVED FIXED

(Core :: Graphics: Layers, enhancement)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

The compositor overlay is a little hard to read, and contains some information that is not so useful (like the overdraw factor) and could use some other information that is very useful (like painting/raster times).

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Overdraw is useful for mobile?

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, diagnostics

This patch collects timings similar to our paint telemetry, and stores them on ShadowLayerForwarder. We collect:
 * Display List building (DL)
 * FrameLayerBuilder (FLB)
 * Rasterization (R)
 * Transaction building/copying (CP)
 * Transaction serialization/sending (TX)
 * Transaction receiving/updating (UP)
 * Composite preparation/frame building (CC_BUILD)
 * Composite execution (CC_EXEC)

The content metrics are sent over IPC immediately after sending a transaction. The metrics are collected into a new Diagnostics class. To compute the average time of a metric, we use the last 2 seconds of samples (to avoid stale frames holding too much weight in the average).

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, use new overlay

This builds a string and composes it to the top-left corner using TextRenderer. I didn't extend the old FPSState mono-space code since this has full text support already.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 3, remove fpsstate

We don't need this anymore.

Comment 5

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, clean-up

Don't tie TextureRenderer to the Compositor. This lets advanced-layers reuse the glyph stuff.

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 4, clean-up

Better clean-up, this separates the texture drawing logic from the composite logic.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Milan Sreckovic [:milan] from comment #1)
> Overdraw is useful for mobile?

Matt and I talked about this briefly - I agree, especially after Bas's memory bandwidth experiment showed that pixel traffic matters a lot. However the way we compute overdraw is pretty inaccurate. D3D11 actually lets you query exactly how many times pixel shaders were invoked: https://msdn.microsoft.com/en-us/library/windows/desktop/ff476192(v=vs.85).aspx

It's possible by dividing that with the expected draw area, we'd get an accurate overdraw number. I'll try it out.

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 5, cleanup part 2

This is a tiny refactoring. I added a WaitforGPUQuery helper, to make blocking on ID3D11DeviceContext::GetData easier. I also moved much of the code out of EndFrame() into a separate Present() call. The next few patches add a bunch of stuff to EndFrame and it was getting messy.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 6, overdraw statistics

This patch adds overfill statistics to the diagnostic overlay. Two statistics are included: the ratio of "pixels shaded" to "pixels invalidated", and "pixels shaded" to "pixels on screen". The first shows how well we handle occlusion hints, the second shows how well we invalidate.

The "pixels shaded" metric is determined by a D3D11 query, which we stick in the pipeline and delay querying until the next frame, so as to not block the compositor.

Note #1: The act of rendering the overlay itself adds to the shaded pixel count, I'll attempt to correct this in a separate patch.

Note #2: We used to expose the overfill ratio to layout, but the only use for this was a disabled test. I removed that feature and the test.

Note #3: I did not update the OGL compositor, so it continues to use the older CPU method of guessing affected pixel counts.

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 7, gpu draw time

So far these patches will show the time taken by ContainerPrepare and ContainerRender, but not the actual time the GPU spends executing commands. We can get this by sticking timestamp queries into the command stream.

For most pages the time is negligible, but with complex layers (for example, the nojs 3d animation), the time is quite significant all around.

(Note that I moved the queries before the Present call, since otherwise the draw time is kind of meaningless.)

Comment 11

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 8, new font

This replaces the diagnostic font with Consolas, which is fixed width (but really I just wanted bolder text). I used CBFG to make the bitmap, and ImageMagick to make it 8-bit.

This also fixes a bug where the opacity didn't work since ClearRect cleared the blend state.

[1] http://www.codehead.co.uk/cbfg/

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: screenshot

Here's a screenshot of the new overlay.

Comment 13

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 9, fix timing

This just moves the timing commands earlier so we don't include the diagnostic overlay.

Comment 14

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 6, overdraw statistics

Actually, these patches ended up being very hard to share with AL. This is the same "overdraw statistics" patch, but split out into a new DiagnosticsD3D11 class that can be used outside of the Compositor.

Comment 15

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 7, draw timing

Same patch, but using DiagnosticsD3D11.

Comment 16

[Bas Schouten (:bas.schouten)]

Comment on attachment 8853784 [details] [diff] [review]
part 8, new font

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

If we want to do this we need to add two fonts. The text renderer is used in some places to present vastly larger amounts of data. A 9 pixel fixed width makes this considerably less readable.

Comment 17

[Bas Schouten (:bas.schouten)]

Comment on attachment 8853831 [details] [diff] [review]
part 6, overdraw statistics

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

Nit: The comment could use some work, 'overfilling' to some extent is expected, think of text for example, the comment doesn't reflact that too well. Also why would the ratio of shaded pixels to window size be at most 1.0?

::: gfx/layers/d3d11/HelpersD3D11.h
@@ +13,5 @@
> +namespace layers {
> +
> +template <typename T> static inline bool
> +WaitForGPUQuery(ID3D11Device* aDevice, ID3D11DeviceContext* aContext, ID3D11Query* aQuery, T* aOut)
> +{

Be aware this makes profiling with the overlay on much more risky, the performance characteristics of this are 'not good'.

::: gfx/layers/wr/WebRenderCanvasLayer.h
@@ +28,5 @@
>    }
>  
>    virtual void Initialize(const Data& aData) override;
>  
> +  virtual CompositableForwarder* GetForwarder() override;

Is this meant to be in here?

Comment 18

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Bas Schouten (:bas.schouten) from comment #17)
> Comment on attachment 8853831 [details] [diff] [review]
> part 6, overdraw statistics
> 
> Review of attachment 8853831 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nit: The comment could use some work, 'overfilling' to some extent is
> expected, think of text for example, the comment doesn't reflact that too
> well. Also why would the ratio of shaded pixels to window size be at most
> 1.0?

Right, thanks, that doesn't make any sense. I'll fix.

> ::: gfx/layers/d3d11/HelpersD3D11.h
> @@ +13,5 @@
> > +namespace layers {
> > +
> > +template <typename T> static inline bool
> > +WaitForGPUQuery(ID3D11Device* aDevice, ID3D11DeviceContext* aContext, ID3D11Query* aQuery, T* aOut)
> > +{
> 
> Be aware this makes profiling with the overlay on much more risky, the
> performance characteristics of this are 'not good'.

I'm hoping that coupling it with the existing block-for-previous-frame code, and double-buffering the queries, will make it not so bad.

> ::: gfx/layers/wr/WebRenderCanvasLayer.h
> @@ +28,5 @@
> >    }
> >  
> >    virtual void Initialize(const Data& aData) override;
> >  
> > +  virtual CompositableForwarder* GetForwarder() override;
> 
> Is this meant to be in here?

Yes, it's a fix for unified build bustage.

Comment 19

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 8, allow for multiple fonts

This refactors TextRenderer a bit so it can have multiple fonts. The FontData.h file now exposes a FontBitmapInfo struct, and TextRenderer caches this and the bitmap in a FontCache structure.

The RenderText function specifies which font to use.

Comment 20

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 9, add consolas

This adds Consolas as a fixed-width font option, and switches the debug overlay to use it.

Comment 21

[Bas Schouten (:bas.schouten)]

Comment on attachment 8856334 [details] [diff] [review]
part 8, allow for multiple fonts

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

::: gfx/layers/d3d11/CompositorD3D11.cpp
@@ +869,5 @@
>  
>    mContext->Draw(4, 0);
> +
> +  // Restore the default blend state.
> +  mContext->OMSetBlendState(mAttachments->mPremulBlendState, sBlendFactor, 0xFFFFFFFF);

Umm... we shouldn't need to add this back :-)?

Comment 22

[Bas Schouten (:bas.schouten)]

(In reply to David Anderson [:dvander] from comment #20)
> Created attachment 8856335 [details] [diff] [review]
> part 9, add consolas
> 
> This adds Consolas as a fixed-width font option, and switches the debug
> overlay to use it.

Can you add a screenshot of the fixed-width font option? I suspect I'll like the other one much better so maybe make it preffable? :-D

Comment 23

[David Anderson [:dvander] - inactive, e-mail if emergency]

(In reply to Bas Schouten (:bas.schouten) from comment #21)
> Comment on attachment 8856334 [details] [diff] [review]
> part 8, allow for multiple fonts
> 
> Review of attachment 8856334 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d11/CompositorD3D11.cpp
> @@ +869,5 @@
> >  
> >    mContext->Draw(4, 0);
> > +
> > +  // Restore the default blend state.
> > +  mContext->OMSetBlendState(mAttachments->mPremulBlendState, sBlendFactor, 0xFFFFFFFF);
> 
> Umm... we shouldn't need to add this back :-)?

It was supposed to go in another patch, but: we need it because we sometimes Clear before drawing overlays, and if we don't put the blend state back then we lose background opacity on the overlay.

Comment 24

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: font-comparison.png

Bas, here's a font comparison. If you like the bottom one better I'll add a pref.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/38f9f8c344da
Collect diagnostics on paint times for the compositor overlay. (bug 1352151 part 1, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c661d28e055b
Switch the compositor to a new debug overlay. (bug 1352151 part 2, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e32bed22b39
Remove FPSState. (bug 1352151 part 3, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/f80960d9c4fe
Remove the TextureRender dependency on Compositor. (bug 1352151 part 4, r=mattwoodrow)
https://hg.mozilla.org/integration/mozilla-inbound/rev/df901e207ad2
Refactor CompositorD3D11::EndFrame. (bug 1352151 part 5, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bfb1ea0619f
Improve pixel fill statistics in the D3D11 compositor overlay. (bug 1352151 part 6, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee3f0299a518
Add GPU draw time to the compositor diagnostic overlay. (bug 1352151 part 7, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/b72d2a630431
Allow TextRenderer to render multiple fonts. (bug 1352151 part 8, r=bas)
https://hg.mozilla.org/integration/mozilla-inbound/rev/c05000863d49
Add a fixed-width font for the compositor's debug overlay. (bug 1352151 part 9, r=bas)

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/38f9f8c344da
https://hg.mozilla.org/mozilla-central/rev/c661d28e055b
https://hg.mozilla.org/mozilla-central/rev/2e32bed22b39
https://hg.mozilla.org/mozilla-central/rev/f80960d9c4fe
https://hg.mozilla.org/mozilla-central/rev/df901e207ad2
https://hg.mozilla.org/mozilla-central/rev/2bfb1ea0619f
https://hg.mozilla.org/mozilla-central/rev/ee3f0299a518
https://hg.mozilla.org/mozilla-central/rev/b72d2a630431
https://hg.mozilla.org/mozilla-central/rev/c05000863d49