Bug 67499 - [chromium] Support CCHeadsUpDisplay in threaded compositing mode
Summary: [chromium] Support CCHeadsUpDisplay in threaded compositing mode
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Shawn Singh
Depends on: 67417 79723
Blocks: 79400
  Show dependency treegraph
Reported: 2011-09-02 10:23 PDT by Nat Duca
Modified: 2012-04-13 11:08 PDT (History)
7 users (show)

See Also:

Patch (33.52 KB, patch)
2012-04-10 19:56 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Fixed unit tests build (40.40 KB, patch)
2012-04-11 10:39 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
addressed review comments (34.42 KB, patch)
2012-04-12 13:11 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Patch for landing (34.42 KB, patch)
2012-04-12 17:06 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cq-01 (6.36 MB, application/zip)
2012-04-12 21:32 PDT, WebKit Review Bot
no flags Details
Patch for landing (34.46 KB, patch)
2012-04-13 09:40 PDT, Shawn Singh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nat Duca 2011-09-02 10:23:23 PDT
The CCHeadsUpDisplay needs to be rewritten to deal with the CCThread design.
Comment 1 James Robinson 2012-02-27 17:19:04 PST
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.
Comment 2 James Robinson 2012-02-27 17:35:21 PST
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.
Comment 3 Shawn Singh 2012-03-10 19:31:37 PST
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

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

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

Comment 4 James Robinson 2012-03-10 22:09:23 PST
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.
Comment 5 Nat Duca 2012-03-12 11:33:57 PDT
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.
Comment 6 Shawn Singh 2012-04-10 19:54:35 PDT
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.
Comment 7 Shawn Singh 2012-04-10 19:56:36 PDT
Created attachment 136613 [details]
Comment 8 WebKit Review Bot 2012-04-10 23:52:01 PDT
Comment on attachment 136613 [details]

Attachment 136613 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12380764
Comment 9 Shawn Singh 2012-04-11 10:39:54 PDT
Created attachment 136694 [details]
Fixed unit tests build
Comment 10 James Robinson 2012-04-11 16:59:37 PDT
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
Comment 11 Shawn Singh 2012-04-11 17:37:36 PDT
OK will come back with a new patch soon. =)
Comment 12 Shawn Singh 2012-04-11 18:05:28 PDT
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.
Comment 13 Shawn Singh 2012-04-11 19:56:06 PDT
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?
Comment 14 James Robinson 2012-04-11 20:56:33 PDT
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.
Comment 15 Shawn Singh 2012-04-12 13:11:47 PDT
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
Comment 16 James Robinson 2012-04-12 13:23:51 PDT
Comment on attachment 136957 [details]
addressed review comments

Comment 17 Nat Duca 2012-04-12 13:51:47 PDT
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?
Comment 18 James Robinson 2012-04-12 14:26:43 PDT
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)
Comment 19 Shawn Singh 2012-04-12 17:06:17 PDT
Created attachment 136998 [details]
Patch for landing
Comment 20 Shawn Singh 2012-04-12 17:07:07 PDT
(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.
Comment 21 WebKit Review Bot 2012-04-12 21:32:11 PDT
Comment on attachment 136998 [details]
Patch for landing

Rejecting attachment 136998 [details] from commit-queue.

New failing tests:
Full output: http://queues.webkit.org/results/12396550
Comment 22 WebKit Review Bot 2012-04-12 21:32:17 PDT
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
Comment 23 Shawn Singh 2012-04-13 09:37:39 PDT
(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
Comment 24 Shawn Singh 2012-04-13 09:40:09 PDT
Created attachment 137096 [details]
Patch for landing
Comment 25 Shawn Singh 2012-04-13 11:07:23 PDT
Committed r114147: <http://trac.webkit.org/changeset/114147>
Comment 26 Shawn Singh 2012-04-13 11:08:09 PDT
Comment on attachment 137096 [details]
Patch for landing

Landed this same patch directly instead.