960677 - Improve Scrollgraph to Track Checkerboarding Metrics

Status: RESOLVED WONTFIX

(Core :: Graphics, defect, P2)

Description

[Mason Chang [Inactive] [:mchang]]

Improve scrollgraph to report and track checkerboarding metrics

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: RenderTrace demonstrating checkerboarding

Pretty awesome tool by kats. Copy / paste the dump into http://people.mozilla.org/~kgupta/rendertrace.html. When the red box isn't inside the green box, we have checkerboarding.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Rendertrace is apparently old and inaccurate. Try fixing things in ComputeRenderIntegrity and calculating checkerboarding.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: checkerboard.patch

A stab at trying to compute Checkerboarding based off Rendertrace / APZ Controller's data. Verified that it works for SMS app scrolling up / down. I was taking a look at ComputeRenderIntegrity, and it looks like it's a totally different approach. I was thinking of replacing ComputeRenderIntegrity with the ComputeCheckerboarding from this patch if this is the right approach.

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 8378738 [details] [diff] [review]
checkerboard.patch

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

::: gfx/layers/composite/LayerManagerComposite.h
@@ +200,5 @@
>    /**
> +   * Computers a percentage based on how much of the displayport
> +   * is rendered in the viewport. 1.0 means no checkerboarding.
> +   */
> +  float ComputeCheckerboarding(Layer* aLayer, gfx::Point layerTransform);

How does this differ from http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/LayerManagerComposite.h#198? We should really avoid duplicating this since this function is hard to maintain.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: checkerboard.patch

Let's try again, integrated into ComputeRenderIntegrity. Also put drawing of the actual scroll graph behind a pref.

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8386494 [details] [diff] [review]
checkerboard.patch

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

Please include more context with your patches to make the review easier.

