VAVDA: Use the new, generic video decoder and accelerator infrastructure.

VAVDA: Use the new, generic video decoder and accelerator infrastructure.

A new, generic accelerated video decoder stack has recently been written to
abstract and replace the existing, heavily VAAPI-specific infrastructure, and
to make it reusable on other platforms as well.

Now that the new stack is stable, switch VAVDA to it. Move the VAAPI-specific
H264 code to a new VaapiH264Accelerator class and adapt the whole class to use
the new decoder/accelerator infrastructure.

BUG=410152
TEST=vdaunittest,veaunittest,shaka player with DASH,local video

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

[Pawel Osciak, 2015-03-27 11:51:37]

Patchset #1 (id:1) has been deleted

[Pawel Osciak, 2015-03-27 11:52:32]

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

[Pawel Osciak, 2015-03-27 11:52:57]

isherman@: please histograms,
others: please rest.

Thank you.

[Pawel Osciak, 2015-03-27 12:18:17]

Patchset #1 (id:20001) has been deleted

[Ilya Sherman, 2015-03-30 05:11:47]

https://codereview.chromium.org/1040513003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1040513003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:61628: +</enum>
Hmm, is it really useful to have this histogram that has only a single bucket in
it?  How do you anticipate interpreting the data?  It's typically best to
compare quantities against each other, i.e. to identify a baseline.

[Pawel Osciak, 2015-03-30 05:29:04]

On 2015/03/30 05:11:47, Ilya Sherman wrote:
>
https://codereview.chromium.org/1040513003/diff/60001/tools/metrics/histogram...
> File tools/metrics/histograms/histograms.xml (right):
> 
>
https://codereview.chromium.org/1040513003/diff/60001/tools/metrics/histogram...
> tools/metrics/histograms/histograms.xml:61628: +</enum>
> Hmm, is it really useful to have this histogram that has only a single bucket
in
> it?  How do you anticipate interpreting the data?  It's typically best to
> compare quantities against each other, i.e. to identify a baseline.

