944866 - Introduce a ScrollGraph overlay

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: Demo Gif - smooth scroll with jank at the end (Low FPS)

Currently people are using the FPS counter for measuring scrolling. This is *TERRIBLE*, especially on b2g where some frames might come from trivial updates like the status bar or an async animation. 60 FPS, 50 FPS doesn't mean we're smoothly scrolling a layer

This scroll graphs work well with momentum scrolling (trackpad or touch). We could extend this code to show other types of graph for async animation.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: Messy WIP

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Messy WIP

Comment 3

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add DrawLines to Compositor.h and implement for OGL

Part 2 wont be ready yet but would be nice to get this out of the way. Right now with OGL it's easy to support the EffectChain (I think?), but maybe it's overkill and we should just have a simple DrawLines with a flat color.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add DrawLines to Compositor.h and implement for OGL

Comment 5

[Andreas Gal :gal]

Comment on attachment 8340661 [details] [diff] [review]
Part 1: Add DrawLines to Compositor.h and implement for OGL

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

I will pretend I am qualified to review this to get you going.

::: gfx/layers/Compositor.h
@@ +297,5 @@
>    /**
> +   * Tell the compositor to draw lines connecting the points. Behaves like
> +   * DrawQuad.
> +   */
> +  virtual void DrawLines(const nsTArray<gfx::Point>& aLines, const gfx::Rect& aClipRect,

I would use vector<> at the call site and pass a pointer and length in here. nsTArray is so yesterday.

::: gfx/layers/opengl/CompositorOGL.cpp
@@ +1569,5 @@
>      mGLContext->fEnableVertexAttribArray(aTexCoordAttribIndex);
>    }
>  
>    mGLContext->fEnableVertexAttribArray(aVertAttribIndex);
> +  if (mDrawType == LOCAL_GL_LINE_STRIP) {

mDrawType is pretty nasty. How about a DrawQuadInternal where you pass in the draw type?

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add DrawLines to Compositor.h and implement for OGL. r=gal

Comment 7

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=238a748290fd

Comment 8

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b47cf733fa34

Comment 9

[Benoit Girard (:BenWa)]

Part 2 is held back because it's harder to get working for both APZC and non APZC. I may just land it for non APZC now and port to APZC later.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b47cf733fa34

Comment 11

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add layers.scroll-graph

https://tbpl.mozilla.org/?tree=Try&rev=c2b263756169

Comment 12

[Benoit Girard (:BenWa)]

This doesn't support APZC very well. The counters stays at the top left of the displayport which is offscreen when not at the top of the layer/page. I'll push to improve it there in a follow-up.

Comment 13

[Andreas Gal :gal]

Comment on attachment 8341499 [details] [diff] [review]
Part 2: Add layers.scroll-graph

This is not the right patch. Its a very cool patch though :)

Comment 14

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add layers.scroll-graph

Right patch this time :)

Comment 15

[Benoit Girard (:BenWa)]

Comment on attachment 8341562 [details] [diff] [review]
Part 2: Add layers.scroll-graph

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

::: modules/libpref/src/init/all.js
@@ +4240,5 @@
>  pref("layers.max-active", -1);
> +// When a layer is moving it will add a scroll graph to measure the smoothness
> +// of the movement. Data will be output to stderr and can be graph more
> +// accuratly in a spreadsheet. NOTE: This pref triggers composites to refresh
> +// the graph.

Opps, I intended to do this but haven't done it yet.

Comment 16

[Andreas Gal :gal]

Comment on attachment 8341562 [details] [diff] [review]
Part 2: Add layers.scroll-graph

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

r=me. You will need to explain to people how this works and what the graph means and want to add a Gaia pref for this? (or ask someone to do it for you).

