V4L2VEA: Improve H264 stream header handling.

V4L2VEA: Improve H264 stream header handling.

Currently, V4L2VEA assumes that the first H264 bitstream buffer coming from
the hardware encoder will contain exactly an SPS+PPS pair. It then caches
that buffer and prepends its contents before each keyframe. It also uses it
for the remainder of the session, even if additional/new SPS+PPS pairs are
provided later on.

However, the first buffer may contain additional NALUs, and only SPS+PPS
should be extracted and used. Moreover, the SPS+PPS pair may change later
on. Finally, some hardware encoders may support prepending each IDR with
the header, without the need for V4L2VEA to handle this.

This CL adds support for the V4L2_CID_MPEG_VIDEO_H264_SPS_PPS_BEFORE_IDR
V4L2 control, which instructs the HW encoder to inject SPS+PPS pair before
each IDR in the stream. If that control is supported, we do not have to
inject the header ourselves.

If the control is not supported, we need to keep injecting, however -
instead of making the above assumptions - we parse the stream, cache the
latest SPS and PPS, and inject them before any found IDR, if possible.

The VEA unittest is also modified, to verify that the stream includes an
SPS+PPS pair before each IDR.

Also, remove usage of linked_ptr for clearer ownership management of
buffers.

BUG=639238
TEST=veatest on various ARM platforms

Committed: https://crrev.com/db74e2bf3a0bb5a7b4e2c4ed6b14e75ffbcffcc3
Cr-Commit-Position: refs/heads/master@{#414332}

[Pawel Osciak, 2016-08-23 08:49:36]

Patchset #1 (id:1) has been deleted

[Pawel Osciak, 2016-08-23 08:55:35]

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

[kcwu, 2016-08-23 10:02:22]

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:70: : at_device(false),
buffer_ref(nullptr), address(nullptr), length(0) {}
Isn't buffer_ref nullptr by default ctor?

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:405: namespace {
Move to beginning of this file?
Looks like no rules about the ordering in style guide, though?

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:418: size_t size_to_copy = src_size +
kH264StartCodeSize;
I'd like to write
kH264StartCodeSize + src_size
to align with copy order. Anyway, up to you.

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:444: DVLOG(1) << "Output data did not
fit in the BitstreamBuffer";
Original code will still copy up to remaining_dst_size. Should we keep this
behavior?

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:455: while
(parser.AdvanceToNextNALU(&nalu) == H264Parser::kOk) {
Two questions:
1. Are SPS and PPS always before IDRSlice?
2. How much data after IDRSlice? Should we optimize it? For example, copy rest
data after IDRSlice once we found IDRSlice.

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:481: cached_h264_header_size_ +
nalu.size + kH264StartCodeSize >=
== is ok?

[kcwu, 2016-08-24 07:16:45]

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:455: while
(parser.AdvanceToNextNALU(&nalu) == H264Parser::kOk) {
On 2016/08/23 10:02:22, kcwu wrote:
> Two questions:
> 1. Are SPS and PPS always before IDRSlice?
> 2. How much data after IDRSlice? Should we optimize it? For example, copy rest
> data after IDRSlice once we found IDRSlice.

Please ignore above questions. I learned how it works per chat with pawel
offline.

[Pawel Osciak, 2016-08-24 07:42:51]

ptal

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:70: : at_device(false),
buffer_ref(nullptr), address(nullptr), length(0) {}
On 2016/08/23 10:02:21, kcwu wrote:
> Isn't buffer_ref nullptr by default ctor?

Good point.

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:405: namespace {
On 2016/08/23 10:02:21, kcwu wrote:
> Move to beginning of this file?
> Looks like no rules about the ordering in style guide, though?

Done.

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:418: size_t size_to_copy = src_size +
kH264StartCodeSize;
On 2016/08/23 10:02:21, kcwu wrote:
> I'd like to write
> kH264StartCodeSize + src_size
> to align with copy order. Anyway, up to you.

Yes, I like that too. Done.

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:444: DVLOG(1) << "Output data did not
fit in the BitstreamBuffer";
On 2016/08/23 10:02:22, kcwu wrote:
> Original code will still copy up to remaining_dst_size. Should we keep this
> behavior?

0-sized buffer will tell the client we dropped something more clearly I feel.

https://codereview.chromium.org/2274493002/diff/20001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:481: cached_h264_header_size_ +
nalu.size + kH264StartCodeSize >=
On 2016/08/23 10:02:22, kcwu wrote:
> == is ok?

Done.

[Pawel Osciak, 2016-08-24 07:43:54]

The CQ bit was checked by posciak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 07:44:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kcwu, 2016-08-24 08:15:48]

nits

https://codereview.chromium.org/2274493002/diff/40001/media/gpu/v4l2_video_en...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2274493002/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:81: }
}  // namespace

https://codereview.chromium.org/2274493002/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:482: remaining_dst_size)
DVLOG(1) to say we skip inserting SPS and PPS due to space is not enough.

[kcwu, 2016-08-24 08:24:57]

lgtm first since only nits remains

[Pawel Osciak, 2016-08-24 08:27:04]

https://codereview.chromium.org/2274493002/diff/40001/media/gpu/v4l2_video_en...
File media/gpu/v4l2_video_encode_accelerator.cc (right):

https://codereview.chromium.org/2274493002/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:81: }
On 2016/08/24 08:15:48, kcwu wrote:
> }  // namespace

Done.

https://codereview.chromium.org/2274493002/diff/40001/media/gpu/v4l2_video_en...
media/gpu/v4l2_video_encode_accelerator.cc:482: remaining_dst_size)
On 2016/08/24 08:15:48, kcwu wrote:
> DVLOG(1) to say we skip inserting SPS and PPS due to space is not enough.

Done.

[Pawel Osciak, 2016-08-24 08:27:11]

The CQ bit was checked by posciak@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-24 08:27:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-24 09:50:28]

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

