[chromium] Batch up texture uploads so that they can be updated incrementally
Created attachment 111695 [details] Patch
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.
Comment on attachment 111695 [details] Patch Attachment 111695 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10181072
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.
Created attachment 111809 [details] Patch
(In reply to comment #5) > Created an attachment (id=111809) [details] > Patch Haha, whoops. Not that the missing class was very interesting.
Created attachment 111873 [details] Patch
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); }?
(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?
Created attachment 112001 [details] Patch
LGTM. Seems like a useful initial refactoring.
Comment on attachment 112001 [details] Patch Could you fold the TextureAllocator into the CCTextureUpdater?
Created attachment 112258 [details] fold allocator into updater
Comment on attachment 112258 [details] fold allocator into updater R=me
Committed r98360: <http://trac.webkit.org/changeset/98360>