Use shared D3D9 texture to transport the compositor's backing buffer to the browser...

Use shared D3D9 texture to transport the compositor's backing buffer to the browser process for presentation.

Implemented ImageTransportSurface for Linux (without texture sharing), XP, Vista and 7. XP. The non-texture sharing Linux and XP paths just present directly to the compositing child window owned by the browser process as before.

PassThroughImageTransportSurface still needs a proper name. I will move it into its own file once that is decided.

I moved AcceleratedSurfaceBuffersSwapped outside of the platform specific ifdefs and made the signature the same on all platforms for greater consistency.

I removed the code related to sharing surfaces between processes and synchronizing resize and swapping out of GpuCommandBufferStub. It is all now in ImageTransportSurface implementations.
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110355

[apatrick_chromium, 2011-10-20 22:29:47]

This is going to take a few iterations but I think its ready to start the review
process. Are there any parts you would like me to farm out to other reviewers?

There are passing try runs on past patch sets. I'll start off a batch on this
most recent one as well...

[apatrick_chromium, 2011-10-20 22:30:27]

http://codereview.chromium.org/8060045/diff/48006/gpu/DEPS
File gpu/DEPS (left):

http://codereview.chromium.org/8060045/diff/48006/gpu/DEPS#oldcode2
gpu/DEPS:2: "+webkit/glue/plugins",
Don't know why this is in here. Will remove.

[apatrick_chromium, 2011-11-03 20:55:20]

I pared this patch back to the essentials and split out independent parts into
new patches, which are now landed. PTAL. The try jobs on #15 actually apply to
#16; I forgot to upload first. I haven't tested touch UI.

[jonathan.backer, 2011-11-03 23:09:07]

SG. Mostly nits.

http://codereview.chromium.org/8060045/diff/59001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_views.cc (right):

