WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED DUPLICATE of
bug 72191
71023
[chromium] Paint directly to shared memory buffers
https://bugs.webkit.org/show_bug.cgi?id=71023
Summary
[chromium] Paint directly to shared memory buffers
David Reveman
Reported
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.
Attachments
Patch
(55.40 KB, patch)
2011-10-27 10:52 PDT
,
David Reveman
no flags
Details
Formatted Diff
Diff
Patch
(55.59 KB, patch)
2011-10-27 12:29 PDT
,
David Reveman
jamesr
: review-
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
David Reveman
Comment 1
2011-10-27 10:52:01 PDT
Created
attachment 112706
[details]
Patch
WebKit Review Bot
Comment 2
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.
David Reveman
Comment 3
2011-10-27 11:19:44 PDT
Adds chromium command-line switch --enable-shared-memory-painting:
http://codereview.chromium.org/8413006
Darin Fisher (:fishd, Google)
Comment 4
2011-10-27 11:30:15 PDT
Comment on
attachment 112706
[details]
Patch WebKit API changes LGTM
James Robinson
Comment 5
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
James Robinson
Comment 6
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!
James Robinson
Comment 7
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.
David Reveman
Comment 8
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.
David Reveman
Comment 9
2011-10-27 12:29:02 PDT
Created
attachment 112730
[details]
Patch
David Reveman
Comment 10
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.
David Reveman
Comment 11
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.
James Robinson
Comment 12
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.
James Robinson
Comment 13
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)
James Robinson
Comment 14
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.
David Reveman
Comment 15
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.
David Reveman
Comment 16
2011-11-11 15:26:17 PST
*** This bug has been marked as a duplicate of
bug 72191
***
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug