Call vaInitialize() at PreSandbox stage.

Call vaInitialize() at PreSandbox stage.

libva will dlopen driver when vaInitialize() get called. This call
should be called before sandboxing otherwise it will fail.

BUG=244386
TEST=ppapi_example_video_decode

[commit-bot: I haz the power, 2013-05-31 11:08:43]

No LGTM from a valid reviewer yet. Only full committers are accepted.
Even if an LGTM may have been provided, it was from a non-committer or
a lowly provisional committer, _not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[Ami GONE FROM CHROMIUM, 2013-05-31 16:19:24]

This change makes me sad.

We intentionally minimize the work done in PreSandbox, not only for security,
but also because the cost is paid at process-startup time.  Can you time the
impact of this change on lumpy/stumpy?

Does simply dlopen()'ing the relevant driver .so in PreSandbox (hardcoding its
path) fix your use case?

BTW chromium CR process note: upload your change, then click the "Public+Mail
Comments ('m')" link, then add reviewers/cc (for VAAPI-related stuff use posciak
or me as reviewer, and cc the other), then "Public All My Drafts" to send the
mail.  You don't hit the Commit checkbox until you have ell-gee-tee-em's from
all reviewers.  More info at
http://dev.chromium.org/developers/contributing-code#TOC-Request-review

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

https://codereview.chromium.org/15955009/diff/1/content/common/gpu/media/vaap...
content/common/gpu/media/vaapi_wrapper.cc:9: #include "base/message_loop.h"
This is a pretty unobvious #include for GetDefaultXDisplay.

https://codereview.chromium.org/15955009/diff/1/content/common/gpu/media/vaap...
content/common/gpu/media/vaapi_wrapper.cc:419: #define
VAAPI_DLSYM_OR_RETURN_ON_ERROR(name, handle)                          \
This doesn't need to be a macro (and doesn't need to be copy/pasta).

[Pawel Osciak, 2013-05-31 16:25:36]

On 2013/05/31 11:08:20, haltonhuo wrote:

I'm confused, what is this fixing? I'm not aware of any failures on
vaInitialize(). Is this in preparation for some changes to libva/intel-driver?

Adding Jorge for security implications.

[Pawel Osciak, 2013-05-31 16:26:24]

Please also always add me as reviewer on all VAAPI-related CLs.

[Jorge Lucangeli Obes, 2013-05-31 17:29:15]

On 2013/05/31 16:26:24, posciak wrote:
> Please also always add me as reviewer on all VAAPI-related CLs.

Please don't land this until we understand what's failing and what we need to
add to make it work.

There's two ways to pre-init stuff: PreSandbox here and
sandbox_seccomp_bpf_linux.cc::WarmupPolicy. I'd argue the right way to do this
is dlopen()'ing /usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so in WarmupPolicy.

[haltonhuo, 2013-06-03 02:05:17]

On 2013/05/31 17:29:15, Jorge Lucangeli Obes wrote:
> Please don't land this until we understand what's failing and what we need to
> add to make it work.
> 
> There's two ways to pre-init stuff: PreSandbox here and
> sandbox_seccomp_bpf_linux.cc::WarmupPolicy. I'd argue the right way to do this
> is dlopen()'ing /usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so in
WarmupPolicy.

