content: Add support for Video Decode Acceleration on GBM

content: Add support for Video Decode Acceleration on GBM
    
This CL adds Video Decode Acceleration support via VAAPI to Ozone GBM. It uses
MediaOzonePlatform abstraction for such. VAAPI wrapper and implementation needed to be changed,
adding some kind of fallback for libva-drm and GBM instead.


BUG=380884
TEST=./chrome --no-sandbox --ozone-platform=gbm --ignore-gpu-blacklist ../../third_party/WebKit/PerformanceTests/resources/bear-1280x720.mp4

[vignatti (out of this project), 2014-07-23 23:45:13]

I'd like to send this out for the blessing of you guys before I start to trim
some wrong things in this CL. So, I see the following possible changes:

ozone related:
- move MediaOzonePlatformGbm to media/ozone/, so it would be better organized,
also according the platform implementations in ui/ozone/. But to make that
possible, we would have to move the whole Vaapi stuff out of content/ (right now
we have content::VaapiVideoDecodeAccelerator() being called from
MediaOzonePlatformGbm::CreateVideoDecodeAccelerator)

media related: 
- for organization's sake mentioned above, I envision moving the whole vaapi and
h264 code from content/common/gpu/media/ to media/. The code in there seems weak
coupled with the rest of content and video acceleration, so I don't see any
problem on doing that right now.

Thank you.

[spang, 2014-07-24 19:31:24]

On 2014/07/23 23:45:13, vignatti wrote:
> I'd like to send this out for the blessing of you guys before I start to trim
> some wrong things in this CL. So, I see the following possible changes:
> 
> ozone related:
> - move MediaOzonePlatformGbm to media/ozone/, so it would be better organized,
> also according the platform implementations in ui/ozone/. But to make that
> possible, we would have to move the whole Vaapi stuff out of content/ (right
now
> we have content::VaapiVideoDecodeAccelerator() being called from
> MediaOzonePlatformGbm::CreateVideoDecodeAccelerator)
> 
> media related: 
> - for organization's sake mentioned above, I envision moving the whole vaapi
and
> h264 code from content/common/gpu/media/ to media/. The code in there seems
weak
> coupled with the rest of content and video acceleration, so I don't see any
> problem on doing that right now.
> 
> Thank you.

Thanks for taking this on!

I looked very briefly. One concern is, please avoid adding a OZONE_PLATFORM_GBM
#ifdef. It looks like you actually want a feature-based ifdef to enable libva
drm support. So a USE_LIBVA_DRM #ifdef might be appropriate, although if we can
structure it to avoid it altogether that's even better.

[vignatti (out of this project), 2014-07-28 17:48:07]

On 2014/07/24 19:31:24, spang wrote:
> On 2014/07/23 23:45:13, vignatti wrote:
> > I'd like to send this out for the blessing of you guys before I start to
trim
> > some wrong things in this CL. So, I see the following possible changes:
> > 
> > ozone related:
> > - move MediaOzonePlatformGbm to media/ozone/, so it would be better
organized,
> > also according the platform implementations in ui/ozone/. But to make that
> > possible, we would have to move the whole Vaapi stuff out of content/ (right
> now
> > we have content::VaapiVideoDecodeAccelerator() being called from
> > MediaOzonePlatformGbm::CreateVideoDecodeAccelerator)
> > 
> > media related: 
> > - for organization's sake mentioned above, I envision moving the whole vaapi
> and
> > h264 code from content/common/gpu/media/ to media/. The code in there seems
> weak
> > coupled with the rest of content and video acceleration, so I don't see any
> > problem on doing that right now.
> > 
> > Thank you.
> 
> Thanks for taking this on!
> 
> I looked very briefly. One concern is, please avoid adding a
OZONE_PLATFORM_GBM
> #ifdef. It looks like you actually want a feature-based ifdef to enable libva
> drm support. So a USE_LIBVA_DRM #ifdef might be appropriate, although if we
can
> structure it to avoid it altogether that's even better.

