Bug 70454 – [chromium] Batch up texture uploads so that they can be updated incrementally

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Adrienne Walker]

[chromium] Batch up texture uploads so that they can be updated incrementally

[Adrienne Walker]

Created attachment 111695 [details]
Patch

[Adrienne Walker]

Comment on attachment 111695 [details]
Patch

Work in progress patch.

If we want to interleave input and drawing during a commit, we probably need something like this.  I figure a class than can collect all the rects will have a better chance of estimating upload times in the GPU process, if we assume that it's proportional to pixels uploaded.  Insert lots of hand-waving here.

Right now CCTextureUpdater just has an updateAllTextures function, but it'd be easy enough to expose a way to partially iterate through them when needed.

One concern is that maybe this is the wrong architectural approach to upload textures while still having the compositor thread be responsive.  James suggested something like using multiple contexts, but I'm not totally sure I trust the driver to handle that responsibly on all platforms.

[WebKit Review Bot]

Comment on attachment 111695 [details]
Patch

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

[James Robinson]

Comment on attachment 111695 [details]
Patch

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

> Source/WebCore/WebCore.gypi:3547
> +            'platform/graphics/chromium/cc/CCTextureUpdater.cpp',
> +            'platform/graphics/chromium/cc/CCTextureUpdater.h',

i think you forgot to git add these files, they aren't showing up in the patch.

[Adrienne Walker]

Created attachment 111809 [details]
Patch

[Adrienne Walker]

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

Haha, whoops.  Not that the missing class was very interesting.

[Adrienne Walker]

Created attachment 111873 [details]
Patch

[Nat Duca]

Comment on attachment 111873 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:156
> +        m_layerTreeHost->beginCommitOnCCThread(m_layerTreeHostImpl.get());

Random aside: I just renamed the OnCCThread suffixes to onImplThread for clarity.

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

updateCompositorResources really just gets the list of resources to update, right?

> Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:422
> +    updater.updateAllTextures(m_layerTreeHostImpl->context(), m_layerTreeHostImpl->contentsTextureAllocator());

I guess your next step is to factor this to be something like updater.hasMore() { updater.updateMoreTextures(..., 6); }?

[Adrienne Walker]

(In reply to comment #8)
> (From update of attachment 111873 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111873&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCSingleThreadProxy.cpp:156
> > +        m_layerTreeHost->beginCommitOnCCThread(m_layerTreeHostImpl.get());
> 
> Random aside: I just renamed the OnCCThread suffixes to onImplThread for clarity.

Thanks for the heads up.  I'll rename them.  It looks like this patch doesn't apply to ToT anyway.

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:421
> > +    m_layerTreeHost->updateCompositorResources(m_layerTreeHostImpl->context(), m_layerTreeHostImpl->contentsTextureAllocator(), updater);
> 
> updateCompositorResources really just gets the list of resources to update, right?

Is this a "so it shouldn't be called update" suggestion? ;)

Unfortunately, VideoLayerChromium still does the upload in updateCompositorResources.  I considering converting VideoLayerChromium to use a texture updater and the tex map sub image class.  That class is also a little bit weird because you have to tell the video frame provider when you're done copying bits out of the frame, so there's this additional post-upload cleanup step that none of the other texture updaters have to deal with.  On top of that, on platforms where you have hardware video acceleration, this path gets skipped anyway.

In short, I punted, updateCompositorResources now updates some resources and collects others for deferred updating.

> > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:422
> > +    updater.updateAllTextures(m_layerTreeHostImpl->context(), m_layerTreeHostImpl->contentsTextureAllocator());
> 
> I guess your next step is to factor this to be something like updater.hasMore() { updater.updateMoreTextures(..., 6); }?

Would it be helpful for you if I just landed that change now with this patch?

[Adrienne Walker]

Created attachment 112001 [details]
Patch

[Alexandre Elias]

LGTM.  Seems like a useful initial refactoring.

[James Robinson]

Comment on attachment 112001 [details]
Patch

Could you fold the TextureAllocator into the CCTextureUpdater?

[Adrienne Walker]

Created attachment 112258 [details]
fold allocator into updater

[James Robinson]

Comment on attachment 112258 [details]
fold allocator into updater

R=me

[Adrienne Walker]

Committed r98360: <http://trac.webkit.org/changeset/98360>