Refactor Vaapi video decoder/encoder in preparation of Freon support

Refactor Vaapi video decoder/encoder in preparation of Freon support

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

Committed: https://crrev.com/289e96375e75be0d9158b86df2bd4faf2d7b2a64
Cr-Commit-Position: refs/heads/master@{#309355}

[llandwerlin-old, 2014-12-17 13:11:16]

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

[llandwerlin-old, 2014-12-17 13:11:17]

This is a split of the  https://codereview.chromium.org/490233002/

There is almost no new changes to the code apart from a few nits from piman@.

reveman@chromium.org: Please review changes in ui/glx

posciak@chromium.org: Please review changes in content/common/gpu/media

scherkus@chromium.org: Please review changes in content/common/gpu/media

piman@chromium.org: Please review changes

Thanks

[scherkus (not reviewing), 2014-12-17 17:47:56]

lgtm

[piman, 2014-12-17 18:57:06]

a few things...

https://codereview.chromium.org/807853005/diff/40001/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/807853005/diff/40001/content/common/BUILD.gn#...
content/common/BUILD.gn:280: if (cpu_arch == "arm" && use_x11) {
I don't think this is right - we don't want to use vaapi on arm && !use_x11

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

https://codereview.chromium.org/807853005/diff/40001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:274: #elif
defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY)
Is this code going to behave gracefully before the ozone bits are added?
If not, can we keep the && defined(USE_X11) here, and remove it in the patch
that adds the ozone bits

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

https://codereview.chromium.org/807853005/diff/40001/content/common/gpu/media...
content/common/gpu/media/gpu_video_encode_accelerator.cc:192:
scoped_ptr<media::VideoEncodeAccelerator> encoder;
same here wrt USE_X11

https://codereview.chromium.org/807853005/diff/40001/content/common/gpu/media...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/807853005/diff/40001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.h:149: const base::Closure&
report_error__to_uma_cb);
nit: spurious _ added