http://codereview.chromium.org/8060045/diff/59001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_views.cc:1181: int
gpu_host_id) {
NOTREACHED()

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view.h:198: // this case is
currently used for accelerated plugins.
nix the comment?

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:284: int
gpu_host_id) {
Bad merge? Diff shows edit in place below. Can we have the #else NOTREACHED()?

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_gtk.cc:1010: int
gpu_host_id) {
NOTREACHED()

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:221: 
isn't this in gpu_process_host_ui_shim.cc?

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1887: base::Closure
acknowledge_task =
Should be able to simplify with call to GpuProcessHostUIShim?

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/gpu_chan...
File content/common/gpu/gpu_channel_manager.cc (right):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/gpu_chan...
content/common/gpu/gpu_channel_manager.cc:126: CHECK(render_view_id);
You sure you want to CHECK and not DCHECK? Not sure why this was added to this
CL.

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/gpu_comm...
File content/common/gpu/gpu_command_buffer_stub.h (left):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/gpu_comm...
content/common/gpu/gpu_command_buffer_stub.h:126: uint64 GetSurfaceId();
Thanks for the cleanup. My bad...

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/gpu_mess...
File content/common/gpu/gpu_messages.h (left):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/gpu_mess...
content/common/gpu/gpu_messages.h:248: #if defined(OS_MACOSX) ||
defined(UI_COMPOSITOR_IMAGE_TRANSPORT)
I understand that this code compiles generally, but it is a little misleading
not to have it behind an #if defined because it is not used on most platforms.
You're putting the onus on the reader to find the IPC handler to discover
this...

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/gpu_mess...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/gpu_mess...
content/common/gpu/gpu_messages.h:146: IPC_MESSAGE_ROUTED0(GpuMsg_ResizeViewACK)
Naming is inconsistent with other IPCs handled by the ImageTransportSurface...

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface.h (right):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface.h:55: virtual
~ImageTransportSurface();
This was meant to be an interface so as not to break the style guidelines
regarding multiple inheritance. Why did you add these stubbed methods? For the
DISALLOW_COPY_AND_ASSIGN?

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface.h:140: : public
gfx::GLSurfaceAdapter,
I suspect that this is the only GLSurfaceAdapter that you need. Maybe merge the
two classes. You could also have it call CreateViewGLSurface directly.

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_linux.cc:376: void
EGLImageTransportSurface::OnResizeViewACK() {
NOTREACHED?

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_linux.cc:535: }
NOTREACHED()

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_linux.cc:617: 
NOTREACHED()

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_mac.cc (right):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_mac.cc:245: }
NOTREACHED()

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_mac.cc:452: }
NOTREACHED()

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_win.cc (right):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_win.cc:26: : public
gfx::GLSurfaceAdapter,
Why use this? Why not subclass the appropriate surface...

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_win.cc:126: TransportDIB::Handle
shm_handle) {
NOTIMPLEMENTED?

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_win.cc:129: void
PbufferImageTransportSurface::OnResizeViewACK() {
NOTIMPLEMENTED()?

[apatrick_chromium, 2011-11-07 20:40:04]

Sorry for rebasing. I think I addressed everything. Hopefully we can avoid a lot
of debate about GLSurfaceAdapter. I would rather remove it in another CL if you
feel strongly about it.

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:221: 
On 2011/11/03 23:09:07, jonathan.backer wrote:
> isn't this in gpu_process_host_ui_shim.cc?

That one sends to the GpuProcessHostUIShim rather than the GpuProcessHost.

http://codereview.chromium.org/8060045/diff/59001/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_win.cc:1887: base::Closure
acknowledge_task =
On 2011/11/03 23:09:07, jonathan.backer wrote:
> Should be able to simplify with call to GpuProcessHostUIShim?

This one's going direct to the IO thread.

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/gpu_mess...
File content/common/gpu/gpu_messages.h (left):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/gpu_mess...
content/common/gpu/gpu_messages.h:248: #if defined(OS_MACOSX) ||
defined(UI_COMPOSITOR_IMAGE_TRANSPORT)
On 2011/11/03 23:09:07, jonathan.backer wrote:
> I understand that this code compiles generally, but it is a little misleading
> not to have it behind an #if defined because it is not used on most platforms.
> You're putting the onus on the reader to find the IPC handler to discover
> this...

I guess the trade off is, if they aren't behind an ifdef, you don't need to send
jobs to trybots to see if they compile or not when you make changes.

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface.h (right):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface.h:55: virtual
~ImageTransportSurface();
On 2011/11/03 23:09:07, jonathan.backer wrote:
> This was meant to be an interface so as not to break the style guidelines
> regarding multiple inheritance. Why did you add these stubbed methods? For the
> DISALLOW_COPY_AND_ASSIGN?

The style guide actually requires that interface classes have a virtual
destructor and allows a no-arg constructor.

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface.h:140: : public
gfx::GLSurfaceAdapter,
On 2011/11/03 23:09:07, jonathan.backer wrote:
> I suspect that this is the only GLSurfaceAdapter that you need. Maybe merge
the
> two classes. You could also have it call CreateViewGLSurface directly.

I added GLSurfaceAdapter to avoid making the inheritance any deeper than it
already is. Adapter is a well-known pattern to avoid that. I was thinking of
going in the direction of having GLSurface implementations that derive only from
GLSurfaceAdapter and then chain them together at runtime. I think I can get rid
of a fair amount of redundant code in gfx/ui/gl that way.

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
File content/common/gpu/image_transport_surface_win.cc (right):

http://codereview.chromium.org/8060045/diff/59001/content/common/gpu/image_tr...
content/common/gpu/image_transport_surface_win.cc:26: : public
gfx::GLSurfaceAdapter,
On 2011/11/03 23:09:07, jonathan.backer wrote:
> Why use this? Why not subclass the appropriate surface...

See comment in image_transport_surface.cc

[jonathan.backer, 2011-11-07 22:47:41]

lgtm

[jonathan.backer, 2011-11-08 13:32:50]

On 2011/11/07 22:47:41, jonathan.backer wrote:
> lgtm

Works on touchui=1. Thanks for the wait.

[apatrick_chromium, 2011-11-08 20:42:27]

+darin for content/browser.

[apatrick_chromium, 2011-11-09 19:11:36]

On 2011/11/08 20:42:27, apatrick_chromium wrote:
> +darin for content/browser.

darin, ping :)

[apatrick_chromium, 2011-11-16 00:03:50]

ben, can you take a look at this for content/browser for owners? darin is busy.

[Ben Goodger (Google), 2011-11-16 16:35:54]

lgtm