Delete GpuVideoService and move GpuVideoDecodeAccelerator ownership to stubs

Delete GpuVideoService and move GpuVideoDecodeAccelerator ownership to stubs

This CL divvies up the GpuVideoService functionality among the relevant
classes.

BUG=NONE
TEST=NONE

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

[vrk (LEFT CHROMIUM), 2011-06-30 20:43:30]

apatrick: OWNERS + GpuChannel/Stub changes
fischman: everything

Thanks!

[apatrick_chromium, 2011-06-30 21:11:16]

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_channel.cc
File content/common/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_channel....
content/common/gpu/gpu_channel.cc:352: bool result =
stubs_by_video_decoder_id_.insert(std::make_pair(
Rather than maintaining a map of video decoder id to stub in the channel and
handling the IPCs there, can you instead route OnCreateVideoDecoder and
OnDestroyVideoDecoder directly the stub's route id?

Also, I don't know that the video decoder needs to have its own route ID?
GpuCommandBufferStub::OnMessageReceived could just pass IPCs on to the video
decoder if it doesn't handle them.

[Ami GONE FROM CHROMIUM, 2011-06-30 22:20:39]

I like it!

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_channel.h
File content/common/gpu/gpu_channel.h (right):

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_channel....
content/common/gpu/gpu_channel.h:181: std::map<int32, GpuCommandBufferStub*>
stubs_by_video_decoder_id_;
Does it work to use an IDMap<GpuCommandBufferStub, IDMapExternalPointer> instead
here to make explicit the relationship between this and stubs_, and get the
side-effect of single-thread-checking?

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_command_...
File content/common/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_command_...
content/common/gpu/gpu_command_buffer_stub.h:104: void DestroyVideoDecoder() {
Destroy* implies to me that this class should have a Create* method.
WDYT of release_video_decoder() ?

http://codereview.chromium.org/7292010/diff/1/content/content_common.gypi
File content/content_common.gypi (left):

http://codereview.chromium.org/7292010/diff/1/content/content_common.gypi#old...
content/content_common.gypi:130: 'common/gpu/media/gpu_video_service.cc',
Yay!

Does gpu_video_service_host.{h,cc} want to go byebye as well?
(in another CL?)

[vrk (LEFT CHROMIUM), 2011-07-01 18:12:14]

Things have changed a bit in following apatrick's suggestions. Please take
another look!

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_channel.cc
File content/common/gpu/gpu_channel.cc (right):

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_channel....
content/common/gpu/gpu_channel.cc:352: bool result =
stubs_by_video_decoder_id_.insert(std::make_pair(
On 2011/06/30 21:11:16, apatrick_chromium wrote:
> Rather than maintaining a map of video decoder id to stub in the channel and
> handling the IPCs there, can you instead route OnCreateVideoDecoder and
> OnDestroyVideoDecoder directly the stub's route id?
> 
> Also, I don't know that the video decoder needs to have its own route ID?
> GpuCommandBufferStub::OnMessageReceived could just pass IPCs on to the video
> decoder if it doesn't handle them.

D'oh.

Emphatic yes to everything! These suggestions cleaned up a lot of unnecessary
indirection. Thanks!

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_channel.h
File content/common/gpu/gpu_channel.h (right):

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_channel....
content/common/gpu/gpu_channel.h:181: std::map<int32, GpuCommandBufferStub*>
stubs_by_video_decoder_id_;
On 2011/06/30 22:20:39, Ami Fischman wrote:
> Does it work to use an IDMap<GpuCommandBufferStub, IDMapExternalPointer>
instead
> here to make explicit the relationship between this and stubs_, and get the
> side-effect of single-thread-checking?

No need for this map anymore! Create/Destroy messages directly routed to stub.

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_command_...
File content/common/gpu/gpu_command_buffer_stub.h (right):

http://codereview.chromium.org/7292010/diff/1/content/common/gpu/gpu_command_...
content/common/gpu/gpu_command_buffer_stub.h:104: void DestroyVideoDecoder() {
On 2011/06/30 22:20:39, Ami Fischman wrote:
> Destroy* implies to me that this class should have a Create* method.
> WDYT of release_video_decoder() ?

Destroying is now done directly in GpuCommandBufferStub, so no need! yay

http://codereview.chromium.org/7292010/diff/1/content/content_common.gypi
File content/content_common.gypi (left):

http://codereview.chromium.org/7292010/diff/1/content/content_common.gypi#old...
content/content_common.gypi:130: 'common/gpu/media/gpu_video_service.cc',
On 2011/06/30 22:20:39, Ami Fischman wrote:
> Yay!
> 
> Does gpu_video_service_host.{h,cc} want to go byebye as well?
> (in another CL?)

Yes, GpuVideoServiceHost looks to be yet more unnecessary indirection. Will tear
it out in my next CL!

[Ami GONE FROM CHROMIUM, 2011-07-01 20:18:16]

LGTM
As noted offline, it's a bit funky that GVDAH sends a GpuCommandBufferMsg, since
all other GCBMsg's are sent by CommandBufferProxy.  Hopefully a different CL can
remove this nonuniformity.

[brettw, 2011-07-07 16:57:59]

Rubber stamp LGTM.

[apatrick_chromium, 2011-07-07 18:22:14]

LGTM