Start the VDA device poll thread in Initialize()

Start the VDA device poll thread in Initialize()

We may need to be returning completed input buffers from the device while we run
DecodeBufferInitial() to set up the decoder.  Start the device thread then in
Initialize() so the device is polled for input buffers to return.

This is can be necessary in some cases where the decoder may run out of buffers
to enqueue inputs on while scanning for valid input starting positions.

BUG=None
TEST=local build, run on CrOS snow

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258097

[sheu, 2014-03-07 04:05:22]

posciak@: PTAL.  This should fix the stuck business for Tegra VDA.

[Pawel Osciak, 2014-03-07 06:14:46]

https://chromiumcodereview.appspot.com/189993002/diff/1/content/common/gpu/me...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left):

https://chromiumcodereview.appspot.com/189993002/diff/1/content/common/gpu/me...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416:
NOTIFY_ERROR(PLATFORM_FAILURE);
Could you please explain/add to CL description why we had to move this out of
here? It's not clear to me. Initialize() does report PLATFORM_FAILURE too, so
it's not because it's too early to do this from Initialize(), right?

[Pawel Osciak, 2014-03-07 06:15:17]

On 2014/03/07 04:05:22, sheu wrote:
> posciak@: PTAL.  This should fix the stuck business for Tegra VDA.

Please make sure to test with vdatest if you haven't and also MSE.

[sheu, 2014-03-07 23:51:05]

Updated.  PTAL

https://chromiumcodereview.appspot.com/189993002/diff/1/content/common/gpu/me...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left):

https://chromiumcodereview.appspot.com/189993002/diff/1/content/common/gpu/me...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416:
NOTIFY_ERROR(PLATFORM_FAILURE);
On 2014/03/07 06:14:47, Pawel Osciak wrote:
> Could you please explain/add to CL description why we had to move this out of
> here? It's not clear to me. Initialize() does report PLATFORM_FAILURE too, so
> it's not because it's too early to do this from Initialize(), right?

D'oh.  I was thinking of:

  https://chromiumcodereview.appspot.com/185403020/

after which Initialize() is synchronous and returns bool instead of erroring.

Will fix.

[Pawel Osciak, 2014-03-09 02:34:50]

https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left):

https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416:
NOTIFY_ERROR(PLATFORM_FAILURE);
Could you instead just:

if (state != kUninitialized)
  NOTIFY_ERROR()

?

[sheu, 2014-03-10 19:38:42]

https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left):

https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416:
NOTIFY_ERROR(PLATFORM_FAILURE);
On 2014/03/09 02:34:50, Pawel Osciak wrote:
> Could you instead just:
> 
> if (state != kUninitialized)
>   NOTIFY_ERROR()
> 
> ?

Not sure what you mean here.  We should error out also if thread start fails.

[Pawel Osciak, 2014-03-11 00:07:43]

https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left):

https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416:
NOTIFY_ERROR(PLATFORM_FAILURE);
On 2014/03/10 19:38:42, sheu wrote:
> On 2014/03/09 02:34:50, Pawel Osciak wrote:
> > Could you instead just:
> > 
> > if (state != kUninitialized)
> >   NOTIFY_ERROR()
> > 
> > ?
> 
> Not sure what you mean here.  We should error out also if thread start fails.

Sorry, should have been more precise. The issue here is that you had to move
NOTIFY_ERROR out of {Start,Stop}DevicePoll because you didn't want to
NOTIFY_ERROR in Initialize().

What I'm suggesting is to keep NOTIFY_ERROR in those methods, but only call it
if state is not kUnintialized (i.e. it's ok to NOTIFY_ERROR).

Or maybe even better, check for it in the actual NOTIFY_ERROR macro?

[shivdasp, 2014-03-12 12:12:26]

https://codereview.chromium.org/189993002/diff/10001/content/common/gpu/media...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://codereview.chromium.org/189993002/diff/10001/content/common/gpu/media...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:289: 
It seems there is a race condition here. The DevicePollTask() can get scheduled
before this thread creates the decoder_thread_ and access to decoder_thread_ in
DevicePollTask() will seg fault.
StartDevicePoll() if done after decoder_thread_ starts.

[sheu, 2014-03-12 20:51:21]

https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (left):

https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:1416:
NOTIFY_ERROR(PLATFORM_FAILURE);
On 2014/03/11 00:07:44, Pawel Osciak wrote:
> On 2014/03/10 19:38:42, sheu wrote:
> > On 2014/03/09 02:34:50, Pawel Osciak wrote:
> > > Could you instead just:
> > > 
> > > if (state != kUninitialized)
> > >   NOTIFY_ERROR()
> > > 
> > > ?
> > 
> > Not sure what you mean here.  We should error out also if thread start
fails.
> 
> Sorry, should have been more precise. The issue here is that you had to move
> NOTIFY_ERROR out of {Start,Stop}DevicePoll because you didn't want to
> NOTIFY_ERROR in Initialize().
> 
> What I'm suggesting is to keep NOTIFY_ERROR in those methods, but only call it
> if state is not kUnintialized (i.e. it's ok to NOTIFY_ERROR).
> 
> Or maybe even better, check for it in the actual NOTIFY_ERROR macro?

I'd be more inclined to check in the macro, but even then I'm not really
inclined to conditionally do NOTIFY_ERROR.  If a call is made in a location
where state matters, I'd prefer that the caller take care of doing things that
are appropriate for the state, especially since the state is part of decoder
thread state and it would be a whole 'nother can of worms to make that state
safe to query from the child thread as well.

https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp...
File content/common/gpu/media/v4l2_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/189993002/diff/10001/content/common/gp...
content/common/gpu/media/v4l2_video_decode_accelerator.cc:289: 
On 2014/03/12 12:12:27, shivdasp wrote:
> It seems there is a race condition here. The DevicePollTask() can get
scheduled
> before this thread creates the decoder_thread_ and access to decoder_thread_
in
> DevicePollTask() will seg fault.
> StartDevicePoll() if done after decoder_thread_ starts.

You're completely correct here.  Thanks.  Fixed.

[sheu, 2014-03-13 00:10:17]

For the record, I did run vda_unittests, flash video, and html5 video
(with/without MSE).  The race just didn't come up.

PTAL, in any case!

[sheu, 2014-03-13 06:23:49]

Updated, per discussion.

[Pawel Osciak, 2014-03-14 04:45:59]

lgtm

Thanks for updating.

[sheu, 2014-03-14 06:54:34]

Adding fischman@ for owners.

PTAL, thanks!

[Ami GONE FROM CHROMIUM, 2014-03-16 01:42:18]

LGTM % please update description to describe why the new functionality is
interesting?
(hinted at in your first reviewlog message, but not spelled out)

[sheu, 2014-03-18 22:43:26]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-03-18 22:47:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/189993002/50001

[commit-bot: I haz the power, 2014-03-18 22:55:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-18 22:55:18]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[sheu, 2014-03-19 19:05:22]

The CQ bit was checked by sheu@chromium.org

[commit-bot: I haz the power, 2014-03-19 19:06:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sheu@chromium.org/189993002/50001

[commit-bot: I haz the power, 2014-03-19 21:16:18]

Change committed as 258097