Enable Accelerated Video Decode in Linux

Enable Accelerated Video Decode in Linux

We don't have supported Accelerated Video Decode in Linux, because Linux
graphics drivers and devices are fragmented. To support all would be nightmare.

However, it isn't good reason to disable Accelerated Video Decode on brand-new
powerful modern devices with latest driver. Many Ubuntu laptop users are sad on
Youtube.

If we only allow brand-new devices with latest driver, I believe we can support
it to Linux. I suggest to support it if a user
- have +IvyBridge (i.e. 3rd-generation Intel Processor released in 2013)
- have +0.34 of VAAPI version, which VaapiWrapper::VADisplayState::Initialize() already checks.

In addition, VaapiTFPPicture has been dead code since ChromeOS moved to Freon.
We should either remove VaapiTFPPicture or use VaapiTFPPicture.

TEST=all unittests, run on Ubuntu
BUG=590209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel

[dshwang, 2016-02-26 14:15:55]

Description was changed from

==========
Enable Accelerated Video Decode In Linux

We don't have supported Accelerated Video Decode in Linux, because Linux
graphics drivers and devices are fragmented. To support all would be nightmare.

However, it's good reason to disable Accelerated Video Decode on brand-new
powerful modern device with latest driver. Many Ubuntu laptop users are sad on
Youtube.

If we only allow brand-new devices with latest driver, I believe we can support
it to Linux. I suggest to support it if a user
- have +IvyBridge (i.e. 3rd-generation Intel Processor released in 2013)
- have +0.34 of VAAPI version, which VaapiWrapper::VADisplayState::Initialize()
already check.

In addition, VaapiTFPPicture has been dead code since ChromeOS moved to Freon.
We should either remove VaapiTFPPicture or use VaapiTFPPicture.

TEST=all unittests, run on Ubuntu
BUG=590209
==========

to

==========
Enable Accelerated Video Decode In Linux

We don't have supported Accelerated Video Decode in Linux, because Linux
graphics drivers and devices are fragmented. To support all would be nightmare.

However, it's good reason to disable Accelerated Video Decode on brand-new
powerful modern device with latest driver. Many Ubuntu laptop users are sad on
Youtube.

If we only allow brand-new devices with latest driver, I believe we can support
it to Linux. I suggest to support it if a user
- have +IvyBridge (i.e. 3rd-generation Intel Processor released in 2013)
- have +0.34 of VAAPI version, which VaapiWrapper::VADisplayState::Initialize()
already check.

In addition, VaapiTFPPicture has been dead code since ChromeOS moved to Freon.
We should either remove VaapiTFPPicture or use VaapiTFPPicture.

TEST=all unittests, run on Ubuntu
BUG=590209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[dshwang, 2016-02-26 14:21:46]

dongseong.hwang@intel.com changed reviewers:
+ avi@chromium.org, dalecurtis@chromium.org, jorgelo@chromium.org,
posciak@chromium.org

[dshwang, 2016-02-26 14:21:47]

Pawel, PTAL?

[dshwang, 2016-02-26 14:33:10]

Description was changed from

==========
Enable Accelerated Video Decode In Linux

We don't have supported Accelerated Video Decode in Linux, because Linux
graphics drivers and devices are fragmented. To support all would be nightmare.

However, it's good reason to disable Accelerated Video Decode on brand-new
powerful modern device with latest driver. Many Ubuntu laptop users are sad on
Youtube.

If we only allow brand-new devices with latest driver, I believe we can support
it to Linux. I suggest to support it if a user
- have +IvyBridge (i.e. 3rd-generation Intel Processor released in 2013)
- have +0.34 of VAAPI version, which VaapiWrapper::VADisplayState::Initialize()
already check.

In addition, VaapiTFPPicture has been dead code since ChromeOS moved to Freon.
We should either remove VaapiTFPPicture or use VaapiTFPPicture.

TEST=all unittests, run on Ubuntu
BUG=590209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Enable Accelerated Video Decode in Linux

We don't have supported Accelerated Video Decode in Linux, because Linux
graphics drivers and devices are fragmented. To support all would be nightmare.

However, it isn't good reason to disable Accelerated Video Decode on brand-new
powerful modern devices with latest driver. Many Ubuntu laptop users are sad on
Youtube.

