gpu: reference target surfaces through a globally unique surface id.

gpu: reference target surfaces through a globally unique surface id.

This allows the gpu process to ignore all knowledge of renderers. It simplifies
some of the gpu <-> browser and gpu <-> renderer IPC, but mostly paves the way
for non-renderer clients.

Surfaces are kept in a global GpuSurfaceTracker which is just a thread-safe map.

BUG=99516
TEST=covered by existing tests. Run chrome, open poster circle (or other accelerated content page).


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

[piman, 2012-01-14 02:37:24]

Infrastructure to use the GPU process from the browser, part 2 of many. Sorry
for the bomb... There's a lot of files but a lot of the changes are very
mechanical.

apatrick, backer: you are the main reviewers. In particular, maybe Jonathan can
focus on the accelerated_surface_container/image_transport_surface bits, and Al
can look at the gpu_process_host/gpu_channel stuff, as well as the RWH/RVH bits,
and the new GpuSurfaceTracker.
ben: OWNERS duty

Note: for the image_transport_surface IPCs, I got into naming conflicts, I did a
mechanical renaming, which may appear a little confusing:
(old)->(new)
surface_handle -> shm_handle (the transport dib that's used for transporting
software bits)
surface_id -> surface_handle (the opaque value that is meant to represent the
native handle for the transport surface)
(added) surface_id (the id of the surface, replaces the
renderer_id/render_widget_id pair).

[jonathan.backer, 2012-01-16 19:47:26]

LGTM

Nothing but nits. Thanks for the WeakPtr to GpuCommandBufferStub cleanup.

