Implement a reorder queue in VTVideoDecodeAccelerator.

Implement a reorder queue in VTVideoDecodeAccelerator.

This CL does not implement that actual computation of the order, becuase
the original plan of just copying the implementation from the VAAPI was
buggy. A second CL will follow with the computation once the bugs are
fully understood.

BUG=133828
Committed: https://crrev.com/e01b2b9885d764591a3fd2b67f76ccd3c5e2248b
Cr-Commit-Position: refs/heads/master@{#305905}

[sandersd (OOO until July 31), 2014-11-14 19:14:44]

sandersd@chromium.org changed reviewers:
+ dalecurtis@chromium.org, posciak@chromium.org

[sandersd (OOO until July 31), 2014-11-14 19:14:45]

sandersd@chromium.org changed required reviewers:
+ dalecurtis@chromium.org, posciak@chromium.org

[DaleCurtis, 2014-11-14 19:27:48]

Haven't dug through it yet, but is there a platform agnostic unit test which
covers this code? If not is it possible to add one for this code?

You've got a semi-complex state machine with reordering now, so having some
basic tests would be great :)

[sandersd (OOO until July 31), 2014-11-14 19:44:52]

On 2014/11/14 19:27:48, DaleCurtis wrote:
> Haven't dug through it yet, but is there a platform agnostic unit test which
> covers this code? If not is it possible to add one for this code?
> 
> You've got a semi-complex state machine with reordering now, so having some
> basic tests would be great :)

There is a "unit" test, work on integrating it was abandoned for two reasons:
(1) it doesn't have CGL support, and implementing that was taking a while and
(2) it wasn't compatible with our frame reordering. With (2) nearly solved, I'll
take another stab at (1) before 41.

There are some functional tests of this system, and we're not yet plugged into
any of them.

I'd also like to do some stress testing (ie, try lots of different videos) next
week.

[DaleCurtis, 2014-11-14 21:11:45]

Mostly looks good to me, but needs some docs and is still blocked on submission
of the first CL pending posciak's comments.

https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
content/common/gpu/media/vt_video_decode_accelerator.cc:570: while
(ProcessReorderQueue() || ProcessTaskQueue());
Is this something that could block for a long time? Possibly better off with a
PostTask past some limit?

https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
File content/common/gpu/media/vt_video_decode_accelerator.h (right):

https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
content/common/gpu/media/vt_video_decode_accelerator.h:78: int32_t
pic_order_cnt;
Needs docs.  It'd be nice for all of them too.

[sandersd (OOO until July 31), 2014-11-14 21:28:52]

https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
content/common/gpu/media/vt_video_decode_accelerator.cc:570: while
(ProcessReorderQueue() || ProcessTaskQueue());
On 2014/11/14 21:11:45, DaleCurtis wrote:
> Is this something that could block for a long time? Possibly better off with a
> PostTask past some limit?

This would be worth investigating. I do expect this processing to be relatively
short.

It is bounded by the size of the task queue, since that cannot grow during
processing, and by the number of available pictures, as the reorder queue will
fill up once they are gone.

In the common case, less than three items (reorder queue or task queue items)
would be processed. The extreme cases are:
  - Flush arrives, entire reorder queue will be sent and then possibly refilled.
(~10 items processed, if there were enough available pictures.)
  - Pictures are assigned, most of the reorder queue is sent and refilled. (~10
again, as pictures are requested 5 at a time.)

https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
File content/common/gpu/media/vt_video_decode_accelerator.h (right):

https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
content/common/gpu/media/vt_video_decode_accelerator.h:78: int32_t
pic_order_cnt;
On 2014/11/14 21:11:45, DaleCurtis wrote:
> Needs docs.  It'd be nice for all of them too.

Done.

[DaleCurtis, 2014-11-14 21:41:54]

https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
content/common/gpu/media/vt_video_decode_accelerator.cc:570: while
(ProcessReorderQueue() || ProcessTaskQueue());
On 2014/11/14 21:28:52, sandersd wrote:
> On 2014/11/14 21:11:45, DaleCurtis wrote:
> > Is this something that could block for a long time? Possibly better off with
a
> > PostTask past some limit?
> 
> This would be worth investigating. I do expect this processing to be
relatively
> short.
> 
> It is bounded by the size of the task queue, since that cannot grow during
> processing, and by the number of available pictures, as the reorder queue will
> fill up once they are gone.
> 
> In the common case, less than three items (reorder queue or task queue items)
> would be processed. The extreme cases are:
>   - Flush arrives, entire reorder queue will be sent and then possibly
refilled.
> (~10 items processed, if there were enough available pictures.)
>   - Pictures are assigned, most of the reorder queue is sent and refilled.
(~10
> again, as pictures are requested 5 at a time.)

