Bug 54710 – [chromium] Create a LayerChromium layerTreeAsText. Add HUD to LayerRendererChromium that draws compositor FPS and, optionally, the layer tree.

• Component: New Bugs
• Status: RESOLVED FIXED

[Nat Duca]

[chromium] Create a LayerChromium layerTreeAsText. Add HUD to LayerRendererChromium that draws compositor FPS and, optionally, the layer tree.

[Nat Duca]

Created attachment 82907 [details]
Patch

[WebKit Review Bot]

Attachment 82907 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7918939

[Vangelis Kokkevis]

Comment on attachment 82907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82907&action=review

I think this would make a great debugging tool.  I would like to see it though leverage more of the code that's already in place for ContentLayers for rendering the debug info.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:565
> +    ts << "drawsContent: " << drawsContent() << "\n";

It would be very helpful to also include:
1. layer bounds
2. Render surface info

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:791
> +    if (!m_hudTexture)

I'm not particularly fond of the amount of code that's duplicated between this method and ContentLayerChromium.  Is it possible to create new layer type (maybe derived from ContentLayerChromium) that's used for displaying the debug information?  It would take a bit of refactoring of the ContentLayerChromium to account for the lack of an m_owner but I think it would be worthwhile.  Also mind you that this will need to be timed appropriately with: https://bugs.webkit.org/show_bug.cgi?id=54315

> Source/WebKit/chromium/src/WebViewImpl.cpp:2324
> +            m_layerRenderer->setHudEnabled(m_page->settings()->showDebugBorders());

I would prefer activating the HUD with a different flag.  The debug borders can be distracting but I can see developer wanting to see just the FPS meter.

[Nat Duca]

