Enable fire-and-forget Destroy of HW video decoder, and misc other improvements.

Enable fire-and-forget Destroy of HW video decoder, and misc other improvements.
- Instead of requiring the client to wait for NotifyDestroyDone
  we do the asynchronous OMX teardown dance on the client's
  behalf.
- Prettify/simplify error handling in OVDA for easier matching of
  errors to OMX_Core.h and to remove redundant information.
- Enable previously-DISABLED_ early-teardown unittests!
- Remove passing VideoDecoder_Dev object in PPP_VideoDecoder_Dev
  calls, because it was unnecssary, and because the
  ~VideoDecoder_Dev dtor is now not a no-op so we don't want to
  call it spuriously.
- Remove accidentally re-added
  gpu_video_service_host.cc (originally removed in 92251,
  accidentally reintroduced by a bad rebase in 92383).

BUG=none
TEST=ovdatest passes (incl. early-teardown tests) and gles2 works (including reload and EOS handling, no crashes)

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

[Ami GONE FROM CHROMIUM, 2011-07-14 01:41:46]

Brett: please OWNERS review content/{renderer,common}, ppapi, and
webkit/plugins/ppapi.
Victoria: please review all of it.

[vhiremath, 2011-07-14 14:29:15]

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
File content/common/gpu/media/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
content/common/gpu/media/omx_video_decode_accelerator.cc:438: client_ = NULL;
Do we really want to do it here ?
What if OVDA has a pending Reset() call from plugin that is about to call
NotifyResetDone. And it also calls Destroy immediately (very common case user
just seeks to new location and close tab immediately)

How will this be handled in plugin.
Will plugin not wait on ResetDone once destroy is called ?

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
content/common/gpu/media/omx_video_decode_accelerator.cc:527:
base::PlatformThread::Sleep(5);
Why do we want to delay PostTask here ?
May cause any race-condition otherwise ?

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
content/common/gpu/media/omx_video_decode_accelerator.cc:531: }
Please explain how this logic works.
Note: We have the callbacks registered with openmax.
Those are still valid. 
So we continue to receive callbacks on omx thread till omx-component is alive.
Then from there we do PostTask on MessageLoop to say FillBufferDoneTask
(corresponding to FillBufferCallback).
So how does this loop help to convert that PostTask into NOP ?


