Instead of painting to temporary buffers and copying to shared memory buffers used for texture uploads. Paint directly to shared memory buffers.
Created attachment 112706 [details] Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Adds chromium command-line switch --enable-shared-memory-painting: http://codereview.chromium.org/8413006
Comment on attachment 112706 [details] Patch WebKit API changes LGTM
Comment on attachment 112706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112706&action=review I don't understand why you are adding extra 'prepare' steps to painting and uploading. Could you elaborate? > Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.h:109 > + LayerTextureUpdaterShm(PassOwnPtr<LayerPainterChromium>); explicit
Comment on attachment 112706 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112706&action=review > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:401 > + // Blocking call to CCThreadProxy::allocateOnImplThread > + TRACE_EVENT("allocate", this, 0); > + CCCompletionEvent completion; > + s_ccThread->postTask(createCCThreadTask(this, &CCThreadProxy::allocateOnImplThread, AllowCrossThreadAccess(&completion))); > + completion.wait(); woah what? This is not right at all!
It looks like you are trying to map the shared memory segments before doing the paint, but you shouldn't need to do that when painting to an SkPicture - you only need the shared memory segment to exist when playing back the SkPicture into tiles. This change would be significantly less invasive if you did that map only when the segment was actually needed.
(In reply to comment #7) > It looks like you are trying to map the shared memory segments before doing the paint, but you shouldn't need to do that when painting to an SkPicture - you only need the shared memory segment to exist when playing back the SkPicture into tiles. This change would be significantly less invasive if you did that map only when the segment was actually needed. Yes, but that would mean that the SkPicture playback would happen on the impl thread. There's a few reasons I don't like that idea. The compositor thread gets busier, less time to redraw as a result of scroll changes. An out-of-process compositor becomes significantly harder to create as we now have to transfer SkPictures between processes. It will be important to keep the compositor lightweight and responsive for the long term unified compositor case where multiple processes are talking to it. The PBO patch I'm working on also builds on top of this patch and allows painting directly to shared memory without using a SkPicture.
Created attachment 112730 [details] Patch
(In reply to comment #5) > (From update of attachment 112706 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112706&action=review > > I don't understand why you are adding extra 'prepare' steps to painting and uploading. Could you elaborate? Plumbing to allow allocation of compositor resources before painting of layer contents. I assume you understand the reason for this after on my previous comment. > > > Source/WebCore/platform/graphics/chromium/LayerTextureUpdaterCanvas.h:109 > > + LayerTextureUpdaterShm(PassOwnPtr<LayerPainterChromium>); > > explicit Fixed.
(In reply to comment #6) > (From update of attachment 112706 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=112706&action=review > > > Source/WebCore/platform/graphics/chromium/cc/CCThreadProxy.cpp:401 > > + // Blocking call to CCThreadProxy::allocateOnImplThread > > + TRACE_EVENT("allocate", this, 0); > > + CCCompletionEvent completion; > > + s_ccThread->postTask(createCCThreadTask(this, &CCThreadProxy::allocateOnImplThread, AllowCrossThreadAccess(&completion))); > > + completion.wait(); > > woah what? This is not right at all! On the condition that we want to allow compositor resource allocation before painting, please elaborate if there's still something wrong here.
Sorry for jumping in without full context, Nat filled me in on what's up here. I like the idea of keeping rasterization out of the critical path. I think we should approach this piece-by-piece to keep it a bit more manageable: 1.) Add support for painting into an SkPicture and then rasterizing into tile-sized chunks 2.) Add support for doing the playback into mapped memory segments to avoid an extra memcpy() Doing step (1) first will give us a lot of flexibility down the road. To get (2) done effectively I think we'll want to refactor the way we do layer iterations to make it more general - this patch adds a few copy-pasted layer iterations that are going to be a headache to maintain IMO. Would you mind breaking (1) out into its own patch and doing it first? It'd be really useful for many things we are considering.
Comment on attachment 112730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112730&action=review > Source/WebCore/platform/graphics/chromium/ManagedTexture.cpp:108 > + m_mappedBuffer = static_cast<uint8_t*>(extensions->mapTexSubImage2DCHROMIUM(GraphicsContext3D::TEXTURE_2D, 0, destRect.x(), destRect.y(), destRect.width(), destRect.height(), m_format, GraphicsContext3D::UNSIGNED_BYTE, Extensions3DChromium::WRITE_ONLY)); keep in mind this call can fail and return NULL - you have to be deal with that case (generally by malloc()ing a buffer and using texSubImage2D)
Comment on attachment 112730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=112730&action=review > Source/WebCore/platform/graphics/chromium/TiledLayerChromium.cpp:156 > +void TiledLayerChromium::prepareCompositorResources(GraphicsContext3D* context, CCTextureUpdater& updater) This also looks mostly like a copy-paste of updateCompositorResources() > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:430 > +void CCLayerTreeHost::prepareCompositorResources(GraphicsContext3D* context, CCTextureUpdater& updater) I think that functions like this are not going to be maintainable. This is almost entirely a copy-paste of updateCompositorResources() - it seems like what you need is an iterator pattern so we can keep the logic of 'how do we go through the layer list' in one place and the and potentially do different things at each layer.
(In reply to comment #12) > Sorry for jumping in without full context, Nat filled me in on what's up here. > > I like the idea of keeping rasterization out of the critical path. I think we should approach this piece-by-piece to keep it a bit more manageable: > > 1.) Add support for painting into an SkPicture and then rasterizing into tile-sized chunks > 2.) Add support for doing the playback into mapped memory segments to avoid an extra memcpy() > > Doing step (1) first will give us a lot of flexibility down the road. To get (2) done effectively I think we'll want to refactor the way we do layer iterations to make it more general - this patch adds a few copy-pasted layer iterations that are going to be a headache to maintain IMO. Couldn't agree more. I'm happy to fix this. > > Would you mind breaking (1) out into its own patch and doing it first? It'd be really useful for many things we are considering. Sounds good. I'll do that.
*** This bug has been marked as a duplicate of bug 72191 ***