[Ozone] Extends the lifetime of VaapiWrapper

content/common/gpu/media/vaapi_drm_picture.cc

Index: content/common/gpu/media/vaapi_drm_picture.cc
diff --git a/content/common/gpu/media/vaapi_drm_picture.cc b/content/common/gpu/media/vaapi_drm_picture.cc
index dfa4124eaa5ca53fda215d95bd90b63a01ee7111..cc94e56afb12e5d065f334edf680efdf8bf7056b 100644
--- a/content/common/gpu/media/vaapi_drm_picture.cc
+++ b/content/common/gpu/media/vaapi_drm_picture.cc
@@ -8,7 +8,6 @@
 #include "content/common/gpu/media/vaapi_wrapper.h"
 #include "third_party/libva/va/drm/va_drm.h"
 #include "third_party/libva/va/va.h"
-#include "third_party/libva/va/va_drmcommon.h"
 #include "ui/gfx/gpu_memory_buffer.h"
 #include "ui/gl/gl_bindings.h"
 #include "ui/gl/gl_image_ozone_native_pixmap.h"
@@ -22,45 +21,19 @@ namespace {
 // to BGRX_8888.
 const gfx::BufferFormat kPictureForGLImageFormat = gfx::BufferFormat::BGRX_8888;
 
-uint32_t BufferFormatToVAFourCC(gfx::BufferFormat fmt) {
-  switch (fmt) {
-    case gfx::BufferFormat::BGRX_8888:
-      return VA_FOURCC_BGRX;
-    case gfx::BufferFormat::UYVY_422:
-      return VA_FOURCC_UYVY;
-    default:
-      NOTREACHED();
-      return 0;
-  }
-}
-
-uint32_t BufferFormatToVARTFormat(gfx::BufferFormat fmt) {
-  switch (fmt) {
-    case gfx::BufferFormat::UYVY_422:
-      return VA_RT_FORMAT_YUV422;
-    case gfx::BufferFormat::BGRX_8888:
-      return VA_RT_FORMAT_RGB32;
-    default:
-      NOTREACHED();
-      return 0;
-  }
-}
-
 }  // namespace
 
 namespace content {
 
 VaapiDrmPicture::VaapiDrmPicture(
-    VaapiWrapper* vaapi_wrapper,
+    const scoped_refptr<VaapiWrapper>& vaapi_wrapper,
     const base::Callback<bool(void)>& make_context_current,
     int32 picture_buffer_id,
     uint32 texture_id,
     const gfx::Size& size)
     : VaapiPicture(picture_buffer_id, texture_id, size),
       vaapi_wrapper_(vaapi_wrapper),
-      make_context_current_(make_context_current),
-      weak_this_factory_(this) {
-}
+      make_context_current_(make_context_current) {}
 
 VaapiDrmPicture::~VaapiDrmPicture() {
   if (gl_image_ && make_context_current_.Run()) {
@@ -71,57 +44,7 @@ VaapiDrmPicture::~VaapiDrmPicture() {
   }
 }
 
-scoped_refptr<VASurface> VaapiDrmPicture::CreateVASurfaceForPixmap(
-    scoped_refptr<ui::NativePixmap> pixmap,
-    gfx::Size pixmap_size) {
-  // Get the dmabuf of the created buffer.
-  int dmabuf_fd = pixmap->GetDmaBufFd();
-  if (dmabuf_fd < 0) {
-    LOG(ERROR) << "Failed to get dmabuf from an Ozone NativePixmap";
-    return nullptr;
-  }
-  int dmabuf_pitch = pixmap->GetDmaBufPitch();
-
-  // Create a VASurface out of the created buffer using the dmabuf.
-  VASurfaceAttribExternalBuffers va_attrib_extbuf;
-  memset(&va_attrib_extbuf, 0, sizeof(va_attrib_extbuf));
-  va_attrib_extbuf.pixel_format =
-      BufferFormatToVAFourCC(pixmap->GetBufferFormat());
-  va_attrib_extbuf.width = pixmap_size.width();
-  va_attrib_extbuf.height = pixmap_size.height();
-  va_attrib_extbuf.data_size = pixmap_size.height() * dmabuf_pitch;
-  va_attrib_extbuf.num_planes = 1;
-  va_attrib_extbuf.pitches[0] = dmabuf_pitch;
-  va_attrib_extbuf.offsets[0] = 0;
-  va_attrib_extbuf.buffers = reinterpret_cast<unsigned long*>(&dmabuf_fd);
-  va_attrib_extbuf.num_buffers = 1;
-  va_attrib_extbuf.flags = 0;
-  va_attrib_extbuf.private_data = NULL;
-
-  std::vector<VASurfaceAttrib> va_attribs;
-  va_attribs.resize(2);
-
-  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;
-
-  scoped_refptr<VASurface> va_surface = vaapi_wrapper_->CreateUnownedSurface(
-      BufferFormatToVARTFormat(pixmap->GetBufferFormat()), pixmap_size,
-      va_attribs);
-  if (!va_surface) {
-    LOG(ERROR) << "Failed to create VASurface for an Ozone NativePixmap";
-    return nullptr;
-  }
-
-  return va_surface;
-}
-
+// static
 scoped_refptr<ui::NativePixmap> VaapiDrmPicture::CreateNativePixmap(
     gfx::Size size,
     gfx::BufferFormat format) {
@@ -143,17 +66,14 @@ bool VaapiDrmPicture::Initialize() {
     return false;
   }
 
-  va_surface_ = CreateVASurfaceForPixmap(pixmap_, size());
+  va_surface_ = vaapi_wrapper_->CreateVASurfaceForPixmap(pixmap_, size());
   if (!va_surface_) {
     LOG(ERROR) << "Failed creating VASurface for NativePixmap";
     return false;
   }
 
-  // Weak pointers can only bind to methods without return values,
-  // hence we cannot bind ProcessPixmap here. Instead we use a
-  // static function to solve this problem.
-  pixmap_->SetProcessingCallback(base::Bind(&VaapiDrmPicture::CallProcessPixmap,
-                                            weak_this_factory_.GetWeakPtr()));
+  pixmap_->SetProcessingCallback(
+      base::Bind(&VaapiDrmPicture::ProcessPixmap, vaapi_wrapper_));
 
   if (!make_context_current_.Run())
     return false;
@@ -181,51 +101,47 @@ bool VaapiDrmPicture::DownloadFromSurface(
 }
 
 // static
-scoped_refptr<ui::NativePixmap> VaapiDrmPicture::CallProcessPixmap(
-    base::WeakPtr<VaapiDrmPicture> weak_ptr,
+scoped_refptr<ui::NativePixmap> VaapiDrmPicture::ProcessPixmap(

[Pawel Osciak, 2015/11/20 10:09:18]

Could we have this in VaapiWrapper, and not static?

[william.xie1, 2015/11/23 03:33:58]

Done.

+    const scoped_refptr<VaapiWrapper>& vaapi_wrapper,
+    const scoped_refptr<ui::NativePixmap>& pixmap,
     gfx::Size target_size,

[Pawel Osciak, 2015/11/20 10:09:18]

s/pixmap/source_pixmap/

[william.xie1, 2015/11/23 03:33:58]

Done.

     gfx::BufferFormat target_format) {
-  if (!weak_ptr.get()) {
-    LOG(ERROR) << "Failed processing NativePixmap as processing "
-                  "unit(VaapiDrmPicture) is deleted";
+  scoped_refptr<VASurface> va_surface =
+      vaapi_wrapper->CreateVASurfaceForPixmap(pixmap, pixmap->size());
+  if (!va_surface) {
+    LOG(ERROR) << "Failed creating VA Surface for pixmap";
     return nullptr;
   }
-  return weak_ptr->ProcessPixmap(target_size, target_format);
-}
 
-scoped_refptr<ui::NativePixmap> VaapiDrmPicture::ProcessPixmap(
-    gfx::Size target_size,
-    gfx::BufferFormat target_format) {
-  if (!processed_va_surface_.get() ||
-      processed_va_surface_->size() != target_size ||
-      processed_va_surface_->format() !=
-          BufferFormatToVARTFormat(target_format)) {
-    processed_pixmap_ = CreateNativePixmap(target_size, target_format);
-    if (!processed_pixmap_) {
-      LOG(ERROR) << "Failed creating an Ozone NativePixmap for processing";
-      processed_va_surface_ = nullptr;
-      return nullptr;
-    }
-    processed_va_surface_ =
-        CreateVASurfaceForPixmap(processed_pixmap_, target_size);
-    if (!processed_va_surface_) {
-      LOG(ERROR) << "Failed creating VA Surface for pixmap";
-      processed_pixmap_ = nullptr;
-      return nullptr;
-    }
+  scoped_refptr<ui::NativePixmap> processed_pixmap =
+      CreateNativePixmap(target_size, target_format);
+  if (!processed_pixmap) {
+    LOG(ERROR) << "Failed creating an Ozone NativePixmap for processing";
+    va_surface = nullptr;
+    return nullptr;

[Pawel Osciak, 2015/11/20 10:09:18]

This is not needed, as va_surface will fall out of scope on return.

Same for all other local variables in other return paths as well.

[william.xie1, 2015/11/23 03:33:58]

Done.

+  }
+
+  scoped_refptr<VASurface> processed_va_surface =
+      vaapi_wrapper->CreateVASurfaceForPixmap(processed_pixmap, target_size);
+  if (!processed_va_surface) {
+    LOG(ERROR) << "Failed creating processed VA Surface for pixmap";
+    va_surface = nullptr;
+    processed_pixmap = nullptr;
+    return nullptr;
   }
 
-  DCHECK(processed_pixmap_);
+  DCHECK(processed_pixmap);
   bool vpp_result =
-      vaapi_wrapper_->BlitSurface(va_surface_, processed_va_surface_);
+      vaapi_wrapper->BlitSurface(va_surface, processed_va_surface);

[Pawel Osciak, 2015/11/20 10:09:18]

Perhaps:

if (!vaapi_wrapper->BlitSurface(va_surface, processed_va_surface)) {
}

[william.xie1, 2015/11/23 03:33:58]

Done.

   if (!vpp_result) {
     LOG(ERROR) << "Failed scaling NativePixmap";
-    processed_pixmap_ = nullptr;
-    processed_va_surface_ = nullptr;
+    va_surface = nullptr;
+    processed_pixmap = nullptr;
+    processed_va_surface = nullptr;
     return nullptr;
   }
 
-  return processed_pixmap_;
+  return processed_pixmap;
 }
 
 scoped_refptr<gl::GLImage> VaapiDrmPicture::GetImageToBind() {