If SHM pixmaps support is available, for example, Intel drivers now support t...

If SHM pixmaps support is available, for example, Intel drivers now support that 
(http://cgit.freedesktop.org/xorg/driver/xf86-video-intel/commit/?id=4b7142baa0b3bf6f38843d06aadc579d8624cefc), 
use SHM pixmaps support to accelerate windowless plugin painting. 
Modify WindowlessPaint to directly use Xlib interfaces for SHM pixmaps support, 
similarly to the way how backing_store_x handles different SHM support levels provided by X server.

BUG=50912
TEST=Open the page "http://disney.go.com/official-sites/demi-lovato/albums" using Chromium browser,
compare the CPU usage of browser and X server before and after the change, and confirm CPU usage
is reduced with this change (for example, on an Atom N450 Netbook with MeeGo 1.0 and Chromium browser
6.0.417.0 there's >30% CPU usage reduction, especially X server CPU usage is reduced by half).

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=55020

[Evan Martin, 2010-08-03 07:37:26]

assigning reviewer based on a guess

[agl, 2010-08-03 16:15:16]

piman: hopefully you have a good understanding of what's going on here, but it
looks reasonable to me.

http://codereview.chromium.org/3052039/diff/5001/6003
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/3052039/diff/5001/6003#newcode34
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:34: #include
"app/x11_util_internal.h"
Should be in the main block of includes, above.

http://codereview.chromium.org/3052039/diff/5001/6003#newcode428
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:428: // create a shared
memory pixmap basing on the image,
s/basing/based/?

http://codereview.chromium.org/3052039/diff/5001/6003#newcode469
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:469: if (xgc)
XFreeGC(event.display, xgc);
The XFreeGC needs to be on a new line.

[piman, 2010-08-03 17:15:57]

http://codereview.chromium.org/3052039/diff/5001/6001
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/3052039/diff/5001/6001#newcode571
chrome/plugin/webplugin_proxy.cc:571:
delegate_->SetWindowlessBuffer(windowless_buffer);
Why not pass the windowless_dib_ directly instead of mapping it twice ?

http://codereview.chromium.org/3052039/diff/5001/6002
File webkit/glue/plugins/webplugin_delegate_impl.h (right):

http://codereview.chromium.org/3052039/diff/5001/6002#newcode19
webkit/glue/plugins/webplugin_delegate_impl.h:19: #include "base/scoped_ptr.h"
keep alpha order

http://codereview.chromium.org/3052039/diff/5001/6002#newcode307
webkit/glue/plugins/webplugin_delegate_impl.h:307: // transport DIB for
windowless plugin
transport->Transport
Maybe explain here that it's only used for SHM pixmap drawing.

http://codereview.chromium.org/3052039/diff/5001/6003
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/3052039/diff/5001/6003#newcode34
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:34: #include
"app/x11_util_internal.h"
On 2010/08/03 16:15:16, agl wrote:
> Should be in the main block of includes, above.

Actually I believe this one must be included last, like npapi_x11.h because they
include X headers that pollute the namespace.

http://codereview.chromium.org/3052039/diff/5001/6003#newcode415
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:415: if
(shared_memory_support_ == x11_util::SHARED_MEMORY_PIXMAP) {
Also check windowless_dib_, because in test_shell, it will be NULL.
Speaking of which, I'm not 100% sure it's ok to depend on chrome internal stuff
in the webkit glue. Make sure you build test_shell too. Maybe you will need to
create the SHM Pixmap in the WebPluginProxy, and pass that around instead of the
TransportDIB.

http://codereview.chromium.org/3052039/diff/5001/6003#newcode424
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:424: // make plugin_draw_rect
relative to the plugin window
make -> Make

http://codereview.chromium.org/3052039/diff/5001/6003#newcode431
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:431: DefaultDepth(display,
0));
The depth should be 32 explicitly. DefaultDepth(display, 0) is the depth of the
root window, unrelated to the transport dib's depth.

http://codereview.chromium.org/3052039/diff/5001/6003#newcode433
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:433: // in case the drawing
area does not start with the plugin window origin,
in -> In

http://codereview.chromium.org/3052039/diff/5001/6003#newcode440
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:440: DefaultDepth(display,
0));
Similarly, you'll need to force a depth of 32 here so that the GC is compatible
with both shm_pixmap and pixmap.

http://codereview.chromium.org/3052039/diff/5001/6003#newcode442
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:442: // copy the current
image into the pixmap, so the plugin can draw over it.
copy -> Copy

http://codereview.chromium.org/3052039/diff/5001/6003#newcode447
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:447: XSync(display, FALSE);
I don't believe this sync is necessary. The plugin shares the display with
chrome so the drawing it does will be sent after this copy and it should happen
in order. Also on this side, there's no synchronization needed for the SHM since
we know the bits are already here (the old code depends on it).

http://codereview.chromium.org/3052039/diff/5001/6003#newcode453
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:453: event.display = display;
we should share this between the 2 paths. It should be the same display in both
cases.

http://codereview.chromium.org/3052039/diff/5001/6003#newcode462
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:462: // copy the rendered
image pixmap back into the shm pixmap
copy->Copy

[darin (slow to review), 2010-08-03 19:00:00]

http://codereview.chromium.org/3052039/diff/5001/6003
File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):

