Connect GpuVideoDecodeServiceHost with ggl::Context and CommandBufferProxy

Connect GpuVideoDecodeServiceHost with ggl::Context and CommandBufferProxy

BUG=53714
A GpuVideoDecodeServiceHost needs to be connected with a ggl::Contect and its
associated comand buffer for the following reasons:

1. The gpu video decoder in gpu process needs to be in the correct GLES2 context.
2. On context lost the gpu video decoder needs to destroy itself.

This patch is able to connect the GpuVideoDecoderHost to the context although
the context is not passed into the decoder, so the code path is currently broken.

In a future patch we need to do the following:
1. Inject a ggl::Context into IpcVideoDecoder.
2. Complete the plumbing so that inside the gpu process we can associate a
   video decoder with a GLES2 context.

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

[Alpha Left Google, 2010-08-30 19:04:18]

Please review this patch. The other change to context depends on this one.

[jiesun, 2010-08-30 19:54:32]

http://codereview.chromium.org/3215008/diff/1/6
File chrome/renderer/gpu_video_service_host.cc (left):

http://codereview.chromium.org/3215008/diff/1/6#oldcode36
chrome/renderer/gpu_video_service_host.cc:36: return NULL;
I am thinking that for those platform without hardware support, we could
fallback before doing the real message exchange for creating video decoder.

http://codereview.chromium.org/3215008/diff/1/6
File chrome/renderer/gpu_video_service_host.cc (right):

http://codereview.chromium.org/3215008/diff/1/6#newcode37
chrome/renderer/gpu_video_service_host.cc:37: 
??. Add TODO?
and not all destroy are triggered by Channel tear down. we still need expost
this interface.

http://codereview.chromium.org/3215008/diff/1/8
File chrome/renderer/media/ipc_video_decoder.cc (right):

http://codereview.chromium.org/3215008/diff/1/8#newcode51
chrome/renderer/media/ipc_video_decoder.cc:51: // TODO(hclam): This is a wrong
place to initialize the demuxer stream's
why?

[Alpha Left Google, 2010-08-30 20:32:51]

http://codereview.chromium.org/3215008/diff/1/6
File chrome/renderer/gpu_video_service_host.cc (left):

http://codereview.chromium.org/3215008/diff/1/6#oldcode36
chrome/renderer/gpu_video_service_host.cc:36: return NULL;
On 2010/08/30 19:54:32, jiesun wrote:
> I am thinking that for those platform without hardware support, we could
> fallback before doing the real message exchange for creating video decoder.

Fallback as in software decoding?

http://codereview.chromium.org/3215008/diff/1/6
File chrome/renderer/gpu_video_service_host.cc (right):

http://codereview.chromium.org/3215008/diff/1/6#newcode37
chrome/renderer/gpu_video_service_host.cc:37: 
On 2010/08/30 19:54:32, jiesun wrote:
> ??. Add TODO?
> and not all destroy are triggered by Channel tear down. we still need expost
> this interface.

Oops sorry I forgot to upload the cleaned up patch. You can see it completely
removed in the new patch.

http://codereview.chromium.org/3215008/diff/1/8
File chrome/renderer/media/ipc_video_decoder.cc (right):

http://codereview.chromium.org/3215008/diff/1/8#newcode51
chrome/renderer/media/ipc_video_decoder.cc:51: // TODO(hclam): This is a wrong
place to initialize the demuxer stream's
On 2010/08/30 19:54:32, jiesun wrote:
> why?

This should be called on the decoder thread just to be safe.

[jiesun, 2010-08-30 21:16:13]

http://codereview.chromium.org/3215008/diff/7001/6003
File chrome/renderer/ggl/ggl.h (right):

http://codereview.chromium.org/3215008/diff/7001/6003#newcode113
chrome/renderer/ggl/ggl.h:113: 
Is context deletion the only place we need to Destroy A GpuVideoDecoderHost?
should we expose the interface for destruction for this GpuVideoDecoderHost?

http://codereview.chromium.org/3215008/diff/7001/6007
File chrome/renderer/gpu_video_service_host.h (right):

http://codereview.chromium.org/3215008/diff/7001/6007#newcode53
chrome/renderer/gpu_video_service_host.h:53: 
if we navigate the renderer view to another site, do we get context lost event
or do we use the same context? how do we destroy those decoders if the latter is
true?

http://codereview.chromium.org/3215008/diff/7001/6008
File chrome/renderer/media/ipc_video_decoder.cc (right):

http://codereview.chromium.org/3215008/diff/7001/6008#newcode53
chrome/renderer/media/ipc_video_decoder.cc:53:
demuxer_stream->EnableBitstreamConverter();
I think pipeline serialize Initilize() on filters which exist on different
threads, therefore it does not make difference to call this in either decoder
thread or renderer thread.

[jiesun, 2010-08-30 21:19:58]

