Initial port of accelerated compositor to Mac OS X 10.6. Reused...

Initial port of accelerated compositor to Mac OS X 10.6. Reused
infrastructure added for Pepper 3D and Core Animation plugins to
render the compositor's output. The implementation allocates a fake
"plugin window handle" on the browser side which is the "root" handle,
containing the compositor's output, and which, if present, is drawn
before any other accelerated plugin instances.

Added messages from GPU process to browser process for handling window
resizing and presentation of output. Added support to GGL for "view"
contexts on Mac OS X, used only for the accelerated compositor, and
requiring explicit resize notifications.

The remainder of this port will go into the WebKit repository under
https://bugs.webkit.org/show_bug.cgi?id=43398 after this for
dependency reasons.

Tested manually with CSS 3D and WebGL demos. Several stability and
correctness issues remain and will be addressed in following CLs;
however, the current code works for the majority of basic use cases
including switching between accelerated compositing on and off, and
scrolling of content.

BUG=38969
TEST=none


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

[Ken Russell (switch to Gerrit), 2010-08-03 02:47:24]

Please review. Thanks.

[pink (ping after 24hrs), 2010-08-03 16:51:47]

drive-by review.

http://codereview.chromium.org/3067026/diff/25001/26002
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/3067026/diff/25001/26002#newcode16
chrome/browser/gpu_process_host.h:16: #include "chrome/common/gpu_messages.h"
can you forward-declare the needed types instead of including this?

http://codereview.chromium.org/3067026/diff/25001/26002#newcode94
chrome/browser/gpu_process_host.h:94: const
GpuHostMsg_AcceleratedSurfaceSetIOSurface_Params params);
pass by reference to avoid the struct copy?

http://codereview.chromium.org/3067026/diff/25001/26004
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h
(right):

http://codereview.chromium.org/3067026/diff/25001/26004#newcode85
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h:85:
AcceleratedSurfaceContainerMac* root_container_;
Who owns this object? Is this guaranteed to go away before the root does?

http://codereview.chromium.org/3067026/diff/25001/26009
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3067026/diff/25001/26009#newcode655
chrome/browser/renderer_host/render_widget_host_view_mac.mm:655: // Additionally
dirty the entire region of the window to make
window, or view?

http://codereview.chromium.org/3067026/diff/25001/26012
File chrome/common/gpu_messages.h (right):

