Bug 71023

Summary: [chromium] Paint directly to shared memory buffers
Product: WebKit Reporter: David Reveman <reveman>
Component: WebCore Misc.Assignee: David Reveman <reveman>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: enne, fishd, jamesr, levin+threading, nduca, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch jamesr: review-

Description David Reveman 2011-10-27 08:54:46 PDT
Instead of painting to temporary buffers and copying to shared memory buffers used for texture uploads. Paint directly to shared memory buffers.
Comment 1 David Reveman 2011-10-27 10:52:01 PDT
Created attachment 112706 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-27 10:54:08 PDT
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 3 David Reveman 2011-10-27 11:19:44 PDT
Adds chromium command-line switch --enable-shared-memory-painting: http://codereview.chromium.org/8413006
Comment 4 Darin Fisher (:fishd, Google) 2011-10-27 11:30:15 PDT
Comment on attachment 112706 [details]
Patch

WebKit API changes LGTM
Comment 5 James Robinson 2011-10-27 11:38:13 PDT
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 6 James Robinson 2011-10-27 11:42:56 PDT
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!
Comment 7 James Robinson 2011-10-27 11:44:21 PDT
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.
Comment 8 David Reveman 2011-10-27 12:16:32 PDT
(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.
Comment 9 David Reveman 2011-10-27 12:29:02 PDT
Created attachment 112730 [details]
Patch
Comment 10 David Reveman 2011-10-27 12:32:24 PDT
(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.
Comment 11 David Reveman 2011-10-27 12:37:20 PDT
(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.
Comment 12 James Robinson 2011-10-27 13:30:54 PDT
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 13 James Robinson 2011-10-27 13:33:07 PDT
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 14 James Robinson 2011-10-27 13:51:33 PDT
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.
Comment 15 David Reveman 2011-10-27 13:54:53 PDT
(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.
Comment 16 David Reveman 2011-11-11 15:26:17 PST

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