forget one thing.

http://codereview.chromium.org/3215008/diff/7001/6006
File chrome/renderer/gpu_video_service_host.cc (left):

http://codereview.chromium.org/3215008/diff/7001/6006#oldcode36
chrome/renderer/gpu_video_service_host.cc:36: return NULL;
I think we should keep this statement to make fallback (to ffmpeg decode engine)
earlier and easier. How do you think?

[Alpha Left Google, 2010-08-30 21:54:12]

http://codereview.chromium.org/3215008/diff/7001/6003
File chrome/renderer/ggl/ggl.h (right):

http://codereview.chromium.org/3215008/diff/7001/6003#newcode113
chrome/renderer/ggl/ggl.h:113: 
On 2010/08/30 21:16:13, jiesun wrote:
> Is context deletion the only place we need to Destroy A GpuVideoDecoderHost?
> should we expose the interface for destruction for this GpuVideoDecoderHost?

My thought is destroying a GpuVideoDecoderHost doesn't need to involve
ggl::Context so doesn't need to define a method here.

http://codereview.chromium.org/3215008/diff/7001/6006
File chrome/renderer/gpu_video_service_host.cc (left):

http://codereview.chromium.org/3215008/diff/7001/6006#oldcode36
chrome/renderer/gpu_video_service_host.cc:36: return NULL;
On 2010/08/30 21:19:58, jiesun wrote:
> I think we should keep this statement to make fallback (to ffmpeg decode
engine)
> earlier and easier. How do you think?

I don't think checking here helps, we create the GpuVideoDecoderHost in
IpcVideoDecoder::Initialize() which is too late to switch the filter already.
This has to be fixed together with the filter factory stuff, I'd let andrew fix
the filter factory first before we worry about switching filter.

http://codereview.chromium.org/3215008/diff/7001/6007
File chrome/renderer/gpu_video_service_host.h (right):

http://codereview.chromium.org/3215008/diff/7001/6007#newcode53
chrome/renderer/gpu_video_service_host.h:53: 
On 2010/08/30 21:16:13, jiesun wrote:
> if we navigate the renderer view to another site, do we get context lost event
> or do we use the same context? how do we destroy those decoders if the latter
is
> true?

I need to ask Al to know but context lost and destruction of GpuVideoDecoderHost
I'm planning to fix as I get to those parts. Right now I just assume we never
have to destruct a decoder host and let it crash when bad things happen.

http://codereview.chromium.org/3215008/diff/7001/6008
File chrome/renderer/media/ipc_video_decoder.cc (right):

http://codereview.chromium.org/3215008/diff/7001/6008#newcode53
chrome/renderer/media/ipc_video_decoder.cc:53:
demuxer_stream->EnableBitstreamConverter();
On 2010/08/30 21:16:13, jiesun wrote:
> I think pipeline serialize Initilize() on filters which exist on different
> threads, therefore it does not make difference to call this in either decoder
> thread or renderer thread.

The problem is that this calls touches the AVCodecContext, we could do this by
having EnableBitstreamConverter() to set a flag and then initialize the bsf
lazily. I'd like to keep this TODO so we be aware of the potential problems.

[jiesun, 2010-08-30 22:23:01]

http://codereview.chromium.org/3215008/diff/7001/6006
File chrome/renderer/gpu_video_service_host.cc (left):

http://codereview.chromium.org/3215008/diff/7001/6006#oldcode36
chrome/renderer/gpu_video_service_host.cc:36: return NULL;
changing filter is hard here, but I am talking about ffmpeg decode engine. I
could switch engines probably.

No matter whether this is beneficial to software decoding fallback. we are
likely to check this somewhere during initialization, right? where you plan to
check this?

http://codereview.chromium.org/3215008/diff/7001/6007
File chrome/renderer/gpu_video_service_host.h (right):

http://codereview.chromium.org/3215008/diff/7001/6007#newcode53
chrome/renderer/gpu_video_service_host.h:53: 
Sounds good, but I would keep current code until you come up with better one. Or
you find we do not need this interface function at all. Another way probably is
ref-counting the GpuVideoDecoderHost. Is that what you are planning?

[Alpha Left Google, 2010-08-30 22:27:35]

http://codereview.chromium.org/3215008/diff/7001/6006
File chrome/renderer/gpu_video_service_host.cc (left):

http://codereview.chromium.org/3215008/diff/7001/6006#oldcode36
chrome/renderer/gpu_video_service_host.cc:36: return NULL;
On 2010/08/30 22:23:01, jiesun wrote:
> changing filter is hard here, but I am talking about ffmpeg decode engine. I
> could switch engines probably.
> 
> No matter whether this is beneficial to software decoding fallback. we are
> likely to check this somewhere during initialization, right? where you plan to
> check this?

The check should probably be in GpuVideoServiceHost instead of here.