http://codereview.chromium.org/3067026/diff/25001/26012#newcode25
chrome/common/gpu_messages.h:25: struct
GpuHostMsg_AcceleratedSurfaceSetIOSurface_Params {
provide a ctor to initialize these members?

http://codereview.chromium.org/3067026/diff/25001/26018
File chrome/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/3067026/diff/25001/26018#newcode76
chrome/gpu/gpu_command_buffer_stub.h:76: // The following two fields are used on
Mac OS X to identify the window
Should these be ifdef'd?

[apatrick, 2010-08-03 17:35:32]

http://codereview.chromium.org/3067026/diff/25001/26001
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/3067026/diff/25001/26001#newcode170
chrome/browser/gpu_process_host.cc:170: #endif
nit: #elif

http://codereview.chromium.org/3067026/diff/25001/26001#newcode206
chrome/browser/gpu_process_host.cc:206: #endif
nit: #elif

http://codereview.chromium.org/3067026/diff/25001/26002
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/3067026/diff/25001/26002#newcode91
chrome/browser/gpu_process_host.h:91: #endif
nit: #elif

http://codereview.chromium.org/3067026/diff/25001/26003
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc
(right):

http://codereview.chromium.org/3067026/diff/25001/26003#newcode35
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:35:
root_container_ = NULL;
root_container_ looks like it could be a WeakPtr. If you did that some of the
special cases for root_container_ would go away.

http://codereview.chromium.org/3067026/diff/25001/26003#newcode121
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:121:
if (container != root_container_) {
At some point in the future, is the plan to integrate plugin draing with the
compositor so they can be transformed and blended with the rest of the page
rather than always on top? If so, can you put a big comment in the header file
explaining what a root container is in relation to the plugin containers and why
it is temporary in nature?

http://codereview.chromium.org/3067026/diff/25001/26013
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/3067026/diff/25001/26013#newcode106
chrome/common/gpu_messages_internal.h:106: #endif
nit: #elif

http://codereview.chromium.org/3067026/diff/25001/26015
File chrome/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/3067026/diff/25001/26015#newcode128
chrome/gpu/gpu_channel.cc:128: #elif defined(OS_MACOSX)
Not for this changelist: I would like to see some kind of platform neutral
abstraction for identifying the target of a view command buffer.

http://codereview.chromium.org/3067026/diff/25001/26017
File chrome/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/3067026/diff/25001/26017#newcode182
chrome/gpu/gpu_command_buffer_stub.cc:182: NOTREACHED();
What will 10.5 do if the compositor is enabled? Is there code to prevent the
compositor from being enabled on 10.5?

http://codereview.chromium.org/3067026/diff/25001/26017#newcode188
chrome/gpu/gpu_command_buffer_stub.cc:188: gpu_thread->Send(
Should this happen on 10.5?

http://codereview.chromium.org/3067026/diff/25001/26019
File chrome/renderer/ggl/ggl.cc (right):

http://codereview.chromium.org/3067026/diff/25001/26019#newcode205
chrome/renderer/ggl/ggl.cc:205: command_buffer_->SetWindowSize(size);
SetWindowSize and ResizeOffscreenFrameBuffer have the same signature. Can they
be combined?

http://codereview.chromium.org/3067026/diff/25001/26020
File chrome/renderer/ggl/ggl.h (right):

http://codereview.chromium.org/3067026/diff/25001/26020#newcode55
chrome/renderer/ggl/ggl.h:55: int render_view_id);
It's awfully confusing identifying the view with two different IDs.

http://codereview.chromium.org/3067026/diff/25001/26020#newcode61
chrome/renderer/ggl/ggl.h:61: void ResizeOnscreenContext(Context* context, const
gfx::Size& size);
Can we do it this way on all platforms for consistency? Can we then combine
ResizeOnscreen and ResizeOffscreen into a generic Resize function?

http://codereview.chromium.org/3067026/diff/25001/26026
File chrome/renderer/webgles2context_impl.h (right):

http://codereview.chromium.org/3067026/diff/25001/26026#newcode31
chrome/renderer/webgles2context_impl.h:31: virtual void
resizeOnscreenContent(const WebKit::WebSize&);
Same signature as resizeOffscreenContent. Can it be combined?

[Ken Russell (switch to Gerrit), 2010-08-04 00:24:36]

http://codereview.chromium.org/3067026/diff/25001/26001
File chrome/browser/gpu_process_host.cc (right):

http://codereview.chromium.org/3067026/diff/25001/26001#newcode170
chrome/browser/gpu_process_host.cc:170: #endif
On 2010/08/03 17:35:32, apatrick wrote:
> nit: #elif

Done.

http://codereview.chromium.org/3067026/diff/25001/26001#newcode206
chrome/browser/gpu_process_host.cc:206: #endif
On 2010/08/03 17:35:32, apatrick wrote:
> nit: #elif

Done.

http://codereview.chromium.org/3067026/diff/25001/26002
File chrome/browser/gpu_process_host.h (right):

http://codereview.chromium.org/3067026/diff/25001/26002#newcode16
chrome/browser/gpu_process_host.h:16: #include "chrome/common/gpu_messages.h"
On 2010/08/03 16:51:47, pink wrote:
> can you forward-declare the needed types instead of including this?

Done.

http://codereview.chromium.org/3067026/diff/25001/26002#newcode91
chrome/browser/gpu_process_host.h:91: #endif
On 2010/08/03 17:35:32, apatrick wrote:
> nit: #elif

Done.

http://codereview.chromium.org/3067026/diff/25001/26002#newcode94
chrome/browser/gpu_process_host.h:94: const
GpuHostMsg_AcceleratedSurfaceSetIOSurface_Params params);
On 2010/08/03 16:51:47, pink wrote:
> pass by reference to avoid the struct copy?

Done. (That was just a typo.)

http://codereview.chromium.org/3067026/diff/25001/26003
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc
(right):

http://codereview.chromium.org/3067026/diff/25001/26003#newcode35
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:35:
root_container_ = NULL;
On 2010/08/03 17:35:32, apatrick wrote:
> root_container_ looks like it could be a WeakPtr. If you did that some of the
> special cases for root_container_ would go away.

Thanks for the suggestion. These objects' lifetimes are managed differently so
I'm going to leave this as is for now.

http://codereview.chromium.org/3067026/diff/25001/26003#newcode121
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.cc:121:
if (container != root_container_) {
On 2010/08/03 17:35:32, apatrick wrote:
> At some point in the future, is the plan to integrate plugin draing with the
> compositor so they can be transformed and blended with the rest of the page
> rather than always on top? If so, can you put a big comment in the header file
> explaining what a root container is in relation to the plugin containers and
why
> it is temporary in nature?

Yes, the root container concept will hopefully go away at some point. I've added
more comments to the header indicating its purpose.

http://codereview.chromium.org/3067026/diff/25001/26004
File chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h
(right):

http://codereview.chromium.org/3067026/diff/25001/26004#newcode85
chrome/browser/renderer_host/accelerated_surface_container_manager_mac.h:85:
AcceleratedSurfaceContainerMac* root_container_;
On 2010/08/03 16:51:47, pink wrote:
> Who owns this object? Is this guaranteed to go away before the root does?

It's owned by this AcceleratedSurfaceContainerManagerMac identically to the
non-root containers. It's only separated out for easy access and identification.

http://codereview.chromium.org/3067026/diff/25001/26009
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/3067026/diff/25001/26009#newcode655
chrome/browser/renderer_host/render_widget_host_view_mac.mm:655: // Additionally
dirty the entire region of the window to make
On 2010/08/03 16:51:47, pink wrote:
> window, or view?

You're right, view. Changed.

http://codereview.chromium.org/3067026/diff/25001/26012
File chrome/common/gpu_messages.h (right):

http://codereview.chromium.org/3067026/diff/25001/26012#newcode25
chrome/common/gpu_messages.h:25: struct
GpuHostMsg_AcceleratedSurfaceSetIOSurface_Params {
On 2010/08/03 16:51:47, pink wrote:
> provide a ctor to initialize these members?

Done.

http://codereview.chromium.org/3067026/diff/25001/26013
File chrome/common/gpu_messages_internal.h (right):

http://codereview.chromium.org/3067026/diff/25001/26013#newcode106
chrome/common/gpu_messages_internal.h:106: #endif
On 2010/08/03 17:35:32, apatrick wrote:
> nit: #elif

Done.

http://codereview.chromium.org/3067026/diff/25001/26015
File chrome/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/3067026/diff/25001/26015#newcode128
chrome/gpu/gpu_channel.cc:128: #elif defined(OS_MACOSX)
On 2010/08/03 17:35:32, apatrick wrote:
> Not for this changelist: I would like to see some kind of platform neutral
> abstraction for identifying the target of a view command buffer.

Agreed.

http://codereview.chromium.org/3067026/diff/25001/26017
File chrome/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/3067026/diff/25001/26017#newcode182
chrome/gpu/gpu_command_buffer_stub.cc:182: NOTREACHED();
On 2010/08/03 17:35:32, apatrick wrote:
> What will 10.5 do if the compositor is enabled? Is there code to prevent the
> compositor from being enabled on 10.5?

Vangelis indicates that we need to add more code to the Chromium port of this
functionality up in the WebKit repository to determine whether we can correctly
enable the compositor. Having the NOTREACHED() here is best to have a clear
failure mode.

http://codereview.chromium.org/3067026/diff/25001/26017#newcode188
chrome/gpu/gpu_command_buffer_stub.cc:188: gpu_thread->Send(
On 2010/08/03 17:35:32, apatrick wrote:
> Should this happen on 10.5?

It will only be triggered if a "view" context is created, i.e., the
PluginWindowHandle passed in the constructor is non-zero. Currently this will
only happen for the accelerated compositor, so it is okay for this code to be
unconditional.

http://codereview.chromium.org/3067026/diff/25001/26018
File chrome/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/3067026/diff/25001/26018#newcode76
chrome/gpu/gpu_command_buffer_stub.h:76: // The following two fields are used on
Mac OS X to identify the window
On 2010/08/03 16:51:47, pink wrote:
> Should these be ifdef'd?

No. I don't want the constructor's invocation to be conditionalized per
platform.

http://codereview.chromium.org/3067026/diff/25001/26019
File chrome/renderer/ggl/ggl.cc (right):

http://codereview.chromium.org/3067026/diff/25001/26019#newcode205
chrome/renderer/ggl/ggl.cc:205: command_buffer_->SetWindowSize(size);
On 2010/08/03 17:35:32, apatrick wrote:
> SetWindowSize and ResizeOffscreenFrameBuffer have the same signature. Can they
> be combined?

Per my comment above I really do not want to combine these yet because they
behave very differently on the Mac. We can try combining them once more issues
in this overall code have been debugged.

http://codereview.chromium.org/3067026/diff/25001/26020
File chrome/renderer/ggl/ggl.h (right):

http://codereview.chromium.org/3067026/diff/25001/26020#newcode55
chrome/renderer/ggl/ggl.h:55: int render_view_id);
On 2010/08/03 17:35:32, apatrick wrote:
> It's awfully confusing identifying the view with two different IDs.

Yes, and I'm sorry about that, but I've left the TODO there for this purpose. I
would like to check in something that works and then iterate on it.

http://codereview.chromium.org/3067026/diff/25001/26020#newcode61
chrome/renderer/ggl/ggl.h:61: void ResizeOnscreenContext(Context* context, const
gfx::Size& size);
On 2010/08/03 17:35:32, apatrick wrote:
> Can we do it this way on all platforms for consistency? Can we then combine
> ResizeOnscreen and ResizeOffscreen into a generic Resize function?

Perhaps, but I would like to defer that to another CL. Windows and Linux do not
require any explicit resize notifications for on-screen contexts. On-screen and
off-screen contexts behave quite differently on the Mac. I would like this code
to settle while we fix other issues and then think about how it can be
refactored.

http://codereview.chromium.org/3067026/diff/25001/26026
File chrome/renderer/webgles2context_impl.h (right):

http://codereview.chromium.org/3067026/diff/25001/26026#newcode31
chrome/renderer/webgles2context_impl.h:31: virtual void
resizeOnscreenContent(const WebKit::WebSize&);
On 2010/08/03 17:35:32, apatrick wrote:
> Same signature as resizeOffscreenContent. Can it be combined?

Same answer as above.

[apatrick, 2010-08-04 01:23:47]

LGTM