Implement OSMesa image transport for TOUCH_UI builds

Implements OSMesaImageTransportSurface in the GPU for sending GLSurface from the GPU to the browser.
Implements AcceleratedSurfaceContainerTouchOSMesa in the browser's renderer for receiving a GLSurface contents and building a GLTexture in the browser for compositing.

BUG=
TEST=./out/Debug/chrome --use-gl=osmesa --show-fps-counter

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

[jonathan.backer, 2011-09-20 15:16:29]

http://codereview.chromium.org/7980006/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/accelerated_surface_container_touch.h (right):

http://codereview.chromium.org/7980006/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.h:39: uint64
id_;
Nuke this?

http://codereview.chromium.org/7980006/diff/2001/content/common/gpu/image_tra...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/7980006/diff/2001/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface_linux.cc:557:
glBindTexture(GL_TEXTURE_2D, texture_);
I think you can get rid of a bit of code duplication by calling OnResize(): It
has to glTexImage2D the texture size and rebind to the FBO
(glFramebufferTexture2DEXT) anyway.

[danakj, 2011-09-21 17:45:22]

On 2011/09/20 15:16:29, jonathan.backer wrote:
>
http://codereview.chromium.org/7980006/diff/2001/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/accelerated_surface_container_touch.h
(right):
> 
>
http://codereview.chromium.org/7980006/diff/2001/chrome/browser/renderer_host...
> chrome/browser/renderer_host/accelerated_surface_container_touch.h:39: uint64
> id_;
> Nuke this?

Done.
 
>
http://codereview.chromium.org/7980006/diff/2001/content/common/gpu/image_tra...
> File content/common/gpu/image_transport_surface_linux.cc (right):
> 
>
http://codereview.chromium.org/7980006/diff/2001/content/common/gpu/image_tra...
> content/common/gpu/image_transport_surface_linux.cc:557:
> glBindTexture(GL_TEXTURE_2D, texture_);
> I think you can get rid of a bit of code duplication by calling OnResize(): It
> has to glTexImage2D the texture size and rebind to the FBO
> (glFramebufferTexture2DEXT) anyway.

No longer relevant, the OSMesa stuff in the image transport doesn't need any GL
textures.

[danakj, 2011-09-21 18:18:57]

Currently IPC messages are as follows:

SurfaceNew(identifier) -> To notify/request for a new surface
SurfaceNewACK(identifier, handle) -> To ACK and give back a surface id and
shared memory handle
 - The id given back is the same as passed in SurfaceNew for GLX/EGL
implementations
 - The id given back is generated on browser side for OSMesa and given  back
here
 - The handle is empty for GLX/EGL implementations as they don't use shm.  It is
a valid handle for OSMesa.

The id given back in the OSMesa case is used for identifying the buffer in other
messages such as SwapBuffers and Release.

Option 1: Leave as is

Option 2: Pass back a union for different implementations in the ACK
union {
  struct glx {
    id
  }
  struct egl {
    id
  }
  struct osmesa {
    id
    handle
  }
}

Option 3: Use the handle for identifying everything browser side in OSMesa case,
and use a union for the ACK. Change Release and SwapBuffers to use this union
also.
union {
  id  // for glx and egl
  handle  // for osmesa
}

Option 4: ?

[jonathan.backer, 2011-09-21 20:27:36]