More important than the length is the time taken. Do you have estimates for how
long this takes? This is on the main gpu thread as far as I can tell, which
seems like it could impact compositing.

[sandersd (OOO until July 31), 2014-11-14 21:54:17]

Patchset #3 (id:40001) has been deleted

[sandersd (OOO until July 31), 2014-11-14 21:58:09]

On 2014/11/14 21:41:54, DaleCurtis wrote:
>
https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
> File content/common/gpu/media/vt_video_decode_accelerator.cc (right):
> 
>
https://codereview.chromium.org/727893002/diff/1/content/common/gpu/media/vt_...
> content/common/gpu/media/vt_video_decode_accelerator.cc:570: while
> (ProcessReorderQueue() || ProcessTaskQueue());
> On 2014/11/14 21:28:52, sandersd wrote:
> > On 2014/11/14 21:11:45, DaleCurtis wrote:
> > > Is this something that could block for a long time? Possibly better off
with
> a
> > > PostTask past some limit?
> > 
> > This would be worth investigating. I do expect this processing to be
> relatively
> > short.
> > 
> > It is bounded by the size of the task queue, since that cannot grow during
> > processing, and by the number of available pictures, as the reorder queue
will
> > fill up once they are gone.
> > 
> > In the common case, less than three items (reorder queue or task queue
items)
> > would be processed. The extreme cases are:
> >   - Flush arrives, entire reorder queue will be sent and then possibly
> refilled.
> > (~10 items processed, if there were enough available pictures.)
> >   - Pictures are assigned, most of the reorder queue is sent and refilled.
> (~10
> > again, as pictures are requested 5 at a time.)
> 
> More important than the length is the time taken. Do you have estimates for
how
> long this takes? This is on the main gpu thread as far as I can tell, which
> seems like it could impact compositing.

While I can't yet say what the extremes are, I added some tracing and I didn't
see that while loop take more than 0.15ms during playback. So assuming the
extremes are 10x worse, it shouldn't need to be optimized yet.

[DaleCurtis, 2014-11-14 22:00:54]

1.5ms seems fine to me for extreme (and hopefully unlikely cases), for posterity
60fps == ~16.7ms per frame.

lgtm % the promise that some unit tests exist before this is actually enabled.

[sandersd (OOO until July 31), 2014-11-14 22:04:41]

On 2014/11/14 22:00:54, DaleCurtis wrote:
> 1.5ms seems fine to me for extreme (and hopefully unlikely cases), for
posterity
> 60fps == ~16.7ms per frame.
> 
> lgtm % the promise that some unit tests exist before this is actually enabled.

Wait, I never promised that, I only promised before the branch cut!

[DaleCurtis, 2014-11-14 22:07:50]

Should I take back my approval then? :p (that's fine though, we have agnostic
tests it seems)

[sandersd (OOO until July 31), 2014-11-19 23:22:08]

posciak: ping?

[sandersd (OOO until July 31), 2014-11-26 21:24:27]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-11-26 21:25:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727893002/80001

[commit-bot: I haz the power, 2014-11-26 21:25:47]

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

[commit-bot: I haz the power, 2014-11-26 21:25:49]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[sandersd (OOO until July 31), 2014-11-26 21:28:15]

sandersd@chromium.org changed required reviewers:
- posciak@chromium.org

[sandersd (OOO until July 31), 2014-11-26 21:28:27]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-11-26 21:29:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727893002/80001

[commit-bot: I haz the power, 2014-11-26 21:34:03]

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

[commit-bot: I haz the power, 2014-11-26 21:34:04]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/86990)
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[sandersd (OOO until July 31), 2014-11-26 21:51:10]

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-11-26 21:52:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/727893002/100001

[commit-bot: I haz the power, 2014-11-26 22:38:29]

Committed patchset #5 (id:100001)

[commit-bot: I haz the power, 2014-11-26 22:39:08]

Patchset 5 (id:??) landed as
https://crrev.com/e01b2b9885d764591a3fd2b67f76ccd3c5e2248b
Cr-Commit-Position: refs/heads/master@{#305905}