right. OZONE_PLATFORM_GBM is a bit non-sense actually cause without both X11 and
OZONE_PLATFORM_GBM in there, the VAAPI would get broken.

Maybe we could think the code being added into vaapi_wrapper simply as a
fallback case, using 'else' instead. In fact, Ozone-Wayland could use pretty
much the same code then besides the library initialization part and the headers
inclusion (we could change further). So I think doing like this would work much
better; patch set 4 with these changes was just uploaded.

xhwang@ do you have a better insight on what to do with vaapi changes I've been
adding? Or any other way to organize the code in here?

[vignatti (out of this project), 2014-07-29 14:31:01]

scherkus@ PTAL.

[scherkus (not reviewing), 2014-07-29 17:48:55]

+posciak

apologies I left a bunch of nits before seeing your bigger-picture questions


with respect to the location of the code, I believe other VDA implementations
may have stronger ties to content/ so they've historically been kept together

also is the intent to maintain two sets of nearly-identical VAAPI code?

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/media_ozone_platform_gbm.cc (right):

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/media_ozone_platform_gbm.cc:21: // MediaOzonePlatform:
nit: "MediaOzonePlatform implementation."

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/media_ozone_platform_gbm.h (right):

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/media_ozone_platform_gbm.h:8: namespace media {
is it typical to have differing namespaces for ozone?

are there other examples of this?

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator_drm.cc (right):

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator_drm.cc:16: #include
"content/common/gpu/media/vaapi_video_decode_accelerator_drm.h"
looks like this .cc file has incorrect include order

this file should be included first, followed by a blank line, then <foo>
includes, then a blank line, then the rest

would you mind fixing?

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator_drm.cc:171: if
(va_wrapper_) {
nit: a lot of this existing code seems to use no {} for single-line if
statements

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator_drm.cc:236: fd_ = 0;
is this really necessary? if we're protecting against other code checking fd_
after the dtor exits we've got bigger problems on our hands!

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator_drm.h (right):

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator_drm.h:11: #include
<list>
might have missed this ... but what changed to require this new #include?

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator_drm.h:47:
VaapiVideoDecodeAccelerator(
needs explicit keyword

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

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:581: void
VaapiWrapper::UnmapImage(VAImage* image) {
blank line before this

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

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.h:103: #if defined(USE_X11)
out of curiosity, is USE_X11 sticking around for vaapi, or is this more of a
transitional thing?

https://codereview.chromium.org/385793002/diff/60001/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/385793002/diff/60001/content/content_common.g...
content/content_common.gypi:763: [ 'use_x11 == 1', {
remove space between [ and '

[vignatti (out of this project), 2014-07-30 21:51:30]

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/media_ozone_platform_gbm.cc (right):

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/media_ozone_platform_gbm.cc:21: // MediaOzonePlatform:
On 2014/07/29 17:48:54, scherkus wrote:
> nit: "MediaOzonePlatform implementation."

Done.

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/media_ozone_platform_gbm.h (right):

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/media_ozone_platform_gbm.h:8: namespace media {
On 2014/07/29 17:48:54, scherkus wrote:
> is it typical to have differing namespaces for ozone?
> 
> are there other examples of this?

in particular for media, we decided to maintain the new MediaOzonePlatform class
inside media namespace, so the developers that know better the details of the
code would deal with it. 

We had a fairly long discussion a few weeks ago about it:
https://codereview.chromium.org/269673005/

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator_drm.cc (right):

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator_drm.cc:16: #include
"content/common/gpu/media/vaapi_video_decode_accelerator_drm.h"
On 2014/07/29 17:48:54, scherkus wrote:
> looks like this .cc file has incorrect include order
> 
> this file should be included first, followed by a blank line, then <foo>
> includes, then a blank line, then the rest
> 
> would you mind fixing?

right, I fixed. The problem happened because I've copied the same structure from
vaapi_video_decode_accelerator.cc, which has this wrong order.

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator_drm.cc:171: if
(va_wrapper_) {
On 2014/07/29 17:48:54, scherkus wrote:
> nit: a lot of this existing code seems to use no {} for single-line if
> statements

Done.

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator_drm.cc:236: fd_ = 0;
On 2014/07/29 17:48:54, scherkus wrote:
> is this really necessary? if we're protecting against other code checking fd_
> after the dtor exits we've got bigger problems on our hands!

fd_ was initialized in the constructor and for safety it's being zero'ed in the
dtor. Simple as that. I don't get why you are worried with this tbh. In any case
I removed the *other* fd_ from TFPPicture that was being superfluous used.

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
File content/common/gpu/media/vaapi_video_decode_accelerator_drm.h (right):

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator_drm.h:11: #include
<list>
On 2014/07/29 17:48:54, scherkus wrote:
> might have missed this ... but what changed to require this new #include?

if I don't add this in here it complains about line 207: 
std::list<VASurfaceID> available_va_surfaces_;

but why this doesn't happen with aapi_video_decode_accelerator.h? Good question.

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_video_decode_accelerator_drm.h:47:
VaapiVideoDecodeAccelerator(
On 2014/07/29 17:48:54, scherkus wrote:
> needs explicit keyword

sorry, what? Are you asking to change all the class to
VaapiVideoDecodeAcceleratorDRM?

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

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.cc:581: void
VaapiWrapper::UnmapImage(VAImage* image) {
On 2014/07/29 17:48:55, scherkus wrote:
> blank line before this

Done.

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

https://codereview.chromium.org/385793002/diff/60001/content/common/gpu/media...
content/common/gpu/media/vaapi_wrapper.h:103: #if defined(USE_X11)
On 2014/07/29 17:48:55, scherkus wrote:
> out of curiosity, is USE_X11 sticking around for vaapi, or is this more of a
> transitional thing?

it's a good question that I don't know the answer. I just recently started to
deal with vaapi and meet the developers.

It's reasonable to think that X11 will be there while consumers exist for it. I
think we should at least give a feedback for the developers about our usage, say
try to have a common interface for the different backends etc.

https://codereview.chromium.org/385793002/diff/60001/content/content_common.gypi
File content/content_common.gypi (right):

https://codereview.chromium.org/385793002/diff/60001/content/content_common.g...
content/content_common.gypi:763: [ 'use_x11 == 1', {
On 2014/07/29 17:48:55, scherkus wrote:
> remove space between [ and '

Done.

[vignatti (out of this project), 2014-07-30 22:00:10]

On 2014/07/29 17:48:55, scherkus wrote:
> 
> with respect to the location of the code, I believe other VDA implementations
> may have stronger ties to content/ so they've historically been kept together
> 
> also is the intent to maintain two sets of nearly-identical VAAPI code?

ideally no. If you guys prefer I can work on extracting the window-system part
of it.. I'm just lacking a bit of insight on how to do it. Ideas?

[vignatti (out of this project), 2014-07-30 23:04:40]

On 2014/07/30 22:00:10, vignatti wrote:
> On 2014/07/29 17:48:55, scherkus wrote:
> > 
> > with respect to the location of the code, I believe other VDA
implementations
> > may have stronger ties to content/ so they've historically been kept
together
> > 
> > also is the intent to maintain two sets of nearly-identical VAAPI code?
> 
> ideally no. If you guys prefer I can work on extracting the window-system part
> of it.. I'm just lacking a bit of insight on how to do it. Ideas?

BTW the whole vaapi_video_decode_accelerator_drm.cc has 872 lines of code and
the diffstat of it with its X11 counter-part is 109 insertions(+), 58
deletions(-). So given the proportion I think it's worth to simply ugly-wrap
using USE_X11 macros around and have the DRM as a fallback, i.e. using a single
vaapi_video_decode_accelerator.cc for both implementations.

[scherkus (not reviewing), 2014-07-31 01:19:33]

On 2014/07/30 23:04:40, vignatti wrote:
> On 2014/07/30 22:00:10, vignatti wrote:
> > On 2014/07/29 17:48:55, scherkus wrote:
> > > 
> > > with respect to the location of the code, I believe other VDA
> implementations
> > > may have stronger ties to content/ so they've historically been kept
> together
> > > 
> > > also is the intent to maintain two sets of nearly-identical VAAPI code?
> > 
> > ideally no. If you guys prefer I can work on extracting the window-system
part
> > of it.. I'm just lacking a bit of insight on how to do it. Ideas?
> 
> BTW the whole vaapi_video_decode_accelerator_drm.cc has 872 lines of code and
> the diffstat of it with its X11 counter-part is 109 insertions(+), 58
> deletions(-). So given the proportion I think it's worth to simply ugly-wrap
> using USE_X11 macros around and have the DRM as a fallback, i.e. using a
single
> vaapi_video_decode_accelerator.cc for both implementations.

Anything other than duplicating the code would be preferable. Perhaps there's a
path forward teasing out the X11-vs-DRM bits as well, but posciak@ is the expert
with this code.

[vignatti (out of this project), 2014-07-31 19:54:57]

On 2014/07/31 01:19:33, scherkus wrote:
> Anything other than duplicating the code would be preferable. Perhaps there's
a
> path forward teasing out the X11-vs-DRM bits as well, but posciak@ is the
expert
> with this code.

sure sure. New patchset sent. PTAL.

[Pawel Osciak, 2014-08-01 11:17:44]

I'm sorry, but this is too many #ifdefs.

High level comments, I'd suggest:
- abstracting TFPPicture into PlatformPicture or something similar and having a
different implementation for a common virtual method called Output (or something
similar), that would either Upload or Bind, and similarly abstract other things
as required;
- I don't think we need to out-ifdef all the methods in VaapiWrapper, things
like MapImage work in both cases. The only difference should probably be in
Create; please just have the factory method handle the difference;

[marcheu, 2014-08-07 01:26:26]

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

https://chromiumcodereview.appspot.com/385793002/diff/100001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:311: 0, GL_RGBA,
GL_UNSIGNED_BYTE, buffer);
This is a synchronous, CPU-side copy. This copy does not exist today with the
X11 TFP path today (we use zero-copy there) and there is no reason to add it.

Can we use an eglimage instead?

[vignatti (out of this project), 2014-08-12 08:19:02]

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

https://chromiumcodereview.appspot.com/385793002/diff/100001/content/common/g...
content/common/gpu/media/vaapi_video_decode_accelerator.cc:311: 0, GL_RGBA,
GL_UNSIGNED_BYTE, buffer);
On 2014/08/07 01:26:25, marcheu wrote:
> This is a synchronous, CPU-side copy. This copy does not exist today with the
> X11 TFP path today (we use zero-copy there) and there is no reason to add it.
> 
> Can we use an eglimage instead?

FYI point taken. We are working on that.

[seanvk, 2014-08-21 18:40:46]

On 2014/08/12 08:19:02, vignatti wrote:
>
https://chromiumcodereview.appspot.com/385793002/diff/100001/content/common/g...
> File content/common/gpu/media/vaapi_video_decode_accelerator.cc (right):
> 
>
https://chromiumcodereview.appspot.com/385793002/diff/100001/content/common/g...
> content/common/gpu/media/vaapi_video_decode_accelerator.cc:311: 0, GL_RGBA,
> GL_UNSIGNED_BYTE, buffer);
> On 2014/08/07 01:26:25, marcheu wrote:
> > This is a synchronous, CPU-side copy. This copy does not exist today with
the
> > X11 TFP path today (we use zero-copy there) and there is no reason to add
it.
> > 
> > Can we use an eglimage instead?
> 
> FYI point taken. We are working on that.

I agree with Pawel.  There are way too many ifdefs.  A lot of our VAAPI commands
work in both cases regardless.

[vignatti (out of this project), 2015-02-09 20:58:29]

CLs 817023005 and 825163004 have done this job. So I'm closing this one. Thanks
everyone involved!