http://codereview.chromium.org/3052039/diff/5001/6003#newcode415
webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:415: if
(shared_memory_support_ == x11_util::SHARED_MEMORY_PIXMAP) {
this is a scary long function.  mind breaking it up into meaningful units?

[yuqiang.xian, 2010-08-04 10:18:25]

On 2010/08/03 16:15:16, agl wrote:
> piman: hopefully you have a good understanding of what's going on here, but it
> looks reasonable to me.
> 
> http://codereview.chromium.org/3052039/diff/5001/6003
> File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):
> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode34
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:34: #include
> "app/x11_util_internal.h"
> Should be in the main block of includes, above.
The updated patch doesn't modify this piece of code anymore.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode428
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:428: // create a shared
> memory pixmap basing on the image,
> s/basing/based/?
Agree.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode469
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:469: if (xgc)
> XFreeGC(event.display, xgc);
> The XFreeGC needs to be on a new line.
Agree.

[yuqiang.xian, 2010-08-04 10:34:42]

On 2010/08/03 17:15:57, piman wrote:
> http://codereview.chromium.org/3052039/diff/5001/6001
> File chrome/plugin/webplugin_proxy.cc (right):
> 
> http://codereview.chromium.org/3052039/diff/5001/6001#newcode571
> chrome/plugin/webplugin_proxy.cc:571:
> delegate_->SetWindowlessBuffer(windowless_buffer);
> Why not pass the windowless_dib_ directly instead of mapping it twice ?
Yes. We can directly pass windowless_dib_. However we now directly pass the SHM
pixmap from webplugin proxy to delegate impl in the updated patch.

> 
> http://codereview.chromium.org/3052039/diff/5001/6002
> File webkit/glue/plugins/webplugin_delegate_impl.h (right):
> 
> http://codereview.chromium.org/3052039/diff/5001/6002#newcode19
> webkit/glue/plugins/webplugin_delegate_impl.h:19: #include "base/scoped_ptr.h"
> keep alpha order
This modification is not needed in the updated patch.

