Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Description • 11 years ago

To get more objective data on measuring checkerboarding, we can add some logging to APZC code to report the number of pixel-milliseconds of checkerboarding (that is, number of pixels that were checkerboarded multiplied by the number of milliseconds that they were checkerboarded). The stuff we dump for rendertrace logging can be used to calculate this.

We can log it to logcat, or save the value on the tree manager and have it displayed on-screen or something.

	Nick Lebedev [:nl]
	Comment 1 • 10 years ago

Hi, don't you mind if I take this bug?

	Mason Chang [Inactive] [:mchang]
	Comment 2 • 10 years ago

Sure! Please do!

	Nick Lebedev [:nl]
	Comment 3 • 10 years ago

Hi,

could you please describe briefly where we would like to put the logging of checkerboarding? I'm thinking that we would like to put it into method http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/AsyncPanZoomController.h#248 so every time it is invoked we measure the checkerboarded space and log it. But I might be wrong.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Reporter
	Comment 4 • 10 years ago

Hey, sorry for the delay. Botond's away until next week so I'll jump in. I don't think we want to put it inside IsCurrentlyCheckerboarding. Instead, what I would suggest is to add a new method that takes the current sample time, similar to AsyncPanZoomController::UpdateAnimation, and returns the checkerboard area multiplied by the time that frame was visible (the current sample time minus mLastSampleTime).

Then, you should call that function just before the call to AdvanceAnimations at [1], and accumulate the total into a member variable in AsyncCompositionManager, which can be reported based on a pref or something to logcat.

One of the reasons I'm suggesting using the timestamp like this is because there could be different layers in the layer tree which share the same APZC instance, and we want to make sure we don't double-count those checkerboarding amounts. The call to AdvanceAnimations already updates the mLastSampleTime so if you put the call just before that it won't be a problem.

Does that sound reasonable?

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp?rev=7fce65c28e7c#530

	Nick Lebedev [:nl]
	Comment 5 • 10 years ago

Yes, sound reasonable.

I've prepared a WIP patch, could you take a look at the patch and answer a few questions:

1. What is the proper way to compute the size of the checkerboarded area? I've currently put a stub for intersecting the regions which is wrong. I think we need to subtract painted area from visible one but didn't find a good way to do it.

2. Do we want to log just the amount of area or we want to log it in pairs (layer : amount of area) or somehow else?

Btw, i need some more device testing to see whether console/logcat will show logs at all :)

	Botond Ballo [:botond]
	Comment 6 • 10 years ago