[commit-bot: I haz the power, 2016-08-24 09:50:29]

Dry run: This issue passed the CQ dry run.

[Pawel Osciak, 2016-08-25 05:43:08]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2016-08-25 05:43:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-25 05:47:51]

Committed patchset #3 (id:60001)

[commit-bot: I haz the power, 2016-08-25 05:50:14]

Description was changed from

==========
V4L2VEA: Improve H264 stream header handling.

Currently, V4L2VEA assumes that the first H264 bitstream buffer coming from
the hardware encoder will contain exactly an SPS+PPS pair. It then caches
that buffer and prepends its contents before each keyframe. It also uses it
for the remainder of the session, even if additional/new SPS+PPS pairs are
provided later on.

However, the first buffer may contain additional NALUs, and only SPS+PPS
should be extracted and used. Moreover, the SPS+PPS pair may change later
on. Finally, some hardware encoders may support prepending each IDR with
the header, without the need for V4L2VEA to handle this.

This CL adds support for the V4L2_CID_MPEG_VIDEO_H264_SPS_PPS_BEFORE_IDR
V4L2 control, which instructs the HW encoder to inject SPS+PPS pair before
each IDR in the stream. If that control is supported, we do not have to
inject the header ourselves.

If the control is not supported, we need to keep injecting, however -
instead of making the above assumptions - we parse the stream, cache the
latest SPS and PPS, and inject them before any found IDR, if possible.

The VEA unittest is also modified, to verify that the stream includes an
SPS+PPS pair before each IDR.

Also, remove usage of linked_ptr for clearer ownership management of
buffers.

BUG=639238
TEST=veatest on various ARM platforms
==========

to

==========
V4L2VEA: Improve H264 stream header handling.

Currently, V4L2VEA assumes that the first H264 bitstream buffer coming from
the hardware encoder will contain exactly an SPS+PPS pair. It then caches
that buffer and prepends its contents before each keyframe. It also uses it
for the remainder of the session, even if additional/new SPS+PPS pairs are
provided later on.

However, the first buffer may contain additional NALUs, and only SPS+PPS
should be extracted and used. Moreover, the SPS+PPS pair may change later
on. Finally, some hardware encoders may support prepending each IDR with
the header, without the need for V4L2VEA to handle this.

This CL adds support for the V4L2_CID_MPEG_VIDEO_H264_SPS_PPS_BEFORE_IDR
V4L2 control, which instructs the HW encoder to inject SPS+PPS pair before
each IDR in the stream. If that control is supported, we do not have to
inject the header ourselves.

If the control is not supported, we need to keep injecting, however -
instead of making the above assumptions - we parse the stream, cache the
latest SPS and PPS, and inject them before any found IDR, if possible.

The VEA unittest is also modified, to verify that the stream includes an
SPS+PPS pair before each IDR.

Also, remove usage of linked_ptr for clearer ownership management of
buffers.

BUG=639238
TEST=veatest on various ARM platforms

Committed: https://crrev.com/db74e2bf3a0bb5a7b4e2c4ed6b14e75ffbcffcc3
Cr-Commit-Position: refs/heads/master@{#414332}
==========

[commit-bot: I haz the power, 2016-08-25 05:50:15]

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