Bug 36782 – Add Layer Tree output capability to DRT

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Marrin]

Today, it's difficult to do meaningful tests of composited content. There is no way to dump the render tree used for composited content (the GraphicsLayers) like there is for non-composited content. Adding this would give us much better test coverage.

Steps to implement:

1) Implement cross-platform dump format output for the GraphicsLayer tree.
2) Add a layerTreeAsText call to DRT to get this formatted output (with all the plumbing through the system)
3) Add initial tests to use the functionality

This gets us most of the way there, but leaves off one important testing area. Under the GraphicsLayers are platform specific objects which are used to do the actual rendering. On Mac this is based on CALayer, and on Windows it's based on CACFLayer. There are often multiple platform specific layers for each GraphicsLayer, and these layers have their own hierarchy. The GraphicsLayer and platform specific layer hierarchies must stay in sync, but there is currently no consistency checking going on. We should add such checking to the platform specific subclasses of GraphicsLayer (GraphicsLayerCA and GraphicsLayerCACF) and then assert when the checks fail. That way DRT will assert when testing the debug builds and the consistency checking won't get in the way of release performance.

[Chris Marrin]

Created attachment 51997 [details]
Patch with test case

Please do not review this patch yet. It is the Mac side. I need to add the Windows side to it. This is just a placeholder so I can put the patch on Windows and finish the work.

[Chris Marrin]

Actually, please do look the patch over. The general structure and test case are final. I just need to add the WebKit and DRT parts of the plumbing for Windows. Please comment on what's there so far.

[Simon Fraser (smfr)]

Comment on attachment 51997 [details]
Patch with test case

> Index: WebCore/page/Frame.cpp
> ===================================================================

> +String Frame::layerTreeAsText()
> +{
> +    if (!contentRenderer())
> +        return String();
> +
> +    GraphicsLayer* rootLayer = contentRenderer()->compositor()->rootPlatformLayer();
> +    if (!rootLayer)
> +        return String();
> +        
> +    return rootLayer->layerTreeAsText();
> +}

This needs to have #if ENABLE() guards inside it.


> Index: WebCore/page/Frame.h
> ===================================================================

> +        String layerTreeAsText();

This method should be |const|.

> Index: WebCore/platform/graphics/GraphicsLayer.cpp
> ===================================================================

> -void GraphicsLayer::dumpLayer(TextStream& ts, int indent) const
> +void GraphicsLayer::dumpLayer(TextStream& ts, int indent, bool debug) const
>  {
>      writeIndent(ts, indent);
> -    ts << "(" << "GraphicsLayer" << " " << static_cast<void*>(const_cast<GraphicsLayer*>(this));
> -    ts << " \"" << m_name << "\"\n";
> -    dumpProperties(ts, indent);
> +    if (debug)
> +        ts << "(" << "GraphicsLayer" << " " << static_cast<void*>(const_cast<GraphicsLayer*>(this));
> +    else
> +        ts << "(" << "GraphicsLayer";

I'd prefer this as:

  ts << "(" << "GraphicsLayer";
  if (debug)
    ts << " " << static_cast<void*>(const_cast<GraphicsLayer*>(this));

> -void GraphicsLayer::dumpProperties(TextStream& ts, int indent) const
> +void GraphicsLayer::dumpProperties(TextStream& ts, int indent, bool debug) const

> +    if (debug) {
> +        if (m_client)
> +            ts << static_cast<void*>(m_client);
> +        else
> +            ts << "none";

Better as "(no client)" I think.

> +        m_replicaLayer->dumpLayer(ts, indent+2, debug);

Spaces around the "+" please.

> -            m_children[i]->dumpLayer(ts, indent+2);
> +            m_children[i]->dumpLayer(ts, indent+2, debug);

Ditto.

> Index: WebCore/platform/graphics/GraphicsLayer.h
> ===================================================================
> +    void dumpLayer(TextStream&, int indent = 0, bool debug = false) const;

I can imagine that we want finer granularity than "debug"/"not debug" in the output in future. I'd like to see this modeled after RenderAsTextBehavior in RenderTreeAsText.h. That also has the advantage of making the callsite easier to read (no mysterious "true" or "false").

Another nice addition would be a debug-only method that can be called from gdb, just like showLayerTree() in RenderObject.h.


> Index: LayoutTests/compositing/geometry/preserve-3d-switching.html
> ===================================================================

I think this test should be committed separately. It would also be a little neater if the test dumped the layer output into a <pre> element.


> Index: LayoutTests/platform/mac/compositing/geometry/preserve-3d-switching-expected.txt
> ===================================================================
> +  (position 0.00 0.00)
> +  (anchor 0.50 0.50)
> +  (bounds 800.00 600.00)
> +  (opacity 1.00)
> +  (usingTiledLayer 0)
> +  (m_preserves3D 0)

Odd that this has m_

> +  (drawsContent 0)
> +  (m_backfaceVisibility visible)

Ditto. Maybe just avoid dumping properties that have default values.

> +    (GraphicsLayer "CALayer(0x105216350) GraphicsLayer(0x105215fe0) Document Node"

This looks like debug output, which should never end up in a test result.

> +          (childrenTransform [1.00 0.00 0.00 0.00] [0.00 1.00 0.00 0.00] [0.00 0.00 1.00 -0.00] [0.00 0.00 0.00 1.00])

What is the story with outputting floating point. Can we make sure we don't get precision issues between platforms?

[Chris Marrin]

Created attachment 52212 [details]
Replacement patch

This makes changes from Simon's review and adds Windows side of patch.

[WebKit Review Bot]

Attachment 52212 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/graphics/GraphicsLayer.cpp:395:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
WebCore/platform/graphics/GraphicsLayer.cpp:489:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Simon Fraser (smfr)]

Comment on attachment 52212 [details]
Replacement patch

> Index: WebCore/page/Frame.cpp
> ===================================================================

> +#if USE(ACCELERATED_COMPOSITING)
> +String Frame::layerTreeAsText() const

This should be inside the method, so that the method is still present in non-accel-comp builds (which avoids breaking the exports).

The method can return an empty string if ACCELERATED_COMPOSITING is off.

> Index: WebCore/page/Frame.h
> ===================================================================

> +#if USE(ACCELERATED_COMPOSITING)
> +        String layerTreeAsText() const;
> +#endif

This would not be wrapped here.

> Index: WebCore/platform/graphics/GraphicsLayer.cpp
> ===================================================================

> +#ifndef NDEBUG
> +void showGraphicsLayerTree(const WebCore::GraphicsLayer* layer)
> +{
> +    if (!layer)
> +        return;
> +
> +    WebCore::String output = layer->layerTreeAsText(LayerTreeAsTextDebug);

Doesn't LayerTreeAsTextDebug needs to be WebCore::LayerTreeAsTextDebug ?

> Index: WebKit/win/WebFrame.cpp
> ===================================================================

> +#if USE(ACCELERATED_COMPOSITING)
> +    String text = frame->layerTreeAsText();
> +#else
> +    String text;
> +#endif

You wouldn't need the #ifdef here.

r=me

[Chris Marrin]

Landed in http://trac.webkit.org/changeset/56934

[WebKit Review Bot]

http://trac.webkit.org/changeset/56934 appears to have broken Chromium Win Release, Chromium Mac Release, and Chromium Linux Release

[Eric Seidel (no email)]

Looks like those were just the first builders to break.  Looks like basically all builders broke from this change.

Sad that this got an r+ when the EWS had warned that this broke builds. :(