(In reply to comment #3)
> I think this would make a great debugging tool.  I would like to see it though leverage more of the code that's already in place for ContentLayers for rendering the debug info.
I'm 100% with you, I hate this duplication. The approach of using ContentLayer is 100% my end goal. I actually have another CL in flight that removes m_owner from ContentLayer to enable this ... I'll upload that shortly so you can see. I created this HUD to help me debug a bug in that change..

So, my proposal is, land this. Once this lands, I'll land my m_owner code and move the drawing into a CCPainterInerface and the uglies will "go away."

> It would be very helpful to also include:
> 1. layer bounds
> 2. Render surface info
Totally.

> Also mind you that this will need to be timed appropriately with: https://bugs.webkit.org/show_bug.cgi?id=54315
Talked with JamesR about this last night. I think we were thinking to land land his update change, then my m_owner change. Mod a few minor edits, this HUD stuff can go either before or after the update change.

> I would prefer activating the HUD with a different flag.
It feels clunky that we have to go through the page's Settings object to get this setting in from Chromium. Is there a better way? Page Settings have a showRepaintCounter, I could use that and plumb it up to Chromium. Or should I make something new?

[Vangelis Kokkevis]

(In reply to comment #4)
> (In reply to comment #3)
> > I think this would make a great debugging tool.  I would like to see it though leverage more of the code that's already in place for ContentLayers for rendering the debug info.
> I'm 100% with you, I hate this duplication. The approach of using ContentLayer is 100% my end goal. I actually have another CL in flight that removes m_owner from ContentLayer to enable this ... I'll upload that shortly so you can see. I created this HUD to help me debug a bug in that change..

That's great to hear!

> 
> So, my proposal is, land this. Once this lands, I'll land my m_owner code and move the drawing into a CCPainterInerface and the uglies will "go away."

Unless there's a reason to rush this change in, why not make the necessary m_owner changes and then check-in the HUD?  I think I'd prefer a single clean check-in.  

> 
> > It would be very helpful to also include:
> > 1. layer bounds
> > 2. Render surface info
> Totally.
> 
> > Also mind you that this will need to be timed appropriately with: https://bugs.webkit.org/show_bug.cgi?id=54315
> Talked with JamesR about this last night. I think we were thinking to land land his update change, then my m_owner change. Mod a few minor edits, this HUD stuff can go either before or after the update change.
> 
> > I would prefer activating the HUD with a different flag.
> It feels clunky that we have to go through the page's Settings object to get this setting in from Chromium. Is there a better way? Page Settings have a showRepaintCounter, I could use that and plumb it up to Chromium. Or should I make something new?

Since this is a chrome-only flag, it can be added to WebSettingsImpl. See this change for an example:
https://bugs.webkit.org/attachment.cgi?id=82803

[Nat Duca]

> Unless there's a reason to rush this change in, why not make the necessary m_owner changes and then check-in the HUD?  I think I'd prefer a single clean check-in.  

Didn't mean to imply that I was rushing! I was approaching this from "changes for debugging" and a "changes for threading" perspective. It sounds like you feel that these should be done at the same time so I'll go do that.

[James Robinson]

Comment on attachment 82907 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=82907&action=review

R- because this will won't submit

> Source/WebCore/ChangeLog:10
> +        No new tests. (OOPS!)

There's a presubmit check that will barf on this - you have to remove this line and (preferably) replace it with some text describing the patch + why it doesn't need a test.

[James Robinson]

(In reply to comment #6)
> > Unless there's a reason to rush this change in, why not make the necessary m_owner changes and then check-in the HUD?  I think I'd prefer a single clean check-in.  
> 
> Didn't mean to imply that I was rushing! I was approaching this from "changes for debugging" and a "changes for threading" perspective. It sounds like you feel that these should be done at the same time so I'll go do that.

Keep the m_owner changes and HUD as separate patches, please :)

[Nat Duca]

Created attachment 83374 [details]
Patch

[Nat Duca]

Thinking this through, I think its best to leave the HUD drawing code completely independent of ContentLayer's update code, even though it duplicates some code.  Two reasons, based on discussions with jamesr:
1. Given the purpose of this system as a debugging aide, it seems better to have this system work even when, for example, content layer drawing is broken.
2. The refactoring that makes a clean path for GraphicsContext creation is going to while in coming, due to other things in the pipe.


(In reply to comment #9)
> Created an attachment (id=83374) [details]
> Patch

[Nat Duca]

Created attachment 83529 [details]
Patch

[Vangelis Kokkevis]

Comment on attachment 83529 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83529&action=review

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:363
> +    m_presentTimeHistory[m_currentFrameNumber%2] = currentTime();

needs spaces around %

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:897
> +    if (m_showRepaintCounter)

I believe that the repaint counter in WebKit is somewhat different as it keeps a repaint count per layer (which is quite useful).  Is having a global repaint counter for the page useful?

[Vangelis Kokkevis]

(In reply to comment #10)
> Thinking this through, I think its best to leave the HUD drawing code completely independent of ContentLayer's update code, even though it duplicates some code.  Two reasons, based on discussions with jamesr:
> 1. Given the purpose of this system as a debugging aide, it seems better to have this system work even when, for example, content layer drawing is broken.
> 2. The refactoring that makes a clean path for GraphicsContext creation is going to while in coming, due to other things in the pipe.

Ok, I see the rationale behind #1 but I think the excessive code duplication outweighs the benefits here. We could create a new class that will serve as a base class for both ContentLayerChromium and some InfoLayerChromium (serving the HUD and any other debug display needs we have) to isolate ContentLayerChromium failures from HUD failure.

If that doesn't work, I think at a minimum the code for creating the GraphicsContext for the HUD and drawing the textured quad for it should be moved in a more generic helper class outside the LayerRendererChromium.  There's a lot of code there which makes an already large source file even larger. 

 
> 
> 
> (In reply to comment #9)
> > Created an attachment (id=83374) [details] [details]
> > Patch

[Nat Duca]

The specific duplication is the setup code for a GraphicsContext. Everything else is HUD specific.

Enne is working on a path that will make a PlatformCanvas abstraction that makes this cleaner. When that lands, I will use it.


(In reply to comment #13)
> (In reply to comment #10)
> > Thinking this through, I think its best to leave the HUD drawing code completely independent of ContentLayer's update code, even though it duplicates some code.  Two reasons, based on discussions with jamesr:
> > 1. Given the purpose of this system as a debugging aide, it seems better to have this system work even when, for example, content layer drawing is broken.
> > 2. The refactoring that makes a clean path for GraphicsContext creation is going to while in coming, due to other things in the pipe.
> 
> Ok, I see the rationale behind #1 but I think the excessive code duplication outweighs the benefits here. We could create a new class that will serve as a base class for both ContentLayerChromium and some InfoLayerChromium (serving the HUD and any other debug display needs we have) to isolate ContentLayerChromium failures from HUD failure.
> 
> If that doesn't work, I think at a minimum the code for creating the GraphicsContext for the HUD and drawing the textured quad for it should be moved in a more generic helper class outside the LayerRendererChromium.  There's a lot of code there which makes an already large source file even larger. 
> 
> 
> > 
> > 
> > (In reply to comment #9)
> > > Created an attachment (id=83374) [details] [details] [details]
> > > Patch

(In reply to comment #13)
> (In reply to comment #10)
> > Thinking this through, I think its best to leave the HUD drawing code completely independent of ContentLayer's update code, even though it duplicates some code.  Two reasons, based on discussions with jamesr:
> > 1. Given the purpose of this system as a debugging aide, it seems better to have this system work even when, for example, content layer drawing is broken.
> > 2. The refactoring that makes a clean path for GraphicsContext creation is going to while in coming, due to other things in the pipe.
> 
> Ok, I see the rationale behind #1 but I think the excessive code duplication outweighs the benefits here. We could create a new class that will serve as a base class for both ContentLayerChromium and some InfoLayerChromium (serving the HUD and any other debug display needs we have) to isolate ContentLayerChromium failures from HUD failure.
> 
> If that doesn't work, I think at a minimum the code for creating the GraphicsContext for the HUD and drawing the textured quad for it should be moved in a more generic helper class outside the LayerRendererChromium.  There's a lot of code there which makes an already large source file even larger. 
> 
> 
> > 
> > 
> > (In reply to comment #9)
> > > Created an attachment (id=83374) [details] [details] [details]
> > > Patch

[Nat Duca]

Created attachment 83751 [details]
Patch

[WebKit Review Bot]

Attachment 83751 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7985622

[James Robinson]

Comment on attachment 83751 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=83751&action=review

Left some nitpicks.  I'm not sure how the debugging bits are going to get set - will that go through Settings or some other process?

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:16
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are
> + * met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above
> + * copyright notice, this list of conditions and the following disclaimer
> + * in the documentation and/or other materials provided with the
> + * distribution.
> + *     * Neither the name of Google Inc. nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.

it's 2011 now, and this is technically the wrong license header format (should be 2-clause). Take a look at Source/WebCore/LICENSE-APPLE and replace the "Apple Inc." in the first line with "Google Inc." to get the most correct incantation.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:79
> +        hudSize.setWidth(std::min(2048, m_layerRenderer->rootVisibleRect().width()));
> +        hudSize.setHeight(std::min(2048, m_layerRenderer->rootVisibleRect().height()));

nit: webkit style is to have a using declaration at the top of the .cpp and just use min()

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:87
> +    // render pixels into the texture

nit: WebKit comments start with a capital letter and end with a period.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:90
> +#if PLATFORM(SKIA)

this block gets a lot simpler with enne's PlatformCanvas thingy

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:47
> +    CCHeadsUpDisplay(LayerRendererChromium* owner);

nit: explicit

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:70
> +    double m_presentTimeHistory[2]; // Units are in seconds.

nit: include the unit in the type name so it's clearer without having to look at the header.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:73
> +    bool m_showFPSCounter : 1;
> +    bool m_showPlatformLayerTree : 1;

no real need to bitpack these, so just leave out the : 1

> Source/WebKit/chromium/public/WebSettings.h:101
> +    virtual void setShowFPSCounter(bool) = 0;
> +    virtual bool showFPSCounter() const = 0;
> +    virtual void setShowPlatformLayerTree(bool) = 0;
> +    virtual bool showPlatformLayerTree() const = 0;

I'm not sure who/what sets these guys, but if you only plan to manipulate these from within WebViewImpl, then you only need to put the bits on WebSettingsImpl and not WebSettings.  WebSettings is supposed to match WebCore::Settings directly - if you have chromium-specific stuff it's better add those on WebSettingsImpl since they don't map to Settings.  In WebViewImpl the m_webSettings object is of type OwnPtr<WebSettingsImpl>.

[Nat Duca]

> I'm not sure who/what sets these guys, but if you only plan to manipulate these from within WebViewImpl....

The plan is to have --show-fps-counter and --show-composited-layer-tree as well as related about:flags...

http://codereview.chromium.org/6581004/

This forces me to extend WebSettings as well.

[Nat Duca]

Created attachment 84599 [details]
Patch

[James Robinson]

Comment on attachment 84599 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84599&action=review

Lots more nitpicks. Overall it looks fine, though.  I promise that all these nitpicks will make your future WebKit patching life easier :)

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:55
> +
> +

Unnecessary whitespace here

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.cpp:128
> +    m_headsUpDisplay.clear(); // explicitly destroy the HUD before the TextureManager dies

Caps, period

> Source/WebCore/platform/graphics/chromium/LayerRendererChromium.h:129
> +    CCHeadsUpDisplay* getHeadsUpDisplay() { return m_headsUpDisplay.get(); }

nit: in webkit getters don't have "get" in the name, so this should be headsUpDisplay();

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:39
> +#if PLATFORM(SKIA)

nit: should be USE(SKIA) nowadays. I don't know why this was changed, tbqh.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:143
> +    // draw fps

Caps, period

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:158
> +    // draw layer tree, if enabled

Caps, period

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:40
> +class CCHeadsUpDisplay {
> +public:
> +    explicit CCHeadsUpDisplay(LayerRendererChromium* owner);

nit:
Since this is single-ownership, add WTF_MAKE_NONCOPYABLE here (like in PlatformCanvas), add a static PassOwnPtr<CCHeadsUpDisplay> create(), and make the constructor private so you have to use the create() function.  That way the type system will help make sure that you don't leak these guys accidentally - you have to either assign it to an OwnPtr<> or call .leakPtr() explicitly.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:55
> +    void drawHudContents(GraphicsContext* ctx, const IntSize& hudSize);

nit: don't name the "ctx" parameter name here.

[Nat Duca]

Created attachment 84653 [details]
Patch

[James Robinson]

Comment on attachment 84653 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=84653&action=review

Looks fine.  Two very minor nits - I'm fine with landing as is or going one more.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.cpp:114
> +void CCHeadsUpDisplay::drawHudContents(GraphicsContext* ctx, const IntSize& hudSize)
> +{
> +    FontDescription medFontDesc;

nit: WK prefers full words (context, medium) over abbr.

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:60
> +    CCHeadsUpDisplay(LayerRendererChromium* owner);

nit: still needs explicit

[Nat Duca]

Created attachment 84656 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 84656 [details]
Patch

Clearing flags on attachment: 84656

Committed r80327: <http://trac.webkit.org/changeset/80327>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.