Also, it is recursively calling back on itself.
How long does this outlive other threads ?
What is exit criteria ? or i did not understand the whole logic :(

[Ami GONE FROM CHROMIUM, 2011-07-14 16:23:03]

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
File content/common/gpu/media/omx_video_decode_accelerator.cc (right):

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
content/common/gpu/media/omx_video_decode_accelerator.cc:438: client_ = NULL;
On 2011/07/14 14:29:15, vhiremath wrote:
> Do we really want to do it here ?
> What if OVDA has a pending Reset() call from plugin that is about to call
> NotifyResetDone. And it also calls Destroy immediately (very common case user
> just seeks to new location and close tab immediately)
> 
> How will this be handled in plugin.
> Will plugin not wait on ResetDone once destroy is called ?

It is incorrect to call Destroy() while a Reset() or Flush() is outstanding
(enforced at line 427 with:
DCHECK_EQ(current_state_change_, NO_TRANSITION);
).

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
content/common/gpu/media/omx_video_decode_accelerator.cc:527:
base::PlatformThread::Sleep(5);
On 2011/07/14 14:29:15, vhiremath wrote:
> Why do we want to delay PostTask here ?

To reduce the CPU load of what otherwise would be a spinning busyloop.

> May cause any race-condition otherwise ?

No, no race condition.

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
content/common/gpu/media/omx_video_decode_accelerator.cc:531: }
On 2011/07/14 14:29:15, vhiremath wrote:
> Please explain how this logic works.
> Note: We have the callbacks registered with openmax.
> Those are still valid. 
> So we continue to receive callbacks on omx thread till omx-component is alive.
> Then from there we do PostTask on MessageLoop to say FillBufferDoneTask
> (corresponding to FillBufferCallback).
> So how does this loop help to convert that PostTask into NOP ?
> 
> 
> Also, it is recursively calling back on itself.
> How long does this outlive other threads ?
> What is exit criteria ? or i did not understand the whole logic :(

I think so.
The purpose of this method is simple: keep the MessageLoop from exiting during
its Quit() method, until OMX is guaranteed to not make any more callbacks.  That
guarantee is only made after OMX_FreeHandle returns, at which point we set
component_handle_ to NULL (which is why this method checks that at the top and
returns if it is already NULL).
MessageLoop's shutdown semantics are that it drains all pending tasks before
exiting.  As long as there is a pending task, it won't quit.  So this method
keeps enqueuing a new task as long as shutdown isn't complete.  Because
MessageLoop processes tasks FIFO, the eventual shutdown will be processed (i.e.
this semi-busyloop doesn't starve out the shutdown tasks).

Note that this method isn't trying to turn anything into a no-op.
Destroy() sets client_=NULL before returning, guaranteeing no further callbacks
will be made on the client, effectively turning further
{Fill,Empty}BufferDoneTask callbacks into no-op's, as far as the client is
concerned.

[vrk (LEFT CHROMIUM), 2011-07-14 20:34:00]

LGTM

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
File content/common/gpu/media/omx_video_decode_accelerator.h (right):

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
content/common/gpu/media/omx_video_decode_accelerator.h:21: #include
"base/memory/ref_counted.h"
nit: includes in ABC order

http://codereview.chromium.org/7361010/diff/10001/content/common/gpu/media/om...
File content/common/gpu/media/omx_video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/7361010/diff/10001/content/common/gpu/media/om...
content/common/gpu/media/omx_video_decode_accelerator_unittest.cc:777:
ASSERT_GT(num_concurrent_decoders, kMaxSupportedNumConcurrentDecoders);
Optional nit: Maybe add a comment to briefly explain what's going on here?

http://codereview.chromium.org/7361010/diff/10001/content/renderer/pepper_pla...
File content/renderer/pepper_platform_video_decoder_impl.h (right):

http://codereview.chromium.org/7361010/diff/10001/content/renderer/pepper_pla...
content/renderer/pepper_platform_video_decoder_impl.h:70: // Host for
GpuVideoDecodeAccelerator.
supernit: Maybe say "Holds a GpuVideoDecodeAcceleratorHost" instead

http://codereview.chromium.org/7361010/diff/10001/ppapi/c/dev/ppb_video_decod...
File ppapi/c/dev/ppb_video_decoder_dev.h (left):

http://codereview.chromium.org/7361010/diff/10001/ppapi/c/dev/ppb_video_decod...
ppapi/c/dev/ppb_video_decoder_dev.h:20: //   configure the decoder there.
?

[Ami GONE FROM CHROMIUM, 2011-07-14 20:46:09]

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
File content/common/gpu/media/omx_video_decode_accelerator.h (right):

http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
content/common/gpu/media/omx_video_decode_accelerator.h:21: #include
"base/memory/ref_counted.h"
On 2011/07/14 20:34:01, Victoria Kirst wrote:
> nit: includes in ABC order

Done.

http://codereview.chromium.org/7361010/diff/10001/content/common/gpu/media/om...
File content/common/gpu/media/omx_video_decode_accelerator_unittest.cc (right):

http://codereview.chromium.org/7361010/diff/10001/content/common/gpu/media/om...
content/common/gpu/media/omx_video_decode_accelerator_unittest.cc:777:
ASSERT_GT(num_concurrent_decoders, kMaxSupportedNumConcurrentDecoders);
On 2011/07/14 20:34:01, Victoria Kirst wrote:
> Optional nit: Maybe add a comment to briefly explain what's going on here?

Done.

http://codereview.chromium.org/7361010/diff/10001/content/renderer/pepper_pla...
File content/renderer/pepper_platform_video_decoder_impl.h (right):

http://codereview.chromium.org/7361010/diff/10001/content/renderer/pepper_pla...
content/renderer/pepper_platform_video_decoder_impl.h:70: // Host for
GpuVideoDecodeAccelerator.
On 2011/07/14 20:34:01, Victoria Kirst wrote:
> supernit: Maybe say "Holds a GpuVideoDecodeAcceleratorHost" instead

Done.

http://codereview.chromium.org/7361010/diff/10001/ppapi/c/dev/ppb_video_decod...
File ppapi/c/dev/ppb_video_decoder_dev.h (left):

http://codereview.chromium.org/7361010/diff/10001/ppapi/c/dev/ppb_video_decod...
ppapi/c/dev/ppb_video_decoder_dev.h:20: //   configure the decoder there.
On 2011/07/14 20:34:01, Victoria Kirst wrote:
> ?

Done.

[Ami GONE FROM CHROMIUM, 2011-07-15 04:02:55]

On 2011/07/14 16:23:03, Ami Fischman wrote:
>
http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
> File content/common/gpu/media/omx_video_decode_accelerator.cc (right):
> 
>
http://codereview.chromium.org/7361010/diff/2001/content/common/gpu/media/omx...
> content/common/gpu/media/omx_video_decode_accelerator.cc:438: client_ = NULL;
> On 2011/07/14 14:29:15, vhiremath wrote:
> > Do we really want to do it here ?
> > What if OVDA has a pending Reset() call from plugin that is about to call
> > NotifyResetDone. And it also calls Destroy immediately (very common case
user
> > just seeks to new location and close tab immediately)
> > 
> > How will this be handled in plugin.
> > Will plugin not wait on ResetDone once destroy is called ?
> It is incorrect to call Destroy() while a Reset() or Flush() is outstanding
> (enforced at line 427 with:
> DCHECK_EQ(current_state_change_, NO_TRANSITION);
> ).

I guess I should have been clearer: this situation is suboptimal, and I'd like
to support Destroy() at any time (incl. during a previously-requested state
transition) but I think the current form is worth checking in b/c it gets us
almost all of the benefit of fire-and-forget Destroy() and the edge-case that it
doesn't handle (Destroy() being called after Flush() or Reset() but before their
completion fires) is very short lived (20-50ms) so unlikely to be triggered in
the wild.

[Ami GONE FROM CHROMIUM, 2011-07-15 16:25:19]

brettw: i can haz OWNERS review?

[brettw, 2011-07-15 16:57:21]

spot-checked rubberstamp LGTM

http://codereview.chromium.org/7361010/diff/14001/ppapi/thunk/ppb_video_decod...
File ppapi/thunk/ppb_video_decoder_thunk.cc (right):

http://codereview.chromium.org/7361010/diff/14001/ppapi/thunk/ppb_video_decod...
ppapi/thunk/ppb_video_decoder_thunk.cc:86: if (enter.failed())
In this case I would do:
  if (enter.succeeded())
    enter.object()->Destroy();
To save a line.

[Ami GONE FROM CHROMIUM, 2011-07-15 17:10:14]

Thanks.

http://codereview.chromium.org/7361010/diff/14001/ppapi/thunk/ppb_video_decod...
File ppapi/thunk/ppb_video_decoder_thunk.cc (right):

http://codereview.chromium.org/7361010/diff/14001/ppapi/thunk/ppb_video_decod...
ppapi/thunk/ppb_video_decoder_thunk.cc:86: if (enter.failed())
On 2011/07/15 16:57:21, brettw wrote:
> In this case I would do:
>   if (enter.succeeded())
>     enter.object()->Destroy();
> To save a line.

Done.