1061393 - [LayerScope] Dump Display List on Layerscope viewer

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Boris Chiou [:boris]]

According to our design, we also need to dump display list. In other words, we should dump the display list, and draw & combine the related rectangles together on the viewer. We should also add different tabs for display list and layer tree.

Comment 1

[u459114]

So far, we have two ways to dump display list
1. Dump to console
   Dump display list to console is not that useful. Too many data on the screen at once.
2. Dump to html file
   Better then #1, but still pure text and hard to read.
   You need to turn on compile flag/ recompile/ turn on preference, which is complex for even gfx engineers.
3. Dump to profiler(Cleopatra)
   Absolutely better then #1 and #2. But it still non real-time solution, you need to capture log and then display the parsed result in Cleopatra.

I think dump display list in layerscope is more promising. You can see both layer buffer and display list information of each frame real-time.

Here is my plan:
1. Send display list dump log to layerscope in chrome process 
2. Send dump log with layer tree data to layerscope viewer
3. At viewer side, leverage code from layerTreeView.js in Cleopatra.

Comment 2

[u459114]

Attachment: WIP export display list log to layerscope

Comment 3

[u459114]

Attachment: WIP show display list log in layer proerty table

Comment 4

[u459114]

Attachment: WIP UI

Comment 5

[u459114]

Attachment: Viewer - reveal display list information

Comment 6

[u459114]

Attachment: WIP Send display list info to the layerscope viewer

Comment 7

[u459114]

Attachment: WIP Send display list info to the layerscope viewer

Comment 8

[u459114]

Attachment: Send display list info to layerscope viewer via websocket

Comment 9

[u459114]

Attachment: Send display list info to layerscope viewer

Comment 10

[u459114]

Attachment: Send display list info to layerscope viewer

Comment 11

[u459114]

Almost done. Correct typo and more comment before send to review.

Comment 12

[u459114]

Attachment: Send display list info to layerscope viewer

Comment 13

[u459114]

Comment on attachment 8638290 [details] [diff] [review]
Send display list info to layerscope viewer

Hi Dan,
This patch do the following thing
1. Sends display list log from content process to LayerScope in chrome process
2. LayerScope sends display list log, by protocol buffer package, to LayerScope viewer.(Another patch of LayerScopeViewer will represent this information in a more friendly way, compares to dump plain text in console.
3. Send some more attributes to LayerScope viewer for scene reconstruction
   i. scale(CSStoDevicePixel scale factor) 
      LayerScopeAutoFrame(int64_t aFrameStamp, float aScale)
   ii. Texture coordinates of each draw
      LayerScope::SetDrawRects(aQuads, aLayerRects, aTextureRects);
4. Correct the expression of display item log. Always take this form
   name(value)
   I notice the syntax of display item is not unified. Some of them put name inside parenthesis 
   (rgba 100, 200, 200, 125)
   While some of them put name out of parenthesis, for example
   layerBounds(%d,%d,%d,%d)
5. (Minor) remove a useless function in nsFrame.h

Hi BenWa,
Since this patch changes display list log, which may introduce a problem to cleopatra, wouldn't you mind to take a look on it?

Comment 14

[u480271]

Comment on attachment 8638290 [details] [diff] [review]
Send display list info to layerscope viewer

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

R+ with nits fixed.

::: gfx/layers/Layers.cpp
@@ +1598,5 @@
> +{
> +  log.SetLength(0);
> +
> +  if (gfxUtils::DumpDisplayList()) {
> +    // This function retruns a plain text string which consists of two things

retruns -> returns

@@ +1605,5 @@
> +    // We know the target layer of each display item by information in #1.
> +    // Here is an example of a Text display item line log in #1
> +    //   Text p=0xa9850c00 f=0x0xaa405b00(.....
> +    // f keeps the address of the target client layer of a display item.
> +    // For LayerSceope, this display-item-to-clien-layer mapping is not enough

LayerSceope -> LayerScope
clien -> client

@@ +1609,5 @@
> +    // For LayerSceope, this display-item-to-clien-layer mapping is not enough
> +    // since LayerScope, which lives in the chrome process, knows only
> +    // composite layers. As so, we need
> +    // display-item-to-client-layer-to-layer-composite
> +    // mapping. That's the reason we inter #2 into the log

inter -> enter

@@ +1610,5 @@
> +    // since LayerScope, which lives in the chrome process, knows only
> +    // composite layers. As so, we need
> +    // display-item-to-client-layer-to-layer-composite
> +    // mapping. That's the reason we inter #2 into the log
> +    log.AppendPrintf("0x%p\n%s",(void *)this, mDisplayListLog.get());

prefer keeping * with the type, so (void*). Also spaces after , so:
log.AppendPrintf("0x%p\n%s", (void*) this, mDisplayListLog.get());

Comment 15

[Benoit Girard (:BenWa)]

Comment on attachment 8638290 [details] [diff] [review]
Send display list info to layerscope viewer

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

::: layout/base/nsDisplayList.cpp
@@ -2092,4 @@
>  void
>  nsDisplaySolidColor::WriteDebugInfo(std::stringstream& aStream)
>  {
> -  aStream << " (rgba "

Yes, these changes will break the parsing for https://github.com/bgirard/cleopatra/blob/master/js/layerTreeView.js#L137.

Note that you can test a display list in cleopatra by going to:
http://people.mozilla.org/~bgirard/cleopatra/ and pasting a displaylist there instead of a profile. You have to past the line starting at 'Painting --- before optimization':
https://github.com/bgirard/cleopatra/blob/master/js/appUI.js#L48

I'm not sure if it's really worthwhile to make these changes considering we need to fix any tools that parses this data but I'd be happy to accept any PR.

Comment 16

[u459114]

Attachment: Send display list info to layerscope viewer

Comment 17

[u459114]

Attachment: Send display list info to layerscope viewer

fix thread assertion.

Comment 18

[Ting-Yu Lin [:TYLin] (PST, UTC-7)]

Comment on attachment 8641695 [details] [diff] [review]
Send display list info to layerscope viewer

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

Two nits.

::: gfx/layers/LayerScope.cpp
@@ +691,5 @@
> +            pRect->set_h(mTextureRects[i].height);
> +        }
> +
> +        for (std::list<GLuint>::iterator it = mTexIDs.begin(); it != mTexIDs.end(); it++) {
> +            dp->add_texids(*it);

A perfect place to use range-based for loop. For example,

for (GLuint texId: mTextIds) {
  dp->add_texids(texId);
}

::: layout/base/nsLayoutUtils.cpp
@@ +3364,5 @@
>      gfxUtils::sDumpPaintFile = savedDumpFile;
>      gPaintCount++;
>  #endif
> +
> +    UniquePtr<std::stringstream> lsStream = MakeUnique<std::stringstream>();

Can |std::stringsstream lsStream| be used here and pass &lsStream to PrintDisplayList?

Comment 19

[u459114]

Try result
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cfd79ae1bb3

Comment 20

[u459114]

Attachment: Send display list info to layerscope viewer

Rebase and fix what TY mentioned.

Comment 21

[u459114]

Attachment: Send display list info to layerscope viewer

Fix threading issue

Comment 22

[u459114]

Attachment: Send display list info to layerscope viewer

Remove debug log

Comment 23

[u459114]

Update try result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f60325d24d88

Comment 24

[u459114]

Please help to check-in attachment 8642310 [details] [diff] [review]

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/006e8f78affe

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/006e8f78affe

Comment 27

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.