> 
> http://codereview.chromium.org/3052039/diff/5001/6002#newcode307
> webkit/glue/plugins/webplugin_delegate_impl.h:307: // transport DIB for
> windowless plugin
> transport->Transport
> Maybe explain here that it's only used for SHM pixmap drawing.
Modification is not needed in the updated patch.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003
> File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):
> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode34
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:34: #include
> "app/x11_util_internal.h"
> On 2010/08/03 16:15:16, agl wrote:
> > Should be in the main block of includes, above.
> 
> Actually I believe this one must be included last, like npapi_x11.h because
they
> include X headers that pollute the namespace.
Also, not needed in the updated patch.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode415
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:415: if
> (shared_memory_support_ == x11_util::SHARED_MEMORY_PIXMAP) {
> Also check windowless_dib_, because in test_shell, it will be NULL.
> Speaking of which, I'm not 100% sure it's ok to depend on chrome internal
stuff
> in the webkit glue. Make sure you build test_shell too. Maybe you will need to
> create the SHM Pixmap in the WebPluginProxy, and pass that around instead of
the
> TransportDIB.
test_shell is built without any problem using either the original patch or the
updated patch. Yes the updated patch creates SHM pixmap in WebPluginProxy and
pass that instead of TransportDIB to delegate impl.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode424
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:424: // make
plugin_draw_rect
> relative to the plugin window
> make -> Make
Agree.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode431
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:431: DefaultDepth(display,
> 0));
> The depth should be 32 explicitly. DefaultDepth(display, 0) is the depth of
the
> root window, unrelated to the transport dib's depth.
We have to use DefaultDepth to create the pixmap as the plugin instance (e.g.
the flash player) requires the pixmap to have the DefaultDepth. In many cases
the DefaultDepth is 24, but X server typically uses 32-bits to represent a pixel
with depth 24. We should also let this fall into the optimized SHM pixmap path
if the pixmap bpp and color masks match the image's. We enforce this check in
WebpluginProxy.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode433
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:433: // in case the drawing
> area does not start with the plugin window origin,
> in -> In
Agree.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode440
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:440: DefaultDepth(display,
> 0));
> Similarly, you'll need to force a depth of 32 here so that the GC is
compatible
> with both shm_pixmap and pixmap.
Similar to the comments above about the pixmap depth and bpp.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode442
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:442: // copy the current
> image into the pixmap, so the plugin can draw over it.
> copy -> Copy
Agree.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode447
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:447: XSync(display, FALSE);
> I don't believe this sync is necessary. The plugin shares the display with
> chrome so the drawing it does will be sent after this copy and it should
happen
> in order. Also on this side, there's no synchronization needed for the SHM
since
> we know the bits are already here (the old code depends on it).
Agree.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode453
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:453: event.display =
display;
> we should share this between the 2 paths. It should be the same display in
both
> cases.
Agree.

> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode462
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:462: // copy the rendered
> image pixmap back into the shm pixmap
> copy->Copy
Agree.

[yuqiang.xian, 2010-08-04 10:37:41]

On 2010/08/03 19:00:00, darin wrote:
> http://codereview.chromium.org/3052039/diff/5001/6003
> File webkit/glue/plugins/webplugin_delegate_impl_gtk.cc (right):
> 
> http://codereview.chromium.org/3052039/diff/5001/6003#newcode415
> webkit/glue/plugins/webplugin_delegate_impl_gtk.cc:415: if
> (shared_memory_support_ == x11_util::SHARED_MEMORY_PIXMAP) {
> this is a scary long function.  mind breaking it up into meaningful units?
This becomes shorter after moving the SHM pixmaps creating stuff to
WebPluginProxy in the updated patch. Thanks.

[piman, 2010-08-04 19:11:39]

http://codereview.chromium.org/3052039/diff/22001/1004
File chrome/plugin/webplugin_proxy.cc (right):

http://codereview.chromium.org/3052039/diff/22001/1004#newcode93
chrome/plugin/webplugin_proxy.cc:93: XFreePixmap(x11_util::GetXDisplay(),
windowless_shm_pixmap_);
If you have a delegate, reset its WindowlessShmPixmap to None here.

http://codereview.chromium.org/3052039/diff/22001/1004#newcode511
chrome/plugin/webplugin_proxy.cc:511: }
I think this block belongs to SetWindowlessBuffer. In any case, it should happen
before UpdateGeometry because that calls into the plugin which may trigger a
redraw, or at least another UpdateGeometry (see comment above).

[yuqiang.xian, 2010-08-05 01:07:33]

Thanks for the comments, piman.

On 2010/08/04 19:11:39, piman wrote:
> http://codereview.chromium.org/3052039/diff/22001/1004
> File chrome/plugin/webplugin_proxy.cc (right):
> 
> http://codereview.chromium.org/3052039/diff/22001/1004#newcode93
> chrome/plugin/webplugin_proxy.cc:93: XFreePixmap(x11_util::GetXDisplay(),
> windowless_shm_pixmap_);
> If you have a delegate, reset its WindowlessShmPixmap to None here.
The delegate should have already been deleted before the proxy being deleted,
according to the logic in WebPluginDelegateStub. So I think we don't need reset
it here and actually we can not (as the delegate is a dangling pointer now).

> 
> http://codereview.chromium.org/3052039/diff/22001/1004#newcode511
> chrome/plugin/webplugin_proxy.cc:511: }
> I think this block belongs to SetWindowlessBuffer. In any case, it should
happen
> before UpdateGeometry because that calls into the plugin which may trigger a
> redraw, or at least another UpdateGeometry (see comment above).
Agree, in the new patch.

Thanks.

[piman, 2010-08-05 01:09:28]

LGTM. I will push as soon as possible.