https://codereview.chromium.org/807853005/diff/40001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/807853005/diff/40001/content/content_tests.gy...
content/content_tests.gypi:1624: ['use_x11==1', {
This condition is already true in this block. Did you mean to remove it from
l.1604?

https://codereview.chromium.org/807853005/diff/40001/ui/gl/gl_image_glx.cc
File ui/gl/gl_image_glx.cc (right):

https://codereview.chromium.org/807853005/diff/40001/ui/gl/gl_image_glx.cc#ne...
ui/gl/gl_image_glx.cc:62: case GL_RGBA:
what about GL_RGB? It's handled in all other functions.

[llandwerlin-old, 2014-12-17 21:24:59]

https://codereview.chromium.org/807853005/diff/40001/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/807853005/diff/40001/content/common/BUILD.gn#...
content/common/BUILD.gn:280: if (cpu_arch == "arm" && use_x11) {
On 2014/12/17 18:57:06, piman (Very slow to review) wrote:
> I don't think this is right - we don't want to use vaapi on arm && !use_x11

The logic indicates that's what was enabled before.

For ChromeOS, hardware video decode/encode is only available with X11.
I'm just relaxing the constraint x86, where this CL allows it all the time (even
though it's going to fail at runtime).

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

https://codereview.chromium.org/807853005/diff/40001/content/common/gpu/media...
content/common/gpu/media/gpu_video_decode_accelerator.cc:274: #elif
defined(OS_CHROMEOS) && defined(ARCH_CPU_X86_FAMILY)
On 2014/12/17 18:57:06, piman (Very slow to review) wrote:
> Is this code going to behave gracefully before the ozone bits are added?
> If not, can we keep the && defined(USE_X11) here, and remove it in the patch
> that adds the ozone bits 

I tried this code on Ozone and it fails (gracefully) when initializing the
hardware decoder which triggers the software fallback path.

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

https://codereview.chromium.org/807853005/diff/40001/content/common/gpu/media...
content/common/gpu/media/gpu_video_encode_accelerator.cc:192:
scoped_ptr<media::VideoEncodeAccelerator> encoder;
On 2014/12/17 18:57:06, piman (Very slow to review) wrote:
> same here wrt USE_X11

That will fail gracefully when calling ->Initialize() or return an empty list
when calling ->GetSupportedProfiles().

https://codereview.chromium.org/807853005/diff/40001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/807853005/diff/40001/content/content_tests.gy...
content/content_tests.gypi:1624: ['use_x11==1', {
On 2014/12/17 18:57:06, piman (Very slow to review) wrote:
> This condition is already true in this block. Did you mean to remove it from
> l.1604?

Thanks, removing this.

https://codereview.chromium.org/807853005/diff/40001/ui/gl/gl_image_glx.cc
File ui/gl/gl_image_glx.cc (right):

https://codereview.chromium.org/807853005/diff/40001/ui/gl/gl_image_glx.cc#ne...
ui/gl/gl_image_glx.cc:62: case GL_RGBA:
On 2014/12/17 18:57:06, piman (Very slow to review) wrote:
> what about GL_RGB? It's handled in all other functions.

Thanks, adding that.

[piman, 2014-12-17 21:42:02]

https://codereview.chromium.org/807853005/diff/40001/content/common/BUILD.gn
File content/common/BUILD.gn (right):

https://codereview.chromium.org/807853005/diff/40001/content/common/BUILD.gn#...
content/common/BUILD.gn:280: if (cpu_arch == "arm" && use_x11) {
On 2014/12/17 21:24:58, llandwerlin wrote:
> On 2014/12/17 18:57:06, piman (Very slow to review) wrote:
> > I don't think this is right - we don't want to use vaapi on arm && !use_x11
> 
> The logic indicates that's what was enabled before.

No... in the chromeos and !use_x11 case we would skip the entire (is_chromeos &&
use_x11) block.
After your change, we would go inside the is_chromeos block, fail the (cpu_arch
== "arm" && use_x11) if, so use the else clause and try to use vaapi.

> For ChromeOS, hardware video decode/encode is only available with X11.
> I'm just relaxing the constraint x86, where this CL allows it all the time
(even
> though it's going to fail at runtime).

I understand the intent, but I don't think that's what you've done here.

[llandwerlin-old, 2014-12-18 12:02:11]

On 2014/12/17 21:42:02, piman (Very slow to review) wrote:
> https://codereview.chromium.org/807853005/diff/40001/content/common/BUILD.gn
> File content/common/BUILD.gn (right):
> 
>
https://codereview.chromium.org/807853005/diff/40001/content/common/BUILD.gn#...
> content/common/BUILD.gn:280: if (cpu_arch == "arm" && use_x11) {
> On 2014/12/17 21:24:58, llandwerlin wrote:
> > On 2014/12/17 18:57:06, piman (Very slow to review) wrote:
> > > I don't think this is right - we don't want to use vaapi on arm &&
!use_x11
> > 
> > The logic indicates that's what was enabled before.
> 
> No... in the chromeos and !use_x11 case we would skip the entire (is_chromeos
&&
> use_x11) block.
> After your change, we would go inside the is_chromeos block, fail the
(cpu_arch
> == "arm" && use_x11) if, so use the else clause and try to use vaapi.
> 
> > For ChromeOS, hardware video decode/encode is only available with X11.
> > I'm just relaxing the constraint x86, where this CL allows it all the time
> (even
> > though it's going to fail at runtime).
> 
> I understand the intent, but I don't think that's what you've done here.

Thanks, that wasn't quite right. I also spotted another remaining issue above in
the same file.

[piman, 2014-12-18 19:51:59]

LGTM though I would like resolution on the RefCounted vs RefCountedThreadSafe
issue. Do we use VaapiWrapper on multiple threads or not? If not we should
remove the locks. If yes, we should make it RefCountedThreadSafe (though I
suspect there would be extra issues wrt e.g. using the X Display from multiple
threads).

[piman, 2014-12-18 21:53:19]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-12-18 21:54:37]

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

[commit-bot: I haz the power, 2014-12-18 22:02:29]

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

[commit-bot: I haz the power, 2014-12-18 22:02:30]

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

[piman, 2014-12-18 22:53:38]

https://codereview.chromium.org/807853005/diff/80001/content/common/gpu/media...
File content/common/gpu/media/DEPS (right):

https://codereview.chromium.org/807853005/diff/80001/content/common/gpu/media...
content/common/gpu/media/DEPS:5: "+ui",
You shouldn't need all of ui, just ui/gl and ui/gfx, that you're already
inheriting from parent directories.

[llandwerlin-old, 2014-12-19 15:09:16]

On 2014/12/18 22:53:38, piman (Very slow to review) wrote:
>
https://codereview.chromium.org/807853005/diff/80001/content/common/gpu/media...
> File content/common/gpu/media/DEPS (right):
> 
>
https://codereview.chromium.org/807853005/diff/80001/content/common/gpu/media...
> content/common/gpu/media/DEPS:5: "+ui",
> You shouldn't need all of ui, just ui/gl and ui/gfx, that you're already
> inheriting from parent directories.

Thanks, I will upload an update tomorrow and also make VaapiWrapper
RefCountedThreadSafe.

[llandwerlin-old, 2014-12-20 15:52:09]

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

[commit-bot: I haz the power, 2014-12-20 15:53:06]

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

[commit-bot: I haz the power, 2014-12-20 17:49:46]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-12-20 17:50:48]

Patchset 6 (id:??) landed as
https://crrev.com/289e96375e75be0d9158b86df2bd4faf2d7b2a64
Cr-Commit-Position: refs/heads/master@{#309355}

[dcheng, 2014-12-20 18:49:43]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/804353003/ by dcheng@chromium.org.

The reason for reverting is: Caused build failure on Linux ChromiumOS GN:
ninja: error: '../../content/common/content/common/gpu/media/va_x11.sigs',
needed by 'obj/content/common/libva_generate_stubs.inputdeps.stamp', missing
and no known rule to make it
.

[dcheng, 2014-12-20 18:49:59]

dcheng@chromium.org changed reviewers:
+ dcheng@chromium.org

[dcheng, 2014-12-20 18:50:00]

Link to the failing build:
http://build.chromium.org/p/chromium.chromiumos/buildstatus?builder=Linux%20C...

[Pawel Osciak, 2014-12-20 20:02:34]

On 2014/12/18 19:51:59, piman (Very slow to review) wrote:
> LGTM though I would like resolution on the RefCounted vs RefCountedThreadSafe
> issue. Do we use VaapiWrapper on multiple threads or not? If not we should
> remove the locks. If yes, we should make it RefCountedThreadSafe (though I
> suspect there would be extra issues wrt e.g. using the X Display from multiple
> threads).

VaapiWrapper is accessed from multiple thread, the decoder thread and the
GpuChild tread. Also, if we have multiple decoders running, it can be accessed
from multiple decoder threads as well.
Only the functions that do not make X calls are called from decoder threads, the
ones that require X are called from GpuChild only.

[Pawel Osciak, 2014-12-21 00:25:07]

On 2014/12/20 20:02:34, Pawel Osciak wrote:
> On 2014/12/18 19:51:59, piman (Very slow to review) wrote:
> > LGTM though I would like resolution on the RefCounted vs
RefCountedThreadSafe
> > issue. Do we use VaapiWrapper on multiple threads or not? If not we should
> > remove the locks. If yes, we should make it RefCountedThreadSafe (though I
> > suspect there would be extra issues wrt e.g. using the X Display from
multiple
> > threads).
> 
> VaapiWrapper is accessed from multiple thread, the decoder thread and the
> GpuChild tread. Also, if we have multiple decoders running, it can be accessed
> from multiple decoder threads as well.
> Only the functions that do not make X calls are called from decoder threads,
the
> ones that require X are called from GpuChild only.

To further clarify, in the case of multiple decoders, what I meant by
VaapiWrapper accessed on each decoder's thread was libva itself being accessed
from multiple decoder threads (each decoder has its own instance of
VaapiWrapper). In that case however each has a separate libva context for
concurrent calls on decoder threads, and is still serialized for X on GpuChild.

[piman, 2014-12-22 19:12:57]

On 2014/12/21 00:25:07, Pawel Osciak wrote:
> On 2014/12/20 20:02:34, Pawel Osciak wrote:
> > On 2014/12/18 19:51:59, piman (Very slow to review) wrote:
> > > LGTM though I would like resolution on the RefCounted vs
> RefCountedThreadSafe
> > > issue. Do we use VaapiWrapper on multiple threads or not? If not we should
> > > remove the locks. If yes, we should make it RefCountedThreadSafe (though I
> > > suspect there would be extra issues wrt e.g. using the X Display from
> multiple
> > > threads).
> > 
> > VaapiWrapper is accessed from multiple thread, the decoder thread and the
> > GpuChild tread. Also, if we have multiple decoders running, it can be
accessed
> > from multiple decoder threads as well.
> > Only the functions that do not make X calls are called from decoder threads,
> the
> > ones that require X are called from GpuChild only.
> 
> To further clarify, in the case of multiple decoders, what I meant by
> VaapiWrapper accessed on each decoder's thread was libva itself being accessed
> from multiple decoder threads (each decoder has its own instance of
> VaapiWrapper). In that case however each has a separate libva context for
> concurrent calls on decoder threads, and is still serialized for X on
GpuChild.

Then, with this CL that adds refcounting, what ensures the destructor (which
implicitly does X calls, I believe, in e.g. vaTerminate) is called on the main
thread?

[Pawel Osciak, 2014-12-23 01:23:38]

On 2014/12/22 19:12:57, piman (Very slow to review) wrote:
> On 2014/12/21 00:25:07, Pawel Osciak wrote:
> > On 2014/12/20 20:02:34, Pawel Osciak wrote:
> > > On 2014/12/18 19:51:59, piman (Very slow to review) wrote:
> > > > LGTM though I would like resolution on the RefCounted vs
> > RefCountedThreadSafe
> > > > issue. Do we use VaapiWrapper on multiple threads or not? If not we
should
> > > > remove the locks. If yes, we should make it RefCountedThreadSafe (though
I
> > > > suspect there would be extra issues wrt e.g. using the X Display from
> > multiple
> > > > threads).
> > > 
> > > VaapiWrapper is accessed from multiple thread, the decoder thread and the
> > > GpuChild tread. Also, if we have multiple decoders running, it can be
> accessed
> > > from multiple decoder threads as well.
> > > Only the functions that do not make X calls are called from decoder
threads,
> > the
> > > ones that require X are called from GpuChild only.
> > 
> > To further clarify, in the case of multiple decoders, what I meant by
> > VaapiWrapper accessed on each decoder's thread was libva itself being
accessed
> > from multiple decoder threads (each decoder has its own instance of
> > VaapiWrapper). In that case however each has a separate libva context for
> > concurrent calls on decoder threads, and is still serialized for X on
> GpuChild.
> 
> Then, with this CL that adds refcounting, what ensures the destructor (which
> implicitly does X calls, I believe, in e.g. vaTerminate) is called on the main
> thread?

VaapiVideoDecodeAccelerator derives from VideoDecodeAccelerator, which has a
specialized deleter that calls VDA::Destroy() to destroy itself first, which
should happen only as a consequence of GpuCommandBufferStub going away via
observer implementation of OnWillDestroyStub in GpuVideoDecodeAccelerator. My
understanding was that this was enough?