There is some history here, VAVEA one
(https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...)
is the same. In general, the base for this is
Media.GpuVideoDecoderInitializeStatus
(https://code.google.com/p/chromium/codesearch#chromium/src/tools/metrics/hist...),
and the %error is Media.VAVDA.DecoderFailure /
Media.GpuVideoDecoderInitializeStatus.

[Ilya Sherman, 2015-03-30 05:49:23]

Okay.  LGTM.

[wuchengli, 2015-03-30 11:02:55]

First round of comments. I'll review vaapi_video_decode_accelerator.cc tomorrow.

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/h264_dpb.h (right):

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:24: struct H264Picture : public
base::RefCounted<H264Picture> {
This is complicated enough to be a class.

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/h264_dpb.h:79: DISALLOW_COPY_AND_ASSIGN(H264Picture);
This should be in private section according to base/macros.h.

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:403: // surfaces, and
reschedule DecodeTask instead.
nit: I think one TODO in vaapi_video_decode_accelerator.h is enough. This
doesn't need to duplicate.

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_encode_accelerator.h (right):

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_encode_accelerator.h:213:
scoped_refptr<H264Picture> current_pic_;
why not scoped_ptr? There is only one reference to this.

[kcwu, 2015-03-31 15:20:10]

I'll continue review vaapi_video_decode_accelerator.cc tomorrow.

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1038:
memset(&slice_param, 0, sizeof(VASliceParameterBufferH264));
I prefer sizeof(slice_param) instead.

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.h:176: 
two blank line

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right):

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_encode_accelerator.cc:264: current_pic_ =
new H264Picture();
Is EndFrame() always paired with BeginFrame() ?
If so, how about delete the reference to current_pic_ in EndFrame() ?

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_encode_accelerator.h (right):

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_encode_accelerator.h:213:
scoped_refptr<H264Picture> current_pic_;
On 2015/03/30 11:02:55, wuchengli wrote:
> why not scoped_ptr? There is only one reference to this.

Because H264Picture is ref-counted.

[wuchengli, 2015-04-01 05:17:38]

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:857: if (state_ ==
kResetting || state_ == kDestroying)
auto_lock(lock_); to protect |state_|?

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_video_encode_accelerator.h (right):

https://codereview.chromium.org/1040513003/diff/60001/content/common/gpu/medi...
content/common/gpu/media/vaapi_video_encode_accelerator.h:213:
scoped_refptr<H264Picture> current_pic_;
On 2015/03/31 15:20:09, kcwu wrote:
> On 2015/03/30 11:02:55, wuchengli wrote:
> > why not scoped_ptr? There is only one reference to this.
> 
> Because H264Picture is ref-counted.
Acknowledged.

[Pawel Osciak, 2015-04-03 07:06:00]

very small diff. ptal. thanks.

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
File content/common/gpu/media/h264_dpb.h (right):

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
content/common/gpu/media/h264_dpb.h:24: struct H264Picture : public
base::RefCounted<H264Picture> {
On 2015/03/30 11:02:54, wuchengli wrote:
> This is complicated enough to be a class.

Done.

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
content/common/gpu/media/h264_dpb.h:79: DISALLOW_COPY_AND_ASSIGN(H264Picture);
On 2015/03/30 11:02:54, wuchengli wrote:
> This should be in private section according to base/macros.h.

Done.

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:403: // surfaces, and
reschedule DecodeTask instead.
On 2015/03/30 11:02:55, wuchengli wrote:
> nit: I think one TODO in vaapi_video_decode_accelerator.h is enough. This
> doesn't need to duplicate.

Done.

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:857: if (state_ ==
kResetting || state_ == kDestroying)
On 2015/04/01 05:17:38, wuchengli wrote:
> auto_lock(lock_); to protect |state_|?

Done.

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:1038:
memset(&slice_param, 0, sizeof(VASliceParameterBufferH264));
On 2015/03/31 15:20:09, kcwu wrote:
> I prefer sizeof(slice_param) instead.

Me too. Done.

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.h:176: 
On 2015/03/31 15:20:09, kcwu wrote:
> two blank line

This was intentional to make a clear delineation between the "methods used by
accelerator implementations." section and what follows.

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
File content/common/gpu/media/vaapi_video_encode_accelerator.cc (right):

https://chromiumcodereview.appspot.com/1040513003/diff/60001/content/common/g...
content/common/gpu/media/vaapi_video_encode_accelerator.cc:264: current_pic_ =
new H264Picture();
On 2015/03/31 15:20:09, kcwu wrote:
> Is EndFrame() always paired with BeginFrame() ?
> If so, how about delete the reference to current_pic_ in EndFrame() ?

Yes, I considered that, but I thought it would be better not to make functional
changes to VEA in this CL, in an unlikely case anything broke. That could make
the whole unrelated VDA change be reverted for a VEA issue.
I'll also keep this for later if you would be ok with that, sorry!

[Pawel Osciak, 2015-04-03 07:08:47]

whoops, had to reupload. ptal.

[Pawel Osciak, 2015-04-03 08:12:41]

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

[Pawel Osciak, 2015-04-03 08:12:41]

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

[Pawel Osciak, 2015-04-03 08:12:50]

The CQ bit was unchecked by posciak@chromium.org

[wuchengli, 2015-04-07 09:36:47]

lgtm

https://chromiumcodereview.appspot.com/1040513003/diff/80001/content/common/g...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1040513003/diff/80001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.h:194: // Protects input
buffer and surface queues and state_.
Not related to this CL. What are all the variables protected by |lock_|?
|state_|, |input_buffers_|, and |available_va_surfaces_|?
|available_va_surfaces_| is not protected by |lock_| in
TryFinishSurfaceSetChange().

[kcwu1, 2015-04-07 09:42:46]

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

[kcwu1, 2015-04-07 09:42:47]

lgtm

https://chromiumcodereview.appspot.com/1040513003/diff/80001/content/common/g...
File content/common/gpu/media/h264_dpb.h (right):

https://chromiumcodereview.appspot.com/1040513003/diff/80001/content/common/g...
content/common/gpu/media/h264_dpb.h:31: using Vector =
std::vector<scoped_refptr<H264Picture>>;
move this line and enum to front of ctor.

https://chromiumcodereview.appspot.com/1040513003/diff/80001/content/common/g...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1040513003/diff/80001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.h:194: // Protects input
buffer and surface queues and state_.
On 2015/04/07 09:36:47, wuchengli wrote:
> Not related to this CL. What are all the variables protected by |lock_|?
> |state_|, |input_buffers_|, and |available_va_surfaces_|?
> |available_va_surfaces_| is not protected by |lock_| in
> TryFinishSurfaceSetChange().

I have the same concern.

[Pawel Osciak, 2015-04-08 08:36:04]

https://chromiumcodereview.appspot.com/1040513003/diff/80001/content/common/g...
File content/common/gpu/media/h264_dpb.h (right):

https://chromiumcodereview.appspot.com/1040513003/diff/80001/content/common/g...
content/common/gpu/media/h264_dpb.h:31: using Vector =
std::vector<scoped_refptr<H264Picture>>;
On 2015/04/07 09:42:47, kcwu1 wrote:
> move this line and enum to front of ctor.

Done.

https://chromiumcodereview.appspot.com/1040513003/diff/80001/content/common/g...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://chromiumcodereview.appspot.com/1040513003/diff/80001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.h:194: // Protects input
buffer and surface queues and state_.
On 2015/04/07 09:36:47, wuchengli wrote:
> Not related to this CL. What are all the variables protected by |lock_|?
> |state_|, |input_buffers_|, and |available_va_surfaces_|?
> |available_va_surfaces_| is not protected by |lock_| in
> TryFinishSurfaceSetChange().

Yes, looks like a dormant bug, but shouldn't have been an issue, at this point
decoder thread should be idle.
I plan to reorganize all this anyway to remove locking, so will address this
then. Thanks for spotting.

[Pawel Osciak, 2015-04-08 08:36:09]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2015-04-08 08:36:10]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, wuchengli@chromium.org, kcwu@google.com
Link to the patchset:
https://chromiumcodereview.appspot.com/1040513003/#ps100001 (title: "Address nit
from PS3")

[commit-bot: I haz the power, 2015-04-08 08:36:49]

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

[commit-bot: I haz the power, 2015-04-08 08:47:55]

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

[commit-bot: I haz the power, 2015-04-08 08:47:57]

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

[Pawel Osciak, 2015-04-08 09:00:22]

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

[Pawel Osciak, 2015-04-08 09:00:23]

brettw@: Please content/common/BUILD.gn.

Thanks.

[Pawel Osciak, 2015-04-10 04:45:37]

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

[Pawel Osciak, 2015-04-10 04:46:13]

dpranke@: may I please ask for OWNERS for content/common/BUILD.gn? Thank you.

[Dirk Pranke, 2015-04-10 16:57:31]

The change looks fine, but I'm not an OWNER for that file (my top-level
ownership of src/BUILD.gn does not recurse down to all gn files).

