Reland: Refactor Vaapi video decoder/encoder in preparation of Freon support

Reland: Refactor Vaapi video decoder/encoder in preparation of Freon support

Previous CL : https://codereview.chromium.org/807853005

This introduces VaapiPicture which abstracts the output picture for
the VaapiVideoDecodeAccelerator.
This also splits out the x11 specific functions of LibVA into their
own signature file.
And finally this adds back GLImageGLX to match the code path that will
be used on Freon, using GLImageLinuxDMABuffer.

BUG=403058
TEST=video_decode_accelerator_unittest and video_encode_accelerator_unittest
TBR=scherkus@chromium.org

Committed: https://crrev.com/2c653de36889e6ccfe8b9c7258a614d6a808f328
Cr-Commit-Position: refs/heads/master@{#309648}

[llandwerlin-old, 2014-12-20 20:17:17]

lionel.g.landwerlin@intel.com changed reviewers:
+ piman@chromium.org, posciak@chromium.org, scherkus@chromium.org

[llandwerlin-old, 2014-12-20 20:17:18]

posciak@ scherkus@ piman@ : Sorry for the noise, the GN build fail because of a
wrong path.

[llandwerlin-old, 2014-12-20 20:19:33]

If there a try bot for GN, please feel free to launch it. Thanks.

[Pawel Osciak, 2014-12-20 20:46:52]

lgtm % nits

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:316: new
VASurface(available_va_surfaces_.front(), requested_pic_size_,
requested_pic_size_ won't be correct if awaiting_va_surfaces_recycle_ == true.
This should not be called in that state, but to be sure, can we add a DCHECK
please?

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_video_decode_accelerator.h:198: // Return a
TFPPicture associated with given client-provided id.
s/TFPPicture/VaapiPicture/

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_wrapper.cc:740: bool
VaapiWrapper::PutSurfaceIntoPixmap(VASurfaceID va_surface_id,
Please don't move around things that don't change, to reduce the diff size.

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_wrapper.h (left):

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_wrapper.h:100: // Put data from |va_surface_id|
into |x_pixmap| of size |size|,
Please don't move around things that don't change, to reduce the diff size.

https://codereview.chromium.org/817023005/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/817023005/diff/1/content/content_common.gypi#...
content/content_common.gypi:806: 'common/gpu/media/va.sigs',
I think va.sigs should not be under use_x11?

https://codereview.chromium.org/817023005/diff/1/content/content_common.gypi#...
content/content_common.gypi:817: 'common/gpu/media/va.sigs',
Why do we have it twice?

[llandwerlin-old, 2014-12-20 21:11:43]

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:316: new
VASurface(available_va_surfaces_.front(), requested_pic_size_,
On 2014/12/20 20:46:51, Pawel Osciak wrote:
> requested_pic_size_ won't be correct if awaiting_va_surfaces_recycle_ == true.
> This should not be called in that state, but to be sure, can we add a DCHECK
> please?

Acknowledged.

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_video_decode_accelerator.h (right):

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_video_decode_accelerator.h:198: // Return a
TFPPicture associated with given client-provided id.
On 2014/12/20 20:46:51, Pawel Osciak wrote:
> s/TFPPicture/VaapiPicture/

Acknowledged.

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_wrapper.cc:740: bool
VaapiWrapper::PutSurfaceIntoPixmap(VASurfaceID va_surface_id,
On 2014/12/20 20:46:51, Pawel Osciak wrote:
> Please don't move around things that don't change, to reduce the diff size.

Acknowledged.

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
File content/common/gpu/media/vaapi_wrapper.h (left):

https://codereview.chromium.org/817023005/diff/1/content/common/gpu/media/vaa...
content/common/gpu/media/vaapi_wrapper.h:100: // Put data from |va_surface_id|
into |x_pixmap| of size |size|,
On 2014/12/20 20:46:51, Pawel Osciak wrote:
> Please don't move around things that don't change, to reduce the diff size.

Acknowledged.

https://codereview.chromium.org/817023005/diff/1/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/817023005/diff/1/content/content_common.gypi#...
content/content_common.gypi:806: 'common/gpu/media/va.sigs',
On 2014/12/20 20:46:51, Pawel Osciak wrote:
> I think va.sigs should not be under use_x11?

I thought it would be easier to read. Will change

https://codereview.chromium.org/817023005/diff/1/content/content_common.gypi#...
content/content_common.gypi:817: 'common/gpu/media/va.sigs',
On 2014/12/20 20:46:52, Pawel Osciak wrote:
> Why do we have it twice?

This is to compile the whole of Vaapi* file on Ozone too.
But when trying to use initialize VaapiWrapper, it will fail because there is no
valid display (unless you're on X11) :
https://codereview.chromium.org/817023005/patch/1/10018#newcode229
Then it will fallback to software decode/encode.

[llandwerlin-old, 2014-12-21 18:14:56]

On 2014/12/20 21:11:43, llandwerlin wrote:
> 
> 
>
https://codereview.chromium.org/817023005/diff/1/content/content_common.gypi#...
> content/content_common.gypi:806: 'common/gpu/media/va.sigs',
> On 2014/12/20 20:46:51, Pawel Osciak wrote:
> > I think va.sigs should not be under use_x11?
> 
> I thought it would be easier to read. Will change
> 

I couldn't find a way to make this work in gyp.
Not sure whether it's possible to add items to a field within a variable under
conditions in gyp.

[Pawel Osciak, 2014-12-22 00:44:03]

On 2014/12/21 18:14:56, llandwerlin wrote:
> On 2014/12/20 21:11:43, llandwerlin wrote:
> > 
> > 
> >
>
https://codereview.chromium.org/817023005/diff/1/content/content_common.gypi#...
> > content/content_common.gypi:806: 'common/gpu/media/va.sigs',
> > On 2014/12/20 20:46:51, Pawel Osciak wrote:
> > > I think va.sigs should not be under use_x11?
> > 
> > I thought it would be easier to read. Will change
> > 
> 
> I couldn't find a way to make this work in gyp.
> Not sure whether it's possible to add items to a field within a variable under
> conditions in gyp.

I see, I don't know myself to be honest. Perhaps it's not an issue, the other
reviewers may have more insight.

[piman, 2014-12-22 19:15:59]

On 2014/12/22 00:44:03, Pawel Osciak wrote:
> On 2014/12/21 18:14:56, llandwerlin wrote:
> > On 2014/12/20 21:11:43, llandwerlin wrote:
> > > 
> > > 
> > >
> >
>
https://codereview.chromium.org/817023005/diff/1/content/content_common.gypi#...
> > > content/content_common.gypi:806: 'common/gpu/media/va.sigs',
> > > On 2014/12/20 20:46:51, Pawel Osciak wrote:
> > > > I think va.sigs should not be under use_x11?
> > > 
> > > I thought it would be easier to read. Will change
> > > 
> > 
> > I couldn't find a way to make this work in gyp.
> > Not sure whether it's possible to add items to a field within a variable
under
> > conditions in gyp.
> 
> I see, I don't know myself to be honest. Perhaps it's not an issue, the other
> reviewers may have more insight.

I thought it's possible, but I wouldn't hold the review for that if it doesn't
seem to work trivially.

[piman, 2014-12-22 19:21:52]

https://codereview.chromium.org/817023005/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/817023005/diff/20001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:148: VaapiWrapper::~VaapiWrapper() {
Can you add a DCHECK that this is called on the main thread?
Also, can you describe why, despite refcounting, this is ensured?

[llandwerlin-old, 2014-12-22 23:36:42]

https://codereview.chromium.org/817023005/diff/20001/content/common/gpu/media...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/817023005/diff/20001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:148: VaapiWrapper::~VaapiWrapper() {
On 2014/12/22 19:21:52, piman (Very slow to review) wrote:
> Can you add a DCHECK that this is called on the main thread?
> Also, can you describe why, despite refcounting, this is ensured?

All the pictures are owned by VaapiVideoDecodeAccelerator, these are the only
classes having references to VaapiWrapper (apart from
VaapiVideoDecodeAccelerator itself).
Therefore when VaapiVideoDecodeAccelerator is destroyed, all the references on
VaapiWrapper will be released and it will be destroyed by the same thread that
destroys VaapiVideoDecodeAccelerator.

I notice (only now) that VaapiH264Decoder has a simple pointer to VaapiWrapper.
I guess we could avoid refcounting entirely, the destruction of the resources
doesn't change thread before/after this CL. We're just trying to make the code
look like it's been called from different places.

[piman, 2014-12-23 00:39:40]

On Mon, Dec 22, 2014 at 3:36 PM, <lionel.g.landwerlin@intel.com> wrote:

>
> https://codereview.chromium.org/817023005/diff/20001/
> content/common/gpu/media/vaapi_wrapper.cc
> File content/common/gpu/media/vaapi_wrapper.cc (right):
>
> https://codereview.chromium.org/817023005/diff/20001/
> content/common/gpu/media/vaapi_wrapper.cc#newcode148
> content/common/gpu/media/vaapi_wrapper.cc:148:
> VaapiWrapper::~VaapiWrapper() {
> On 2014/12/22 19:21:52, piman (Very slow to review) wrote:
>
>> Can you add a DCHECK that this is called on the main thread?
>> Also, can you describe why, despite refcounting, this is ensured?
>>
>
> All the pictures are owned by VaapiVideoDecodeAccelerator, these are the
> only classes having references to VaapiWrapper (apart from
> VaapiVideoDecodeAccelerator itself).
> Therefore when VaapiVideoDecodeAccelerator is destroyed, all the
> references on VaapiWrapper will be released and it will be destroyed by
> the same thread that destroys VaapiVideoDecodeAccelerator.
>
> I notice (only now) that VaapiH264Decoder has a simple pointer to
> VaapiWrapper. I guess we could avoid refcounting entirely, the
> destruction of the resources doesn't change thread before/after this CL.
> We're just trying to make the code look like it's been called from
> different places.
>

Can we remove refcounting altogether then? It's a lot simpler to reason
about, and has lower overhead.

>
> https://codereview.chromium.org/817023005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Pawel Osciak, 2014-12-23 00:45:22]

On 2014/12/23 00:39:40, piman (Very slow to review) wrote:
> On Mon, Dec 22, 2014 at 3:36 PM, <mailto:lionel.g.landwerlin@intel.com> wrote:
> 
> >
> > https://codereview.chromium.org/817023005/diff/20001/
> > content/common/gpu/media/vaapi_wrapper.cc
> > File content/common/gpu/media/vaapi_wrapper.cc (right):
> >
> > https://codereview.chromium.org/817023005/diff/20001/
> > content/common/gpu/media/vaapi_wrapper.cc#newcode148
> > content/common/gpu/media/vaapi_wrapper.cc:148:
> > VaapiWrapper::~VaapiWrapper() {
> > On 2014/12/22 19:21:52, piman (Very slow to review) wrote:
> >
> >> Can you add a DCHECK that this is called on the main thread?
> >> Also, can you describe why, despite refcounting, this is ensured?
> >>
> >
> > All the pictures are owned by VaapiVideoDecodeAccelerator, these are the
> > only classes having references to VaapiWrapper (apart from
> > VaapiVideoDecodeAccelerator itself).
> > Therefore when VaapiVideoDecodeAccelerator is destroyed, all the
> > references on VaapiWrapper will be released and it will be destroyed by
> > the same thread that destroys VaapiVideoDecodeAccelerator.
> >
> > I notice (only now) that VaapiH264Decoder has a simple pointer to
> > VaapiWrapper. I guess we could avoid refcounting entirely, the
> > destruction of the resources doesn't change thread before/after this CL.
> > We're just trying to make the code look like it's been called from
> > different places.
> >
> 
> Can we remove refcounting altogether then? It's a lot simpler to reason
> about, and has lower overhead.

My main issue had been that we were using WeakPtr before. We can definitely
contain this into one class and remove refcounting, just making sure that
VaapiWrapper is destroyed after everything else and after the decoder thread is
stopped (which Destroy takes care of now already).

[llandwerlin-old, 2014-12-23 14:19:04]

PTAL, please if you could run the try bots (at least the chromiumos ones),
Thanks.

[piman, 2014-12-23 20:12:19]

Thanks! LGTM.

[piman, 2014-12-23 20:12:23]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-12-23 20:13:28]

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

[commit-bot: I haz the power, 2014-12-23 20:19:33]

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

[commit-bot: I haz the power, 2014-12-23 20:19:34]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[llandwerlin-old, 2014-12-24 15:09:09]

Looks like the failures came from something outside this CL.
Missing scherkus@'s lgtm too.

[llandwerlin-old, 2014-12-24 15:09:15]

The CQ bit was checked by lionel.g.landwerlin@intel.com

[commit-bot: I haz the power, 2014-12-24 15:09:36]

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

[llandwerlin-old, 2014-12-24 15:11:25]

Checking whether tests are less flaky.

[commit-bot: I haz the power, 2014-12-24 15:14:55]

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

[commit-bot: I haz the power, 2014-12-24 15:14:56]

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

[Pawel Osciak, 2014-12-24 23:54:54]

scherkus@: I'm going to TBR you since this is a reland of CL you previously
LGTMed and we really need to get this in asap. Hope you don't mind. Thanks.

[Pawel Osciak, 2014-12-24 23:55:01]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2014-12-24 23:55:49]

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

[commit-bot: I haz the power, 2014-12-25 00:02:49]

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

[commit-bot: I haz the power, 2014-12-25 00:02:50]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Pawel Osciak, 2014-12-25 00:03:22]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2014-12-25 00:03:33]

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

[commit-bot: I haz the power, 2014-12-25 00:11:08]

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

[commit-bot: I haz the power, 2014-12-25 00:11:09]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Pawel Osciak, 2014-12-25 00:13:45]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2014-12-25 00:14:03]

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

[commit-bot: I haz the power, 2014-12-25 00:21:03]

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

[commit-bot: I haz the power, 2014-12-25 00:21:04]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[llandwerlin-old, 2014-12-25 00:45:28]

The CQ bit was checked by lionel.g.landwerlin@intel.com

[commit-bot: I haz the power, 2014-12-25 00:46:29]

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

[commit-bot: I haz the power, 2014-12-25 00:54:16]

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

[commit-bot: I haz the power, 2014-12-25 00:54:17]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Pawel Osciak, 2014-12-25 08:44:04]

The CQ bit was checked by posciak@chromium.org

[commit-bot: I haz the power, 2014-12-25 08:44:33]

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

[commit-bot: I haz the power, 2014-12-25 08:52:34]

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

[commit-bot: I haz the power, 2014-12-25 08:52:35]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[llandwerlin-old, 2014-12-25 17:55:25]

The CQ bit was checked by lionel.g.landwerlin@intel.com

[commit-bot: I haz the power, 2014-12-25 17:56:28]

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

[commit-bot: I haz the power, 2014-12-25 19:05:31]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-12-25 19:06:26]

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