Integrated Pepper3D v2 with the accelerated compositor....

Integrated Pepper3D v2 with the accelerated compositor.
- It now uses GGL instead of a child window.
- Fixed a bug where GLES2 decoder set texture state without updating TextureInfo.
- Fixed Pepper3D demos and added awesome 3D CSS programmer art.
- Removed ggl::GetCurrentContext to prevent further abuse. Fixed said abuse :)
- GGL exposes GLES2Implementation to allow issue of GL calls on a particular context without making that context globally current.
- Removed redundant "this context" argument from GGL SwapBuffers completion callback.
- Temporarily removed context lost notification. I need to figure out the best semantics.
TEST=Run Pepper3D and WebGL demos and YouTube videos on Windows and Mac.
BUG=none


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

[apatrick_chromium, 2010-10-20 20:08:07]

WebKit side to follow. Please review. Thanks.

[Vangelis Kokkevis, 2010-10-21 21:59:39]

Looks good in general, although I'm not that familiar with that codebase.  Some
minor comments inline..

http://codereview.chromium.org/3531008/diff/31002/24004
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/3531008/diff/31002/24004#newcode192
chrome/renderer/pepper_plugin_delegate_impl.cc:192: ggl::Context* parent_context
= context->context();
need to guard against a NULL context

http://codereview.chromium.org/3531008/diff/31002/24005
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3531008/diff/31002/24005#newcode2502
chrome/renderer/render_view.cc:2502: DCHECK(context);
Is context always guaranteed to be != NULL?  What if there's no driver support?

http://codereview.chromium.org/3531008/diff/31002/24012
File webkit/glue/plugins/pepper_graphics_3d.h (right):

http://codereview.chromium.org/3531008/diff/31002/24012#newcode51
webkit/glue/plugins/pepper_graphics_3d.h:51: // Assciates this device with the
given plugin instance. You can pass NULL to
typo: Associates

http://codereview.chromium.org/3531008/diff/31002/24015
File webkit/glue/plugins/pepper_plugin_instance.h (right):

http://codereview.chromium.org/3531008/diff/31002/24015#newcode200
webkit/glue/plugins/pepper_plugin_instance.h:200: // Get the bound graphics
context as a concrete @D graphics context or returns
typo: 2D

[apatrick_chromium, 2010-10-22 19:39:38]

Issues addressed. Any thoughts Darin?

http://codereview.chromium.org/3531008/diff/31002/24004
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/3531008/diff/31002/24004#newcode192
chrome/renderer/pepper_plugin_delegate_impl.cc:192: ggl::Context* parent_context
= context->context();
On 2010/10/21 21:59:39, Vangelis Kokkevis wrote:
> need to guard against a NULL context

Done.

http://codereview.chromium.org/3531008/diff/31002/24005
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/3531008/diff/31002/24005#newcode2502
chrome/renderer/render_view.cc:2502: DCHECK(context);
On 2010/10/21 21:59:39, Vangelis Kokkevis wrote:
> Is context always guaranteed to be != NULL?  What if there's no driver
support? 

The code above assumes its non-null. I'm not sure if that is a good assumption
though. I cleaned up a little and left a note for alpha about context lost.

http://codereview.chromium.org/3531008/diff/31002/24012
File webkit/glue/plugins/pepper_graphics_3d.h (right):

http://codereview.chromium.org/3531008/diff/31002/24012#newcode51
webkit/glue/plugins/pepper_graphics_3d.h:51: // Assciates this device with the
given plugin instance. You can pass NULL to
On 2010/10/21 21:59:39, Vangelis Kokkevis wrote:
> typo: Associates

Done.

http://codereview.chromium.org/3531008/diff/31002/24015
File webkit/glue/plugins/pepper_plugin_instance.h (right):

http://codereview.chromium.org/3531008/diff/31002/24015#newcode200
webkit/glue/plugins/pepper_plugin_instance.h:200: // Get the bound graphics
context as a concrete @D graphics context or returns
On 2010/10/21 21:59:39, Vangelis Kokkevis wrote:
> typo: 2D

Done.

[Ken Russell (switch to Gerrit), 2010-10-26 22:11:54]

This looks good to me. One minor code comment and two test questions.

Please run the compositor and WebGL on Windows and Mac OS X after these changes.

http://codereview.chromium.org/3531008/diff/71001/72003
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/3531008/diff/71001/72003#newcode187
chrome/renderer/pepper_plugin_delegate_impl.cc:187: return false;
This block of code is already contained in
WebGraphicsContext3DCommandBufferImpl::initialize(), which will be called by the
block of code below. It isn't necessary here.

http://codereview.chromium.org/3531008/diff/71001/72009
File gpu/demos/pepper_gpu_demo.html (right):

http://codereview.chromium.org/3531008/diff/71001/72009#newcode6
gpu/demos/pepper_gpu_demo.html:6: <body style="background-color:Silver;
-webkit-perspective: 1000;">
Did you mean to leave this here?

