H264Decoder: Handle gaps in frame_num.

H264Decoder: Handle gaps in frame_num.

Add support for gaps in frame_num as per H264 spec. Also, switch to a
more precise new picture detection. Finally, some general cleanup.

BUG=181565
TEST=vdatest

Committed: https://crrev.com/8f3237aaf21e53ab7b9f4e2ed7057a523cf4a206
Cr-Commit-Position: refs/heads/master@{#353936}

[Pawel Osciak, 2015-09-25 07:51:32]

posciak@chromium.org changed reviewers:
+ kcwu@chromium.org, wuchengli@chromium.org

[Pawel Osciak, 2015-09-25 07:51:33]

ptal

[niklase, 2015-09-29 20:25:47]

niklase@google.com changed reviewers:
+ niklase@google.com

[niklase, 2015-09-29 20:25:48]

Any action on the review side?

[kcwu, 2015-09-30 11:57:24]

On 2015/09/29 20:25:48, niklase wrote:
> Any action on the review side?

Sorry for the delay. Taipei office was closed due to typhoon. I started to
review but haven't finished yet.

[kcwu, 2015-10-02 13:06:04]

Basically lgtm
I'm not familiar with h264 yet, so just review in general.

https://codereview.chromium.org/1369673002/diff/1/content/common/gpu/media/h2...
File content/common/gpu/media/h264_decoder.cc (right):

https://codereview.chromium.org/1369673002/diff/1/content/common/gpu/media/h2...
content/common/gpu/media/h264_decoder.cc:304: case 2:
{} block since temp_pic_order_cnt defiend inside case 2.

https://codereview.chromium.org/1369673002/diff/1/content/common/gpu/media/h2...
content/common/gpu/media/h264_decoder.cc:1174: // 7.4.1.2.4, assumes
non-interlaced.
why IsNewPrimaryCodedPicture doesn't compare the value of pic_parameter_set_id,
pic_parameter_set_id, etc. as spec? are those value checking implicitly implied
in the code flow?

https://codereview.chromium.org/1369673002/diff/1/content/common/gpu/media/h2...
File content/common/gpu/media/h264_decoder.h (right):

https://codereview.chromium.org/1369673002/diff/1/content/common/gpu/media/h2...
content/common/gpu/media/h264_decoder.h:139: bool IsNewPrimaryCodedPicture(const
media::H264SliceHeader* slice_hdr);
const method

https://codereview.chromium.org/1369673002/diff/1/content/common/gpu/media/v4...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1369673002/diff/1/content/common/gpu/media/v4...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1781: int index
= VIDEO_MAX_FRAME;
Why it is okay to set buf_idx=VIDEO_MAX_FRAME for nonexisting frame? Is is just
dummy unused value?

[Pawel Osciak, 2015-10-09 06:39:25]

https://chromiumcodereview.appspot.com/1369673002/diff/1/content/common/gpu/m...
File content/common/gpu/media/h264_decoder.cc (right):

https://chromiumcodereview.appspot.com/1369673002/diff/1/content/common/gpu/m...
content/common/gpu/media/h264_decoder.cc:304: case 2:
On 2015/10/02 13:06:03, kcwu wrote:
> {} block since temp_pic_order_cnt defiend inside case 2.

Done.

https://chromiumcodereview.appspot.com/1369673002/diff/1/content/common/gpu/m...
content/common/gpu/media/h264_decoder.cc:1174: // 7.4.1.2.4, assumes
non-interlaced.
On 2015/10/02 13:06:03, kcwu wrote:
> why IsNewPrimaryCodedPicture doesn't compare the value of
pic_parameter_set_id,
> pic_parameter_set_id, etc. as spec? are those value checking implicitly
implied
> in the code flow?

Done.

https://chromiumcodereview.appspot.com/1369673002/diff/1/content/common/gpu/m...
File content/common/gpu/media/h264_decoder.h (right):

https://chromiumcodereview.appspot.com/1369673002/diff/1/content/common/gpu/m...
content/common/gpu/media/h264_decoder.h:139: bool IsNewPrimaryCodedPicture(const
media::H264SliceHeader* slice_hdr);
On 2015/10/02 13:06:03, kcwu wrote:
> const method

Done.

https://chromiumcodereview.appspot.com/1369673002/diff/1/content/common/gpu/m...
File content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1369673002/diff/1/content/common/gpu/m...
content/common/gpu/media/v4l2_slice_video_decode_accelerator.cc:1781: int index
= VIDEO_MAX_FRAME;
On 2015/10/02 13:06:04, kcwu wrote:
> Why it is okay to set buf_idx=VIDEO_MAX_FRAME for nonexisting frame? Is is
just
> dummy unused value?

VIDEO_MAX_FRAME is used to indicate an unused entry, as it's not a valid buffer
id.

[Pawel Osciak, 2015-10-09 06:40:41]

posciak@chromium.org changed reviewers:
+ xhwang@chromium.org

[Pawel Osciak, 2015-10-09 06:41:16]

xhwang: please OWNERS for media/filters. Thanks.

[xhwang, 2015-10-12 18:01:18]

media/filters lgtm. I also have some nits in other code.

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
File content/common/gpu/media/h264_decoder.cc (right):

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.cc:101: bool
H264Decoder::InitNonexistingPicture(scoped_refptr<H264Picture> pic,
nit: in media code, we pass const-ref of scoped_refptr to avoid extra
AddRef/Release. But feel free to follow the convention of existing code here :)

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.cc:144: switch
(curr_pic_->pic_order_cnt_type) {
nit: can you have an enum for |pic_order_cnt_type|? It's pretty hard to read
"case 0/1/2".

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.cc:191: case 0: {
ditto

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.cc:1073: return false;
In what case could this fail? (wondering why we replaced the DCHECK)

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.cc:1157: if (!InitNonexistingPicture(pic,
unused_short_term_frame_num))
Can we follow the RAII
(https://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization) rule
here? e.g. combine the above two lines into a single factory function, something
like:

static scoped_refptr<H264Picture> pic =
H264Picture::CreateNonexistingPicture(unused_short_term_frame_num);
if (!pic)
 return false;

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
File content/common/gpu/media/h264_decoder.h (right):

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.h:139: bool IsNewPrimaryCodedPicture(const
media::H264SliceHeader* slice_hdr) const;
Here and throughout this CL, for input only parameters (e.g. slice_hdr), pass as
const ref instead of const pointer?

[Pawel Osciak, 2015-10-13 01:23:22]

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
File content/common/gpu/media/h264_decoder.cc (right):

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.cc:101: bool
H264Decoder::InitNonexistingPicture(scoped_refptr<H264Picture> pic,
On 2015/10/12 18:01:17, xhwang wrote:
> nit: in media code, we pass const-ref of scoped_refptr to avoid extra
> AddRef/Release. But feel free to follow the convention of existing code here
:)

Yes, I'd like to do that too, but for now wanted to follow the convention and
refactor in later CLs.

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.cc:144: switch
(curr_pic_->pic_order_cnt_type) {
On 2015/10/12 18:01:17, xhwang wrote:
> nit: can you have an enum for |pic_order_cnt_type|? It's pretty hard to read
> "case 0/1/2".

This is intentional, as the H264 spec uses "pic order count type 0/1/2"
explicitly, and for people familiar with the spec this would be the most
understandable. I feel in this case coming up with descriptive names would be
confusing.

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.cc:1073: return false;
On 2015/10/12 18:01:17, xhwang wrote:
> In what case could this fail? (wondering why we replaced the DCHECK)

Previously we were using std::map::operator[], which would create an element in
the map if the id was not in it. It would've been empty, but we wouldn't crash.
But that wasn't the best idea, much better to explicitly find() and fail if
given id isn't present. That's what I did in this change so now we should check
for nullptr.

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.cc:1157: if (!InitNonexistingPicture(pic,
unused_short_term_frame_num))
On 2015/10/12 18:01:17, xhwang wrote:
> Can we follow the RAII
> (https://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization) rule
> here? e.g. combine the above two lines into a single factory function,
something
> like:
> 
> static scoped_refptr<H264Picture> pic =
> H264Picture::CreateNonexistingPicture(unused_short_term_frame_num);
> if (!pic)
>  return false;

My intention with H264Picture was to keep it as POD-like as possible and keep
H264 decoder details in H264Decoder. InitNonexistingPicture() doesn't need much,
but its equivalent for the regular pictures, InitCurrPicture(), requires
H264Decoder class state and methods, which would not be feasible to pass to
H264Picture and teach it about details of H264Decoder inner workings. Moreover,
InitCurrPicture() doesn't actually produce a new H264Picture, but uses one
passed to us by the AcceleratedVideoDecoder. So I preferred to keep
InitNonexistingPicture() symmetric.

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
File content/common/gpu/media/h264_decoder.h (right):

https://chromiumcodereview.appspot.com/1369673002/diff/20001/content/common/g...
content/common/gpu/media/h264_decoder.h:139: bool IsNewPrimaryCodedPicture(const
media::H264SliceHeader* slice_hdr) const;
On 2015/10/12 18:01:17, xhwang wrote:
> Here and throughout this CL, for input only parameters (e.g. slice_hdr), pass
as
> const ref instead of const pointer?

I agree and had considered this, but wanted to keep refactoring in this CL to a
minimum, as it would have touched a lot of lines of code and this is already a
relatively complicated and sensitive change, and would complicate review. But I
fully agree and were actually planning to do more refactoring on this class as
this is quite old code and could use it, but would prefer to do that in separate
CLs.

[xhwang, 2015-10-13 17:22:43]

Thanks for the detailed info. All make sense to me. Still lgtm.

[Pawel Osciak, 2015-10-14 00:15:32]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2015-10-14 00:15:33]

The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@chromium.org
Link to the patchset: https://chromiumcodereview.appspot.com/1369673002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-10-14 00:16:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1369673002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1369673002/20001

[commit-bot: I haz the power, 2015-10-14 01:17:57]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-10-14 01:20:30]

Patchset 2 (id:??) landed as
https://crrev.com/8f3237aaf21e53ab7b9f4e2ed7057a523cf4a206
Cr-Commit-Position: refs/heads/master@{#353936}