PPB_VideoDecoder_Dev::Initialize is now synchronous!

PPB_VideoDecoder_Dev::Initialize is now synchronous!

Apparently flash can't deal with async init, so we make it synchronous.
We keep processing in the GPU process asynchronous and just take the blocking
hit on the renderer.

BUG=none
TEST=gles2, ovdatest, trybots

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

[Ami GONE FROM CHROMIUM, 2011-07-28 23:27:41]

brettw: OWNERS please
piman: FYI & for flash-side sanity-check
vrk: please review

[piman, 2011-07-28 23:40:55]

LGTM with the approach. A few suggestions...

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

http://codereview.chromium.org/7474006/diff/1/content/common/gpu/gpu_messages...
content/common/gpu/gpu_messages.h:500:
IPC_MESSAGE_ROUTED0(AcceleratedVideoDecoderHostMsg_InitializeDone)
This message isn't used any more, right ? Remove ?

http://codereview.chromium.org/7474006/diff/1/content/common/gpu/media/gpu_vi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7474006/diff/1/content/common/gpu/media/gpu_vi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:100: DCHECK(stub_ &&
stub_->scheduler());
Add a DCHECK(!init_done_msg_) ? Also, you wanna initialize that new field.

http://codereview.chromium.org/7474006/diff/1/ppapi/c/dev/ppb_video_decoder_d...
File ppapi/c/dev/ppb_video_decoder_dev.h (right):

http://codereview.chromium.org/7474006/diff/1/ppapi/c/dev/ppb_video_decoder_d...
ppapi/c/dev/ppb_video_decoder_dev.h:64: const PP_VideoConfigElement*
decoder_config);
Does it make sense to fold this into Create ?

[brettw, 2011-07-28 23:42:22]

It kind of sucks to have to block the renderer while we do this. And since Flash
runs in a separate process (normally), there's actually no gain in blocking the
renderer.

Can this be done by just blocking the plugin process? That may make some things
more difficult since it will make in-process decoding from Flash impractical. We
can have an async init, and a trusted API for sync init that we implement in the
proxy.

[brettw, 2011-07-28 23:43:06]

Antoine: do you have an opinion on what we should do for the blocking init?

[piman, 2011-07-28 23:49:51]

On Thu, Jul 28, 2011 at 4:43 PM, <brettw@chromium.org> wrote:

> Antoine: do you have an opinion on what we should do for the blocking init?


Yes, I suggested it.

Only blocking the plugin process doesn't work because if the renderer still
runs and tries to send a sync message (e.g. event) it'll deadlock.

Antoine


>
>
>
http://codereview.chromium.**org/7474006/<http://codereview.chromium.org/7474...
>

[brettw, 2011-07-28 23:59:33]

I get the deadlock, thanks.

For future reference, is there something in here that requires going to the
WebKit thread? Is this potentially something we can route to the GPU process
directly from the I/O thread of the renderer? We eventually want to move most or
all plugin -> renderer sync messages to be handled on the I/O thread so we don't
have to synchronize with WebKit.

[piman, 2011-07-29 00:07:19]

On Thu, Jul 28, 2011 at 4:59 PM, <brettw@chromium.org> wrote:

> I get the deadlock, thanks.
>
> For future reference, is there something in here that requires going to the
> WebKit thread? Is this potentially something we can route to the GPU
> process
> directly from the I/O thread of the renderer? We eventually want to move
> most or
> all plugin -> renderer sync messages to be handled on the I/O thread so we
> don't
> have to synchronize with WebKit.


I want to get to that eventually for GPU stuff (3d and video decode), but
there's a lot of non-trivial things to figure out for synchronization. Some
of Al's recent changes should make it easier.

Antoine


>
>
http://codereview.chromium.**org/7474006/<http://codereview.chromium.org/7474...
>

[Ami GONE FROM CHROMIUM, 2011-07-29 05:58:47]

brettw / piman: PTAL
Thanks for the quick turnaround!

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

http://codereview.chromium.org/7474006/diff/1/content/common/gpu/gpu_messages...
content/common/gpu/gpu_messages.h:500:
IPC_MESSAGE_ROUTED0(AcceleratedVideoDecoderHostMsg_InitializeDone)
On 2011/07/28 23:40:59, piman wrote:
> This message isn't used any more, right ? Remove ?

Right.  Done.

http://codereview.chromium.org/7474006/diff/1/content/common/gpu/media/gpu_vi...
File content/common/gpu/media/gpu_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7474006/diff/1/content/common/gpu/media/gpu_vi...
content/common/gpu/media/gpu_video_decode_accelerator.cc:100: DCHECK(stub_ &&
stub_->scheduler());
On 2011/07/28 23:40:59, piman wrote:
> Add a DCHECK(!init_done_msg_) ? Also, you wanna initialize that new field.