(In reply to Nick Lebedev [:nl] from comment #5)
> 1. What is the proper way to compute the size of the checkerboarded area?
> I've currently put a stub for intersecting the regions which is wrong. I
> think we need to subtract painted area from visible one but didn't find a
> good way to do it.

You can do it with nsRegion::Sub():

nsRegion result;
result.Sub(visible, painted);  // read as 'result = Sub(visible, painted)'

You can then call Area() on the resulting nsRegion to get the number of checkerboarded pixels.

> 2. Do we want to log just the amount of area or we want to log it in pairs
> (layer : amount of area) or somehow else?

I think pairs of (layer : area) would make sense, as that contains the most information. 

I think it would also make sense to print the timestamp of the frame, so that we can identify checkerboarding messages for different layers in the same frame.

Finally, I think it would make sense to calculate the checkerboarded area in Screen pixels rather than CSS pixels, so that areas for different layers (and, across timestamps, areas for the same layer at different levels of zoom) can be compared.

A client who was then interested in the total number of checkerboarded pixels for a given frame in relation to the screen size, could add up the areas for all the layers in that frame.

	Botond Ballo [:botond]
	Comment 7 • 10 years ago

Hey Nick - are you still working on this? 

Let me know if you'd like any further clarification on my suggestions in comment 6 - I know some of this stuff, especially getting things in the right coordinate system, can be tricky.

	Nick Lebedev [:nl]
	Comment 8 • 10 years ago

Yes, I do, give me a few more days - I'll come up with the updated patch.

	Nick Lebedev [:nl]
	Comment 9 • 10 years ago

Updated the patched and yes - I think I've encountered problems with getting areas in proper coordinate system. Could you advice how to transform all the values into screen pixels. I may guess that I need to divide all the css values by framemetrics's zoom value here but I'm not sure:

  CSSPoint currentScrollOffset = mFrameMetrics.GetScrollOffset() + mTestAsyncScrollOffset;
  CSSRect painted = mLastContentPaintMetrics.mDisplayPort + mLastContentPaintMetrics.GetScrollOffset();
  painted.Inflate(CSSMargin::FromAppUnits(nsMargin(1, 1, 1, 1)));   // fuzz for rounding error
  CSSRect visible = CSSRect(currentScrollOffset, mFrameMetrics.CalculateCompositedSizeInCssPixels());

	Botond Ballo [:botond]
	Comment 10 • 10 years ago

Comment on attachment 8511977 [details] [diff] [review]
Add logging of the data about checkerboarded area. v1.

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

Thanks, Nick, this looks pretty good! I have a few comments. r- for now as I'd like to see a new patch with the comments addressed.

Regarding the conversion to Screen pixels, there are two steps:

  1. First, CSS -> "local Screen" pixels.
     For this, multiply the quantity in CSS pixels by mFrameMetrics.GetZoom().

  2. "local Screen" -> "global Screen" pixels.
     For this:

       ScreenRect localScreenRect = ...; 
       Matrix4x4 apzcToScreen;  
       if (APZCTreeManager* treeManager = >GetApzcTreeManager()) {
         apzcToScreen = treeManager->GetScreenToApzcTransform(this).Inverse();
       }
       ScreenRect globalScreenRect = TransformTo<ScreenPixel>(apzcToScreen, localScreenRect);

You can then round the global screen rect with the non-member function RoundedToInt(), and create an nsIntRect from its fields (feel free to add a ToUntyped() function to ScreenPixel, similar to the one for RenderTargetPixel [1]).

(Note that in bug 1055741, "local Screen" pixels are being renamed to "ParentLayer" pixels, so if that bug lands before this one, this code might have to change slightly. I'm happy to help with that change.)

Finally, when posting the updated patch, please flag :ehsan for review as well, just for the nsIConsoleService bits.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/Units.h?force=1#262

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +2470,5 @@
> +  CSSRect painted = mLastContentPaintMetrics.mDisplayPort + mLastContentPaintMetrics.GetScrollOffset();
> +  painted.Inflate(CSSMargin::FromAppUnits(nsMargin(1, 1, 1, 1)));   // fuzz for rounding error
> +  CSSRect visible = CSSRect(currentScrollOffset, mFrameMetrics.CalculateCompositedSizeInCssPixels());
> +
> +  nsRect paintedRect(painted.X(), painted.Y(), painted.Width(), painted.Height());

Please use nsIntRect and nsIntRegion instead of nsRect and nsRegion. nsRect and nsRegion are used to represent quantities in "app units", which are 1/60 of a CSS pixel. nsIntRect and nsIntRegion are currently used for everything else, such as CSS units, Screen units etc.

(Apologies if my code snippet in comment 6 was misleading in this regard.)

@@ +2477,5 @@
> +  nsRegion paintedRegion(paintedRect);
> +  nsRegion visibleRegion(visibleRect);
> +
> +  nsRegion checkerboardedRegion;
> +  checkerboardedRegion.Sub(paintedRegion, visibleRegion);

You have this subtraction backwards. You want 'Sub(visibleRegion, paintedRegion)', as the checkerboarded region is the region of the scrollframe that's on-screen but not painted.

Also, there is an overload of this function which takes rects directly, so you don't need to convert the inputs to regions first.

::: gfx/layers/apz/src/AsyncPanZoomController.h
@@ +167,5 @@
> +   * Returns the size of checkerboarded area in pixels multiplied by
> +   * the number of milliseconds the area were checkerboarded.
> +   * The timeframe the data is computed for is [mLastSampleTime, aTime];
> +   */
> +  int32_t GetCheckerboardedData(const TimeStamp& aTime);

I would call this function 'GetCheckerboardingAmount'.

::: gfx/layers/composite/AsyncCompositionManager.cpp
@@ +521,4 @@
>  }
>  
>  static bool
> +SampleAPZAnimations(const LayerMetricsWrapper& aLayer, TimeStamp aSampleTime, std::stringstream &checkerboardedDataLog)

Nit: the '&' goes beside the type.

@@ +534,5 @@
> +      int32_t checkerboardedData = apzc->GetCheckerboardedData(aSampleTime);
> +
> +      if (checkerboardedData) {
> +        checkerboardedDataLog << "Layer name: "
> +                              << aLayer.Name()

Layer::Name() just returns the name of the type of layer, e.g. "ThebesLayerComposite", "ContainerLayerComposite", etc. There are likely to be multiple layers of the same type, so this is not going to be unique for each layer.

Instead, I would recommend printing the APZC's "ScrollableLayerGuid", which is a tuple of three IDs (layers id, pres shell id, and scroll id) that uniquely identifies an APZC. You can get it via 'apzc->GetGuid()'. For printing it, there is an AppendToString overload in LayersLogging.h that you might find helpful, or just print the fields individually.

@@ +1029,5 @@
>    // in Gecko and partially in Java.
> +  std::stringstream checkerboardedDataLog;
> +  wantNextFrame |= SampleAPZAnimations(LayerMetricsWrapper(root), aCurrentFrame, checkerboardedDataLog);
> +
> +  if (checkerboardedDataLog.str().size()) {

I think it would be clearer to use !empty() than size().

	Botond Ballo [:botond]
	Comment 11 • 10 years ago

Hey Nick, just wondering if you plan to update this patch? It's pretty close to being done :)

	Botond Ballo [:botond]
	Comment 12 • 10 years ago

Since I haven't heard from Nick for a while, I'm going to clear the assignee of the bug.

Nick, if you'd like to continue working on it, feel free to assign it to yourself again.

Otherwise, the bug is available for someone else to take. The remaining work is to take the attached patch and address the review comments in comment 10.

	Botond Ballo [:botond]
	Comment 13 • 9 years ago

Ehsan was telling me today that he would find it really useful to have a optional debug display on the screen (similar to the frame rate counter) which shows how much a tab has checkerboarded over its lifetime - basically a cumulative version of the pixel-milliseconds statistic computed in this patch, aggregated per tab over all scroll frames in the tab.

The WIP patch in this bug could be repurposed to do this along the following lines:

  - Take the AsyncPanZoomController::GetCheckerboardedData() function from the patch
    as-is (with review comments addressed).

  - In AsyncCompositionManager::SampleAPZAnimations(), read the data off the APZC
    as the patch does now, and accumulate it into a map, stored on the 
    LayerManagerComposite, mapping layers ids to a cumulative checkerboarding value 
    for all APZCs belonging to that layers id.

  - In LayerManagerComposite::RenderDebugOverlay(), find the layers id corresponding
    to the current tab (this can be done by searching the layer tree for a RefLayer;
    there should only be one on desktop platforms, and the layers id of the current
    tab will be its GetReferentId()), look it up in the map to get the checkerboarding
    value of the current tab, and render it to screen in a manner similar to how the
    frames-per-second value is rendered in that function.

Ehsan, it you're motivated to implement this and need help with any of the details, feel free to ask me!

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 14 • 9 years ago

I'd like this coordinated with the minimap work, where we're thinking of doing something similar per tile.

	Benoit Girard (:BenWa)
	Comment 15 • 9 years ago

I discussed this in person with Botond. We still want to follow with this approach in this patch independently with highlighting checkerboarding on the minimap since they both would require different data to be reported and for the minimap the data has to be stored across frames.

	Botond Ballo [:botond]
	Comment 16 • 9 years ago

I feel like this has been effectively superseded by bug 1221694 and bug 1226826, so I'm going to close this.