Closed Bug 1352151 Opened 3 years ago Closed 3 years ago

Improve the compositor FPS overlay

Categories

(Core :: Graphics: Layers, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dvander, Assigned: dvander)

Details

Attachments

(11 files, 5 obsolete files)

28.63 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
5.48 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
4.83 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
9.38 KB, patch
mattwoodrow
: review+
Details | Diff | Splinter Review
6.71 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
46.05 KB, image/png
Details
25.76 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
8.74 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
32.50 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
33.39 KB, patch
bas.schouten
: review+
Details | Diff | Splinter Review
173.66 KB, image/png
Details
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).
Overdraw is useful for mobile?
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).
Attachment #8853066 - Flags: review?(matt.woodrow)
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.
Attachment #8853068 - Flags: review?(matt.woodrow)
We don't need this anymore.
Attachment #8853069 - Flags: review?(matt.woodrow)
Attached patch part 4, clean-up (obsolete) — Splinter Review
Don't tie TextureRenderer to the Compositor. This lets advanced-layers reuse the glyph stuff.
Attachment #8853070 - Flags: review?(matt.woodrow)
Attached patch part 4, clean-upSplinter Review
Better clean-up, this separates the texture drawing logic from the composite logic.
Attachment #8853070 - Attachment is obsolete: true
Attachment #8853070 - Flags: review?(matt.woodrow)
Attachment #8853155 - Flags: review?(matt.woodrow)
Attachment #8853066 - Flags: review?(matt.woodrow) → review+
Attachment #8853068 - Flags: review?(matt.woodrow) → review+
Attachment #8853069 - Flags: review?(matt.woodrow) → review+
Attachment #8853155 - Flags: review?(matt.woodrow) → review+
(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.
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.
Attachment #8853781 - Flags: review?(bas)
Attached patch part 6, overdraw statistics (obsolete) — Splinter Review
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.
Attachment #8853782 - Flags: review?(bas)
Attached patch part 7, gpu draw time (obsolete) — Splinter Review
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.)
Attachment #8853783 - Flags: review?(bas)
Attached patch part 8, new font (obsolete) — Splinter Review
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/
Attachment #8853784 - Flags: review?(bas)
Attached image screenshot
Here's a screenshot of the new overlay.
Attached patch part 9, fix timing (obsolete) — Splinter Review
This just moves the timing commands earlier so we don't include the diagnostic overlay.
Attachment #8853796 - Flags: review?(bas)
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.
Attachment #8853782 - Attachment is obsolete: true
Attachment #8853783 - Attachment is obsolete: true
Attachment #8853796 - Attachment is obsolete: true
Attachment #8853782 - Flags: review?(bas)
Attachment #8853783 - Flags: review?(bas)
Attachment #8853796 - Flags: review?(bas)
Attachment #8853831 - Flags: review?(bas)
Same patch, but using DiagnosticsD3D11.
Attachment #8853832 - Flags: review?(bas)
Attachment #8853781 - Flags: review?(bas) → review+
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.
Attachment #8853784 - Flags: review?(bas) → review-
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?
Attachment #8853831 - Flags: review?(bas) → review+
Attachment #8853832 - Flags: review?(bas) → review+
(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.
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.
Attachment #8853784 - Attachment is obsolete: true
Attachment #8856334 - Flags: review?(bas)
This adds Consolas as a fixed-width font option, and switches the debug overlay to use it.
Attachment #8856335 - Flags: review?(bas)
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 :-)?
Attachment #8856334 - Flags: review?(bas) → review+
Attachment #8856335 - Flags: review?(bas) → review+
(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
(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.
Attached image font-comparison.png
Bas, here's a font comparison. If you like the bottom one better I'll add a pref.
Pushed by danderson@mozilla.com:
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)
You need to log in before you can comment on or make changes to this bug.