Summary: | [Chromium] Add support for painting directly to shared memory. | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | David Reveman <reveman> | ||||||||||||||||||||||||||||||||||||||||||||||
Component: | WebKit Misc. | Assignee: | David Reveman <reveman> | ||||||||||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED WONTFIX | ||||||||||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | danakj, enne, jamesr, nduca, piman, schenney, sky | ||||||||||||||||||||||||||||||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 71869 | ||||||||||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
David Reveman
2011-11-11 15:16:18 PST
*** Bug 71023 has been marked as a duplicate of this bug. *** Created attachment 114788 [details]
Patch
Do we know if this is actually a win? (In reply to comment #3) > Do we know if this is actually a win? This is more important for the ability to implement an out-of-process compositor than performance but eliminating an unnecessary memcpy shouldn't hurt. Shared memory resources are not increased by this change so it seems unlikely it would have a negative impact. Only difference is the shared memory is mapped in the renderer for a bit longer than before. I'd be surprised if that has any performance impact. We could make painting directly to shared memory optional but I don't see a good reason for that. Created attachment 115222 [details]
Patch
Comment on attachment 115222 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=115222&action=review This looks good to me. > Source/WebCore/platform/graphics/chromium/BitmapSkPictureCanvasLayerTextureUpdater.cpp:79 > + m_mappedBuffer = NULL; nit: s/NULL/0/ in WebKit Created attachment 115896 [details]
Patch
Created attachment 115911 [details]
Patch
Created attachment 116008 [details]
Patch
Created attachment 117654 [details]
Patch
Created attachment 118717 [details]
Patch
Created attachment 127419 [details]
Prototype (WebKit)
This is a prototype for how to implement efficient painting directly to shared memory. This is just a proof of concept and is currently missing a framework for determining when shared memory can be reused without affecting pending updates.
Created attachment 127420 [details]
Prototype (Chromium)
Comment on attachment 127419 [details] Prototype (WebKit) View in context: https://bugs.webkit.org/attachment.cgi?id=127419&action=review it's not as scary as i imagined :) > Source/WebCore/platform/graphics/chromium/BitmapSkPictureCanvasLayerTextureUpdater.cpp:53 > + if (texture()->mapTransferBuffer()) think this should have width, height arguments? it could just assert on them being wrong even. it's scary to alloc and setPixels with no sizes in sight :) i like how simple this is though! > Source/WebCore/platform/graphics/chromium/BitmapSkPictureCanvasLayerTextureUpdater.cpp:70 > + helper->TexSubImage2D(GraphicsContext3D::TEXTURE_2D, 0, destRect.x(), destRect.y(), destRect.width(), destRect.height(), texture()->format(), GraphicsContext3D::UNSIGNED_BYTE, texture()->shmId(), 0, FALSE); this thing feels like a nice place for an abstraction.. textureUpdater()->updateSharedMemTextureRect() ? or maybe pass texture() to the textureUpdater()->updateTextureRect() and it can do this if{}else{} ? not sure other ideas.. (In reply to comment #14) > (From update of attachment 127419 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=127419&action=review > > it's not as scary as i imagined :) > > > Source/WebCore/platform/graphics/chromium/BitmapSkPictureCanvasLayerTextureUpdater.cpp:53 > > + if (texture()->mapTransferBuffer()) > > think this should have width, height arguments? it could just assert on them being wrong even. it's scary to alloc and setPixels with no sizes in sight :) I guess there could be an ASSERT there to check that sourceRect is less than texture dimensions. > > i like how simple this is though! > > > Source/WebCore/platform/graphics/chromium/BitmapSkPictureCanvasLayerTextureUpdater.cpp:70 > > + helper->TexSubImage2D(GraphicsContext3D::TEXTURE_2D, 0, destRect.x(), destRect.y(), destRect.width(), destRect.height(), texture()->format(), GraphicsContext3D::UNSIGNED_BYTE, texture()->shmId(), 0, FALSE); > > this thing feels like a nice place for an abstraction.. textureUpdater()->updateSharedMemTextureRect() ? > > or maybe pass texture() to the textureUpdater()->updateTextureRect() and it can do this if{}else{} ? This is all very temporary. Things will probably look a bit different in a real patch with a proper GraphicsContext3D API. Created attachment 128291 [details]
Prototype (WebKit)
Add decent memory management and GPU process coordination.
Created attachment 128292 [details]
Prototype (Chromium)
Created attachment 129061 [details]
Prototype (WebKit)
Rebase and threaded compositor support.
Created attachment 129062 [details]
Prototype (Chromium)
Rebase.
Created attachment 129496 [details]
Prototype (WebKit)
Created attachment 129497 [details]
Prototype (Chromium)
Created attachment 132906 [details]
Prototype (WebKit)
Created attachment 132907 [details]
Prototype (Chromium)
Created attachment 135196 [details]
Prototype (WebKit)
Created attachment 135198 [details]
Prototype (Chromium)
Hey David, I'm feeling a bit confused about this bug. I see 20 patches here all labeled prototype. Is there uncertainty about the approach or something? Net/net, how can I provide assistance in order to crisp up the plan and move from prototyping to review and landing? (In reply to comment #26) > Hey David, I'm feeling a bit confused about this bug. I see 20 patches here all labeled prototype. Sorry about the confusion. I've just been rebasing these patches as there's been some interest in trying them out. I haven't had time to do any real work on this lately but I'm hoping to make some progress here soon. Created attachment 135345 [details]
Prototype (WebKit)
just another rebase..
Created attachment 135346 [details]
Prototype (Chromium)
rebase
Created attachment 143867 [details] Patch Requires this chromium CL: http://codereview.chromium.org/10440019/ |