Bug 67499 - [chromium] Support CCHeadsUpDisplay in threaded compositing mode
: [chromium] Support CCHeadsUpDisplay in threaded compositing mode
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
: 67417 79723
: 79400
  Show dependency treegraph
 
Reported: 2011-09-02 10:23 PST by
Modified: 2012-04-13 11:08 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-09-02 10:23:23 PST
The CCHeadsUpDisplay needs to be rewritten to deal with the CCThread design.
------- Comment #1 From 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 From 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 From 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
        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(...);
    }

}
------- Comment #4 From 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 From 2012-03-12 11:33:57 PST -------
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 From 2012-04-10 19:54:35 PST -------
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 From 2012-04-10 19:56:36 PST -------
Created an attachment (id=136613) [details]
Patch
------- Comment #8 From 2012-04-10 23:52:01 PST -------
(From update of attachment 136613 [details])
Attachment 136613 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12380764
------- Comment #9 From 2012-04-11 10:39:54 PST -------
Created an attachment (id=136694) [details]
Fixed unit tests build
------- Comment #10 From 2012-04-11 16:59:37 PST -------
(From update of attachment 136694 [details])
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 From 2012-04-11 17:37:36 PST -------
OK will come back with a new patch soon. =)
------- Comment #12 From 2012-04-11 18:05:28 PST -------
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 From 2012-04-11 19:56:06 PST -------
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 From 2012-04-11 20:56:33 PST -------
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 From 2012-04-12 13:11:47 PST -------
Created an attachment (id=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 From 2012-04-12 13:23:51 PST -------
(From update of attachment 136957 [details])
R=me
------- Comment #17 From 2012-04-12 13:51:47 PST -------
(From update of attachment 136957 [details])
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 From 2012-04-12 14:26:43 PST -------
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 From 2012-04-12 17:06:17 PST -------
Created an attachment (id=136998) [details]
Patch for landing
------- Comment #20 From 2012-04-12 17:07:07 PST -------
(In reply to comment #19)
> Created an attachment (id=136998) [details] [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 From 2012-04-12 21:32:11 PST -------
(From update of attachment 136998 [details])
Rejecting attachment 136998 [details] from commit-queue.

New failing tests:
perf/array-reverse.html
Full output: http://queues.webkit.org/results/12396550
------- Comment #22 From 2012-04-12 21:32:17 PST -------
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
------- Comment #23 From 2012-04-13 09:37:39 PST -------
(In reply to comment #22)
> Created an attachment (id=137036) [details] [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 From 2012-04-13 09:40:09 PST -------
Created an attachment (id=137096) [details]
Patch for landing
------- Comment #25 From 2012-04-13 11:07:23 PST -------
Committed r114147: <http://trac.webkit.org/changeset/114147>
------- Comment #26 From 2012-04-13 11:08:09 PST -------
(From update of attachment 137096 [details])
Landed this same patch directly instead.