VaapiVideoAccelerator: make Vaapi accelerator work with ozone

content/common/gpu/media/vaapi_picture_provider.cc

Index: content/common/gpu/media/vaapi_picture_provider.cc
diff --git a/content/common/gpu/media/vaapi_picture_provider.cc b/content/common/gpu/media/vaapi_picture_provider.cc
new file mode 100644
index 0000000000000000000000000000000000000000..dc3413080c79c893c9a5f4cf1b61b23d23e86995
--- /dev/null
+++ b/content/common/gpu/media/vaapi_picture_provider.cc
@@ -0,0 +1,588 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "content/common/gpu/media/vaapi_picture_provider.h"
+
+#include "base/bind.h"
+#include "base/callback.h"
+#include "ui/gl/gl_bindings.h"
+#include "ui/gl/gl_image.h"
+#if defined(USE_X11)
+#include "third_party/libva/va/va_x11.h"
+#include "ui/gl/gl_context_glx.h"
+#else
+#include "third_party/libva/va/drm/va_drm.h"
+#include "third_party/libva/va/va_drmcommon.h"
+#include "ui/gl/gl_image_egl.h"
+#include "ui/ozone/public/native_pixmap.h"
+#include "ui/ozone/public/ozone_platform.h"
+#include "ui/ozone/public/surface_factory_ozone.h"
+#include <va/va_vpp.h>
+#endif  // USE_X11
+#include "ui/gl/scoped_binders.h"
+
+#define LOG_VA_ERROR_AND_RETURN(input, err_msg, output_code)    \
+  do {                                                          \
+    VAStatus va_status = input;                                 \

[Pawel Osciak, 2014/09/24 11:27:11]

I don't think we need this intermediate.

[llandwerlin-old, 2014/09/25 09:54:18]

I will need it if I'm using vaErrorStr().

+    if (va_status != VA_STATUS_SUCCESS) {                       \
+      DVLOG(1) << err_msg << " : " << va_status;                \

[Pawel Osciak, 2014/09/24 11:27:11]

Could we use vaErrorStr()?

[llandwerlin-old, 2014/09/25 09:54:17]

Acknowledged.

+      output_code;                                              \

[Pawel Osciak, 2014/09/24 11:27:11]

Please don't do it like this. Please move and destruction required to the
destructor.

[llandwerlin-old, 2014/09/25 09:54:18]

Acknowledged.

+      return false;                                             \
+    }                                                           \
+  } while (0)
+
+namespace content {
+
+namespace {

[Pawel Osciak, 2014/09/24 11:27:12]

Do we need this namespace?

[llandwerlin-old, 2014/09/25 09:54:18]

Acknowledged.

[llandwerlin-old, 2014/09/25 09:54:18]

I'm just trying to remove stuff from the content namespace since it's
implementation details of the VaapiPictureProvider.
I will remove.

+
+#if defined(USE_X11)
+
+class TFPPicture;
+
+class X11VaapiPictureProvider : public VaapiPictureProvider {

[Pawel Osciak, 2014/09/24 11:27:10]

Could we have X11* and Drm* classes in separate files please?

[llandwerlin-old, 2014/09/25 09:54:17]

Acknowledged.

+ public:
+  X11VaapiPictureProvider(
+      VADisplay va_display,
+      gfx::GLContextGLX* glx_context,
+      const base::Callback<bool(void)> make_context_current);
+  virtual ~X11VaapiPictureProvider();
+
+  virtual scoped_ptr<VaapiPictureProvider::Picture> CreatePicture(
+      int32 picture_buffer_id,
+      uint32 texture_id,
+      const gfx::Size& size) OVERRIDE;
+
+  void DestroyPicture(TFPPicture* tfp_picture);
+
+  virtual bool PutSurfaceIntoPicture(
+      VASurfaceID va_surface_id,
+      VaapiPictureProvider::Picture* picture) OVERRIDE;
+
+  virtual bool Initialize() OVERRIDE;
+
+ private:
+

[Pawel Osciak, 2014/09/24 11:27:11]

Please no whitespace after public, private, etc.

[llandwerlin-old, 2014/09/25 09:54:17]

Acknowledged.

+  bool BindPicture(TFPPicture* picture);
+
+  gfx::GLContextGLX* glx_context_;
+  base::Callback<bool(void)> make_context_current_;
+
+  Display* x_display_;
+  GLXFBConfig fb_config_;
+
+  VADisplay va_display_;
+};
+
+class TFPPicture : public VaapiPictureProvider::Picture {
+ public:
+  TFPPicture(X11VaapiPictureProvider* provider,
+             int32 picture_buffer_id,
+             uint32 texture_id,
+             const gfx::Size& size,
+             Pixmap x_pixmap,
+             GLXPixmap glx_pixmap)
+      : Picture(picture_buffer_id, texture_id, size),
+        provider_(provider),
+        x_pixmap_(x_pixmap),
+        glx_pixmap_(glx_pixmap) {}
+  virtual ~TFPPicture() { provider_->DestroyPicture(this); }

[Pawel Osciak, 2014/09/24 11:27:11]

Could we have this class destroy itself (passing x_display and mcc() instead of
provider) instead?

[llandwerlin-old, 2014/10/08 09:31:17]

Acknowledged.

+
+  Pixmap x_pixmap() const { return x_pixmap_; }
+  GLXPixmap glx_pixmap() const { return glx_pixmap_; }
+
+ private:
+  X11VaapiPictureProvider* provider_;
+  Pixmap x_pixmap_;
+  GLXPixmap glx_pixmap_;
+};
+
+class XFreeDeleter {
+ public:
+  void operator()(void* x) const { ::XFree(x); }
+};
+
+X11VaapiPictureProvider::X11VaapiPictureProvider(
+    VADisplay va_display,
+    gfx::GLContextGLX* glx_context,
+    const base::Callback<bool(void)> make_context_current)
+    : glx_context_(glx_context),
+      make_context_current_(make_context_current),
+      x_display_(glx_context_->display()),
+      va_display_(va_display) {
+}
+
+X11VaapiPictureProvider::~X11VaapiPictureProvider() {
+}
+
+scoped_ptr<VaapiPictureProvider::Picture>
+X11VaapiPictureProvider::CreatePicture(int32 picture_buffer_id,
+                                       uint32 texture_id,
+                                       const gfx::Size& size) {
+  scoped_ptr<VaapiPictureProvider::Picture> picture;
+
+  if (!make_context_current_.Run())
+    return picture.Pass();
+
+  XWindowAttributes win_attr;

[Pawel Osciak, 2014/09/24 11:27:12]

Please move this code back to TFPPicture::Initialize().

+  int screen = DefaultScreen(x_display_);
+  XGetWindowAttributes(x_display_, RootWindow(x_display_, screen), &win_attr);
+  // TODO(posciak): pass the depth required by libva, not the RootWindow's
+  // depth
+  Pixmap x_pixmap = XCreatePixmap(x_display_,
+                                  RootWindow(x_display_, screen),
+                                  size.width(),
+                                  size.height(),
+                                  win_attr.depth);
+  if (!x_pixmap) {
+    DVLOG(1) << "Failed creating an X Pixmap for TFP";
+    return picture.Pass();
+  }
+
+  static const int pixmap_attr[] = {
+    GLX_TEXTURE_TARGET_EXT,     GLX_TEXTURE_2D_EXT, GLX_TEXTURE_FORMAT_EXT,
+    GLX_TEXTURE_FORMAT_RGB_EXT, GL_NONE,
+  };
+
+  GLXPixmap glx_pixmap =
+    glXCreatePixmap(x_display_, fb_config_, x_pixmap, pixmap_attr);
+  if (!glx_pixmap) {
+    // x_pixmap_ will be freed in the destructor.
+    DVLOG(1) << "Failed creating a GLX Pixmap for TFP";
+    XFreePixmap(x_display_, x_pixmap);
+    return picture.Pass();
+  }
+
+  picture.reset(new TFPPicture(this,
+                               picture_buffer_id,
+                               texture_id,
+                               size,
+                               x_pixmap,
+                               glx_pixmap));
+
+  return picture.Pass();
+}
+
+void X11VaapiPictureProvider::DestroyPicture(TFPPicture* tfp_picture) {
+  // Unbind surface from texture and deallocate resources.
+  if (tfp_picture->glx_pixmap() && make_context_current_.Run()) {
+    glXReleaseTexImageEXT(x_display_,
+                          tfp_picture->glx_pixmap(),
+                          GLX_FRONT_LEFT_EXT);
+    glXDestroyPixmap(x_display_, tfp_picture->glx_pixmap());
+  }
+
+  if (tfp_picture->x_pixmap())
+    XFreePixmap(x_display_, tfp_picture->x_pixmap());
+  XSync(x_display_, False);  // Needed to work around buggy vdpau-driver.
+}
+
+bool X11VaapiPictureProvider::PutSurfaceIntoPicture(

[Pawel Osciak, 2014/09/24 11:27:11]

Please make it a virtual method in Picture and not cast to child.

[llandwerlin-old, 2014/09/25 09:54:17]

Acknowledged.

+    VASurfaceID va_surface_id,
+    VaapiPictureProvider::Picture* picture) {
+  TFPPicture* tfp_picture = static_cast<TFPPicture*>(picture);
+  const gfx::Size& size = tfp_picture->size();
+
+  if (!BindPicture(tfp_picture))
+    return false;
+
+  LOG_VA_ERROR_AND_RETURN(vaPutSurface(va_display_,
+                                       va_surface_id,
+                                       tfp_picture->x_pixmap(),
+                                       0, 0,
+                                       size.width(),
+                                       size.height(),
+                                       0, 0,
+                                       size.width(),
+                                       size.height(),
+                                       NULL,
+                                       0, 0),
+                          "Couldn't put surface into picture",);
+  return true;
+}
+
+bool X11VaapiPictureProvider::Initialize(){
+  if (!make_context_current_.Run()) {
+    DVLOG(1) << "Couldn't make context current";
+    return false;
+  }
+
+  const int fbconfig_attr[] = {
+    GLX_DRAWABLE_TYPE,               GLX_PIXMAP_BIT,
+    GLX_BIND_TO_TEXTURE_TARGETS_EXT, GLX_TEXTURE_2D_BIT_EXT,
+    GLX_BIND_TO_TEXTURE_RGB_EXT,     GL_TRUE,
+    GLX_Y_INVERTED_EXT,              GL_TRUE,
+    GL_NONE,
+  };
+
+  int num_fbconfigs;
+  scoped_ptr<GLXFBConfig, XFreeDeleter> glx_fb_configs(glXChooseFBConfig(
+      x_display_, DefaultScreen(x_display_), fbconfig_attr, &num_fbconfigs));
+
+  if (!glx_fb_configs) {
+    DVLOG(1) << "Couldn't get glx configs";
+    return false;
+  }
+  if (!num_fbconfigs) {
+    DVLOG(1) << "Couldn't get at least a glx config";
+    return false;
+  }
+
+  fb_config_ = glx_fb_configs.get()[0];
+  return true;
+}
+
+bool X11VaapiPictureProvider::BindPicture(TFPPicture* tfp_picture) {

[Pawel Osciak, 2014/09/24 11:27:10]

Also could be moved back to TFPPicture.

The provider should be doing what VAVDA used to, i.e. Initialize() the fb
configs, but not more. Rest is TFPPicture-specific.

[llandwerlin-old, 2014/09/25 09:54:18]

Acknowledged.

+  if (!make_context_current_.Run()) {
+    DVLOG(1) << "Failed making gl context current";
+    return false;
+  }
+
+  gfx::ScopedTextureBinder texture_binder(GL_TEXTURE_2D,
+                                          tfp_picture->texture_id());
+  glXBindTexImageEXT(x_display_, tfp_picture->glx_pixmap(),
+                     GLX_FRONT_LEFT_EXT, NULL);
+
+  return true;
+}
+
+#else
+
+class DrmPicture;
+
+class DrmVaapiPictureProvider : public VaapiPictureProvider {
+ public:
+  DrmVaapiPictureProvider(
+      VADisplay va_display,
+      const base::Callback<bool(void)> make_context_current);
+  virtual ~DrmVaapiPictureProvider();
+
+  virtual scoped_ptr<VaapiPictureProvider::Picture> CreatePicture(
+      int32 picture_buffer_id,
+      uint32 texture_id,
+      const gfx::Size& size) OVERRIDE;
+
+  void DestroyPicture(DrmPicture* drm_picture);
+
+  virtual bool PutSurfaceIntoPicture(
+      VASurfaceID va_surface_id,
+      VaapiPictureProvider::Picture* picture) OVERRIDE;
+
+  virtual bool SetCodedSurfacesSize(const gfx::Size& size) OVERRIDE;
+
+  virtual bool Initialize() OVERRIDE;
+
+ private:
+  bool InitializeVpp(const gfx::Size& size);

[Pawel Osciak, 2014/09/24 11:27:11]

Please document this and other classes.

[llandwerlin-old, 2014/09/25 09:54:17]

Acknowledged.

+
+  bool IsVppInitialized();

[Pawel Osciak, 2014/09/24 11:27:11]

I don't think you will need this if you only DeinitializeVpp() in destructor and
on size change?

[llandwerlin-old, 2014/09/25 09:54:18]

Acknowledged.

+
+  void DeinitializeVpp();
+
+  base::Callback<bool(void)> make_context_current_;
+
+  VADisplay va_display_;
+
+  VAConfigID vpp_config_;
+  VAContextID vpp_context_;
+  VABufferID vpp_buffer_;
+
+  gfx::Size coded_picture_size_;
+};
+
+class DrmPicture : public VaapiPictureProvider::Picture {
+ public:
+  DrmPicture(DrmVaapiPictureProvider* provider,
+             int32 picture_buffer_id,
+             uint32 texture_id,
+             const gfx::Size& size,
+             VASurfaceID va_surface,
+             scoped_refptr<ui::NativePixmap> pixmap,
+             scoped_refptr<gfx::GLImage> gl_image)
+      : Picture(picture_buffer_id, texture_id, size),
+        provider_(provider),
+        va_surface_(va_surface),
+        pixmap_(pixmap),
+        gl_image_(gl_image) {}
+  virtual ~DrmPicture() {
+    provider_->DestroyPicture(this);
+  }
+
+  scoped_refptr<gfx::GLImage> gl_image() const { return gl_image_; }
+  VASurfaceID va_surface() const { return va_surface_; }
+
+ private:
+  DrmVaapiPictureProvider* provider_;
+  VASurfaceID va_surface_;
+  scoped_refptr<ui::NativePixmap> pixmap_;
+  scoped_refptr<gfx::GLImage> gl_image_;
+};
+
+DrmVaapiPictureProvider::DrmVaapiPictureProvider(
+    VADisplay va_display,
+    const base::Callback<bool(void)> make_context_current)
+    : make_context_current_(make_context_current),
+      va_display_(va_display),
+      vpp_config_(VA_INVALID_ID),
+      vpp_context_(VA_INVALID_ID),
+      vpp_buffer_(VA_INVALID_ID) {
+}
+
+DrmVaapiPictureProvider::~DrmVaapiPictureProvider() {
+  DeinitializeVpp();
+}
+
+scoped_ptr<VaapiPictureProvider::Picture>
+DrmVaapiPictureProvider::CreatePicture(int32 picture_buffer_id,
+                                       uint32 texture_id,
+                                       const gfx::Size& size) {
+  VASurfaceAttrib va_attribs[2];
+  VASurfaceAttribExternalBuffers va_attrib_extbuf;
+
+  ui::OzonePlatform* platform = ui::OzonePlatform::GetInstance();
+  ui::SurfaceFactoryOzone* factory = platform->GetSurfaceFactoryOzone();
+
+  scoped_refptr<ui::NativePixmap> pixmap =
+    factory->CreateNativePixmap(size, ui::SurfaceFactoryOzone::RGBA_8888);

[jiajia.qin, 2014/09/17 10:45:24]

Wayland seems not support nativepixmap. If you use vaImage, it will avoid to use
pixmap. That can let this provider becomes common for ozone-*, such as
ozone-gbm, ozone-wayland.

+  unsigned long buffer_fd = pixmap->GetDmaBufFd();
+  VASurfaceID va_surface;
+
+  va_attrib_extbuf.pixel_format = VA_FOURCC_BGRX;
+  va_attrib_extbuf.width = size.width();
+  va_attrib_extbuf.height = size.height();
+  va_attrib_extbuf.data_size = size.height() * pixmap->GetStride();
+  va_attrib_extbuf.num_planes = 1;
+  va_attrib_extbuf.pitches[0] = pixmap->GetStride();
+  va_attrib_extbuf.offsets[0] = 0;
+  va_attrib_extbuf.buffers = &buffer_fd;
+  va_attrib_extbuf.num_buffers = 1;
+  va_attrib_extbuf.flags = 0;
+  va_attrib_extbuf.private_data = NULL;
+
+  va_attribs[0].type = VASurfaceAttribMemoryType;
+  va_attribs[0].flags = VA_SURFACE_ATTRIB_SETTABLE;
+  va_attribs[0].value.type = VAGenericValueTypeInteger;
+  va_attribs[0].value.value.i = VA_SURFACE_ATTRIB_MEM_TYPE_DRM_PRIME;
+
+  va_attribs[1].type = VASurfaceAttribExternalBufferDescriptor;
+  va_attribs[1].flags = VA_SURFACE_ATTRIB_SETTABLE;
+  va_attribs[1].value.type = VAGenericValueTypePointer;
+  va_attribs[1].value.value.p = &va_attrib_extbuf;
+
+  VAStatus status = vaCreateSurfaces(va_display_,
+                                     VA_RT_FORMAT_RGB32,
+                                     size.width(),
+                                     size.height(),
+                                     &va_surface,
+                                     1,
+                                     va_attribs,
+                                     arraysize(va_attribs));
+
+  scoped_ptr<Picture> picture;
+
+  if (status == VA_STATUS_SUCCESS) {

[Pawel Osciak, 2014/09/24 11:27:11]

Do we leak va_surface if not SUCCESS?

[llandwerlin-old, 2014/09/25 09:54:17]

Thanks, moving this all back into TFPPicture so it's properly released when the
picture is deleted.

+    if (!make_context_current_.Run())
+      return picture.Pass();
+
+    EGLint attrs[] = {EGL_IMAGE_PRESERVED_KHR, EGL_TRUE, EGL_NONE};
+    scoped_refptr<gfx::GLImageEGL> gl_image(new gfx::GLImageEGL(size));
+    gl_image->Initialize(

[Pawel Osciak, 2014/09/24 11:27:10]

This can fail.

[llandwerlin-old, 2014/09/25 09:54:17]

Right, I was counting on the following glGetError(), but will check the return
value inside the Initialize() of TFPPicture instead.

+                         EGL_NATIVE_PIXMAP_KHR,
+                         pixmap->GetEGLClientBuffer(),
+                         attrs);
+
+
+    if (static_cast<int>(glGetError()) != GL_NO_ERROR)
+      return picture.Pass();
+
+    picture.reset(new DrmPicture(this,
+                                 picture_buffer_id,
+                                 texture_id,
+                                 size,
+                                 va_surface,
+                                 pixmap,
+                                 gl_image));
+  }
+
+  return picture.Pass();
+}
+
+void DrmVaapiPictureProvider::DestroyPicture(DrmPicture* drm_picture) {
+  VASurfaceID va_surface = drm_picture->va_surface();
+
+  if (!make_context_current_.Run())

[Pawel Osciak, 2014/09/24 11:27:11]

We still want to vaDestroySurfaces if this fails.

[llandwerlin-old, 2014/09/25 09:54:17]

Acknowledged.

+    return;
+
+  // ReleaseTexImage on a GLImageEGL does nothing, do deassociate
+  // the renderer texture from the image, just set the storage of
+  // that texture to NULL
+  gfx::ScopedTextureBinder texture_binder(GL_TEXTURE_2D,
+                                          drm_picture->texture_id());
+  glTexImage2D(GL_TEXTURE_2D,
+               0,
+               GL_RGBA,
+               drm_picture->size().width(),
+               drm_picture->size().height(),
+               0,
+               GL_RGBA,
+               GL_UNSIGNED_BYTE,
+               NULL);
+
+  drm_picture->gl_image()->Destroy(true);
+
+  CHECK_EQ(static_cast<int>(glGetError()), GL_NO_ERROR);
+
+  vaDestroySurfaces(va_display_, &va_surface, 1);

[Pawel Osciak, 2014/09/24 11:27:10]

Do we need to check if va_surface is valid?

[llandwerlin-old, 2014/09/25 09:54:18]

Acknowledged.

+}
+
+bool DrmVaapiPictureProvider::PutSurfaceIntoPicture(
+    VASurfaceID va_surface_id,
+    VaapiPictureProvider::Picture* picture) {
+  DrmPicture* drm_picture = static_cast<DrmPicture*>(picture);
+  VAProcPipelineParameterBuffer* pipeline_param;

[jiajia.qin, 2014/09/16 11:09:55]

Using VPP seems like a little complicated. Do vaGetImage and eglCreateImageKHR
achieve the same effect?

[llandwerlin-old, 2014/09/16 12:28:58]

On SandyBrige/IvyBridge this triggers exactly the same operation internally,
except that with vaGetImage it doesn't seem possible to specify the color
conversion matrix (but that won't make any difference because VPP shaders on
IvyBridge have hardcoded BT709 matrix).

[jiajia.qin, 2014/09/17 10:45:24]

I add this comment because that I notice
DrmVaapiPictureProvider::CreatePicture's body is not simple (same with this
function). You need to config VASurfaceAttrib and
VASurfaceAttribExternalBuffers. In addition, you used pixmap which may not be
supported in wayland. Is there any simplified space? If you use vaGetImage, at
lease, the code will be less that the current one.

+  VARectangle input_region, output_region;
+
+  DCHECK(IsVppInitialized());
+
+  LOG_VA_ERROR_AND_RETURN(vaMapBuffer(va_display_,

[Pawel Osciak, 2014/09/24 11:27:12]

Can we use own instance of VaapiWrapper for all va operations here?

[llandwerlin-old, 2014/09/25 09:54:18]

Would you mind if I did this in another CL?
It feels like it's going to make VaapiWrapper more complicated.

[Pawel Osciak, 2014/10/08 08:17:21]

I'm open to the suggestion, but perhaps we could consider doing this please? It
feels to me that it could actually give us less code here, I think all the va
functions you are using here are already implemented in the wrapper. Is there
more to it than just calling vaapi wrapper methods here instead of va functions
directly?

And we could get rid of resource management here and let VW do it for us...
And we have locking in the wrapper which you are not doing here...

+                                      vpp_buffer_,
+                                      (void**)&pipeline_param),
+                          "Couldn't map buffer",);

[Pawel Osciak, 2014/09/24 11:27:10]

Do we need to unmap on error?

[llandwerlin-old, 2014/09/25 09:54:17]

Either MapBuffer works or it fails and nothing gets mapped.
Not sure what could be the problem here, given that no call could fail between a
successful map and an unmap.

[Pawel Osciak, 2014/10/08 08:17:21]

Acknowledged.

+
+  memset(pipeline_param, 0, sizeof *pipeline_param);
+
+  input_region.x = input_region.y = 0;
+  input_region.width = coded_picture_size_.width();
+  input_region.height = coded_picture_size_.height();
+  pipeline_param->surface_region = &input_region;
+  pipeline_param->surface = va_surface_id;
+  pipeline_param->surface_color_standard = VAProcColorStandardNone;
+
+  output_region.x = output_region.y = 0;
+  output_region.width = drm_picture->size().width();
+  output_region.height = drm_picture->size().height();
+  pipeline_param->output_region = &output_region;
+  pipeline_param->output_background_color = 0xff000000;
+  pipeline_param->output_color_standard = VAProcColorStandardNone;
+
+  LOG_VA_ERROR_AND_RETURN(vaUnmapBuffer(va_display_, vpp_buffer_),

[Pawel Osciak, 2014/09/24 11:27:11]

Would it make sense to keep the buffer mapped? Or do we need to unmap and map it
every time?

[llandwerlin-old, 2014/09/25 09:54:18]

On Intel hardware we need to unmap the buffer object (ie flush the cpu cache) so
the hardware gets the right data.

[Pawel Osciak, 2014/10/08 08:17:21]

Acknowledged.

+                          "Couldn't unmap buffer",);
+
+  LOG_VA_ERROR_AND_RETURN(vaBeginPicture(va_display_,
+                                         vpp_context_,
+                                         drm_picture->va_surface()),
+                          "Couldn't begin picture",);
+
+  LOG_VA_ERROR_AND_RETURN(vaRenderPicture(va_display_,
+                                          vpp_context_,
+                                          &vpp_buffer_, 1),
+                          "Couldn't render picture",);
+
+  LOG_VA_ERROR_AND_RETURN(vaEndPicture(va_display_, vpp_context_),
+                          "Couldn't end picture", );
+
+  if (!make_context_current_.Run())
+    return VA_STATUS_ERROR_OPERATION_FAILED;

[Pawel Osciak, 2014/09/24 11:27:12]

This method returns bool type.

[llandwerlin-old, 2014/09/25 09:54:18]

Acknowledged.

+
+  gfx::ScopedTextureBinder texture_binder(GL_TEXTURE_2D,
+                                          drm_picture->texture_id());
+  drm_picture->gl_image()->BindTexImage(GL_TEXTURE_2D);

[Pawel Osciak, 2014/09/24 11:27:12]

This can fail.

[llandwerlin-old, 2014/09/25 09:54:18]

Acknowledged.

+
+  if (static_cast<int>(glGetError()) != GL_NO_ERROR)

[Pawel Osciak, 2014/09/24 11:27:11]

Is this cast needed?

[llandwerlin-old, 2014/09/25 09:54:17]

Acknowledged.

+    return false;
+
+  return true;
+}
+
+bool DrmVaapiPictureProvider::SetCodedSurfacesSize(const gfx::Size& size) {
+  DeinitializeVpp();
+  return InitializeVpp(size);
+}
+
+
+bool DrmVaapiPictureProvider::Initialize() {
+  return true;
+}
+
+bool DrmVaapiPictureProvider::InitializeVpp(const gfx::Size& size) {
+  LOG_VA_ERROR_AND_RETURN(vaCreateConfig(va_display_,
+                                         VAProfileNone,
+                                         VAEntrypointVideoProc,
+                                         NULL,
+                                         0,
+                                         &vpp_config_),
+                          "Couldn't create config",);
+
+  LOG_VA_ERROR_AND_RETURN(vaCreateContext(va_display_,
+                                          vpp_config_,
+                                          size.width(),
+                                          size.height(),
+                                          0,
+                                          NULL,
+                                          0,
+                                          &vpp_context_),
+                          "Couldn't create context",
+                          DeinitializeVpp());

[Pawel Osciak, 2014/09/24 11:27:11]

I don't think you need this if you DeinitializeVpp in destructor?

[llandwerlin-old, 2014/09/25 09:54:18]

Acknowledged.

+
+  LOG_VA_ERROR_AND_RETURN(vaCreateBuffer(va_display_,
+                                         vpp_context_,
+                                         VAProcPipelineParameterBufferType,
+                                         sizeof(VAProcPipelineParameterBuffer),
+                                         1,
+                                         NULL,
+                                         &vpp_buffer_),
+                          "Couldn't create buffer",
+                          DeinitializeVpp());
+
+  coded_picture_size_ = size;
+
+  return true;
+}
+
+bool DrmVaapiPictureProvider::IsVppInitialized() {
+  return vpp_buffer_ != VA_INVALID_ID;
+}
+
+void DrmVaapiPictureProvider::DeinitializeVpp() {
+  if (vpp_buffer_ != VA_INVALID_ID) {
+    vaDestroyBuffer(va_display_, vpp_buffer_);
+    vpp_buffer_ = VA_INVALID_ID;
+  }
+  if (vpp_context_ != VA_INVALID_ID) {
+    vaDestroyContext(va_display_, vpp_context_);
+    vpp_context_ = VA_INVALID_ID;
+  }
+  if (vpp_config_ != VA_INVALID_ID) {
+    vaDestroyConfig(va_display_, vpp_config_);
+    vpp_config_ = VA_INVALID_ID;
+  }
+}
+
+#endif  // USE_X11
+
+} // namespace
+
+
+scoped_ptr<VaapiPictureProvider> VaapiPictureProvider::Create(
+    VADisplay va_display,
+    gfx::GLContext* gl_context,
+    const base::Callback<bool(void)> make_context_current) {
+  scoped_ptr<VaapiPictureProvider> backend;

[Pawel Osciak, 2014/09/24 11:27:11]

s/backend/provider/

[llandwerlin-old, 2014/09/25 09:54:17]

Acknowledged.

+
+#if defined(USE_X11)
+  backend.reset(new X11VaapiPictureProvider(
+      va_display,
+      static_cast<gfx::GLContextGLX*>(gl_context),
+      make_context_current));
+#else
+  backend.reset(new DrmVaapiPictureProvider(va_display,
+                                            make_context_current));
+#endif  // USE_X11
+
+  if (!backend->Initialize())
+    backend.reset();
+
+  return backend.Pass();

[Pawel Osciak, 2014/09/24 11:27:11]

Pass() shouldn't be required.

[llandwerlin-old, 2014/09/25 09:54:17]

Sadly it doesn't compile without it :

src/base/memory/scoped_ptr.h: In static member function 'static
scoped_ptr<content::VaapiPictureProvider>
content::VaapiPictureProvider::Create(VADisplay, gfx::GLContext*,
base::Callback<bool()>)':
src/base/memory/scoped_ptr.h:311:107: error: 'scoped_ptr<T,
D>::scoped_ptr(scoped_ptr<T, D>&) [with T = content::VaapiPictureProvider; D =
base::DefaultDeleter<content::VaapiPictureProvider>]' is private
MOVE_ONLY_TYPE_FOR_CPP_03(scoped_ptr, RValue)

[Pawel Osciak, 2014/10/08 08:17:21]

Right, sorry, I misread scoped_ptr as refptr.

+}
+
+} // namespace content