Bug 72192 – [Chromium] Initialize compositor resources prior to painting.

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[David Reveman]

Split layer updates into two steps. Step one calculates transforms and visible layer areas. Step two paints layer contents. This allows us to perform a compositor resource preparation step after visible layer areas have been computed but before any painting has occurred.

[David Reveman]

Created attachment 114786 [details]
Patch

[David Levin]

Consider updating the patch to apply cleanly to get the benefit of the ews bots.

[David Reveman]

Created attachment 114791 [details]
Patch

[David Reveman]

Created attachment 114804 [details]
Patch

[David Reveman]

Created attachment 114863 [details]
Patch

[David Reveman]

Created attachment 115078 [details]
Patch

[David Reveman]

I intend to split this up into two different patches:

1. Re-factoring with ability to initialize compositor resources prior to painting.
2. Start texture updates as soon as contents required for update has been painted.

[David Reveman]

Created attachment 115097 [details]
Patch

[David Reveman]

Created attachment 115220 [details]
Patch

[Adrienne Walker]

Comment on attachment 115220 [details]
Patch

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

Aside: You're probably going to conflict with http://trac.webkit.org/changeset/100388, which cleans up some of the setVisibleLayerRect code.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:71
> +    m_proxy->update(texture, sourceRect, destRect);

A task gets posted for every single texture update? I'm a little confused by this.  Is the goal to decrease latency by starting on texture uploads (in the single-threaded case) in the middle of painting? The threaded compositor won't start uploading until the commit begins, so isn't helped by any of this.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.h:239
> +    LayerList m_renderSurfaceUpdateList;
>      LayerList m_updateList;

Can m_updateList maybe be named m_layerUpdateList to differentiate what it is? Maybe a comment here would help too to explain what these lists do.

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:456
> +void CCThreadProxy::beginFrameCompleteOnImplThread(CCCompletionEvent* completion)

During the meeting yesterday, there was some talk on not requiring this begin frame synchronization if we weren't painting to shared memory.  Is that something that'd be easyu to address in this patch?

> Source/WebCore/platform/graphics/chromium/cc/CCTiledLayerImpl.cpp:342
> +                GLC(context, context->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MIN_FILTER, filter));
> +                GLC(context, context->texParameteri(GraphicsContext3D::TEXTURE_2D, GraphicsContext3D::TEXTURE_MAG_FILTER, filter));

Oh, excellent.  This code should have been moved a while ago.

[Nat Duca]

Comment on attachment 115220 [details]
Patch

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

Tests?

> Source/WebCore/platform/graphics/chromium/cc/CCProxy.h:76
> +    virtual void update(LayerTextureUpdater::Texture*, const IntRect& sourceRect, const IntRect& destRect) = 0;

name this so its obvious its asynchronous [potentially]

add comment what it does

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:220
> +}

What are the merits of doing this immediately versus posting a task to the main thread to do the update?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:401
> +        s_ccThread->postTask(createCCThreadTask(this, &CCThreadProxy::beginFrameOnImplThread, AllowCrossThreadAccess(&completion)));

This isn't really quite the right naming. prepareToUpdateOnImplThread?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:435
> +    m_layerTreeHost->prepareCompositorResources(m_layerTreeHostImpl->context(), m_layerTreeHostImpl->contentsTextureAllocator());

This runs on the impl thread, yes? Then it should be named prepareCompositorResourcesOnImplThread

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:443
> +void CCThreadProxy::updateTextureOnImplThread(UpdateEntry* entry)

PassOwnPtr?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:452
> +    m_currentTextureUpdaterOnImplThread->append(entry->m_texture, entry->m_sourceRect, entry->m_destRect);

I'm confused about the interaction between this patch and the COMMIT state works. When do we transition from FrameInProgress to UpdatingResources in this model?

[David Reveman]

Created attachment 115859 [details]
Patch

[David Reveman]

Created attachment 115881 [details]
Patch

[David Reveman]

Created attachment 115884 [details]
Patch

[David Reveman]

Created attachment 116003 [details]
Patch

[David Reveman]

Created attachment 116116 [details]
Patch

[David Reveman]

Created attachment 116293 [details]
Patch

[James Robinson]

Comment on attachment 116293 [details]
Patch

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

I'm not a huge fan of doing a pass through all layers just to check if we need to do a prepare before painting. Can we make that a global (per-compositor) setting now? It looks like in this patch it only changes behavior for video layers by moving the map() earlier.

> Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:54
> +    m_texture->allocateTexture(context, allocator);

not sure i understand this - why do we need to allocate the texture before painting?

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:52
> +        virtual bool needPrepareRect() { return false; }

this name isn't terribly illuminating - what does it mean to 'prepare a rect'? none of the implementations of this seem to use the rect part at all

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:53
> +        virtual void prepareRect(GraphicsContext3D*, TextureAllocator*, const IntRect& /* sourceRect */, const IntRect& /* destRect */) { }

