Bug 67499 – [chromium] Support CCHeadsUpDisplay in threaded compositing mode

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nat Duca]

The CCHeadsUpDisplay needs to be rewritten to deal with the CCThread design.

[James Robinson]

Today the chromium compositor HUD doesn't work in threaded mode.  Here are some of the issues:

1.) The FPS counter and layer tree displays both display string values. This is currently implemented by instantiating a WebCore::Font (actually a pair of them, small and medium) and then using these to format and draw the text runs. The WebCore Font code manipulates several static caches and is definitely not safe to create/destroy from a non-main thread, and possibly not safe to use at all.

2.) The layer tree display in debug mode prints out descriptive layer name for some sets of layers. Currently this is propagated from GraphicsLayerChromium as a WTF::String and is copied to the impl tree in a non-threadsafe manner.  For this to be safe, we have to make an explicit copy of the string using WTF::String::isolatedCopy() whenever crossing a thread boundary.

[James Robinson]

https://bugs.webkit.org/show_bug.cgi?id=79723 takes care of #2.  #1 is definitely harder.  I think the simplest thing to do would be to make more direct text drawing calls using a super simple font and sidestep as much of WebCore's font/text machinery as possible. Monospace, bitmap - so long as it's readable it doesn't really matter.  We could even raster a full alphabet on the main thread and use it as a texture atlas since we only need [0-9A-Za-z] and a very limited set of punctuation for the layer tree.

[Shawn Singh]

Task: implement a minimal, clean font atlas, initialized on main thread, used on impl thread.  

This seems pretty straightforward, can you please let me know if there's something i'm stupidly overlooking?

class CCHeadsUpDisplayFont {
    void initializeFontAtlas(... ) { 
        ASSERT( on main thread );
        setup a ManagedTexture
        paint monospace text on it using PlatformCanvas::Painter and WebCore::Font
    }

    void drawString(position, string, layerRenderer) {
        set up the shader program
        drawStringInternal(...);
    }

    void drawLines(position, height, vector<string>, layerRenderer) {
        set up the shader program
        for each line
            drawStringInternal(...);
    }

private:
    void drawStringInternal(position, string, layerRenderer) {
        for each char, give the tex coords and call drawTexturedQuad(...);
    }

}

[James Robinson]

That sounds good.  I'd suggest that you software paint an atlas on the main thread and then on the impl thread just software paint from atlas -> bitmap and then upload that when it's fully assembled.

[Nat Duca]

I also suggest implementing the drawing not into a GL context but onto a skia canvas. Otherwise, you're gonna create a bunch of draw calls that we'll then have to tune.

[Shawn Singh]

Patch coming in a moment

(1) Threading + heads-up display should work out-of-the-box with this patch

(2) Font atlas only allocated if --show-fps-counter or --show-composited-layer-tree are on.  (in a later patch, we probably should create a helper function "shouldEnableHud(...)")

(3) Approximate memory overhead of the font atlas:  less than 70k.   (128x128 atlas, and 128-entry lookup table of IntRects) 

(4) Unfortunately does perform slightly slower than previously, but this is just debugging code anyway, I don't think its a big deal.

(5) Please let me know if you see any blocking calls anywhere that I am un-aware of.

[Shawn Singh]

Created attachment 136613 [details]
Patch

[WebKit Review Bot]

Comment on attachment 136613 [details]
Patch

Attachment 136613 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12380764

[Shawn Singh]

Created attachment 136694 [details]
Fixed unit tests build

[James Robinson]

Comment on attachment 136694 [details]
Fixed unit tests build

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

> Source/WebCore/platform/graphics/chromium/cc/CCFontAtlas.cpp:99
> +        UChar c = static_cast<UChar>(i);
> +        String str;
> +        str.append(c);

You really want to use LChar here (latin char, 8 bit) instead of casting to UChar. You could just make the loop counter be of type LChar and not cast anywhere

> Source/WebCore/platform/graphics/chromium/cc/CCFontAtlas.h:58
> +    // Draws multiple lines of text where each line of text is separated by '\n'.
> +    // Should only be called only on the impl thread.

You should document here that this will only handle ASCII chars and everything else will be a box.

I can't tell what "targetSize" is here - is it a clip?

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:64
> +    explicit CCHeadsUpDisplay(LayerRendererChromium* owner, CCFontAtlas* headsUpDisplayFontAtlas);

no explicit on c'tors with 2 arguments

> Source/WebCore/platform/graphics/chromium/cc/CCHeadsUpDisplay.h:85
> +    CCFontAtlas* m_fontAtlas;

