V4L2SVDA: Add a VP9Accelerator implementation utilizing the V4L2 VP9 frame API.

V4L2SVDA: Add a VP9Accelerator implementation utilizing the V4L2 VP9 frame API.

Add an implementation of VP9Accelerator to V4L2SliceVideoDecodeAccelerator,
using the new V4L2 VP9 frame API.

Also slightly reorganize context handling and update callbacks
in Vp9Decoder and Vp9Parser.

TEST=vdatest, crosvideo.appspot
BUG=chrome-os-partner:56703
Committed: https://crrev.com/77118c9fc7d4dc3c4fd629ff3088f6fb891c3c5b
Cr-Commit-Position: refs/heads/master@{#414961}

[Pawel Osciak, 2016-08-10 08:23:10]

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

[kcwu, 2016-08-10 09:59:19]

vp9_parser_unittest.cc need update as well.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/filters/vp9_pa...
File media/filters/vp9_parser.cc (right):

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.cc:283: << static_cast<int>(frame_context_idx) << " to
update";
no longer needed to cast for size_t.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/filters/vp9_pa...
File media/filters/vp9_parser.h (right):

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.h:141: static bool IsValid(const Vp9FrameContext&
context);
How about make it non-static member function?

According to style guide,
https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
looks like it's okay.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.h:292: class Context {
If it's class, member variable need to be named foo_ and private?

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
File media/gpu/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:2761:
v4l2_dec_->IsCtrlExposed(V4L2_CID_MPEG_VIDEO_VP9_ENTROPY);
Is "exposed" enough to indicate? Don't you need to check control value?

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:2780: #define
SET_LF_PARAMS_FLAG_IF(cond, flag) \
I feel naive if-statement is easier to understand for these set-if macros.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:2881: v4l2_entropy_ctx->tx_mode
= vp9_compressed_hdr.tx_mode;
tx_mode and reference_mode are still inside v4l2_vp9_entropy_ctx ?

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:2954: if (ref_pictures.size()
!= arraysize(v4l2_decode_param.ref_frames))
How about DCHECK?

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:3005: struct
v4l2_ctrl_vp9_entropy v4l2_entropy;
How about add comment why this is outside the if-block? To prevent we forgot and
move it again.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/vp9_decoder.h
File media/gpu/vp9_decoder.h (right):

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/vp9_decode...
media/gpu/vp9_decoder.h:84: virtual bool RequiresFrameContext() const = 0;
How about rename this to IsRequiringFrameContext ?

[Pawel Osciak, 2016-08-13 01:51:30]

done, ptal.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/filters/vp9_pa...
File media/filters/vp9_parser.cc (right):

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.cc:283: << static_cast<int>(frame_context_idx) << " to
update";
On 2016/08/10 09:59:18, kcwu wrote:
> no longer needed to cast for size_t.

Done.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/filters/vp9_pa...
File media/filters/vp9_parser.h (right):

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.h:141: static bool IsValid(const Vp9FrameContext&
context);
On 2016/08/10 09:59:18, kcwu wrote:
> How about make it non-static member function?
> 
> According to style guide,
> https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
> looks like it's okay.

Done.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/filters/vp9_pa...
media/filters/vp9_parser.h:292: class Context {
On 2016/08/10 09:59:18, kcwu wrote:
> If it's class, member variable need to be named foo_ and private?

Done.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
File media/gpu/v4l2_slice_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:2761:
v4l2_dec_->IsCtrlExposed(V4L2_CID_MPEG_VIDEO_VP9_ENTROPY);
On 2016/08/10 09:59:19, kcwu wrote:
> Is "exposed" enough to indicate? Don't you need to check control value? 

Yes, V4L2 drivers expose only supported controls. In this case we use this
mechanism to indicate if it's required or not.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:2780: #define
SET_LF_PARAMS_FLAG_IF(cond, flag) \
On 2016/08/10 09:59:18, kcwu wrote:
> I feel naive if-statement is easier to understand for these set-if macros.
> 

You might be right now that I look at it, but I'd prefer keeping the style of
the file for now to reduce the changes, otherwise I'd have to change significant
portions in other methods as well.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:2881: v4l2_entropy_ctx->tx_mode
= vp9_compressed_hdr.tx_mode;
On 2016/08/10 09:59:19, kcwu wrote:
> tx_mode and reference_mode are still inside v4l2_vp9_entropy_ctx ?

Moved to v4l2_ctrl_vp9_entropy.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:2954: if (ref_pictures.size()
!= arraysize(v4l2_decode_param.ref_frames))
On 2016/08/10 09:59:18, kcwu wrote:
> How about DCHECK?

Done.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/v4l2_slice...
media/gpu/v4l2_slice_video_decode_accelerator.cc:3005: struct
v4l2_ctrl_vp9_entropy v4l2_entropy;
On 2016/08/10 09:59:18, kcwu wrote:
> How about add comment why this is outside the if-block? To prevent we forgot
and
> move it again.

Good idea. Done.

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/vp9_decoder.h
File media/gpu/vp9_decoder.h (right):

https://chromiumcodereview.appspot.com/2229353002/diff/1/media/gpu/vp9_decode...
media/gpu/vp9_decoder.h:84: virtual bool RequiresFrameContext() const = 0;
On 2016/08/10 09:59:19, kcwu wrote:
> How about rename this to IsRequiringFrameContext ?

Went with IsFrameContextRequired().

[kcwu, 2016-08-17 04:06:09]

nits

https://codereview.chromium.org/2229353002/diff/20001/media/filters/vp9_parser.h
File media/filters/vp9_parser.h (right):

https://codereview.chromium.org/2229353002/diff/20001/media/filters/vp9_parse...
media/filters/vp9_parser.h:311: // Return a callback to update frame context at
a later time with.
s/Return/Returns/

https://codereview.chromium.org/2229353002/diff/20001/media/filters/vp9_parse...
media/filters/vp9_parser.h:336: const ReferenceSlot& GetRefSlot(size_t ref_type)
const;
ref_idx or ref_frame_idx?

"size_t ref_type" looks confusing.

https://codereview.chromium.org/2229353002/diff/20001/media/filters/vp9_parse...
File media/filters/vp9_parser_unittest.cc (right):

https://codereview.chromium.org/2229353002/diff/20001/media/filters/vp9_parse...
media/filters/vp9_parser_unittest.cc:157: Vp9Parser::ContextRefreshCallback
context_refresh_cb =
Since we don't care the detail type here, use "auto" instead?

[Pawel Osciak, 2016-08-19 08:22:44]

ptal

https://codereview.chromium.org/2229353002/diff/20001/media/filters/vp9_parser.h
File media/filters/vp9_parser.h (right):

https://codereview.chromium.org/2229353002/diff/20001/media/filters/vp9_parse...
media/filters/vp9_parser.h:311: // Return a callback to update frame context at
a later time with.
On 2016/08/17 04:06:09, kcwu wrote:
> s/Return/Returns/

Done.

https://codereview.chromium.org/2229353002/diff/20001/media/filters/vp9_parse...
media/filters/vp9_parser.h:336: const ReferenceSlot& GetRefSlot(size_t ref_type)
const;
On 2016/08/17 04:06:09, kcwu wrote:
> ref_idx or ref_frame_idx?
> 
> "size_t ref_type" looks confusing.

Done.

https://codereview.chromium.org/2229353002/diff/20001/media/filters/vp9_parse...
File media/filters/vp9_parser_unittest.cc (right):

https://codereview.chromium.org/2229353002/diff/20001/media/filters/vp9_parse...
media/filters/vp9_parser_unittest.cc:157: Vp9Parser::ContextRefreshCallback
context_refresh_cb =
On 2016/08/17 04:06:09, kcwu wrote:
> Since we don't care the detail type here, use "auto" instead?

Done.

[kcwu, 2016-08-19 08:28:17]

lgtm

[Pawel Osciak, 2016-08-19 08:37:12]

Patchset #4 (id:60001) has been deleted

[Pawel Osciak, 2016-08-19 08:38:46]

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

[Pawel Osciak, 2016-08-19 08:38:46]

+dalecurtis for OWNERS please. Thanks!

[DaleCurtis, 2016-08-22 17:39:10]

dalecurtis@chromium.org changed reviewers:
+ sandersd@chromium.org
- dalecurtis@chromium.org

[DaleCurtis, 2016-08-22 17:39:10]

=>sandersd@ for review.

[sandersd (OOO until July 31), 2016-08-22 20:52:14]

lgtm

[Pawel Osciak, 2016-08-25 20:45:44]

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

[commit-bot: I haz the power, 2016-08-25 20:47:02]

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-25 20:56:00]

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

[commit-bot: I haz the power, 2016-08-25 20:56:01]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Pawel Osciak, 2016-08-26 00:01:56]

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

