Vaapi VEA: always generate IDR when keyframe is requested.

Vaapi VEA: always generate IDR when keyframe is requested.

We need a full IDR frame (not just I slices) when requesting keyframe because
we need all the reference buffers to be reset.

BUG=424443
TEST=verify with video_encode_accelerator_unittest and H264 test app on squawks and daisy

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

[hshi1, 2014-10-29 21:17:03]

hshi@chromium.org changed reviewers:
+ holmer@chromium.org, posciak@chromium.org

[hshi1, 2014-10-29 21:17:04]

PTAL - per our discussion in the bug, we need full IDR (not just I slice) for
keyframes.

[Pawel Osciak, 2014-10-30 00:09:46]

Please also test with the unittest.

https://codereview.chromium.org/686283002/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_video_encode_accelerator.cc (left):

https://codereview.chromium.org/686283002/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_video_encode_accelerator.cc:294: if
(current_pic_.frame_num % idr_period_ == 0) {
This is generating IDR on each i_period, which is not what we want. You want to
generate IDR only on force_keyframe or idr_period, but only an I frame on each
i_period.

[hshi1, 2014-10-30 00:18:01]

Question on unittest: are you suggesting that I add a new file
vaapi_video_encode_accelerator_unittest.cc to verify that idr and
last_idr_frame_num_ are set properly and ref_pic_list is reset, or to run the
test on an actual Intel encoder then parse the bitstream?

https://codereview.chromium.org/686283002/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_video_encode_accelerator.cc (left):

https://codereview.chromium.org/686283002/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_video_encode_accelerator.cc:294: if
(current_pic_.frame_num % idr_period_ == 0) {
On 2014/10/30 00:09:46, Pawel Osciak wrote:
> This is generating IDR on each i_period, which is not what we want. You want
to
> generate IDR only on force_keyframe or idr_period, but only an I frame on each
> i_period.

Done.

[Pawel Osciak, 2014-10-30 00:38:46]

On 2014/10/30 00:18:01, hshi1 wrote:
> Question on unittest: are you suggesting that I add a new file
> vaapi_video_encode_accelerator_unittest.cc to verify that idr and
> last_idr_frame_num_ are set properly and ref_pic_list is reset, or to run the
> test on an actual Intel encoder then parse the bitstream?
> 

The existing video_encode_accelerator_unittest uses a parser and a
BitstreamValidator to test if the required NALUs are in the stream when a
keyframe is requested. Please update it to only treat IDRs as keyframes, instead
of both I frames and IDRs.

[hshi1, 2014-10-30 01:00:26]

On 2014/10/30 00:38:46, Pawel Osciak wrote:
> On 2014/10/30 00:18:01, hshi1 wrote:
> > Question on unittest: are you suggesting that I add a new file
> > vaapi_video_encode_accelerator_unittest.cc to verify that idr and
> > last_idr_frame_num_ are set properly and ref_pic_list is reset, or to run
the
> > test on an actual Intel encoder then parse the bitstream?
> > 
> 
> The existing video_encode_accelerator_unittest uses a parser and a
> BitstreamValidator to test if the required NALUs are in the stream when a
> keyframe is requested. Please update it to only treat IDRs as keyframes,
instead
> of both I frames and IDRs.

I see, uploaded Patch Set #3 PTAL thanks!

[Pawel Osciak, 2014-10-30 01:38:33]

https://codereview.chromium.org/686283002/diff/40001/content/common/gpu/media...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/686283002/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_encode_accelerator_unittest.cc:398: keyframe =
true;
Please instead move keyframe = true to the case above. And then I/SI slice
detection won't be needed.

[hshi1, 2014-10-30 03:48:25]

https://codereview.chromium.org/686283002/diff/40001/content/common/gpu/media...
File content/common/gpu/media/video_encode_accelerator_unittest.cc (right):

https://codereview.chromium.org/686283002/diff/40001/content/common/gpu/media...
content/common/gpu/media/video_encode_accelerator_unittest.cc:398: keyframe =
true;
On 2014/10/30 01:38:33, Pawel Osciak wrote:
> Please instead move keyframe = true to the case above. And then I/SI slice
> detection won't be needed.

Done.

[hshi1, 2014-10-30 19:06:25]

PTAL - this is passing on both Intel (squawks) and ARM (daisy).

[hshi1, 2014-10-30 19:29:44]

hshi@chromium.org changed reviewers:
+ dalecurtis@chromium.org

[hshi1, 2014-10-30 19:29:44]

+Dale (owner of content/renderer/media)

I've already chatted with Pawel last night and ran tests on daisy this morning
as requested. Will also await his feedback when he's online today.

[Pawel Osciak, 2014-10-31 06:23:40]

lgtm % nit: Please update the TEST= line

[DaleCurtis, 2014-10-31 18:08:51]

rs lgtm

[hshi1, 2014-10-31 18:33:43]

The CQ bit was checked by hshi@chromium.org

[commit-bot: I haz the power, 2014-10-31 18:35:11]

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

[commit-bot: I haz the power, 2014-10-31 19:17:21]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-31 19:18:26]

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