Bug 71023 – [chromium] Paint directly to shared memory buffers

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

[David Reveman]

Instead of painting to temporary buffers and copying to shared memory buffers used for texture uploads. Paint directly to shared memory buffers.

[David Reveman]

Created attachment 112706 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[David Reveman]

Adds chromium command-line switch --enable-shared-memory-painting: http://codereview.chromium.org/8413006

[Darin Fisher (:fishd, Google)]

Comment on attachment 112706 [details]
Patch

WebKit API changes LGTM

[James Robinson]

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

[James Robinson]

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!

[James Robinson]

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.

[David Reveman]

(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.

[David Reveman]

Created attachment 112730 [details]
Patch

[David Reveman]

(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.

[David Reveman]

(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.

[James Robinson]

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.

[James Robinson]

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)

[James Robinson]

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.

[David Reveman]

(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.

[David Reveman]

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