Done & done.

http://codereview.chromium.org/7474006/diff/1/ppapi/c/dev/ppb_video_decoder_d...
File ppapi/c/dev/ppb_video_decoder_dev.h (right):

http://codereview.chromium.org/7474006/diff/1/ppapi/c/dev/ppb_video_decoder_d...
ppapi/c/dev/ppb_video_decoder_dev.h:64: const PP_VideoConfigElement*
decoder_config);
On 2011/07/28 23:40:59, piman wrote:
> Does it make sense to fold this into Create ?

Hell yes it does!
Done.

[brettw, 2011-07-29 06:17:42]

owners LGTM

[piman, 2011-07-29 06:53:26]

LGTM, thanks !

[vrk (LEFT CHROMIUM), 2011-07-29 18:11:02]

LGTM

http://codereview.chromium.org/7474006/diff/4006/ppapi/cpp/dev/video_decoder_...
File ppapi/cpp/dev/video_decoder_dev.h (right):

http://codereview.chromium.org/7474006/diff/4006/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.h:30: //  |instance| is the pointer to the
plug-in instance.
document added params?

http://codereview.chromium.org/7474006/diff/4006/ppapi/examples/gles2/gles2.cc
File ppapi/examples/gles2/gles2.cc (right):

http://codereview.chromium.org/7474006/diff/4006/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:220: assert(!video_decoder_->is_null());
nit: is there any way for this to fail?

http://codereview.chromium.org/7474006/diff/4006/ppapi/tests/test_video_decod...
File ppapi/tests/test_video_decoder.cc (right):

http://codereview.chromium.org/7474006/diff/4006/ppapi/tests/test_video_decod...
ppapi/tests/test_video_decoder.cc:33: return "Create: error detecting invalid
context";
nit: technically the configs are invalid, too. Dunno if you care.

http://codereview.chromium.org/7474006/diff/4006/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.h (right):

http://codereview.chromium.org/7474006/diff/4006/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:42: static PP_Resource
Create(PluginInstance* instance,
comment? Perhaps mention that it returns 0 if it fails to create + init

[Ami GONE FROM CHROMIUM, 2011-07-29 18:20:41]

http://codereview.chromium.org/7474006/diff/4006/ppapi/cpp/dev/video_decoder_...
File ppapi/cpp/dev/video_decoder_dev.h (right):

http://codereview.chromium.org/7474006/diff/4006/ppapi/cpp/dev/video_decoder_...
ppapi/cpp/dev/video_decoder_dev.h:30: //  |instance| is the pointer to the
plug-in instance.
On 2011/07/29 18:11:02, Victoria Kirst wrote:
> document added params?

Actually removed existing doco in favor of pointer.

http://codereview.chromium.org/7474006/diff/4006/ppapi/examples/gles2/gles2.cc
File ppapi/examples/gles2/gles2.cc (right):

http://codereview.chromium.org/7474006/diff/4006/ppapi/examples/gles2/gles2.c...
ppapi/examples/gles2/gles2.cc:220: assert(!video_decoder_->is_null());
On 2011/07/29 18:11:02, Victoria Kirst wrote:
> nit: is there any way for this to fail?

In this plugin, not really, b/c we already assert that *context_ is valid
earlier, but this is a sample plugin so I think it's useful to be as explicit as
possible about the things that can/ought to be checked.

http://codereview.chromium.org/7474006/diff/4006/ppapi/tests/test_video_decod...
File ppapi/tests/test_video_decoder.cc (right):

http://codereview.chromium.org/7474006/diff/4006/ppapi/tests/test_video_decod...
ppapi/tests/test_video_decoder.cc:33: return "Create: error detecting invalid
context";
On 2011/07/29 18:11:02, Victoria Kirst wrote:
> nit: technically the configs are invalid, too. Dunno if you care.

I don't, but done.

http://codereview.chromium.org/7474006/diff/4006/webkit/plugins/ppapi/ppb_vid...
File webkit/plugins/ppapi/ppb_video_decoder_impl.h (right):

http://codereview.chromium.org/7474006/diff/4006/webkit/plugins/ppapi/ppb_vid...
webkit/plugins/ppapi/ppb_video_decoder_impl.h:42: static PP_Resource
Create(PluginInstance* instance,
On 2011/07/29 18:11:02, Victoria Kirst wrote:
> comment? Perhaps mention that it returns 0 if it fails to create + init

Done.