VAAPI Wrapper: refactor management of drm file

VAAPI Wrapper: refactor management of drm file

Make sure we use the same file in all instances of VaapiWrapper.

Define a refcounted global instance of VADisplayState that is only initialized
(via vaInitialize()) once and destroyed upon release of the last refcount.
This ensures a single drm_intel_bufmgr per fd, and avoids the double-close
problem as seen in bug 464628.

Pipe a PreSandboxInitialization to open the render node before starting sandbox.

After this CL the VEA/VDA unittests need to invoke the PreSandboxInitialization
as well.

BUG=475250
TEST=verify HW video works on link (check histograms) and VDA/VEA unittests

Committed: https://crrev.com/9583705f893683561b847feeaac48522f14dab34
Cr-Commit-Position: refs/heads/master@{#329410}

[hshi1, 2015-05-07 18:22:27]

hshi@chromium.org changed reviewers:
+ alexst@chromium.org, dnicoara@chromium.org, jorgelo@chromium.org,
piman@chromium.org

[hshi1, 2015-05-07 18:22:28]

PTAL this CL intends to eliminate the need to whitelist the render node in
GpuProcessPolicy by piping a PreSandboxInitialization.

piman@ (content)
jorgelo@ (sandbox)
dnicoara@ and alexst (FYI)

[Jorge Lucangeli Obes, 2015-05-07 20:02:23]

sandbox lgtm. would you need to test on other architectures? I don't remember
exactly what architectures we added |kDriRenderNode0Path| for.

[hshi1, 2015-05-07 20:06:29]

On 2015/05/07 20:02:23, Jorge Lucangeli Obes wrote:
> sandbox lgtm. would you need to test on other architectures? I don't remember
> exactly what architectures we added |kDriRenderNode0Path| for.

re: architectures: VAAPI is Intel-only (see content_common.gypi: vaapi_*.cc are
only included if 'target_arch != "arm" and chromeos == 1')

[dnicoara, 2015-05-07 21:17:10]

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

https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
content/common/gpu/media/vaapi_wrapper.cc:325: va_display_ =
vaGetDisplayDRM(drm_file_.Get().GetPlatformFile());
Previously, weren't we crashing because of double close from the VAAPI? Would
this cause a similar crash if you'd have multiple instances of VaapiWrapper?
You'll need to dup the FD in that case.

https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
content/common/gpu/media/vaapi_wrapper.h:317: static
base::LazyInstance<base::File> drm_file_;
You use base::ScopedFD rather than base::File for simplicity.

[hshi1, 2015-05-07 21:50:17]

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

https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
content/common/gpu/media/vaapi_wrapper.cc:325: va_display_ =
vaGetDisplayDRM(drm_file_.Get().GetPlatformFile());
On 2015/05/07 21:17:09, dnicoara wrote:
> Previously, weren't we crashing because of double close from the VAAPI? Would
> this cause a similar crash if you'd have multiple instances of VaapiWrapper?
> You'll need to dup the FD in that case.

The double close (bug 464628) was because we're using the same FD for the
primary graphics device (/dev/dri/card0) for libva and libdrm_intel. As long as
we use a separate FD for libva (shared by multiple instances of VaapiWrapper) we
would not hit the double close issue.

https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
content/common/gpu/media/vaapi_wrapper.h:317: static
base::LazyInstance<base::File> drm_file_;
On 2015/05/07 21:17:10, dnicoara wrote:
> You use base::ScopedFD rather than base::File for simplicity.

Done.

[dnicoara, 2015-05-07 22:04:27]

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

https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
content/common/gpu/media/vaapi_wrapper.cc:325: va_display_ =
vaGetDisplayDRM(drm_file_.Get().GetPlatformFile());
On 2015/05/07 21:50:17, hshi1 wrote:
> On 2015/05/07 21:17:09, dnicoara wrote:
> > Previously, weren't we crashing because of double close from the VAAPI?
Would
> > this cause a similar crash if you'd have multiple instances of VaapiWrapper?
> > You'll need to dup the FD in that case.
> 
> The double close (bug 464628) was because we're using the same FD for the
> primary graphics device (/dev/dri/card0) for libva and libdrm_intel. As long
as
> we use a separate FD for libva (shared by multiple instances of VaapiWrapper)
we
> would not hit the double close issue.

Are you sure VAApiWrapper is shared? If I open multiple web pages with HTML5
videos, I see libva initializing multiple times. I think it is getting in here
multiple times.

[hshi1, 2015-05-07 22:06:50]

On 2015/05/07 22:04:27, dnicoara wrote:
>
https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
> File content/common/gpu/media/vaapi_wrapper.cc (right):
> 
>
https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
> content/common/gpu/media/vaapi_wrapper.cc:325: va_display_ =
> vaGetDisplayDRM(drm_file_.Get().GetPlatformFile());
> On 2015/05/07 21:50:17, hshi1 wrote:
> > On 2015/05/07 21:17:09, dnicoara wrote:
> > > Previously, weren't we crashing because of double close from the VAAPI?
> Would
> > > this cause a similar crash if you'd have multiple instances of
VaapiWrapper?
> > > You'll need to dup the FD in that case.
> > 
> > The double close (bug 464628) was because we're using the same FD for the
> > primary graphics device (/dev/dri/card0) for libva and libdrm_intel. As long
> as
> > we use a separate FD for libva (shared by multiple instances of
VaapiWrapper)
> we
> > would not hit the double close issue.
> 
> Are you sure VAApiWrapper is shared? If I open multiple web pages with HTML5
> videos, I see libva initializing multiple times. I think it is getting in here
> multiple times.

I didn't mean VAAPIWrapper is shared. What I said was that we only need one FD
for libva (shared among multiple instances of VAAPIWrappers).

When I open multiple web pages with HTML5 videos I can confirm that there are
multiple VAAPIWrapper instances, but we only need one FD because internally
libva takes care of the lifetime.

[piman, 2015-05-07 22:07:26]

On Thu, May 7, 2015 at 3:04 PM, <dnicoara@chromium.org> wrote:

>
>
>
https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
> File content/common/gpu/media/vaapi_wrapper.cc (right):
>
>
>
https://codereview.chromium.org/1137483002/diff/1/content/common/gpu/media/va...
> content/common/gpu/media/vaapi_wrapper.cc:325: va_display_ =
> vaGetDisplayDRM(drm_file_.Get().GetPlatformFile());
> On 2015/05/07 21:50:17, hshi1 wrote:
>
>> On 2015/05/07 21:17:09, dnicoara wrote:
>> > Previously, weren't we crashing because of double close from the
>>
> VAAPI? Would
>
>> > this cause a similar crash if you'd have multiple instances of
>>
> VaapiWrapper?
>
>> > You'll need to dup the FD in that case.
>>
>
>  The double close (bug 464628) was because we're using the same FD for
>>
> the
>
>> primary graphics device (/dev/dri/card0) for libva and libdrm_intel.
>>
> As long as
>
>> we use a separate FD for libva (shared by multiple instances of
>>
> VaapiWrapper) we
>
>> would not hit the double close issue.
>>
>
> Are you sure VAApiWrapper is shared? If I open multiple web pages with
> HTML5 videos, I see libva initializing multiple times. I think it is
> getting in here multiple times.
>

Unless something has changed, we use one VAAPIWrapper per decoder (i.e. per
video tag).

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

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

[hshi1, 2015-05-07 23:10:52]

On 2015/05/07 22:07:26, piman (Very slow to review) wrote:
> Unless something has changed, we use one VAAPIWrapper per decoder (i.e. per
> video tag).

That we have multiple VAAPIWrapper instances initialized using the same fd isn't
going to cause the double-close problem, unless we have one instance of
VAAPIWrapper exporting dmabuf to another instance of VAAPIWrapper.

Per piman@ the solution would be to have a global refcounted instance of
VADisplay that is only initialized (vaInitialize()) once and destroyed upon
release of the last ref.

[hshi1, 2015-05-08 00:40:38]

To answer dnicoara@'s earlier question: it makes no difference whether we dupe
or re-use the fd's for vaInitalize() as drm DRM_IOCTL_PRIME_FD_TO_HANDLE does
not literally compare numerical value of fd, but rather does a lookup for the
gem object using the kernel's internal struct corresponding to the drm file
(shared between all dup'ed fds).

I'd argue that the current CL would not have a double-free problem given the
current implementation of chrome and libva, and as such it is sufficient.

[Pawel Osciak, 2015-05-08 01:17:21]

Did you have a chance to verify HW acceleration still works with this and that
video tests still work? After applying this CL,
video_decode_accelerator_unittest fails to open the device on CrOS 7024.0.0 and
ToT Chrome, the error seems to be:

[0507/181156:VERBOSE2:drm_device.cc(296)] Cannot query for resources for
'/dev/dri/card1'

Look like CanQueryForResources() is failing? Perhaps I'm missing some CrOS-side
changes?

[hshi1, 2015-05-08 01:19:00]

On 2015/05/08 01:17:21, Pawel Osciak wrote:
> Did you have a chance to verify HW acceleration still works with this and that
> video tests still work? After applying this CL,
> video_decode_accelerator_unittest fails to open the device on CrOS 7024.0.0
and
> ToT Chrome, the error seems to be:
> 
> [0507/181156:VERBOSE2:drm_device.cc(296)] Cannot query for resources for
> '/dev/dri/card1'
> 
> Look like CanQueryForResources() is failing? Perhaps I'm missing some
CrOS-side
> changes?

Can you please try TOT CrOS? The card1 error seems to be vgem related.

[Pawel Osciak, 2015-05-08 01:32:46]

On 2015/05/08 01:19:00, hshi1 wrote:
> On 2015/05/08 01:17:21, Pawel Osciak wrote:
> > Did you have a chance to verify HW acceleration still works with this and
that
> > video tests still work? After applying this CL,
> > video_decode_accelerator_unittest fails to open the device on CrOS 7024.0.0
> and
> > ToT Chrome, the error seems to be:
> > 
> > [0507/181156:VERBOSE2:drm_device.cc(296)] Cannot query for resources for
> > '/dev/dri/card1'
> > 
> > Look like CanQueryForResources() is failing? Perhaps I'm missing some
> CrOS-side
> > changes?
> 
> Can you please try TOT CrOS? The card1 error seems to be vgem related.

Tried on ToT (7045.0.0). Unfortunately still same error.

[piman, 2015-05-08 01:53:04]

On 2015/05/08 00:40:38, hshi1 wrote:
> To answer dnicoara@'s earlier question: it makes no difference whether we dupe
> or re-use the fd's for vaInitalize() as drm DRM_IOCTL_PRIME_FD_TO_HANDLE does
> not literally compare numerical value of fd, but rather does a lookup for the
> gem object using the kernel's internal struct corresponding to the drm file
> (shared between all dup'ed fds).
> 
> I'd argue that the current CL would not have a double-free problem given the
> current implementation of chrome and libva, and as such it is sufficient.

I'm uncomfortable with that statement. There really is supposed to be a single
drm_intel_bufmgr per fd. PRIME import/export is one of the problems, but not the
only one. E.g. shared GPU VA space, fence registers etc.
I also believe a malicious client might be able to provoke sharing between 2
vaapi instance (e.g. create 2 decoder and send same textures to both).

[dnicoara, 2015-05-08 14:16:29]

On 2015/05/08 01:32:46, Pawel Osciak wrote:
> On 2015/05/08 01:19:00, hshi1 wrote:
> > On 2015/05/08 01:17:21, Pawel Osciak wrote:
> > > Did you have a chance to verify HW acceleration still works with this and
> that
> > > video tests still work? After applying this CL,
> > > video_decode_accelerator_unittest fails to open the device on CrOS
7024.0.0
> > and
> > > ToT Chrome, the error seems to be:
> > > 
> > > [0507/181156:VERBOSE2:drm_device.cc(296)] Cannot query for resources for
> > > '/dev/dri/card1'
> > > 
> > > Look like CanQueryForResources() is failing? Perhaps I'm missing some
> > CrOS-side
> > > changes?
> > 
> > Can you please try TOT CrOS? The card1 error seems to be vgem related.
> 
> Tried on ToT (7045.0.0). Unfortunately still same error.

That specific message is not an error, and it is related to the VGEM device.

hshi@, you'll need to update the unittests since you'll need to call
VaapiWrapper::PreSandboxInitialization(). The tests are failing since the render
node isn't opened.

[dnicoara, 2015-05-08 14:20:01]

On 2015/05/08 00:40:38, hshi1 wrote:
> To answer dnicoara@'s earlier question: it makes no difference whether we dupe
> or re-use the fd's for vaInitalize() as drm DRM_IOCTL_PRIME_FD_TO_HANDLE does
> not literally compare numerical value of fd, but rather does a lookup for the
> gem object using the kernel's internal struct corresponding to the drm file
> (shared between all dup'ed fds).
> 
> I'd argue that the current CL would not have a double-free problem given the
> current implementation of chrome and libva, and as such it is sufficient.

Ah, I had assumed that libva takes ownership of the FD, but it doesn't sound
like that's happening.

[hshi1, 2015-05-08 23:36:55]

PTAL Patch Set #4.

This defines a global refcounted VADisplayState instance that ensures VADisplay
is only initialized once (hence a single intel buf mgr per fd).

[hshi1, 2015-05-09 01:40:22]

On 2015/05/08 23:36:55, hshi1 wrote:
> PTAL Patch Set #4.
> 
> This defines a global refcounted VADisplayState instance that ensures
VADisplay
> is only initialized once (hence a single intel buf mgr per fd).

Actually never minde Patch Set #4, please go straight to Patch Set #5.

[Pawel Osciak, 2015-05-11 05:51:44]

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

[Pawel Osciak, 2015-05-11 05:51:45]

https://codereview.chromium.org/1137483002/diff/70001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/1137483002/diff/70001/content/common/gpu/medi...
content/common/gpu/media/vaapi_wrapper.h:217: // |va_lock_| must be held on
entry.
va_lock_ is not a member of this class and is per-VaapiWrapper instance, while
this can be global, so this would not work as expected, unless I'm missing
something?

Separately, I'm wondering if we'll be ok not having all VaapiWrapper instances
on one lock if we are sharing one fd and one vaInitialized libva instance?

Also, would it perhaps be possible to make this : public RefCounted instead of
manual refcounting?

[hshi1, 2015-05-11 17:14:39]

https://codereview.chromium.org/1137483002/diff/70001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/1137483002/diff/70001/content/common/gpu/medi...
content/common/gpu/media/vaapi_wrapper.h:217: // |va_lock_| must be held on
entry.
On 2015/05/11 05:51:45, Pawel Osciak wrote:
> va_lock_ is not a member of this class and is per-VaapiWrapper instance, while
> this can be global, so this would not work as expected, unless I'm missing
> something?
> 
> Separately, I'm wondering if we'll be ok not having all VaapiWrapper instances
> on one lock if we are sharing one fd and one vaInitialized libva instance?

I have moved va_lock_ into VADisplayState, which will be a global singleton
instance for USE_OZONE. This has passed all unit tests and works on chrome.

> 
> Also, would it perhaps be possible to make this : public RefCounted instead of
> manual refcounting?

Unfortunately I have thought about that but it wouldn't work because
base::RefCounted would delete the instance when refcount hits zero. The
VADisplayState would still need to be kept alive to hold the |drm_fd_|, yet it
needs to free the VADisplay.

[piman, 2015-05-11 21:54:07]

https://codereview.chromium.org/1137483002/diff/90001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/1137483002/diff/90001/content/common/gpu/medi...
content/common/gpu/media/vaapi_wrapper.cc:1159: drm_fd_.reset(dup(fd));
nit: dup needs to be wrapped into a HANDLE_EINTR macro.

Alternatively, you could have the VADisplayState own the base::File and avoid
the syscall altogether.

https://codereview.chromium.org/1137483002/diff/90001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/1137483002/diff/90001/content/common/gpu/medi...
content/common/gpu/media/vaapi_wrapper.h:327: scoped_ptr<VADisplayState>
va_display_state_;
Why does it have to be different between X11 and OZONE? Can't we have a
singleton VADisplayState on X11? The Display* would play the role of the drm fd.

[hshi1, 2015-05-11 22:26:40]

https://codereview.chromium.org/1137483002/diff/90001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/1137483002/diff/90001/content/common/gpu/medi...
content/common/gpu/media/vaapi_wrapper.cc:1159: drm_fd_.reset(dup(fd));
On 2015/05/11 21:54:07, piman (Very slow to review) wrote:
> nit: dup needs to be wrapped into a HANDLE_EINTR macro.
> 
> Alternatively, you could have the VADisplayState own the base::File and avoid
> the syscall altogether.

Done.

https://codereview.chromium.org/1137483002/diff/90001/content/common/gpu/medi...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/1137483002/diff/90001/content/common/gpu/medi...
content/common/gpu/media/vaapi_wrapper.h:327: scoped_ptr<VADisplayState>
va_display_state_;
On 2015/05/11 21:54:07, piman (Very slow to review) wrote:
> Why does it have to be different between X11 and OZONE? Can't we have a
> singleton VADisplayState on X11? The Display* would play the role of the drm
fd.

Done.

[piman, 2015-05-11 23:22:08]

lgtm

[hshi1, 2015-05-11 23:33:02]

https://codereview.chromium.org/1137483002/diff/110001/content/common/gpu/med...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/1137483002/diff/110001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.h:222: VADisplay GetVADisplay() const {
return va_display_; }
note: minor naming change to accessor: GetVADisplay() -> va_display(), to match
va_lock().

[hshi1, 2015-05-11 23:33:28]

The CQ bit was checked by hshi@chromium.org

[hshi1, 2015-05-11 23:33:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
jorgelo@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1137483002/#ps130001
(title: "minor naming change to accessor")

[commit-bot: I haz the power, 2015-05-11 23:38:59]

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

[hshi1, 2015-05-11 23:49:30]

The CQ bit was unchecked by hshi@chromium.org

[hshi1, 2015-05-11 23:49:52]

On 2015/05/11 23:38:59, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1137483002/130001

unclick CQ bit awaiting posciak's review

[Pawel Osciak, 2015-05-12 01:39:39]

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:328: VA_LOG_ON_ERROR(va_res,
"vaInitialize failed");
Please move this to Initialize() and make it bool (void)

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:491: VA_LOG_ON_ERROR(va_res,
"vaTerminate failed");
Please move this to VADisplayState::Deinitialize() and make it void(void).

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:1034:
va_display_state_.Get().SetDrmFd(drm_file.GetPlatformFile());
if (drm_file.IsValid())
  ...

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:1094: if (refcount_++ == 0) {
va_lock_.AssertAcquired();

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:1114: if (VAAPIVersionLessThan(0, 34))
{
This can be under the if as well.

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:1122: if (--refcount_ > 0)
va_lock_.AssertAcquired();

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:1133: va_initialized_ = false;
va_display_ = nullptr;

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.h:234: int refcount_;
Please add a comment that this is protected by va_lock_.

[hshi1, 2015-05-12 01:58:10]

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:328: VA_LOG_ON_ERROR(va_res,
"vaInitialize failed");
On 2015/05/12 01:39:39, Pawel Osciak wrote:
> Please move this to Initialize() and make it bool (void)

I'm afraid I can't move it there because |report_error_to_uma_cb_| is a member
of VaapiWrapper.

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:491: VA_LOG_ON_ERROR(va_res,
"vaTerminate failed");
On 2015/05/12 01:39:39, Pawel Osciak wrote:
> Please move this to VADisplayState::Deinitialize() and make it void(void).

Same issue: I'm afraid I can't move it there because |report_error_to_uma_cb_|
is a member of VaapiWrapper.

[hshi1, 2015-05-12 02:05:26]

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
File content/common/gpu/media/vaapi_wrapper.cc (right):

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:1034:
va_display_state_.Get().SetDrmFd(drm_file.GetPlatformFile());
On 2015/05/12 01:39:39, Pawel Osciak wrote:
> if (drm_file.IsValid())
>   ...

Done.

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:1094: if (refcount_++ == 0) {
On 2015/05/12 01:39:39, Pawel Osciak wrote:
> va_lock_.AssertAcquired();

Done.

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:1114: if (VAAPIVersionLessThan(0, 34))
{
On 2015/05/12 01:39:39, Pawel Osciak wrote:
> This can be under the if as well.

Well technically we could succeed to vaInitialize on the first instance but get
a version less than 0.34. Then if an Initialize() call comes in immediately for
a second instance, we could bypass the vaInitialize block above, so we do need
the VAAPIVersionLessThan() check outside the if (refcount_++ == 0) block.

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:1122: if (--refcount_ > 0)
On 2015/05/12 01:39:39, Pawel Osciak wrote:
> va_lock_.AssertAcquired();

Done.

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.cc:1133: va_initialized_ = false;
On 2015/05/12 01:39:39, Pawel Osciak wrote:
> va_display_ = nullptr;

Done.

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
File content/common/gpu/media/vaapi_wrapper.h (right):

https://codereview.chromium.org/1137483002/diff/130001/content/common/gpu/med...
content/common/gpu/media/vaapi_wrapper.h:234: int refcount_;
On 2015/05/12 01:39:39, Pawel Osciak wrote:
> Please add a comment that this is protected by va_lock_.

Done.

[hshi1, 2015-05-12 15:35:48]

The CQ bit was checked by hshi@chromium.org

[hshi1, 2015-05-12 15:35:48]

The patchset sent to the CQ was uploaded after l-g-t-m from
jorgelo@chromium.org, piman@chromium.org
Link to the patchset: https://codereview.chromium.org/1137483002/#ps130002
(title: "Posciak's comments.")

[commit-bot: I haz the power, 2015-05-12 15:35:59]

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

[commit-bot: I haz the power, 2015-05-12 15:40:01]

Committed patchset #9 (id:130002)

[commit-bot: I haz the power, 2015-05-12 15:40:53]

Patchset 9 (id:??) landed as
https://crrev.com/9583705f893683561b847feeaac48522f14dab34
Cr-Commit-Position: refs/heads/master@{#329410}