Implement |pic_order_cnt| computation for VTVideoDecode accelerator.

Implement |pic_order_cnt| computation for VTVideoDecode accelerator.

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

[sandersd (OOO until July 31), 2014-11-20 21:23:25]

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

[sandersd (OOO until July 31), 2014-11-20 21:23:25]

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

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

It's the last big CL for VTVideoDecode accelerator. So it's got that going for
it, which is nice.

[DaleCurtis, 2014-11-20 23:03:30]

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
File content/common/gpu/media/h264_poc.cc (right):

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
Definitely needs a unit test in this CL, this is self-contained enough that it
should be easy.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:29: static bool HasMMCO5(const
media::H264SliceHeader& slice_hdr) {
Needs docs.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:36: for (size_t i = 0; i <
arraysize(slice_hdr.ref_pic_marking); i++) {
Why is this a loop? It seems this is all just:

return slice_hdr.ref_pic_marking[0].memory_mgmnt_control_operation == 0 || 5.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:66: bool mmco5 = HasMMCO5(slice_hdr);
Docs or a link to the spec this is implementing?

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:72: switch (sps->pic_order_cnt_type) {
nit: This actually seems like it would look better as a series of if/else
blocks... certainly it would save you some indentation.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:74: {
Move { to above line and save 2 spaces of indent.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:83: // 8-3.
Helps to include summary of this step :) Ditto for all other steps.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:119: // AVC 8.2.1.2.
Again, this is all gibberish to me, so you'll need to include some summary
comments and possibly a link to the spec at the top of the file if possible.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
File content/common/gpu/media/h264_poc.h (right):

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.h:9: struct H264SPS;
Traditionally no indentation here.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.h:17: explicit H264POC();
explicit not necessary for no-argument constructor?

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.h:25: int32_t *pic_order_cnt);
* goes with int32_t.

[sandersd (OOO until July 31), 2014-11-20 23:58:25]

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
File content/common/gpu/media/h264_poc.cc (right):

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:1: // Copyright 2014 The Chromium Authors.
All rights reserved.
On 2014/11/20 23:03:30, DaleCurtis wrote:
> Definitely needs a unit test in this CL, this is self-contained enough that it
> should be easy.

Acknowledged. I'll ping when I have added that.

I won't try for full coverage.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:29: static bool HasMMCO5(const
media::H264SliceHeader& slice_hdr) {
On 2014/11/20 23:03:30, DaleCurtis wrote:
> Needs docs.

Done.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:36: for (size_t i = 0; i <
arraysize(slice_hdr.ref_pic_marking); i++) {
On 2014/11/20 23:03:29, DaleCurtis wrote:
> Why is this a loop? It seems this is all just:
> 
> return slice_hdr.ref_pic_marking[0].memory_mgmnt_control_operation == 0 || 5.

No, it loops until ref_pic_marking[i].mmco == 0, returning true only if it finds
a 5 first. I've rewritten it using ifs, hopefully it is more clear.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:66: bool mmco5 = HasMMCO5(slice_hdr);
On 2014/11/20 23:03:30, DaleCurtis wrote:
> Docs or a link to the spec this is implementing?

Done.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:72: switch (sps->pic_order_cnt_type) {
On 2014/11/20 23:03:30, DaleCurtis wrote:
> nit: This actually seems like it would look better as a series of if/else
> blocks... certainly it would save you some indentation.

I tried it both ways, I think it's more clear as a switch.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:74: {
On 2014/11/20 23:03:29, DaleCurtis wrote:
> Move { to above line and save 2 spaces of indent.

Done.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:83: // 8-3.
On 2014/11/20 23:03:29, DaleCurtis wrote:
> Helps to include summary of this step :) Ditto for all other steps.

Done. At least as much as possible, the steps are rather opaque even with a copy
of the spec handy.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.cc:119: // AVC 8.2.1.2.
On 2014/11/20 23:03:30, DaleCurtis wrote:
> Again, this is all gibberish to me, so you'll need to include some summary
> comments and possibly a link to the spec at the top of the file if possible.

Acknowledged.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
File content/common/gpu/media/h264_poc.h (right):

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.h:9: struct H264SPS;
On 2014/11/20 23:03:30, DaleCurtis wrote:
> Traditionally no indentation here.

Done.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.h:17: explicit H264POC();
On 2014/11/20 23:03:30, DaleCurtis wrote:
> explicit not necessary for no-argument constructor?

Done.

https://codereview.chromium.org/742233002/diff/1/content/common/gpu/media/h26...
content/common/gpu/media/h264_poc.h:25: int32_t *pic_order_cnt);
On 2014/11/20 23:03:30, DaleCurtis wrote:
> * goes with int32_t.

Done.

[DaleCurtis, 2014-11-21 21:42:05]

lgtm % nits. that spec is awful to read :|

https://codereview.chromium.org/742233002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/h264_poc.cc (right):

https://codereview.chromium.org/742233002/diff/20001/content/common/gpu/media...
content/common/gpu/media/h264_poc.cc:44: // Stop at the end of the list.
Is this really the end of the list always? If so, just let it fall out of the
loop?

https://codereview.chromium.org/742233002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/742233002/diff/20001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:666: if (flushing ||
(int32_t)reorder_queue_.size() > reorder_window) {
static_cast, and you're forcing size_t into int32_t... maybe define reorder
window as size_t instead.

[sandersd (OOO until July 31), 2014-11-21 23:33:16]

Now with a unittest.

https://codereview.chromium.org/742233002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/h264_poc.cc (right):

https://codereview.chromium.org/742233002/diff/20001/content/common/gpu/media...
content/common/gpu/media/h264_poc.cc:44: // Stop at the end of the list.
On 2014/11/21 21:42:05, DaleCurtis wrote:
> Is this really the end of the list always? If so, just let it fall out of the
> loop?

Well, the list is zero-terminated, but it might not be as long as the storage
allocated for it.

https://codereview.chromium.org/742233002/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vt_video_decode_accelerator.cc (right):

https://codereview.chromium.org/742233002/diff/20001/content/common/gpu/media...
content/common/gpu/media/vt_video_decode_accelerator.cc:666: if (flushing ||
(int32_t)reorder_queue_.size() > reorder_window) {
On 2014/11/21 21:42:05, DaleCurtis wrote:
> static_cast, and you're forcing size_t into int32_t... maybe define reorder
> window as size_t instead.

Done.

I did it the previous way because there is a strict bound on the size of the
reorder queue, but all the solutions are equally bad.

[DaleCurtis, 2014-11-21 23:39:57]

https://codereview.chromium.org/742233002/diff/60001/media/video/h264_poc_uni...
File media/video/h264_poc_unittest.cc (right):

https://codereview.chromium.org/742233002/diff/60001/media/video/h264_poc_uni...
media/video/h264_poc_unittest.cc:17: sps_.log2_max_frame_num_minus4 = 0;
If the zeros aren't really adding to the readability, just zero initialize sps_
in the constructor? I.e. " : sps_()" ditto for slice_hdr_.  That way you only
need to set the non-zero fields.

[sandersd (OOO until July 31), 2014-11-21 23:44:21]

https://codereview.chromium.org/742233002/diff/60001/media/video/h264_poc_uni...
File media/video/h264_poc_unittest.cc (right):

https://codereview.chromium.org/742233002/diff/60001/media/video/h264_poc_uni...
media/video/h264_poc_unittest.cc:17: sps_.log2_max_frame_num_minus4 = 0;
On 2014/11/21 23:39:57, DaleCurtis wrote:
> If the zeros aren't really adding to the readability, just zero initialize
sps_
> in the constructor? I.e. " : sps_()" ditto for slice_hdr_.  That way you only
> need to set the non-zero fields.

Done.

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

Patchset #5 (id:80001) has been deleted

[DaleCurtis, 2014-11-21 23:51:14]

still lgtm % nits

https://codereview.chromium.org/742233002/diff/100001/media/video/h264_poc_un...
File media/video/h264_poc_unittest.cc (right):

https://codereview.chromium.org/742233002/diff/100001/media/video/h264_poc_un...
media/video/h264_poc_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors.
All rights reserved.
no (c) in new copyrights.

https://codereview.chromium.org/742233002/diff/100001/media/video/h264_poc_un...
media/video/h264_poc_unittest.cc:14: class H264POCTest: public testing::Test {
Add space before :

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

https://codereview.chromium.org/742233002/diff/100001/media/video/h264_poc_un...
File media/video/h264_poc_unittest.cc (right):

https://codereview.chromium.org/742233002/diff/100001/media/video/h264_poc_un...
media/video/h264_poc_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors.
All rights reserved.
On 2014/11/21 23:51:14, DaleCurtis wrote:
> no (c) in new copyrights.

Done.

https://codereview.chromium.org/742233002/diff/100001/media/video/h264_poc_un...
media/video/h264_poc_unittest.cc:14: class H264POCTest: public testing::Test {
On 2014/11/21 23:51:14, DaleCurtis wrote:
> Add space before :

Done.

[sandersd (OOO until July 31), 2014-11-26 22:16:50]

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

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

The CQ bit was checked by sandersd@chromium.org

[commit-bot: I haz the power, 2014-11-26 23:22:40]

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

[commit-bot: I haz the power, 2014-11-27 00:25:26]

Committed patchset #7 (id:140001)

[commit-bot: I haz the power, 2014-11-27 00:26:03]

Patchset 7 (id:??) landed as
https://crrev.com/28a0919f30f41ec5f58b6f445ee3c88f0ea50b5f
Cr-Commit-Position: refs/heads/master@{#305923}