If we only allow brand-new devices with latest driver, I believe we can support
it to Linux. I suggest to support it if a user
- have +IvyBridge (i.e. 3rd-generation Intel Processor released in 2013)
- have +0.34 of VAAPI version, which VaapiWrapper::VADisplayState::Initialize()
already check.

In addition, VaapiTFPPicture has been dead code since ChromeOS moved to Freon.
We should either remove VaapiTFPPicture or use VaapiTFPPicture.

TEST=all unittests, run on Ubuntu
BUG=590209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[dshwang, 2016-02-26 14:34:00]

Description was changed from

==========
Enable Accelerated Video Decode in Linux

We don't have supported Accelerated Video Decode in Linux, because Linux
graphics drivers and devices are fragmented. To support all would be nightmare.

However, it isn't good reason to disable Accelerated Video Decode on brand-new
powerful modern devices with latest driver. Many Ubuntu laptop users are sad on
Youtube.

If we only allow brand-new devices with latest driver, I believe we can support
it to Linux. I suggest to support it if a user
- have +IvyBridge (i.e. 3rd-generation Intel Processor released in 2013)
- have +0.34 of VAAPI version, which VaapiWrapper::VADisplayState::Initialize()
already check.

In addition, VaapiTFPPicture has been dead code since ChromeOS moved to Freon.
We should either remove VaapiTFPPicture or use VaapiTFPPicture.

TEST=all unittests, run on Ubuntu
BUG=590209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

to

==========
Enable Accelerated Video Decode in Linux

We don't have supported Accelerated Video Decode in Linux, because Linux
graphics drivers and devices are fragmented. To support all would be nightmare.

However, it isn't good reason to disable Accelerated Video Decode on brand-new
powerful modern devices with latest driver. Many Ubuntu laptop users are sad on
Youtube.

If we only allow brand-new devices with latest driver, I believe we can support
it to Linux. I suggest to support it if a user
- have +IvyBridge (i.e. 3rd-generation Intel Processor released in 2013)
- have +0.34 of VAAPI version, which VaapiWrapper::VADisplayState::Initialize()
already checks.

In addition, VaapiTFPPicture has been dead code since ChromeOS moved to Freon.
We should either remove VaapiTFPPicture or use VaapiTFPPicture.

TEST=all unittests, run on Ubuntu
BUG=590209
CQ_INCLUDE_TRYBOTS=tryserver.chromium.win:win_optional_gpu_tests_rel;tryserver.chromium.mac:mac_optional_gpu_tests_rel
==========

[Avi (use Gerrit), 2016-02-26 17:57:40]

I'll stamp if area experts approve.

https://codereview.chromium.org/1743543004/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/1743543004/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_data_manager_impl_private.cc:262: };
put operator< here and use std::tie...

struct CPU {
  int family;  // family of the processor
  int model;   // model of processor

  bool operator<(const CPU& other) const {
    return std::tie(family, model) < std::tie(other.family, other.model);
  }
};

https://codereview.chromium.org/1743543004/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_data_manager_impl_private.cc:285: (a.family == b.family
&& a.model < b.model);
... and not in here.

[Jorge Lucangeli Obes, 2016-02-26 18:02:44]

https://codereview.chromium.org/1743543004/diff/20001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/1743543004/diff/20001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:307: #if
defined(OS_CHROMEOS)
Use the "IsChromeOS" function here.

https://codereview.chromium.org/1743543004/diff/20001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:311: I965DrvVideoPath =
"/usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so";
Is this path guaranteed to be correct?

[dshwang, 2016-02-26 18:11:35]

thx for reviewing!

https://codereview.chromium.org/1743543004/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/1743543004/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_data_manager_impl_private.cc:262: };
On 2016/02/26 17:57:40, Avi wrote:
> put operator< here and use std::tie...
> 
> struct CPU {
>   int family;  // family of the processor
>   int model;   // model of processor
> 
>   bool operator<(const CPU& other) const {
>     return std::tie(family, model) < std::tie(other.family, other.model);
>   }
> };

chromium code style prohibits from adding any methods except for initialization
logic.
https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes

Can I add operator< method to struct?

[dshwang, 2016-02-26 18:15:34]

thx for reviewing!

https://codereview.chromium.org/1743543004/diff/20001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/1743543004/diff/20001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:307: #if
defined(OS_CHROMEOS)
On 2016/02/26 18:02:44, Jorge Lucangeli Obes wrote:
> Use the "IsChromeOS" function here.

Done.