nobody seems to be using the sourceRect or destRect parameters. why are they here?

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:55
> +        virtual void updateRect(GraphicsContext3D*, const IntRect& sourceRect, const IntRect& destRect) = 0;

i think at this point you should document the order and thread requirements of each of these calls since there are so many

> Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:60
> +        virtual ~Handler() { };

don't need a trailing ; here

[David Reveman]

(In reply to comment #18)
> (From update of attachment 116293 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116293&action=review
> 
> I'm not a huge fan of doing a pass through all layers just to check if we need to do a prepare before painting. Can we make that a global (per-compositor) setting now? It looks like in this patch it only changes behavior for video layers by moving the map() earlier.

This patch is mostly re-factoring. I've tried to keep as much as possible in follow up patches (See the chain of bugs that depend on this bug).

I'm not a huge fan of the needPrepare pass myself. I'm planning to get rid of it when serializing the prepare and commit steps. Though, that will have to wait until after async commit support has landed.

> 
> > Source/WebCore/platform/graphics/chromium/CanvasLayerTextureUpdater.cpp:54
> > +    m_texture->allocateTexture(context, allocator);
> 
> not sure i understand this - why do we need to allocate the texture before painting?

Sorry, this must be confusing without some insight into the follow up patch. https://bugs.webkit.org/show_bug.cgi?id=72752 adds support for performing texture uploads in parallel with contents painting. This puts certain thread requirements on the texture upload part (updateRect()). E.g. we have to avoid access to the texture manager among other things. Hence, textures need to be allocated prior to painting. We should be able to improve this later by re-factoring texture allocation.

FYI, what's currently running on the main thread vs the impl thread is not obvious. I've uploaded my current async commit patch here: https://bugs.webkit.org/show_bug.cgi?id=73279 it's work in progress but will make this much more clear.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:52
> > +        virtual bool needPrepareRect() { return false; }
> 
> this name isn't terribly illuminating - what does it mean to 'prepare a rect'? none of the implementations of this seem to use the rect part at all

Yea, the 'Rect' suffix doesn't make a whole lot of sense here. I kept it to be consistent with prepareRect. I'd prefer to keep the suffixes here until the async commit patch which removes the rect parameters.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:53
> > +        virtual void prepareRect(GraphicsContext3D*, TextureAllocator*, const IntRect& /* sourceRect */, const IntRect& /* destRect */) { }
> 
> nobody seems to be using the sourceRect or destRect parameters. why are they here?

They are used by follow up patches like the shared memory painting patch that needs them to map an appropriately sized buffer. Like I mentioned above, mostly re-factoring in this patch.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:55
> > +        virtual void updateRect(GraphicsContext3D*, const IntRect& sourceRect, const IntRect& destRect) = 0;
> 
> i think at this point you should document the order and thread requirements of each of these calls since there are so many

I'll definitely do that. FYI, this will become much better when async commit and a more serialized version of the prepare step lands.

> 
> > Source/WebCore/platform/graphics/chromium/LayerTextureUpdater.h:60
> > +        virtual ~Handler() { };
> 
> don't need a trailing ; here

Sure.

[James Robinson]

Let's only leave stuff in this patch that makes sense by itself. We can add more functionality later.  We need to keep the codebase in a good state at all times whenever possible.

[David Reveman]

(In reply to comment #20)
> Let's only leave stuff in this patch that makes sense by itself. We can add more functionality later.  We need to keep the codebase in a good state at all times whenever possible.

There shouldn't be anything in here that puts the codebase in a worse state than before. Considering this is a re-factoring patch that is supposed to change the structure of things such that planned alternative texture uploaders can be implemented, it's hard for me to draw the line between what's useful by itself and what's not. I guess one could make the case that none of it is useful by itself as it's just re-factoring.

Would you prefer if the texture allocation changes were pushed to the follow up patch? Should I remove some of the currently unused function arguments from this patch and have them be added by the patches that require them?

[David Reveman]

These changes have been folded into the latest patch attached to: https://bugs.webkit.org/show_bug.cgi?id=72752

I'll mark this bug as a duplicate if this is indeed the preferred way to land these changes.

[David Reveman]

*** This bug has been marked as a duplicate of bug 72752 ***

[David Reveman]

72752 makes sense without these changes. This bug now depends 72752 instead of the other way around.

[David Reveman]

Created attachment 118711 [details]
Patch

[David Reveman]

Created attachment 120686 [details]
Patch

Rebase on latest version of https://bugs.webkit.org/show_bug.cgi?id=72752

[David Reveman]

Created attachment 120745 [details]
Patch

[David Reveman]

Created attachment 121173 [details]
Patch