Switching to use FFmpegVideoDecodeEngine is probably not a good idea as it makes
the design too adhoc, but we can discuss that later.

http://codereview.chromium.org/3215008/diff/7001/6007
File chrome/renderer/gpu_video_service_host.h (right):

http://codereview.chromium.org/3215008/diff/7001/6007#newcode53
chrome/renderer/gpu_video_service_host.h:53: 
On 2010/08/30 22:23:01, jiesun wrote:
> Sounds good, but I would keep current code until you come up with better one.
Or
> you find we do not need this interface function at all. Another way probably
is
> ref-counting the GpuVideoDecoderHost. Is that what you are planning?

DestroyVideoDecoder isn't being called anywhere, I'd prefer to define it later
when things are more clear instead of having a method that is not used.

[jiesun, 2010-08-30 22:54:41]

LGTM.

I think that currently we guarded IPC video decoder factory with command line
options, which help to prevent !service_available_ case in GpuVideoServiceHost.
Maybe we should add a GpuVideoServiceHost::GetCaps() then we do not need to
check the availabilities inside CreateVideoDecoder....

[scherkus (not reviewing), 2010-08-30 23:20:22]

I'll take a look at this tonight

[scherkus (not reviewing), 2010-08-31 03:03:30]

http://codereview.chromium.org/3215008/diff/7001/6004
File chrome/renderer/gpu_video_decoder_host.cc (right):

http://codereview.chromium.org/3215008/diff/7001/6004#newcode51
chrome/renderer/gpu_video_decoder_host.cc:51: // Save the event handelr before
we perform initialization operations so
handelr -> handler

http://codereview.chromium.org/3215008/diff/7001/6007
File chrome/renderer/gpu_video_service_host.h (right):

http://codereview.chromium.org/3215008/diff/7001/6007#newcode29
chrome/renderer/gpu_video_service_host.h:29: MessageRouter* router,
did you mean OnGpuChannelConnected? (i.e., mean to have "gg" in method name)

if so, fix indentation

http://codereview.chromium.org/3215008/diff/7001/6008
File chrome/renderer/media/ipc_video_decoder.cc (right):

http://codereview.chromium.org/3215008/diff/7001/6008#newcode67
chrome/renderer/media/ipc_video_decoder.cc:67: // TODO(hclam): Fix this.
fix what?

[Alpha Left Google, 2010-08-31 19:31:16]

http://codereview.chromium.org/3215008/diff/7001/6004
File chrome/renderer/gpu_video_decoder_host.cc (right):

http://codereview.chromium.org/3215008/diff/7001/6004#newcode51
chrome/renderer/gpu_video_decoder_host.cc:51: // Save the event handelr before
we perform initialization operations so
On 2010/08/31 03:03:30, scherkus wrote:
> handelr -> handler

Done.

http://codereview.chromium.org/3215008/diff/7001/6007
File chrome/renderer/gpu_video_service_host.h (right):

http://codereview.chromium.org/3215008/diff/7001/6007#newcode29
chrome/renderer/gpu_video_service_host.h:29: MessageRouter* router,
On 2010/08/31 03:03:30, scherkus wrote:
> did you mean OnGpuChannelConnected? (i.e., mean to have "gg" in method name)
> 
> if so, fix indentation

Done.

http://codereview.chromium.org/3215008/diff/7001/6008
File chrome/renderer/media/ipc_video_decoder.cc (right):

http://codereview.chromium.org/3215008/diff/7001/6008#newcode67
chrome/renderer/media/ipc_video_decoder.cc:67: // TODO(hclam): Fix this.
On 2010/08/31 03:03:30, scherkus wrote:
> fix what?

Done.

[scherkus (not reviewing), 2010-09-01 03:29:40]

LGTM but you've declared OnGpuChannelConnected() twice -- please fix or
something!

http://codereview.chromium.org/3215008/diff/16001/17006
File chrome/renderer/gpu_video_service_host.h (right):

http://codereview.chromium.org/3215008/diff/16001/17006#newcode33
chrome/renderer/gpu_video_service_host.h:33: void
OnGpuChannelConnected(GpuChannelHost* channel_host,
wait.. this method is not declared twice

what's going on here? :P

[Alpha Left Google, 2010-09-01 06:32:14]

On 2010/09/01 03:29:40, scherkus wrote:
> LGTM but you've declared OnGpuChannelConnected() twice -- please fix or
> something!
> 
> http://codereview.chromium.org/3215008/diff/16001/17006
> File chrome/renderer/gpu_video_service_host.h (right):
> 
> http://codereview.chromium.org/3215008/diff/16001/17006#newcode33
> chrome/renderer/gpu_video_service_host.h:33: void
> OnGpuChannelConnected(GpuChannelHost* channel_host,
> wait.. this method is not declared twice
> 
> what's going on here? :P

Ooops looks like there's a merge problem, will fix that.