[commit-bot: I haz the power, 2016-08-26 00:03:07]

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-26 00:17:13]

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

[commit-bot: I haz the power, 2016-08-26 00:17:15]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[Pawel Osciak, 2016-08-26 21:57:51]

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

[commit-bot: I haz the power, 2016-08-26 21:59:03]

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-26 23:47:21]

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

[commit-bot: I haz the power, 2016-08-26 23:47:22]

Dry run: This issue passed the CQ dry run.

[Pawel Osciak, 2016-08-26 23:53:21]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2016-08-26 23:53:22]

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

[Pawel Osciak, 2016-08-26 23:53:29]

The CQ bit was unchecked by posciak@chromium.org

[Pawel Osciak, 2016-08-28 01:29:39]

Description was changed from

==========
V4L2SVDA: Add a VP9Accelerator implementation utilizing the V4L2 VP9 frame API.

Add an implementation of VP9Accelerator to V4L2SliceVideoDecodeAccelerator,
using the new V4L2 VP9 frame API.

Also slightly reorganize context handling and update callbacks in Vp9Decoder
and Vp9Parser.

TEST=vdatest, crosvideo.appspot
BUG=
==========

to

==========
V4L2SVDA: Add a VP9Accelerator implementation utilizing the V4L2 VP9 frame API.