Sorry I should of mentionned that calculated the checkerboarding metrics is a lot harder then it seems :).

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +645,5 @@
> +/***
> + * Subtracts the checkerboarded region from the aScreenRegion
> + */
> +void
> +LayerManagerComposite::ComputeCheckerboarding(ContainerLayer* aLayer,

Sorry but this function doesn't really calculate the checkerboard metric for the layer tree. Does it seem to work with local testing? What you need to do is recurse over all of the layer tree and compute the metric for each layer.

There's also complications with mask layers for example.

I'm actually not sure if the compositor has all the information to correctly calculate this. For example a layer may not consume all of the screen because it's not big enough vs. a layer that is checkerboarding.

@@ +648,5 @@
> +void
> +LayerManagerComposite::ComputeCheckerboarding(ContainerLayer* aLayer,
> +                                              nsIntRegion& aScreenRegion,
> +                                              nsIntRegion& aLowPrecisionScreenRegion) {
> +  const FrameMetrics& metrics = aLayer->AsContainerLayer()->GetFrameMetrics();

AsContainerLayer is not need since you already have a container layer.

@@ +650,5 @@
> +                                              nsIntRegion& aScreenRegion,
> +                                              nsIntRegion& aLowPrecisionScreenRegion) {
> +  const FrameMetrics& metrics = aLayer->AsContainerLayer()->GetFrameMetrics();
> +  gfx::Point layerTransform = GetScrollTransform(aLayer);
> +  if (!IsLayerScrolling(metrics, layerTransform)) {

With progressive drawing a scrollable layer can be checkerboarded to.

@@ +660,5 @@
> +  CSSPoint  frameMetricsScroll = metrics.mScrollOffset;
> +
> +  // Based on APZ where frame metrics and the transform sometime diverge.
> +  CSSPoint  scrollOffset(frameMetricsScroll.x - layerTransform.x,
> +      frameMetricsScroll.y - layerTransform.y);

This should align to ( unless it's to long then it should be 2 space indented.

::: gfx/layers/composite/LayerManagerComposite.h
@@ +237,5 @@
>  
>    /**
> +   * Removes the region of the viewport that is not covered by the display port
> +   */
> +  static void ComputeCheckerboarding(ContainerLayer* aLayer,

ComputeCheckerboarding implies that it would return a float similar to ComputeRenderIntegrity() but it does not. The comment doesn't describe something that a function with that name should do. These should be const parameters as well.

If you only need a float then maybe you should return that. The problem is a rectangular region can only approximate the checkerboard region for a complex layer tree so a float parameter is more appropriate unless we need a region.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: checkerboard.patch

Sorry about the lack of context. Yeah, bad function naming, sorry about that. I essentially hooked into how ComputeRenderIntegrityInternal worked. I subtract the checkerboarded region from aScreenRect, which goes through every layer and removes invalid regions from aScreenRect. For a container layer, we remove the checker boarded region based on the current transform.

> I'm actually not sure if the compositor has all the information to correctly calculate this. For example a layer may not consume all of the screen because it's not big enough vs. a layer that is checkerboarding.

Yes, this is a problem. If we only check ComputeRenderIntegrity at the parent process, we won't get the proper APZ transform. 

> There's also complications with mask layers for example.

Is our definition of a mask layer the same as Flash's? http://help.adobe.com/en_US/flash/cs/using/WS9388626D-B940-43f3-87BB-7C3159F5EDE4.html

Comment 8

[Mason Chang [Inactive] [:mchang]]

> Sorry but this function doesn't really calculate the checkerboard metric for the layer tree. Does it 
> seem to work with local testing? What you need to do is recurse over all of the layer tree and compute 
> the metric for each layer.

Yes this worked locally. I can't checkerboard my desktop or with sync scrolling, so i was unable to test with those configurations.

Comment 9

[Benoit Girard (:BenWa)]

(In reply to Mason Chang [:mchang] from comment #7)
> Is our definition of a mask layer the same as Flash's?
> http://help.adobe.com/en_US/flash/cs/using/WS9388626D-B940-43f3-87BB-
> 7C3159F5EDE4.html

From the 2 paragraphs yes. The rest are Flash specific instructions.

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 8387989 [details] [diff] [review]
checkerboard.patch

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

Computing checkerboard properly is difficult to get right. Getting it wrong however can be a disaster because everything that relies on a badly calculated frame metrics will misbehave. I don't want people to chase down regressions that are caused by incorrectly calculated metrics.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +647,5 @@
> +/***
> + * Subtracts the checkerboarded region from the aScreenRegion
> + */
> +void
> +LayerManagerComposite::subtractCheckerboardRegion(ContainerLayer* aLayer,

This function doesn't compute the checkerboarded region for a ContainerLayer. It doesn't even traverse the children or deal with the transformations. Even if you did compute the checkerboarded region correctly it will still be incorrect because it can't be presented by an nsIntRegion. You would need to document the rounding behavior that you're doing.

Comment 11

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8387989 [details] [diff] [review]
checkerboard.patch

Hi Kats and Botond, just checking to see if this is the right way to detect checkerboarding from the compositor. If so, I'll clean it up and remove it from ScrollGraph. Thanks!

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8387989 [details] [diff] [review]
checkerboard.patch

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

This change is pretty big and it's hard for me to say if this is right or not. You seem to be using the ComputeRenderIntegrity which is not what I had in mind, although that might be better as a long-term solution because you can account for progressive tile updates. The measurement I had in mind was to just report the part of the composition bounds that is outside the displayport in AsyncPanZoomController::SampleContentTransformForFrame, but that only works if the displayport is fully painted (which will not be the case with progressive tile updates).

Comment 13

[Botond Ballo [:botond]]

Comment on attachment 8387989 [details] [diff] [review]
checkerboard.patch

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

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +671,5 @@
> +  nsIntRect viewportRect(viewport.x, viewport.y, viewport.width, viewport.height);
> +  nsIntRect paintedIntRect(paintedRect.x, paintedRect.y, paintedRect.width, paintedRect.height);
> +
> +  nsIntRegion checkerboardRegion;
> +  checkerboardRegion = checkerboardRegion.Sub(viewportRect, paintedIntRect);

I believe the code up to here calculates the amount of checkerboarding for a single layer correctly.

I'm not familiar with ComputeRenderIntegrity (which you are modifying to call this function), so I'm not really sure whether the rest of the function is correct. One thing that looks suspicious is that you are subtracting checkerboardRegion, which is in CSS pixels, from aScreenRegion, which I doubt is in CSS pixels.

The final value that is reported as the amount of checkerboarding for a frame should be obtained by walking the layer tree and combining the painted and checkerboarded regions in appropriate ways (for example, a portion of a parent layer's checkerboarded region which is obscured by a child layer's visible region is not checkerboarded as far as the user is concerned), taking into account the transforms on the layers. I don't understand the code in ComputeRenderIntegrity well enough to say if this is what it does - better to ask Chris Lord, who wrote this function.

Comment 14

[Chris Lord [:cwiiis]]

Comment on attachment 8387989 [details] [diff] [review]
checkerboard.patch

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

I don't think the newly added code is correct and it looks like it's duplicating what ComputeRenderIntegrity already does, but without accounting for transforms or progressive rendering. I would suggest trying to understand those functions first and re-purposing those to work outside of just Android.

::: gfx/layers/composite/LayerManagerComposite.cpp
@@ +647,5 @@
> +/***
> + * Subtracts the checkerboarded region from the aScreenRegion
> + */
> +void
> +LayerManagerComposite::subtractCheckerboardRegion(ContainerLayer* aLayer,

I'm not sure what this function is trying to accomplish, but it looks like it would break if there were any transforms or the like.

If you just want a number for checkerboarding, that's what ComputeRenderIntegrity already does - it likely needs some fixing up, as it may not be entirely correct with respect to coordinate spaces, and it makes some Android-specific assumptions, but it accounts for transforms and so on. ComputeRenderIntegrityInternal is the part of it that makes it correct with progressive rendering enabled.

I can't really offer more feedback than this without some more specific questions or information.

@@ -657,4 @@
>  {
>    // We only ever have incomplete rendering when progressive tiles are enabled.
>    Layer* root = GetRoot();
> -  if (!gfxPrefs::UseProgressiveTilePainting() || !root) {

Yeah, this first bit shouldn't have caused a return, it should just have stopped ComputeRenderIntegrityInternal being called. Seeing as we only use this function on Android atm and we have progressive tile painting enabled there, not too big a deal... Good catch though.

Comment 15

[Mason Chang [Inactive] [:mchang]]

(In reply to Chris Lord [:cwiiis] from comment #14)
> Comment on attachment 8387989 [details] [diff] [review]
> checkerboard.patch
> 
> Review of attachment 8387989 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> I'm not sure what this function is trying to accomplish, but it looks like
> it would break if there were any transforms or the like.
> 
> If you just want a number for checkerboarding, that's what
> ComputeRenderIntegrity already does - it likely needs some fixing up, as it
> may not be entirely correct with respect to coordinate spaces, and it makes
> some Android-specific assumptions, but it accounts for transforms and so on.
> ComputeRenderIntegrityInternal is the part of it that makes it correct with
> progressive rendering enabled.
> 
> I can't really offer more feedback than this without some more specific
> questions or information.

Thanks for the feedback! Sorry in taking so long to get back to it. Theoverall goal is to get a number for checkerboarding, e.g. 70% of the frame was painted. I'm still fairly new to the code, so reading into it, my general understanding is that we start with the screenRect, and subtract out non-painted regions from the screenRect. Outside of the ifdef MOZ_ANDROID_OMTC, what else is android specific? Could you please elaborate a bit more on what are the android specific assumptions?

Also, is the progressive rendering corrections the low / high precision rendering? My understanding of progressive rendering is that we render things low quality if we're low on CPU, then re-render it in higher quality when we can catch up. Thanks! I think overall, I'm going to rework this to be extracted outside of Scroll Graph.

Comment 16

[Chris Lord [:cwiiis]]

(In reply to Mason Chang [:mchang] from comment #15)
> Thanks for the feedback! Sorry in taking so long to get back to it.
> Theoverall goal is to get a number for checkerboarding, e.g. 70% of the
> frame was painted.

Right, this is exactly what ComputeRenderIntegrity currently does (returning a value between 0 (no visible rendering) and 1 (whole screen rendered)).

> I'm still fairly new to the code, so reading into it, my
> general understanding is that we start with the screenRect, and subtract out
> non-painted regions from the screenRect. Outside of the ifdef
> MOZ_ANDROID_OMTC, what else is android specific? Could you please elaborate
> a bit more on what are the android specific assumptions?

One assumption it makes is that we want the checkerboarding value for the primary scrollable layer and nothing else. In b2g, the primary scrollable layer is often not the main scrollable layer of an application, and you can have scrollable sub-frames with apzc's too. I think ideally, we'd pass in a layer for which we want checkerboarding numbers for, rather than make this assumption.

Although the code looks like it tries to reconcile between transform differences of the root layer and the primary scrollable layer, I'm not at all sure it's doing so correctly. It may not work correctly, or it may only work coincidentally at the moment. Perhaps it only works with the specific configuration we have on Android - this needs to be audited.

> Also, is the progressive rendering corrections the low / high precision
> rendering? My understanding of progressive rendering is that we render
> things low quality if we're low on CPU, then re-render it in higher quality
> when we can catch up. Thanks! I think overall, I'm going to rework this to
> be extracted outside of Scroll Graph.

Progressive rendering, with respect to tiles, is the splitting of rendering into batches of tiles (allowing smaller batches to be rendered and made visible more quickly, and also allowing rendering to be cancelled between batches).

Low-precision rendering, with respect to tiles, is the splitting of the rendering between a high-precision area (defined by the critical display port), and a larger, low-precision area (defined by the display port). The low-precision area is not always drawn, depending on decisions made based on how close we are to keeping up with rendering in high-precision. Low-precision rendering is always drawn in batches of a single tile too, as coherency is not an issue there (it's already compromised by the render resolution).

ComputeRenderIntegrity first gets a rough 'checkerboarding' number by seeing how much of the display-port intersects with the screen, given the async transform, then refines that number by checking how much of that display-port has actually been rendered, given progressive rendering. This later refinement is done by comparing a layer's valid region (the rendered region) to its visible region (the region that should have been rendered if a layer has completed its render fully), again paying attention to transforms and how much of said areas actually intersect with the screen.

Comment 17

[Mason Chang [Inactive] [:mchang]]

Resolving as wontfix, see https://bugzilla.mozilla.org/show_bug.cgi?id=958592#c1. Although we should still have a metric to log checkerboarding, which is bug 969466.