Mostly nits. It's clear to me you need to compile on Mac. For next round, let's
add Al. It's looking pretty good.

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
File chrome/browser/renderer_host/accelerated_surface_container_touch.cc
(right):

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.cc:163: //
created in the same process as the texture.
Thanks.

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
File chrome/browser/renderer_host/accelerated_surface_container_touch.h (right):

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.h:34: virtual
TransportDIB::Handle handle() const;
Maybe specify the return value in the comment, if not implemented.

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:881: void
RenderWidgetHostViewMac::AcceleratedSurfaceNew(
I'm not sure it makes sense to rename this yet. Currently, it's always an
IOSurface -- and it's being initialized in the GPU process.

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_views_touch.cc
(right):

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_views_touch.cc:208:
surface->Initialize(surface_id);
Do you want to do anything with the return value?

http://codereview.chromium.org/7980006/diff/5017/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/7980006/diff/5017/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host_ui_shim.cc:247:
view->AcceleratedSurfaceSetIOSurface(params.window,
Renamed in one place (noted earlier), but not here. Prefer you didn't rename for
the moment.

http://codereview.chromium.org/7980006/diff/5017/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host_ui_shim.cc:255: surface_id =
params.identifier;
nit: init on the decl?

http://codereview.chromium.org/7980006/diff/5017/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_mac.cc (right):

http://codereview.chromium.org/7980006/diff/5017/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_mac.cc:40: void
RenderWidgetHost::OnAcceleratedSurfaceNew(
I would be careful of this. It may have more than one entry point (say from a
renderer instead of a GPU process).

http://codereview.chromium.org/7980006/diff/5017/content/common/gpu/gpu_messa...
File content/common/gpu/gpu_messages.h (right):

http://codereview.chromium.org/7980006/diff/5017/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:257: // browser side.
Update comment.

http://codereview.chromium.org/7980006/diff/5017/content/common/gpu/image_tra...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/7980006/diff/5017/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface_linux.cc:75: // read from via
XComposite
Thanks.

http://codereview.chromium.org/7980006/diff/5017/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface_linux.cc:276: uint64 surface_id,
TransportDIB::Handle /*surface_handle*/) {
Comment out param name? Haven't seen this before.

[danakj, 2011-09-21 20:58:19]

If we don't rename the MACOSX IPC then I need to duplicate some code inside
MACOSX || TOUCHUI blocks, to have both a MACOSX and TOUCHUI version.

This may be desirable but I want to make sure that's your intent.

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:881: void
RenderWidgetHostViewMac::AcceleratedSurfaceNew(
To have a different name means a number of places need to be copied that are
currently OS_MACOSX || TOUCH_UI.  Unless the touch one remains SetIOSurface (I
am against that).

For instance: gpu_messages.h, and
content/browser/gpu/gpu_process_host_ui_shim.cc:243

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_views_touch.cc
(right):

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_views_touch.cc:208:
surface->Initialize(surface_id);
On 2011/09/21 20:27:36, jonathan.backer wrote:
> Do you want to do anything with the return value?

I don't know, only the GLX one can fail.  In the previous code it just ignored
the fact that it had failed, thus I do the same.  What would you like to happen
?

http://codereview.chromium.org/7980006/diff/5017/content/browser/gpu/gpu_proc...
File content/browser/gpu/gpu_process_host_ui_shim.cc (right):

http://codereview.chromium.org/7980006/diff/5017/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host_ui_shim.cc:238: const
GpuHostMsg_AcceleratedSurfaceNew_Params& params) {
This will have to be renamed back or duplicated for MACOSX.

http://codereview.chromium.org/7980006/diff/5017/content/browser/gpu/gpu_proc...
content/browser/gpu/gpu_process_host_ui_shim.cc:255: surface_id =
params.identifier;
On 2011/09/21 20:27:36, jonathan.backer wrote:
> nit: init on the decl?

it's my C style background I guess. I like to declare things at the top and use
them later. changed.

http://codereview.chromium.org/7980006/diff/5017/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host_mac.cc (right):

http://codereview.chromium.org/7980006/diff/5017/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host_mac.cc:40: void
RenderWidgetHost::OnAcceleratedSurfaceNew(
On 2011/09/21 20:27:36, jonathan.backer wrote:
> I would be careful of this. It may have more than one entry point (say from a
> renderer instead of a GPU process).

What would be the problem if it did have an entry from the renderer ?

http://codereview.chromium.org/7980006/diff/5017/content/common/gpu/image_tra...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/7980006/diff/5017/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface_linux.cc:276: uint64 surface_id,
TransportDIB::Handle /*surface_handle*/) {
On 2011/09/21 20:27:36, jonathan.backer wrote:
> Comment out param name? Haven't seen this before.

It's in the C++ style guide for params that are not used in the function.

http://www.corp.google.com/eng/doc/cppguide.xml?showone=Function_Declarations...

[danakj, 2011-09-21 21:15:37]

Ok, all nits taken care of and submitted.  I will test compile on a mac then
send it off for OWNERS.

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_views_touch.cc
(right):

http://codereview.chromium.org/7980006/diff/5017/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_views_touch.cc:208:
surface->Initialize(surface_id);
It is back practice to ignore return errors, so if it fails to initialize, I'll
destroy the object and return.  Adding it to the containers list is just likely
to run into more difficult to trace bugs.

[sky, 2011-09-21 21:39:55]

Just style nits.

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
File chrome/browser/renderer_host/accelerated_surface_container_touch.cc
(right):

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.cc:27: virtual
bool Initialize(uint64 *surface_id) OVERRIDE;
'uint64 *' -> 'uint64*' here and every where.

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.cc:34:
~AcceleratedSurfaceContainerTouchEGL();
virtual here and all these classes.

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.cc:277:
shared_mem_(0) {
No need to initialize shared_mem_ since it's a scoped_ptr. And if you did, we
use NULL instead of 0.

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.cc:311:
AcceleratedSurfaceContainerTouchOSMesa::
declaration order and definition order should match.

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.cc:321: 
remove newline.

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
File chrome/browser/renderer_host/accelerated_surface_container_touch.h (right):

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.h:28: // The
|surface_id| given to this function may be modified, and the returned
This sentence looks wrong. The return value is a bool.

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.h:30: virtual
bool Initialize(uint64 *surface_id) = 0;
'uint64 *surface_id' -> 'uint64* surface_id'

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.h:32: // Some
implementations of this class use shared memory, this the handle
'this the' -> 'this is the' ?

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
chrome/browser/renderer_host/accelerated_surface_container_touch.h:34: virtual
TransportDIB::Handle handle() const;
Don't use unix_hacker_style for virtual methods.

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host_view_views_touch.cc
(right):

http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host_view_views_touch.cc:205:
AcceleratedSurfaceContainerTouch* surface =
Use a scoped_ptr or scoped_refptr.

[apatrick_chromium, 2011-09-21 22:07:51]

One nit. Let's see how the try bots do...

http://codereview.chromium.org/7980006/diff/4033/content/common/gpu/image_tra...
File content/common/gpu/image_transport_surface_linux.cc (right):

http://codereview.chromium.org/7980006/diff/4033/content/common/gpu/image_tra...
content/common/gpu/image_transport_surface_linux.cc:125: int32 render_view_id,
indentation

[danakj, 2011-09-21 22:10:19]

On 2011/09/21 21:39:55, sky wrote:
> Just style nits.

Thanks, I've got them all addressed and uploaded now.

>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/accelerated_surface_container_touch.cc
> (right):
> 
>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> chrome/browser/renderer_host/accelerated_surface_container_touch.cc:27:
virtual
> bool Initialize(uint64 *surface_id) OVERRIDE;
> 'uint64 *' -> 'uint64*' here and every where.
> 
>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> chrome/browser/renderer_host/accelerated_surface_container_touch.cc:34:
> ~AcceleratedSurfaceContainerTouchEGL();
> virtual here and all these classes.
> 
>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> chrome/browser/renderer_host/accelerated_surface_container_touch.cc:277:
> shared_mem_(0) {
> No need to initialize shared_mem_ since it's a scoped_ptr. And if you did, we
> use NULL instead of 0.

k

>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> chrome/browser/renderer_host/accelerated_surface_container_touch.cc:311:
> AcceleratedSurfaceContainerTouchOSMesa::
> declaration order and definition order should match.

k

>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> chrome/browser/renderer_host/accelerated_surface_container_touch.cc:321: 
> remove newline.
> 
>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/accelerated_surface_container_touch.h
(right):
> 
>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> chrome/browser/renderer_host/accelerated_surface_container_touch.h:28: // The
> |surface_id| given to this function may be modified, and the returned
> This sentence looks wrong. The return value is a bool.

right, changed to modified.

>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> chrome/browser/renderer_host/accelerated_surface_container_touch.h:30: virtual
> bool Initialize(uint64 *surface_id) = 0;
> 'uint64 *surface_id' -> 'uint64* surface_id'
> 
>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> chrome/browser/renderer_host/accelerated_surface_container_touch.h:32: // Some
> implementations of this class use shared memory, this the handle
> 'this the' -> 'this is the' ?

ya

>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> chrome/browser/renderer_host/accelerated_surface_container_touch.h:34: virtual
> TransportDIB::Handle handle() const;
> Don't use unix_hacker_style for virtual methods.

k

>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> File chrome/browser/renderer_host/render_widget_host_view_views_touch.cc
> (right):
> 
>
http://codereview.chromium.org/7980006/diff/4033/chrome/browser/renderer_host...
> chrome/browser/renderer_host/render_widget_host_view_views_touch.cc:205:
> AcceleratedSurfaceContainerTouch* surface =
> Use a scoped_ptr or scoped_refptr.

k

[sky, 2011-09-21 22:17:02]

LGTM

[danakj, 2011-09-22 14:40:04]

try bots clear!

[jonathan.backer, 2011-09-22 17:04:44]

On 2011/09/22 14:40:04, danakj wrote:
> try bots clear!

Tested by hand on OSX. No regression.

[apatrick_chromium, 2011-09-22 21:01:01]

LGTM

[commit-bot: I haz the power, 2011-09-22 21:06:28]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/7980006/4034

[commit-bot: I haz the power, 2011-09-22 21:06:34]

Can't apply patch for file content/common/gpu/DEPS.
While running patch -p1 --forward --force;
patching file content/common/gpu/DEPS
Hunk #1 FAILED at 7.
1 out of 1 hunk FAILED -- saving rejects to file content/common/gpu/DEPS.rej

[commit-bot: I haz the power, 2011-09-23 13:47:25]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/7980006/12002

[commit-bot: I haz the power, 2011-09-23 13:47:30]

Can't apply patch for file content/common/gpu/DEPS.
While running patch -p1 --forward --force;
patching file content/common/gpu/DEPS
Hunk #1 FAILED at 7.
1 out of 1 hunk FAILED -- saving rejects to file content/common/gpu/DEPS.rej

[commit-bot: I haz the power, 2011-09-23 15:04:23]

CQ is trying tha patch. Follow status at
https://chromium-status.appspot.com/cq/danakj@chromium.org/7980006/12002

[commit-bot: I haz the power, 2011-09-23 15:04:28]

Can't apply patch for file content/common/gpu/DEPS.
While running patch -p1 --forward --force;
patching file content/common/gpu/DEPS
Hunk #1 FAILED at 7.
1 out of 1 hunk FAILED -- saving rejects to file content/common/gpu/DEPS.rej

[M-A Ruel, 2012-05-18 14:28:27]

On 2011/09/23 15:04:28, I haz the power (commit-bot) wrote:
> Can't apply patch for file content/common/gpu/DEPS.
> While running patch -p1 --forward --force;
> patching file content/common/gpu/DEPS
> Hunk #1 FAILED at 7.
> 1 out of 1 hunk FAILED -- saving rejects to file content/common/gpu/DEPS.rej

BTW, this patch failed to apply because you had edited it on Windows and
converted the file to CRLF. Please don't do that.

Thanks,

M-A