https://codereview.chromium.org/1743543004/diff/20001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:311: I965DrvVideoPath =
"/usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so";
On 2016/02/26 18:02:44, Jorge Lucangeli Obes wrote:
> Is this path guaranteed to be correct?

no, it depends on linux distribution.

I have next plan to find this path in build time using python script.
If you want, I add the script in this CL.

[Jorge Lucangeli Obes, 2016-02-26 18:19:32]

https://codereview.chromium.org/1743543004/diff/20001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/1743543004/diff/20001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:311: I965DrvVideoPath =
"/usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so";
On 2016/02/26 18:15:34, dshwang wrote:
> On 2016/02/26 18:02:44, Jorge Lucangeli Obes wrote:
> > Is this path guaranteed to be correct?
> 
> no, it depends on linux distribution.
> 
> I have next plan to find this path in build time using python script.
> If you want, I add the script in this CL.

I don't have a preference, just pointing out that the only reason why we were
hardcoding paths here is because we were dealing with Chrome OS.

[DaleCurtis, 2016-02-26 18:25:12]

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

[DaleCurtis, 2016-02-26 18:25:13]

The traditional concern has been that we don't want to maintain a hardware
decode path on Linux, even a new one I'm sure will require maintenance from time
to time.

I defer to posciak@ and sandersd@ who are the primary guardians of this code
these days.

[dshwang, 2016-02-26 18:29:30]

Patchset #3 (id:40001) has been deleted

[Avi (use Gerrit), 2016-02-26 18:33:28]

https://codereview.chromium.org/1743543004/diff/20001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/1743543004/diff/20001/content/browser/gpu/gpu...
content/browser/gpu/gpu_data_manager_impl_private.cc:262: };
On 2016/02/26 18:11:35, dshwang wrote:
> On 2016/02/26 17:57:40, Avi wrote:
> > put operator< here and use std::tie...
> > 
> > struct CPU {
> >   int family;  // family of the processor
> >   int model;   // model of processor
> > 
> >   bool operator<(const CPU& other) const {
> >     return std::tie(family, model) < std::tie(other.family, other.model);
> >   }
> > };
> 
> chromium code style prohibits from adding any methods except for
initialization
> logic.
> https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes
> 
> Can I add operator< method to struct?

Hm.

OK, leave it in the binary_search, but use std::tie there.

[dshwang, 2016-02-26 18:34:59]

On 2016/02/26 18:25:13, DaleCurtis wrote:
> The traditional concern has been that we don't want to maintain a hardware
> decode path on Linux, even a new one I'm sure will require maintenance from
time
> to time.

I think it helps code maintenance because we can take advantage of existing
linux test bots. e.g. content_unittests, media_unittests, content_browsertests
It's difficult to run those tests on really chromeos devices.

In addition, it makes x11 path (e.g. VaapiTFPPicture) covered by test bots.

> I defer to posciak@ and sandersd@ who are the primary guardians of this code
> these days.

posciak@, sandersd@, PTAL?

https://codereview.chromium.org/1743543004/diff/20001/content/common/sandbox_...
File content/common/sandbox_linux/bpf_gpu_policy_linux.cc (right):

https://codereview.chromium.org/1743543004/diff/20001/content/common/sandbox_...
content/common/sandbox_linux/bpf_gpu_policy_linux.cc:311: I965DrvVideoPath =
"/usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so";
On 2016/02/26 18:19:31, Jorge Lucangeli Obes wrote:

> I don't have a preference, just pointing out that the only reason why we were
> hardcoding paths here is because we were dealing with Chrome OS.

Added TODO comment. The script will cover ChromeOS also.

[sandersd (OOO until July 31), 2016-02-26 19:23:23]

On 2016/02/26 18:25:13, DaleCurtis wrote:
> The traditional concern has been that we don't want to maintain a hardware
> decode path on Linux, even a new one I'm sure will require maintenance from
time
> to time.
> 
> I defer to posciak@ and sandersd@ who are the primary guardians of this code
> these days.

My scattered thoughts on this:
  - Dale is correct that we (the media team) don't have the resources to
maintain this properly. Even the Mac VDA, which is much less complex than
VA-API, requires about 1day/mo to keep it working.
  - That said, we all use Linux and it would be easier for us (again the media
team) to test the VDA path if we could easily use it on Linux. It looks like
with this change, we could use --no-sandbox and then VDPAU via vdpau-va-driver.
  - The absolute last thing we want is for users to choose to run with