[brettw, 2015-04-10 18:00:34]

BUILD.gn LGTM

[Pawel Osciak, 2015-04-10 18:12:54]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2015-04-10 18:14:15]

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

[commit-bot: I haz the power, 2015-04-10 18:20:15]

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

[commit-bot: I haz the power, 2015-04-10 18:20:17]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)

[Pawel Osciak, 2015-04-11 02:13:36]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2015-04-11 02:14:23]

The CQ bit was unchecked by posciak@chromium.org

[Pawel Osciak, 2015-04-11 07:36:10]

The CQ bit was checked by posciak@chromium.org

[Pawel Osciak, 2015-04-11 07:36:11]

The patchset sent to the CQ was uploaded after l-g-t-m from kcwu@google.com,
wuchengli@chromium.org, brettw@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/1040513003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-11 07:36:18]

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

[commit-bot: I haz the power, 2015-04-11 10:32:53]

Committed patchset #5 (id:120001)

[commit-bot: I haz the power, 2015-04-11 10:33:35]

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

[Adrian Kuegel, 2015-04-13 08:14:21]

I think this broke the Codesearch bot:
https://build.chromium.org/p/chromium.infra/builders/ChromiumOS%20Codesearch/...

Can you please check? This is kind of urgent, because if it fails, there will be
no updates to the Codesearch index.

[Pawel Osciak, 2015-04-13 08:15:45]

On 2015/04/13 08:14:21, Adrian Kuegel wrote:
> I think this broke the Codesearch bot:
>
https://build.chromium.org/p/chromium.infra/builders/ChromiumOS%20Codesearch/...
> 
> Can you please check? This is kind of urgent, because if it fails, there will
be
> no updates to the Codesearch index.

Sure, looking.

[Pawel Osciak, 2015-04-13 08:40:20]

On 2015/04/13 08:15:45, Pawel Osciak wrote:
> On 2015/04/13 08:14:21, Adrian Kuegel wrote:
> > I think this broke the Codesearch bot:
> >
>
https://build.chromium.org/p/chromium.infra/builders/ChromiumOS%20Codesearch/...
> > 
> > Can you please check? This is kind of urgent, because if it fails, there
will
> be
> > no updates to the Codesearch index.
> 
> Sure, looking.

We've decided to https://codereview.chromium.org/1059853005/.