Should we be transferring ownership of the atlas to here? We have to construct and initialize the atlas on the main thread, but after that point it's just data and we can destroy it anywhere.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:269
> +    // This is owned by the main layer tree host because it needs to be initialized on the main thread.
> +    OwnPtr<CCFontAtlas> m_headsUpDisplayFontAtlas;

I think it'd be nicer if after initializing this the CCLayerTreeHost didn't have to worry about the atlas any more (see above).

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:250
> +    m_hostImpl->initializeLayerRenderer(createContext(), 0);

Having a default value for the second parameter wouldn't be a terrible idea here, it's perfectly valid to initialize a LayerRendererChromium without worrying about an atlas

[Shawn Singh]

OK will come back with a new patch soon. =)

[Shawn Singh]

Alex, I'm CC'ing you so that you can brisk over the next patch (coming tomorrow) and make sure it doesn't clobber your recent changes.

I probably will change the layout of the FPS text and sparkle graph, and I will need to change the FPS text font itself.   But I won't be changing the nice visualization and information you added to it.

[Shawn Singh]

James:  I ran into a small snag when trying to change the ownership mechanism as you suggested.

The problem is that initializeLayerRenderer() not only occurs when the treehost asks the proxy to initializeLayerRenderer, but it also occurs when the treeHost asks the proxy to recreateContext().

There are two options to do it correctly, and neither of them seem as clean as keeping the ownership on the LayerTreeHost:

option 1: re-create a new font atlas every time we have to call proxy->recreateContext(), so that we have something valid to pass to initializeLayerRenderer.  This means not only are we recreating the font atlas on every lost context, but we are also instrumenting yet another code path in the CCProxy just to support the font atlas.

option 2: (almost certainly not a good idea) change the behavior slightly so that the layer renderer will not over-write the existing font atlas when we call proxy->recreateContext().  This means we either have to overload the initializeLRC functions in the proxy and LRC, or create some additional boolean that indicates when the atlas should be overwritten or not when the LRC is initialized...

Personally it seems better to me to simply accept that the CCLayerTreeHost owns the font atlas on the main thread, and just says "oh, yeah, here you go again" every time we have to recreate the context.  Doing that keeps one uniform way of giving the atlas to the hud, and doesn't pollute the proxy api with multiple fontAtlas args everywhere. 

What do you think?

[James Robinson]

https://bugs.webkit.org/show_bug.cgi?id=83415 will move ownership of CCHeadsUpDisplay from LRC up to CCLayerTreeHostImpl, which isn't recreated on a lost context, so I think at that point your option (2) - don't destroy the atlas when recreating the context - will make the most sense.

I guess we could go with the ownership you have in this patch for now.

[Shawn Singh]

Created attachment 136957 [details]
addressed review comments

Ownership is kept as-is for now, please feel free to assign a follow-up bug to me when its ready to be fixed

[James Robinson]

Comment on attachment 136957 [details]
addressed review comments

R=me

[Nat Duca]

Comment on attachment 136957 [details]
addressed review comments

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

Cool. Needless to say, looks good.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:103
> +    // Only allocate the font atlas if we have reason to use the heads-up display.

Though, note, if we allow devtools to turn on fps meter or show damage tracking... what happens?

[James Robinson]

We can figure out how to allocate this lazily when we get to that point.  I imagine the way it would work is when we get the information from the inspector on the main thread we allocate an atlas and then asynchronously pass it to the impl side to use on the next frame (along with the bit saying that we want the hud to be rendered)

[Shawn Singh]

Created attachment 136998 [details]
Patch for landing

[Shawn Singh]

(In reply to comment #19)
> Created an attachment (id=136998) [details]
> Patch for landing

The only difference between this and the version James r+'d is adjusting the length of the fps text so that it fits all the information on linux.

[WebKit Review Bot]

Comment on attachment 136998 [details]
Patch for landing

Rejecting attachment 136998 [details] from commit-queue.

New failing tests:
perf/array-reverse.html
Full output: http://queues.webkit.org/results/12396550

[WebKit Review Bot]

Created attachment 137036 [details]
Archive of layout-test-results from ec2-cq-01

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: ec2-cq-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Shawn Singh]

(In reply to comment #22)
> Created an attachment (id=137036) [details]
> Archive of layout-test-results from ec2-cq-01
> 
> The attached test failures were seen while running run-webkit-tests on the commit-queue.
> Bot: ec2-cq-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

these failures are not my fault - I just double-checked locally just in case, too.

I'll try to land the same patch again right now

[Shawn Singh]

Created attachment 137096 [details]
Patch for landing

[Shawn Singh]

Committed r114147: <http://trac.webkit.org/changeset/114147>

[Shawn Singh]

Comment on attachment 137096 [details]
Patch for landing

Landed this same patch directly instead.