--no-sandbox, but due to the above point there is a good chance there will be
tutorials explaining how to do so.

That last point is extremely important, so at the very least I would recommend a
protection for that.

I do support adding this as a disabled-by-default flag; enabling by default is
going to require someone to own the maintenance.

If we do add a flag but don't have official maintenance then we will want to do
some work to expose this flag more directly in bug reports (and to the user when
things fail). Users often set these things then forget about them entirely, then
blame Chrome when something goes wrong months later.

[Jorge Lucangeli Obes, 2016-02-26 20:51:36]

On 2016/02/26 19:23:23, sandersd wrote:
> On 2016/02/26 18:25:13, DaleCurtis wrote:
> > The traditional concern has been that we don't want to maintain a hardware
> > decode path on Linux, even a new one I'm sure will require maintenance from
> time
> > to time.
> > 
> > I defer to posciak@ and sandersd@ who are the primary guardians of this code
> > these days.
> 
> My scattered thoughts on this:
>   - Dale is correct that we (the media team) don't have the resources to
> maintain this properly. Even the Mac VDA, which is much less complex than
> VA-API, requires about 1day/mo to keep it working.
>   - That said, we all use Linux and it would be easier for us (again the media
> team) to test the VDA path if we could easily use it on Linux. It looks like
> with this change, we could use --no-sandbox and then VDPAU via
vdpau-va-driver.
>   - The absolute last thing we want is for users to choose to run with
> --no-sandbox, but due to the above point there is a good chance there will be
> tutorials explaining how to do so.
> 
> That last point is extremely important, so at the very least I would recommend
a
> protection for that.
> 
> I do support adding this as a disabled-by-default flag; enabling by default is
> going to require someone to own the maintenance.
> 
> If we do add a flag but don't have official maintenance then we will want to
do
> some work to expose this flag more directly in bug reports (and to the user
when
> things fail). Users often set these things then forget about them entirely,
then
> blame Chrome when something goes wrong months later.

Disabled by default is probably a good idea -- it was hard enough to maintain
this on Chrome OS, which we completely control.

[sandersd (OOO until July 31), 2016-02-26 20:58:21]

> Disabled by default is probably a good idea -- it was hard enough to maintain
> this on Chrome OS, which we completely control.

Another option, if we go the unsupported route, would be to add a flag but not
expose it in chrome://flags. In that case it takes enough expertise to enable
that we won't get a lot of bad bug reports.

(At the cost of helping fewer users, sadly.)

[seanvk, 2016-02-29 19:10:12]

On 2016/02/26 20:58:21, sandersd wrote:
> > Disabled by default is probably a good idea -- it was hard enough to
maintain
> > this on Chrome OS, which we completely control.
> 
> Another option, if we go the unsupported route, would be to add a flag but not
> expose it in chrome://flags. In that case it takes enough expertise to enable
> that we won't get a lot of bad bug reports.
> 
> (At the cost of helping fewer users, sadly.)

Who is going to maintain the PCI ID list?  What about compatibility in
distribution intel-driver/libva packages, which significantly lag platforms,
unlike CroS ebuilds. This seems kind of half baked to me.

I definitely would not want to get vaapi bugs filed because someone's trying to
hack a build on ArchLinux or CentOS7 for Chromium with acceleration enabled
through this flag and PCI ID. 

Sean

[dshwang, 2016-02-29 19:29:52]

On 2016/02/29 19:10:12, seanvk wrote:
> On 2016/02/26 20:58:21, sandersd wrote:
> > > Disabled by default is probably a good idea -- it was hard enough to
> maintain
> > > this on Chrome OS, which we completely control.
> > 
> > Another option, if we go the unsupported route, would be to add a flag but
not
> > expose it in chrome://flags. In that case it takes enough expertise to
enable
> > that we won't get a lot of bad bug reports.
> > 
> > (At the cost of helping fewer users, sadly.)
> 
> Who is going to maintain the PCI ID list?  What about compatibility in
> distribution intel-driver/libva packages, which significantly lag platforms,
> unlike CroS ebuilds. This seems kind of half baked to me.

CPUID list is from http://instlatx64.atw.hu/
In fact, I referred to existing perf code;
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/met...
If it's half baked, help me to complete the list.

> I definitely would not want to get vaapi bugs filed because someone's trying
to
> hack a build on ArchLinux or CentOS7 for Chromium with acceleration enabled
> through this flag and PCI ID. 