http://codereview.chromium.org/9194005/diff/7004/content/browser/gpu/gpu_surf...
File content/browser/gpu/gpu_surface_tracker.h (right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/gpu/gpu_surf...
content/browser/gpu/gpu_surface_tracker.h:27: static GpuSurfaceTracker* Get() {
return GetInstance(); }
Why both Get() and GetInstance()?

http://codereview.chromium.org/9194005/diff/7004/content/browser/gpu/gpu_surf...
content/browser/gpu/gpu_surface_tracker.h:37: int LookupSurfaceForRenderer(int
renderer_id, int render_widget_id);
Not sure it matters (this isn't performance critical code), but it'd be
straightforward to have a map from pair<renderer_id,render_widget_id> ->
surface_id to make this a fast lookup.

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/accelerated_surface_container_linux.h
(right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/accelerated_surface_container_linux.h:24: // The
|surface_id| given to this function may be modified, and the modified
s/surface_id/surface_handle?

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/accelerated_surface_container_mac.h (right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/accelerated_surface_container_mac.h:109: // The id
of |surface_|, or 0 if |surface_| is NULL.
s/id/handle?

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/image_transport_client.cc (right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/image_transport_client.cc:237: // We expect to
make the id here, so don't want the other end giving us one
s/id/handle?

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/image_transport_client.cc:244: *surface_handle =
next_id_++;
s/next_id_/next_handle_?

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:384: WebKit::WebPopupType
popup_type);
nit: not sure how this is better.

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/render_widget_host.cc:118: // (and surface id)
that have been created, but whose RWH haven't yet.
See comment in GpuSurfaceTracker --- lookup time easily addressed with second
map (not sure it's worth the extra space).

http://codereview.chromium.org/9194005/diff/7004/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.cc (right):

http://codereview.chromium.org/9194005/diff/7004/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.cc:195: this,
Thanks for replacing the lookup with a WeakPtr.

http://codereview.chromium.org/9194005/diff/7004/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/9194005/diff/7004/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.h:145: // The following two fields is
used to identify the window for the rendering
s/two fields/field

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

http://codereview.chromium.org/9194005/diff/7004/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:50: IPC_STRUCT_MEMBER(uint64, surface_handle)
Comment: It's redundant to pass both surface_id and surface_handle. I'm fine
with it as is.

http://codereview.chromium.org/9194005/diff/7004/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/9194005/diff/7004/content/common/view_messages...
content/common/view_messages.h:1200: int /* surface_id */,
int32?

http://codereview.chromium.org/9194005/diff/7004/content/renderer/gpu/rendere...
File content/renderer/gpu/renderer_gl_context.cc (right):

http://codereview.chromium.org/9194005/diff/7004/content/renderer/gpu/rendere...
content/renderer/gpu/renderer_gl_context.cc:21: 
nit: extra \n

[piman, 2012-01-17 19:40:54]

Note, in this patch I also fixed a failing test, and a struct/class issue.

http://codereview.chromium.org/9194005/diff/7004/content/browser/gpu/gpu_surf...
File content/browser/gpu/gpu_surface_tracker.h (right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/gpu/gpu_surf...
content/browser/gpu/gpu_surface_tracker.h:27: static GpuSurfaceTracker* Get() {
return GetInstance(); }
On 2012/01/16 19:47:26, jonathan.backer wrote:
> Why both Get() and GetInstance()?

I added a comment, it's just syntactic sugar. Singleton (implementation detail)
want GetInstance. For call site clarity, having it named "Get" helps a lot
(wrapping). But they're identical.

http://codereview.chromium.org/9194005/diff/7004/content/browser/gpu/gpu_surf...
content/browser/gpu/gpu_surface_tracker.h:37: int LookupSurfaceForRenderer(int
renderer_id, int render_widget_id);
On 2012/01/16 19:47:26, jonathan.backer wrote:
> Not sure it matters (this isn't performance critical code), but it'd be
> straightforward to have a map from pair<renderer_id,render_widget_id> ->
> surface_id to make this a fast lookup.

Yeah, I figured it's not worth the extra space, plus the added complexity of
maintaining that other map (needs to be done in at the call sites).

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/accelerated_surface_container_linux.h
(right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/accelerated_surface_container_linux.h:24: // The
|surface_id| given to this function may be modified, and the modified
On 2012/01/16 19:47:26, jonathan.backer wrote:
> s/surface_id/surface_handle?

Done. Thanks.

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/accelerated_surface_container_mac.h (right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/accelerated_surface_container_mac.h:109: // The id
of |surface_|, or 0 if |surface_| is NULL.
On 2012/01/16 19:47:26, jonathan.backer wrote:
> s/id/handle?

Done. Thanks.

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/image_transport_client.cc (right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/image_transport_client.cc:237: // We expect to
make the id here, so don't want the other end giving us one
On 2012/01/16 19:47:26, jonathan.backer wrote:
> s/id/handle?

Done. Thanks.

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/image_transport_client.cc:244: *surface_handle =
next_id_++;
On 2012/01/16 19:47:26, jonathan.backer wrote:
> s/next_id_/next_handle_?

Done.

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host.h (right):

http://codereview.chromium.org/9194005/diff/7004/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host.h:384: WebKit::WebPopupType
popup_type);
On 2012/01/16 19:47:26, jonathan.backer wrote:
> nit: not sure how this is better.

It's not, reverted, thanks. I originally had added an argument here, but forgot
to re-indent when removing it.

http://codereview.chromium.org/9194005/diff/7004/content/common/gpu/gpu_comma...
File content/common/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/9194005/diff/7004/content/common/gpu/gpu_comma...
content/common/gpu/gpu_command_buffer_stub.h:145: // The following two fields is
used to identify the window for the rendering
On 2012/01/16 19:47:26, jonathan.backer wrote:
> s/two fields/field

Done. I simplified the comment a bit (because we only have one field).

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

http://codereview.chromium.org/9194005/diff/7004/content/common/gpu/gpu_messa...
content/common/gpu/gpu_messages.h:50: IPC_STRUCT_MEMBER(uint64, surface_handle)
On 2012/01/16 19:47:26, jonathan.backer wrote:
> Comment: It's redundant to pass both surface_id and surface_handle. I'm fine
> with it as is.

So, right now, we need the surface_id for routing to the right RWHV, and we sort
of need the surface_handle because you may have more than one for the same
surface_id in the EGL case.
Arguably, the host side could track which one is the front and back, and we
could avoid this extra param. We could do that later. I wouldn't want to grow
this CL further...

http://codereview.chromium.org/9194005/diff/7004/content/common/view_messages.h
File content/common/view_messages.h (right):

http://codereview.chromium.org/9194005/diff/7004/content/common/view_messages...
content/common/view_messages.h:1200: int /* surface_id */,
On 2012/01/16 19:47:26, jonathan.backer wrote:
> int32?

Done.

http://codereview.chromium.org/9194005/diff/7004/content/renderer/gpu/rendere...
File content/renderer/gpu/renderer_gl_context.cc (right):

http://codereview.chromium.org/9194005/diff/7004/content/renderer/gpu/rendere...
content/renderer/gpu/renderer_gl_context.cc:21: 
On 2012/01/16 19:47:26, jonathan.backer wrote:
> nit: extra \n

Done.

[apatrick_chromium, 2012-01-17 20:06:39]

LGTM. Thanks!

[Ben Goodger (Google), 2012-01-17 22:27:01]

lgtm

[commit-bot: I haz the power, 2012-01-17 23:19:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/9194005/20015

[commit-bot: I haz the power, 2012-01-17 23:19:57]

Presubmit check for 9194005-20015 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: content/public/browser/render_process_host.h

Presubmit checks took 7.9s to calculate.

[piman, 2012-01-17 23:36:32]

+jam for OWNERS in content/public (only 1 file).

[jam, 2012-01-18 19:27:34]

On 2012/01/17 23:36:32, piman wrote:
> +jam for OWNERS in content/public (only 1 file).

lgtm, sorry i just saw this, please IM me if i dont respond promptly in the
future for these kind of reviews

[commit-bot: I haz the power, 2012-01-18 19:44:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/9194005/20015

[commit-bot: I haz the power, 2012-01-18 22:29:59]

Change committed as 118172