874781 - add color bars to layer diagnostics, for vsync checking

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Yay refactoring!

I wanted to have FPS data, layer borders, and I wanted to add frame colorbars.  I didn't want to do them per-platform.  Thus, I called in REFACTOR-MAN to helped me out!  (Worst superhero ever.)

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Part 1, refactor diagnostics and add colorbars

This refactors the diagnostics into Compositor, and adds color bar display.  Diagnostics only work for new-world OMTC compositors, so all the more reason to get there.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: Part 2, move FPS display into generic code

This moves the FPS display into generic code.  Currently only fully implemented for D3D11, it needs DataTextureSourceOGL to be done which should be trivial.  Will do it shortly.

Comment 3

[Nicolas Silva [:nical]]

Comment on attachment 752599 [details] [diff] [review]
Part 2, move FPS display into generic code

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

::: gfx/2d/Factory.cpp
@@ +492,5 @@
> +
> +  RefPtr<SourceSurfaceRawData> newSurf = new SourceSurfaceRawData();
> +
> +  if (newSurf->InitWrappingData(data, aSize, stride, aFormat, true)) {
> +    return newSurf;

should be newSurf.forget();

::: gfx/layers/FPPStats.cpp
@@ +12,5 @@
> +
> +gfx::SourceSurfaceRawData*
> +FPSUtils::AllocateFPSSurface()
> +{
> +}

file should have gone away

Comment 4

[Nicolas Silva [:nical]]

Comment on attachment 752599 [details] [diff] [review]
Part 2, move FPS display into generic code

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

waiting for an updated version of this patch

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 752599 [details] [diff] [review]
Part 2, move FPS display into generic code

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

waiting for an updated version of this patch

::: gfx/layers/composite/TextureHost.h
@@ +116,5 @@
> +  /* Upload a new DataSourceSurface to this DataTextureSource.  It
> +   * must have the same dimensions and format as what it was
> +   * constructed with.
> +   */
> +  virtual bool UploadDataSourceSurface(gfx::DataSourceSurface *aNewSurface) = 0;

Actually, I'd like that this method take alsoe these two arguments:
- nsIntRegion* aDestRegion = nullptr // nullptr means the entire thing
- gfx::IntPoint* aSrcOffset = nullptr // nullptr means {0,0} offset

This is because for another bug we need this DataTextureSource abstraction with these two parameters to do partial updates.

Comment 7

[u459114]

Hi Vlad,
In many case, user cares not only AVG FPS, but also STEDV of FPS. 
We may add more on screen sprite to reveal benchmark data for developer. Please also considerate this possibility of doing this.

Comment 8

[u459114]

To draw FPS in HwcCompositor, it's helpful if the hoster of HwcCompositor & CompositorOGL, which is LayerManagerOGL, is in charge of reading preference value and enable/ disable FPS drawing for both HwcCompositor and CompositorOGL.

Comment 9

[Nicolas Silva [:nical]]

(In reply to C.J. Ku[:CJKu] from comment #8)
> To draw FPS in HwcCompositor, it's helpful if the hoster of HwcCompositor &
> CompositorOGL, which is LayerManagerOGL, is in charge of reading preference
> value and enable/ disable FPS drawing for both HwcCompositor and
> CompositorOGL.

We can only do that on B2G, though. for other platforms only the main thread is allowed to directly access prefs.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: color-bars.patch

Reviving this, now that compositor is much more stable.

This just adds drawing color bars based on the current frame number.  In the future, we can maybe use the actual transaction number from bug 854421 to know exactly when something made it to the screen (though that won't be enough, not all frames have a transaction).

Comment 11

[Benoit Girard (:BenWa)]

Comment on attachment 8416102 [details] [diff] [review]
color-bars.patch

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

Looks great, sorry for the delay.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1073160bd1e4

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1073160bd1e4