I suggest 3 criteria; CPU, GPU, and VAAPI version. 
Above is about only CPU.

Following existing code checks if the machine has +0.34 VAAPI.
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/gpu...

I'll add new code to restrict GPU.

[Julien Isorce Samsung, 2016-04-11 09:55:56]

Hi dshwang, I am just a contributor interested in your CL (I was planing to
enable it too at some point so that's great you did it). Would you mind to add
me to bug 590209 (Why is this bug in "restricted" state btw in the first place
?).
Would it make sense to check os version as well
(base::SysInfo::OperatingSystemVersion()) ? Easy way to require minimal version
of kernel side of libdrm (on which libva depends on).
Because existing code checks for libva version 0.34 which has been released in
2013 June (so it means that your CL will enable usage of vaapi intel-driver
released back from 2 or 3 years ago). Maybe this 0.34 can be bumped to 0.38 to
match libva version in chromium/src/third_party/libva :
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libva/....

[Julien Isorce Samsung, 2016-04-11 10:07:44]

On 2016/04/11 09:55:56, j.isorce wrote:
> Hi dshwang, I am just a contributor interested in your CL (I was planing to
> enable it too at some point so that's great you did it). Would you mind to add
> me to bug 590209 (Why is this bug in "restricted" state btw in the first place
> ?).
> Would it make sense to check os version as well
> (base::SysInfo::OperatingSystemVersion()) ? Easy way to require minimal
version
> of kernel side of libdrm (on which libva depends on).
> Because existing code checks for libva version 0.34 which has been released in
> 2013 June (so it means that your CL will enable usage of vaapi intel-driver
> released back from 2 or 3 years ago). Maybe this 0.34 can be bumped to 0.38 to
> match libva version in chromium/src/third_party/libva :
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/libva/....

Also I suggest to add "exceptions" field in
gpu/config/software_rendering_list_json.cc::"id 48" instead of removing it
entirely. In that exception you can check for "cpu_info" (a string that will be
compared to base::CPU cpu_info, maybe this can be used instead of what you
added), that would also allow to check for other criteria GPU you mentioned, so
using: vendor_id/device_id in that "exception" brackets.

[allan.jensen, 2016-04-15 09:03:27]

I think I duplicated this patch a bit here:
https://codereview.chromium.org/1872393002

The main difference being I also activated JPEG acceleration, and didn't
whitelist.

My experiments shows it works fine on VDPAU implementations too (NVidia
drivers).

[allan.jensen, 2016-04-15 09:16:21]

https://codereview.chromium.org/1743543004/diff/80001/content/browser/gpu/gpu...
File content/browser/gpu/gpu_data_manager_impl_private.cc (right):

https://codereview.chromium.org/1743543004/diff/80001/content/browser/gpu/gpu...
content/browser/gpu/gpu_data_manager_impl_private.cc:279: const CPU search_elem
= {cpuid.family(), cpuid.model()};
This seems completely wrong. 

VAAPI is usually comes from the GPU vendor. This would test the wrong thing on
any system with a discrete GPU.

Also, I haven't had any trouble with a VAAPI SandyBridge systems (except fewer
supported profiles).

[Julien Isorce Samsung, 2016-07-01 10:58:48]

Any plan to go further ?

1: libva and vaapi driver version:
I would suggest to check libva version at build time (so in gn/gyp).
Adding libva_version >= 1.7 on os linux for example would solve to sysroot
problem or just bump files there.
Also in src/media/gpu/vaapi_wrapper.cc you should bump the min required
vaapi backend/driver version to 0.39 if defined(OS_LINUX)

2: hardcoded path to dri:
I have a CL where I also need that path, I can split it to isolate
that change in one single CL:
https://codereview.chromium.org/1542013005/patch/220001/230004
Then you will be able to use base::FilePath dri_path(DRI_DRIVER_DIR);

3: keep it black listed in gpu/config/software_rendering_list_json.cc
The user is already be able to pass "--accelerated_video_decode=1" to command
line.
(it is possible to disable a specific gpu driver bug workaroung from cmd line)

4: Integrate JPEG support from the other CL
https://codereview.chromium.org/1872393002/
that duplicated your changes.

[pdknsk, 2016-08-30 02:14:04]

Since discussion is scattered around a few unsuccessful CLs, I made a thread to
discuss this in a single place.

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GwXWt_7tglc