Add an implementation of VP9Accelerator to V4L2SliceVideoDecodeAccelerator,
using the new V4L2 VP9 frame API.

Also slightly reorganize context handling and update callbacks in Vp9Decoder and
Vp9Parser.

TEST=vdatest, crosvideo.appspot
BUG=chrome-os-partner:56703
==========

[Pawel Osciak, 2016-08-28 01:30:05]

Description was changed from

==========
V4L2SVDA: Add a VP9Accelerator implementation utilizing the V4L2 VP9 frame API.

Add an implementation of VP9Accelerator to V4L2SliceVideoDecodeAccelerator,
using the new V4L2 VP9 frame API.

Also slightly reorganize context handling and update callbacks in Vp9Decoder and
Vp9Parser.

TEST=vdatest, crosvideo.appspot
BUG=chrome-os-partner:56703
==========

to

==========
V4L2SVDA: Add a VP9Accelerator implementation utilizing the V4L2 VP9 frame API.

Add an implementation of VP9Accelerator to V4L2SliceVideoDecodeAccelerator,
using the new V4L2 VP9 frame API.

Also slightly reorganize context handling and update callbacks
in Vp9Decoder and Vp9Parser.

TEST=vdatest, crosvideo.appspot
BUG=chrome-os-partner:56703
==========

[Pawel Osciak, 2016-08-28 01:30:39]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2016-08-28 01:30:52]

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-28 03:07:44]

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

[commit-bot: I haz the power, 2016-08-28 03:07:45]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Pawel Osciak, 2016-08-28 12:01:25]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2016-08-28 12:01:37]

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-28 13:18:55]

Description was changed from

==========
V4L2SVDA: Add a VP9Accelerator implementation utilizing the V4L2 VP9 frame API.

Add an implementation of VP9Accelerator to V4L2SliceVideoDecodeAccelerator,
using the new V4L2 VP9 frame API.

Also slightly reorganize context handling and update callbacks
in Vp9Decoder and Vp9Parser.

TEST=vdatest, crosvideo.appspot
BUG=chrome-os-partner:56703
==========

to

==========
V4L2SVDA: Add a VP9Accelerator implementation utilizing the V4L2 VP9 frame API.

Add an implementation of VP9Accelerator to V4L2SliceVideoDecodeAccelerator,
using the new V4L2 VP9 frame API.

Also slightly reorganize context handling and update callbacks
in Vp9Decoder and Vp9Parser.

TEST=vdatest, crosvideo.appspot
BUG=chrome-os-partner:56703
==========

[commit-bot: I haz the power, 2016-08-28 13:18:56]

Committed patchset #4 (id:80001)

[commit-bot: I haz the power, 2016-08-28 13:20:58]

Description was changed from

==========
V4L2SVDA: Add a VP9Accelerator implementation utilizing the V4L2 VP9 frame API.

Add an implementation of VP9Accelerator to V4L2SliceVideoDecodeAccelerator,
using the new V4L2 VP9 frame API.

Also slightly reorganize context handling and update callbacks
in Vp9Decoder and Vp9Parser.

TEST=vdatest, crosvideo.appspot
BUG=chrome-os-partner:56703
==========

to

==========
V4L2SVDA: Add a VP9Accelerator implementation utilizing the V4L2 VP9 frame API.

Add an implementation of VP9Accelerator to V4L2SliceVideoDecodeAccelerator,
using the new V4L2 VP9 frame API.

Also slightly reorganize context handling and update callbacks
in Vp9Decoder and Vp9Parser.

TEST=vdatest, crosvideo.appspot
BUG=chrome-os-partner:56703

Committed: https://crrev.com/77118c9fc7d4dc3c4fd629ff3088f6fb891c3c5b
Cr-Commit-Position: refs/heads/master@{#414961}
==========

[commit-bot: I haz the power, 2016-08-28 13:20:59]

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