Bug 72191 - [Chromium] Add support for painting directly to shared memory.
Summary: [Chromium] Add support for painting directly to shared memory.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Reveman
URL:
Keywords:
: 71023 (view as bug list)
Depends on: 71869
Blocks:
  Show dependency treegraph
 
Reported: 2011-11-11 15:16 PST by David Reveman
Modified: 2013-04-12 07:21 PDT (History)
7 users (show)

See Also:


Attachments
Patch (6.74 KB, patch)
2011-11-11 15:31 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (6.75 KB, patch)
2011-11-15 12:41 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (5.83 KB, patch)
2011-11-18 15:53 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (5.81 KB, patch)
2011-11-18 17:04 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2011-11-20 16:37 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2011-12-02 10:55 PST, David Reveman
no flags Details | Formatted Diff | Diff
Patch (5.84 KB, patch)
2011-12-11 16:33 PST, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (WebKit) (15.87 KB, patch)
2012-02-16 11:44 PST, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (Chromium) (2.64 KB, patch)
2012-02-16 11:45 PST, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (WebKit) (25.00 KB, patch)
2012-02-22 14:34 PST, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (Chromium) (3.97 KB, patch)
2012-02-22 14:35 PST, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (WebKit) (26.47 KB, patch)
2012-02-27 10:08 PST, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (Chromium) (3.97 KB, patch)
2012-02-27 10:09 PST, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (WebKit) (26.52 KB, patch)
2012-02-29 12:58 PST, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (Chromium) (4.00 KB, patch)
2012-02-29 12:58 PST, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (WebKit) (25.39 KB, patch)
2012-03-20 14:57 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (Chromium) (3.93 KB, patch)
2012-03-20 14:58 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (WebKit) (4.81 KB, patch)
2012-04-02 14:49 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (Chromium) (3.96 KB, patch)
2012-04-02 14:50 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (WebKit) (4.81 KB, patch)
2012-04-03 09:46 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Prototype (Chromium) (3.96 KB, patch)
2012-04-03 09:47 PDT, David Reveman
no flags Details | Formatted Diff | Diff
Patch (38.35 KB, patch)
2012-05-24 12:00 PDT, David Reveman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Reveman 2011-11-11 15:16:18 PST
Map shared memory buffers prior to painting tile-sized chunks using SkPicture. Allows us to paint directly to shared memory instead of temporary buffers.
Comment 1 David Reveman 2011-11-11 15:26:17 PST
*** Bug 71023 has been marked as a duplicate of this bug. ***
Comment 2 David Reveman 2011-11-11 15:31:19 PST
Created attachment 114788 [details]
Patch
Comment 3 James Robinson 2011-11-11 15:42:45 PST
Do we know if this is actually a win?
Comment 4 David Reveman 2011-11-12 17:30:54 PST
(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.
Comment 5 David Reveman 2011-11-15 12:41:03 PST
Created attachment 115222 [details]
Patch
Comment 6 Adrienne Walker 2011-11-16 15:37:50 PST
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
Comment 7 David Reveman 2011-11-18 15:53:16 PST
Created attachment 115896 [details]
Patch
Comment 8 David Reveman 2011-11-18 17:04:20 PST
Created attachment 115911 [details]
Patch
Comment 9 David Reveman 2011-11-20 16:37:27 PST
Created attachment 116008 [details]
Patch
Comment 10 David Reveman 2011-12-02 10:55:38 PST
Created attachment 117654 [details]
Patch
Comment 11 David Reveman 2011-12-11 16:33:37 PST
Created attachment 118717 [details]
Patch
Comment 12 David Reveman 2012-02-16 11:44:47 PST
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.
Comment 13 David Reveman 2012-02-16 11:45:20 PST
Created attachment 127420 [details]
Prototype (Chromium)
Comment 14 Dana Jansens 2012-02-16 12:15:43 PST
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..
Comment 15 David Reveman 2012-02-16 12:25:03 PST
(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.
Comment 16 David Reveman 2012-02-22 14:34:19 PST
Created attachment 128291 [details]
Prototype (WebKit)

Add decent memory management and GPU process coordination.
Comment 17 David Reveman 2012-02-22 14:35:21 PST
Created attachment 128292 [details]
Prototype (Chromium)
Comment 18 David Reveman 2012-02-27 10:08:15 PST
Created attachment 129061 [details]
Prototype (WebKit)

Rebase and threaded compositor support.
Comment 19 David Reveman 2012-02-27 10:09:59 PST
Created attachment 129062 [details]
Prototype (Chromium)

Rebase.
Comment 20 David Reveman 2012-02-29 12:58:10 PST
Created attachment 129496 [details]
Prototype (WebKit)
Comment 21 David Reveman 2012-02-29 12:58:43 PST
Created attachment 129497 [details]
Prototype (Chromium)
Comment 22 David Reveman 2012-03-20 14:57:39 PDT
Created attachment 132906 [details]
Prototype (WebKit)
Comment 23 David Reveman 2012-03-20 14:58:23 PDT
Created attachment 132907 [details]
Prototype (Chromium)
Comment 24 David Reveman 2012-04-02 14:49:43 PDT
Created attachment 135196 [details]
Prototype (WebKit)
Comment 25 David Reveman 2012-04-02 14:50:33 PDT
Created attachment 135198 [details]
Prototype (Chromium)
Comment 26 Nat Duca 2012-04-02 15:32:15 PDT
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?
Comment 27 David Reveman 2012-04-03 00:33:11 PDT
(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.
Comment 28 David Reveman 2012-04-03 09:46:36 PDT
Created attachment 135345 [details]
Prototype (WebKit)

just another rebase..
Comment 29 David Reveman 2012-04-03 09:47:15 PDT
Created attachment 135346 [details]
Prototype (Chromium)

rebase
Comment 30 David Reveman 2012-05-24 12:00:04 PDT
Created attachment 143867 [details]
Patch

Requires this chromium CL: http://codereview.chromium.org/10440019/