http://codereview.chromium.org/3531008/diff/71001/72009#newcode9
gpu/demos/pepper_gpu_demo.html:9: <embed id="plugin"
type="pepper-application/x-gpu-demo" width="512" height="512"
style="position:absolute; top: 30px; -webkit-transform: rotateY(30deg);" />
And the -webkit-transform?

[apatrick_chromium, 2010-11-04 00:24:51]

All done I think. Tested on mac and windows with the changes committed to
webkit. With this landed Pepper 3D v2 should be (barely) functional and suitable
for handing over to a new owner.

Ken, the changes to the Pepper 3D demos are awesome 3D CSS programmer art.

Darin, can you check the PPAPI changes. I'm running a try job to see if this
breaks anything in chrome but I don't know if it will affect other PPAPI
dependents. I only changed the C++ interface. C interface is unchanged.

[Ken Russell (switch to Gerrit), 2010-11-04 00:50:49]

http://codereview.chromium.org/3531008/diff/92001/93004
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/3531008/diff/92001/93004#newcode189
chrome/renderer/pepper_plugin_delegate_impl.cc:189: DCHECK(host->state() ==
GpuChannelHost::CONNECTED);
The entire block of code from "RenderThread* render_thread = ..." to the DCHECK
above is already contained in WebGraphics3DCommandBufferImpl::initialize(),
which is called from WebKit::WebView::graphicsContext3D() below. I think you
should remove it.

[Ken Russell (switch to Gerrit), 2010-11-04 01:10:38]

http://codereview.chromium.org/3531008/diff/92001/93004
File chrome/renderer/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/3531008/diff/92001/93004#newcode189
chrome/renderer/pepper_plugin_delegate_impl.cc:189: DCHECK(host->state() ==
GpuChannelHost::CONNECTED);
On 2010/11/04 00:50:49, kbr wrote:
> The entire block of code from "RenderThread* render_thread = ..." to the
DCHECK
> above is already contained in WebGraphics3DCommandBufferImpl::initialize(),
> which is called from WebKit::WebView::graphicsContext3D() below. I think you
> should remove it.

Per our offline discussion, since host is needed for ggl::CreateOffscreenContext
(a point I missed), moving the call to WebView::graphicsContext3D() higher in
this function is a better idea since it will cause the GpuChannelHost to be
initialized.

[apatrick_chromium, 2010-11-04 01:35:41]

Done

[Ken Russell (switch to Gerrit), 2010-11-04 01:37:50]

LGTM

[darin (slow to review), 2010-11-04 19:31:23]

http://codereview.chromium.org/3531008/diff/121001/122012
File ppapi/cpp/instance.h (right):

http://codereview.chromium.org/3531008/diff/121001/122012#newcode90
ppapi/cpp/instance.h:90: bool BindGraphics(const PP_Resource& graphics);
at least, this should be BindGraphics(const Resource&) so that we're using the
C++ type.  but, maybe... we should instead overload BindGraphics to take both
Graphics2D and Graphics3D to be type-safe.

[apatrick_chromium, 2010-11-04 20:34:44]

I spoke with brettw and we decided to go with overloading BindGraphics. Being
one of the c++ interface, we can add additional overloads in the future without
breaking binary compatibility.

http://codereview.chromium.org/3531008/diff/121001/122012
File ppapi/cpp/instance.h (right):

http://codereview.chromium.org/3531008/diff/121001/122012#newcode90
ppapi/cpp/instance.h:90: bool BindGraphics(const PP_Resource& graphics);
On 2010/11/04 19:31:24, darin wrote:
> at least, this should be BindGraphics(const Resource&) so that we're using
the
> C++ type.  but, maybe... we should instead overload BindGraphics to take both
> Graphics2D and Graphics3D to be type-safe.

Done.

[darin (slow to review), 2010-11-04 20:35:49]

LGTM for the ppapi changes

[jam, 2010-11-15 21:24:19]

http://codereview.chromium.org/3531008/diff/146001/webkit/glue/plugins/pepper...
File webkit/glue/plugins/pepper_plugin_instance.cc (right):

http://codereview.chromium.org/3531008/diff/146001/webkit/glue/plugins/pepper...
webkit/glue/plugins/pepper_plugin_instance.cc:430: bound_graphics_ =
graphics_2d;
moving this line up makes the code below it (434-449) useless, since the old
image isn't used as a new background.  is there a specific reason this was made?
 it's causing a nasty regression with pdfs when the browser window is resized.

[apatrick_chromium, 2010-11-15 22:24:53]

Thanks for the catch.

http://codereview.chromium.org/3531008/diff/146001/webkit/glue/plugins/pepper...
File webkit/glue/plugins/pepper_plugin_instance.cc (right):

http://codereview.chromium.org/3531008/diff/146001/webkit/glue/plugins/pepper...
webkit/glue/plugins/pepper_plugin_instance.cc:430: bound_graphics_ =
graphics_2d;
No that looks like a bad refactor. Are you going to fix or shall I?