::: gfx/layers/composite/ContainerLayerComposite.cpp
@@ +96,5 @@
> +  std::vector<VelocityData> mData;
> +};
> +
> +static void
> +DrawVelGraph(const nsIntRect& aClipRect,

Nit: This fits in one line and would look simpler.

@@ +104,5 @@
> +  gfx::Rect clipRect(aClipRect.x, aClipRect.y, aClipRect.width, aClipRect.height);
> +
> +  TimeStamp now = TimeStamp::Now();
> +
> +  void* key = reinterpret_cast<void*>(gfxPlatform::GetPrefLayersScrollGraph);

Want to do a dedicated static char sLayerVelocityUserDataKey; for this?

@@ +120,5 @@
> +  }
> +
> +  nsIntPoint scrollOffset = aLayer->GetEffectiveVisibleRegion().GetBounds().TopLeft();
> +  velocityData->mData.push_back(
> +    LayerVelocityUserData::VelocityData{now, Point(scrollOffset.x,

Nit: I didn't even know this syntax works. Want to use a real constructor? Would make this shorter.

@@ +144,5 @@
> +      return;
> +    }
> +  }
> +
> +  if (aLayer->GetEffectiveVisibleRegion().GetBounds().width < 300 ||

This might not work well for HVGA phones.

@@ +303,5 @@
>      } else {
>        layerToRender->RenderLayer(clipRect);
>      }
> +
> +    if (gfxPlatform::GetPrefLayersScrollGraph()) {

This is racy but who cares.

Comment 17

[Benoit Girard (:BenWa)]

(In reply to Andreas Gal :gal from comment #16)
> > +  gfx::Rect clipRect(aClipRect.x, aClipRect.y, aClipRect.width, aClipRect.height);
> > +
> > +  TimeStamp now = TimeStamp::Now();
> > +
> > +  void* key = reinterpret_cast<void*>(gfxPlatform::GetPrefLayersScrollGraph);
> 
> Want to do a dedicated static char sLayerVelocityUserDataKey; for this?

Ok. Normally I do things like that to save a few bytes in the executable when it doesn't hurt readability much.

Comment 18

[Benoit Girard (:BenWa)]

(In reply to Andreas Gal :gal from comment #16)
> @@ +144,5 @@
> > +      return;
> > +    }
> > +  }
> > +
> > +  if (aLayer->GetEffectiveVisibleRegion().GetBounds().width < 300 ||
> 
> This might not work well for HVGA phones.
> 

The size of the graph would also be wrong. Let's port this to HVGA phones when we need it. We can always fallback to doing something like 'adb lolcat | grep ScrollGraph' in the time being.

I'll be adding a simple printf to get the data out.

Comment 19

[Benoit Girard (:BenWa)]

Actually I added the printf_stderr code but comment out since it's too noisy and I'm afraid it might drag down performance. For now we can enable it by rebuilding.

Comment 20

[Benoit Girard (:BenWa)]

Attachment: Part 2: Add layers.scroll-graph, r=gal

Comment 21

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=6dc8ce6f823b

Comment 22

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/361907c4a2ce

Documentation coming after it merges.

Comment 23

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c30753241b8d

Comment 24

[Benoit Girard (:BenWa)]

*sigh* This intermittent failure was my fault:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6fa6ac9d11

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/361907c4a2ce
https://hg.mozilla.org/mozilla-central/rev/c30753241b8d
https://hg.mozilla.org/mozilla-central/rev/ad6fa6ac9d11

Comment 26

[u459114]

Great tool!

Hi BenWa,
On B2G, render scroll graph on the device is ok, but if we can sent velocityData data to a viewer(a browser viewer on desktop, which attach with profiled B2G device), draw lines and do statistic analysis in that viewer, I think it's easier to address scrolling jank problem in bug 958592, how do you think?

Comment 27

[u459114]

(In reply to C.J. Ku[:CJKu] from comment #26)
> Great tool!
> 
> Hi BenWa,
> On B2G, render scroll graph on the device is ok, but if we can sent
> velocityData data to a viewer(a browser viewer on desktop, which attach with
> profiled B2G device), draw lines and do statistic analysis in that viewer, I
> think it's easier to address scrolling jank problem in bug 958592, how do
> you think?

typo: it should be bug 979751 - Making more smooth scrolling on scrollable layer

Comment 28

[u459114]

Attachment: jank.jpg

Another reason that I suggest to draw graph in another viewer is because even if the graph looks smooth in the graph sprite, it may not really smooth.

We may need a bigger diagram and do statistic analysis to figure out this small factors in scrolling.

Comment 29

[Benoit Girard (:BenWa)]

Yes. It's planned here:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/ContainerLayerComposite.cpp#150

We should be able to write a small program to convert adb logcat and graph it.

(In reply to C.J. Ku[:CJKu] from comment #28)

I agree. Great example!