Checking the code in
WarmupPolicy(https://code.google.com/p/chromium/codesearch#chromium/src/content/common/sandbox_seccomp_bpf_linux.cc&q=WarmupPolicy&sq=package:chromium&l=1732),
the hard code of va driver cause problems:
1. The driver location varies by different distros. For ubuntu 13.04 64-bit
instance, the driver located at /usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so
(this is how issue 244364 happens)
2. Only works for va intel backend. It actually work/will work on VDPAU(nVidia)
backend.

My idea is to let libva -- va_openDriver() control which driver need to be
dlopen. Two ways here:
1. call vaInitialize (as the patch set #1)
2. spawn vainfo and grep the result

If we want to code change in WarmupPolicy() instead, the code change must be
bigger than path set #1. What's your opinion?

[Pawel Osciak, 2013-06-03 16:09:10]

On 2013/06/03 02:05:17, haltonhuo wrote:
> On 2013/05/31 17:29:15, Jorge Lucangeli Obes wrote:
> > Please don't land this until we understand what's failing and what we need
to
> > add to make it work.
> > 
> > There's two ways to pre-init stuff: PreSandbox here and
> > sandbox_seccomp_bpf_linux.cc::WarmupPolicy. I'd argue the right way to do
this
> > is dlopen()'ing /usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so in
> WarmupPolicy.
> 
> Checking the code in
>
WarmupPolicy(https://code.google.com/p/chromium/codesearch#chromium/src/content/common/sandbox_seccomp_bpf_linux.cc&q=WarmupPolicy&sq=package:chromium&l=1732),
> the hard code of va driver cause problems:
> 1. The driver location varies by different distros. For ubuntu 13.04 64-bit
> instance, the driver located at
/usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so
> (this is how issue 244364 happens)
> 2. Only works for va intel backend. It actually work/will work on
VDPAU(nVidia)
> backend.
> 
> My idea is to let libva -- va_openDriver() control which driver need to be
> dlopen. Two ways here:
> 1. call vaInitialize (as the patch set #1)
> 2. spawn vainfo and grep the result
> 
> If we want to code change in WarmupPolicy() instead, the code change must be
> bigger than path set #1. What's your opinion?


We use vaapi only on CrOS and currently don't have plans to enable it on Linux,
so I think we are ok with hardcoding for now...

[Ami GONE FROM CHROMIUM, 2013-06-03 16:31:09]

On Mon, Jun 3, 2013 at 9:09 AM, <posciak@chromium.org> wrote:

> We use vaapi only on CrOS and currently don't have plans to enable it on
> Linux,
> so I think we are ok with hardcoding for now...
>

Pawel: you're not proposing landing an explicit
dlopen("/usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so") into
WarmupPolicy(), are you??

@halton.huo: can you work around your problem with an LD_PRELOAD?

The proposed solutions in this thread all have down-sides, but the problem
they are trying to solve is one we have explicitly decided not to take on:
VAVDA on linux is unsupported for a reason.  Given that developer
workarounds exist for this developer-only setup, why is it worthwhile to
take the {security and/or performance and/or readability and/or complexity}
hit that each of the proposed solutions require?

[Jorge Lucangeli Obes, 2013-06-03 16:41:23]

On 2013/06/03 16:31:09, Ami Fischman wrote:
> On Mon, Jun 3, 2013 at 9:09 AM, <mailto:posciak@chromium.org> wrote:
> 
> > We use vaapi only on CrOS and currently don't have plans to enable it on
> > Linux,
> > so I think we are ok with hardcoding for now...
> >
> 
> Pawel: you're not proposing landing an explicit
> dlopen("/usr/lib/x86_64-linux-gnu/dri/i965_drv_video.so") into
> WarmupPolicy(), are you??
> 
> @halton.huo: can you work around your problem with an LD_PRELOAD?
> 
> The proposed solutions in this thread all have down-sides, but the problem
> they are trying to solve is one we have explicitly decided not to take on:
> VAVDA on linux is unsupported for a reason.  Given that developer
> workarounds exist for this developer-only setup, why is it worthwhile to
> take the {security and/or performance and/or readability and/or complexity}
> hit that each of the proposed solutions require?

My understanding was that Pawel was explaining why the current hardcoding was
acceptable to us. Reading previous comments I agree that we probably don't want
to add paths that will be used for a feature we're not supporting.

[Ami GONE FROM CHROMIUM, 2013-06-03 16:42:37]

Makes sense.  (also, I hadn't seen that we hard-code the cros paths before)


On Mon, Jun 3, 2013 at 9:41 AM, <jorgelo@chromium.org> wrote:

> On 2013/06/03 16:31:09, Ami Fischman wrote:
>
>  On Mon, Jun 3, 2013 at 9:09 AM, <mailto:posciak@chromium.org> wrote:
>>
>
>  > We use vaapi only on CrOS and currently don't have plans to enable it on
>> > Linux,
>> > so I think we are ok with hardcoding for now...
>> >
>>
>
>  Pawel: you're not proposing landing an explicit
>> dlopen("/usr/lib/x86_64-linux-**gnu/dri/i965_drv_video.so") into
>> WarmupPolicy(), are you??
>>
>
>  @halton.huo: can you work around your problem with an LD_PRELOAD?
>>
>
>  The proposed solutions in this thread all have down-sides, but the problem
>> they are trying to solve is one we have explicitly decided not to take on:
>> VAVDA on linux is unsupported for a reason.  Given that developer
>> workarounds exist for this developer-only setup, why is it worthwhile to
>> take the {security and/or performance and/or readability and/or
>> complexity}
>> hit that each of the proposed solutions require?
>>
>
> My understanding was that Pawel was explaining why the current hardcoding
> was
> acceptable to us. Reading previous comments I agree that we probably don't
> want
> to add paths that will be used for a feature we're not supporting.
>
>
https://codereview.chromium.**org/15955009/<https://codereview.chromium.org/1...
>

[haltonhuo, 2013-06-04 01:15:19]

On 2013/06/03 16:09:10, posciak wrote:
> 
> We use vaapi only on CrOS and currently don't have plans to enable it on
Linux,
> so I think we are ok with hardcoding for now...

hi posciak and all,

I do have a followup patch to enable libva video decode on linux in general. It
works as I wished. To me at least, I do not see any difference and bad effect on
libva support for ChromeOS vs common linux(Ubuntu/Fedora/etc). It won't break
ChromeOS and will bring benefit to Linux, so why not do it?

[Ami GONE FROM CHROMIUM, 2013-06-04 02:21:56]

I decided not to make vavda/linux a supported configuration because the
distro/drivers/hardware fragmentation imposes a large support cost and the
percentage of affected users is tiny (even though I am one of those users).
Supporting windows, the only platform other than cros that has HW video
decode acceleration in Chrome, has turned out to be a significant burden
because of the aforementioned fragmentation, even when we restrict to only
win>=7.  I believe fragmentation is a much bigger problem in the linux
world than the windows world.
I don't think we can reverse position on this until there is a media/
OWNERS who wants to take this on, which currently there isn't.

(to perhaps state the obvious: getting it working on a handful of
up-to-date systems is almost trivial; keeping it running on every
configuration under the sun, or blacklisting outliers, will be a long-term
task someone needs to sign up for)

[haltonhuo, 2013-06-04 03:40:36]

On 2013/06/04 02:21:56, Ami Fischman wrote:
> I decided not to make vavda/linux a supported configuration because the
> distro/drivers/hardware fragmentation imposes a large support cost and the
> percentage of affected users is tiny (even though I am one of those users).
> Supporting windows, the only platform other than cros that has HW video
> decode acceleration in Chrome, has turned out to be a significant burden
> because of the aforementioned fragmentation, even when we restrict to only
> win>=7.  I believe fragmentation is a much bigger problem in the linux
> world than the windows world.
> I don't think we can reverse position on this until there is a media/
> OWNERS who wants to take this on, which currently there isn't.
> 
> (to perhaps state the obvious: getting it working on a handful of
> up-to-date systems is almost trivial; keeping it running on every
> configuration under the sun, or blacklisting outliers, will be a long-term
> task someone needs to sign up for)

It is a pity that linux users can not benefit the HW video decode ability with
little effort. The libva framework is little difference comapared with Chrome
OS.

I guess the meaning of "fragmentation" you mentioned is the path of driver. If
yes, I'd say let libva decide the driver path is better than hard coding.
When the hard code issue get fixed, the following path is just remove linux from
HW video decode blacklist.

If your concern is about the effort of incoming linux related bugs, I can keep
contribute my time on that. Although I'm not expert of libva :)

[Ami GONE FROM CHROMIUM, 2013-06-04 04:03:34]

>
> It is a pity that linux users can not benefit the HW video decode ability
> with
> little effort.


If it was "little effort" I'd agree.  It is manifestly a _lot_ of effort to
maintain HW decode on windows, where the API we use comes from a single OS
vendor.  Linux will be far more work because:


> The libva framework is little difference comapared with Chrome OS.
>

While that is true, the driver stack that libva relies on contains a large
amount of variance.
I'm talking about variance in actual hardware, kernel versions, graphics
driver provider & version, libva-adapter (e.g. vdpau-driver), libva
version, and X version.  The size of the cross-product here makes part-time
maintenance a non-starter.

I guess the meaning of "fragmentation" you mentioned is the path of driver.
> If
> yes, I'd say let libva decide the driver path is better than hard coding.
>

If the state of the world was such that most linux installs had a working
libva setup I might agree with you, because it would mean we could ignore
the rest of the stack.
I don't believe that's the case.  Do you?
(the actual path to the driver .so is the least of my worries - I am
talking about bugs at all layers of the stack)

If your concern is about the effort of incoming linux related bugs, I can
> keep
> contribute my time on that. Although I'm not expert of libva :)
>

Yeah, judging from our experience with windows & cros, it'll take someone
willing to dig into the API (vaapi) as well as the graphics stack(s), not
to mention chrome, to deal with the incoming bug flow.

[haltonhuo, 2013-06-04 06:56:32]

On 2013/06/04 04:03:34, Ami Fischman wrote:
> If it was "little effort" I'd agree.  It is manifestly a _lot_ of effort to
> maintain HW decode on windows, where the API we use comes from a single OS
> vendor.  Linux will be far more work because:

When i'm saying "little effort", I meant the effort that enable linux(at least
latest Ubuntu and Fedora). This won't bring any new code, just simply re-use
current implementation for Chrome OS.

> If the state of the world was such that most linux installs had a working
> libva setup I might agree with you, because it would mean we could ignore
> the rest of the stack.
> I don't believe that's the case.  Do you?
> (the actual path to the driver .so is the least of my worries - I am
> talking about bugs at all layers of the stack)

I can not ensure libva works on all linux distros, neither for everyone else. :)

Remember libva-x11 bug we discussed in issue 16042004
(https://bugs.launchpad.net/ubuntu/+source/libva/+bug/1185298), it is actually
distro issue, chrome code should not compromise the distro bug. In that case,
report to the distro, not to chrome. So we can do the same thing.

I compared libva on ChromeOS, Ubuntu 13.04 and Fedora18, the status is so far so
good.

Platform                                            | VAAPI version  | Have been
Tested
-------------------------------------------------------------------------------------
ChromeOS-Vanilla-4028.0.2013_04_20_1810-r706c4144   | 0.33.0         | Should
covered by Google ChromeOS team
Fedora 18 64bit                                     | 0.33.0         | No, but
driver location are same as chromOS, should work
Ubuntu 13.04 64bit                                  | 0.32.0         | Yes by
halton

To reduce the affect to ChromeOS and Windows, how about to do this for HW video
decode issues that reported to linux?
1. If a bug is caused by incompatible libva compared with ChromeOS's libva, then
just ignore it and suggest to report related distro instead. This might caused
by different version of libva and its below stack(kernel, graphic, drivers). 
2. Else it would be a common bug. Again, it might be inside chrome, or libva, or
drivers, or graphics. If it is inside chrome, fix it then chromeOS and linux
will both benefits. Otherwise fix it for ChromeOS, then goes to situation
#1(incompatible libva) for other linux.

There is one OPEN left is that how can user verify whether a bug happens on
ChromeOS. To me, I do not have ChromeOS device, I just install it in
VirtualBox/VMWare. I do not think video_decode will work in that environment, it
is in black list already.

[Ami GONE FROM CHROMIUM, 2013-06-04 19:33:11]

I believe this is a bad idea.

I think you are severly underestimating the maintenance cost associated
with debugging user bug reports on the wide variety of platforms out there,
and I don't know how to state it more clearly than I have.  Your #1/#2
recipe is predicated on knowing where a given bug lies; finding the answer
to that question is what consumes most bug-report-handling time.

If you disagree you are welcome to find another media/OWNERS who will sign
up for the work involved.  I don't believe any such person exists.

[haltonhuo, 2013-06-05 06:25:24]

On 2013/06/04 19:33:11, Ami Fischman wrote:
> I believe this is a bad idea.
> 
> I think you are severly underestimating the maintenance cost associated
> with debugging user bug reports on the wide variety of platforms out there,
> and I don't know how to state it more clearly than I have.  Your #1/#2
> recipe is predicated on knowing where a given bug lies; finding the answer
> to that question is what consumes most bug-report-handling time.
> 
> If you disagree you are welcome to find another media/OWNERS who will sign
> up for the work involved.  I don't believe any such person exists.

Thanks for your kind follow-up and explanation. No, I do not think I have
influence for any media/OWNERS? 
Please include me if more people care about linux HW video decode as me. So that
I could offer the patch.

[haltonhuo, 2013-06-05 06:27:16]

On 2013/06/05 06:25:24, haltonhuo wrote:
> Thanks for your kind follow-up and explanation. No, I do not think I have
> influence for any media/OWNERS? 
> Please include me if more people care about linux HW video decode as me. So
that
> I could offer the patch.

Ami, BTW, is that a bad idea I upload the second patch so that I can maintain on
codereview? If not, I'll do that.

[Ami GONE FROM CHROMIUM, 2013-06-05 06:29:33]

I suggest you star
https://code.google.com/p/chromium/issues/detail?id=137247 to stay updated
on changes in prioritization of this feature.  I just added a pointer to
this CL to that bug so it'll be available in future if you simply close the
rietveld issue without deleting it.
Uploading new patchsets to stay current is fine.  Please remove the
reviewers from the CL if you want to keep it open but don't expect it to
land anytime soon (that clears this CL off your reviewers' dashboards).

Cheers,
-a

[haltonhuo, 2013-06-05 07:54:59]

On 2013/06/05 06:29:33, Ami Fischman wrote:
> I suggest you star
> https://code.google.com/p/chromium/issues/detail?id=137247 to stay updated
> on changes in prioritization of this feature.  I just added a pointer to
> this CL to that bug so it'll be available in future if you simply close the
> rietveld issue without deleting it.
> Uploading new patchsets to stay current is fine.  Please remove the
> reviewers from the CL if you want to keep it open but don't expect it to
> land anytime soon (that clears this CL off your reviewers' dashboards).
> 
> Cheers,
> -a

The part II patch is https://codereview.chromium.org/16430003/, no request to
review.

[Ami GONE FROM CHROMIUM, 2013-10-31 21:28:51]

Removing myself as reviewer to remove this from my CL list.

[Jorge Lucangeli Obes, 2014-05-22 17:33:42]

On 2013/10/31 21:28:51, Ami Fischman wrote:
> Removing myself as reviewer to remove this from my CL list.

Removing myself as a reviewer. Not sure why this CL keeps getting updated.

[haltonhuo, 2014-05-23 02:03:44]

On 2014/05/22 17:33:42, Jorge Lucangeli Obes wrote:
> On 2013/10/31 21:28:51, Ami Fischman wrote:
> > Removing myself as reviewer to remove this from my CL list.
> 
> Removing myself as a reviewer. Not sure why this CL keeps getting updated.

We use this patch for our linux based projects, keep update to ensure others can
use this patch.