VaapiVideoAccelerator: make Vaapi accelerator work with ozone

content/common/gpu/media/vaapi_video_decode_accelerator.cc

Index: content/common/gpu/media/vaapi_video_decode_accelerator.cc
diff --git a/content/common/gpu/media/vaapi_video_decode_accelerator.cc b/content/common/gpu/media/vaapi_video_decode_accelerator.cc
index 0b3001272ac8e8349362fe10fbd4612e5cbc8e3e..3ee4eededfdd76a2b751201fa175bbc4f61c7bb0 100644
--- a/content/common/gpu/media/vaapi_video_decode_accelerator.cc
+++ b/content/common/gpu/media/vaapi_video_decode_accelerator.cc
@@ -9,13 +9,11 @@
 #include "base/stl_util.h"
 #include "base/strings/string_util.h"
 #include "base/synchronization/waitable_event.h"
-#include "base/threading/non_thread_safe.h"
 #include "content/common/gpu/gpu_channel.h"
 #include "content/common/gpu/media/vaapi_video_decode_accelerator.h"
 #include "media/base/bind_to_current_loop.h"
 #include "media/video/picture.h"
 #include "ui/gl/gl_bindings.h"
-#include "ui/gl/scoped_binders.h"
 
 static void ReportToUMA(
     content::VaapiH264Decoder::VAVDAH264DecoderFailure failure) {
@@ -61,175 +59,16 @@ void VaapiVideoDecodeAccelerator::NotifyError(Error error) {
   }
 }
 
-// TFPPicture allocates X Pixmaps and binds them to textures passed
-// in PictureBuffers from clients to them. TFPPictures are created as
-// a consequence of receiving a set of PictureBuffers from clients and released
-// at the end of decode (or when a new set of PictureBuffers is required).
-//
-// TFPPictures are used for output, contents of VASurfaces passed from decoder
-// are put into the associated pixmap memory and sent to client.
-class VaapiVideoDecodeAccelerator::TFPPicture : public base::NonThreadSafe {
- public:
-  ~TFPPicture();
-
-  static linked_ptr<TFPPicture> Create(
-      const base::Callback<bool(void)>& make_context_current,
-      const GLXFBConfig& fb_config,
-      Display* x_display,
-      int32 picture_buffer_id,
-      uint32 texture_id,
-      gfx::Size size);
-
-  int32 picture_buffer_id() {
-    return picture_buffer_id_;
-  }
-
-  gfx::Size size() {
-    return size_;
-  }
-
-  int x_pixmap() {
-    return x_pixmap_;
-  }
-
-  // Bind texture to pixmap. Needs to be called every frame.
-  bool Bind();
-
- private:
-  TFPPicture(const base::Callback<bool(void)>& make_context_current,
-             Display* x_display,
-             int32 picture_buffer_id,
-             uint32 texture_id,
-             gfx::Size size);
-
-  bool Initialize(const GLXFBConfig& fb_config);
-
-  base::Callback<bool(void)> make_context_current_;
-
-  Display* x_display_;
-
-  // Output id for the client.
-  int32 picture_buffer_id_;
-  uint32 texture_id_;
-
-  gfx::Size size_;
-
-  // Pixmaps bound to this texture.
-  Pixmap x_pixmap_;
-  GLXPixmap glx_pixmap_;
-
-  DISALLOW_COPY_AND_ASSIGN(TFPPicture);
-};
-
-VaapiVideoDecodeAccelerator::TFPPicture::TFPPicture(
-    const base::Callback<bool(void)>& make_context_current,
-    Display* x_display,
-    int32 picture_buffer_id,
-    uint32 texture_id,
-    gfx::Size size)
-    : make_context_current_(make_context_current),
-      x_display_(x_display),
-      picture_buffer_id_(picture_buffer_id),
-      texture_id_(texture_id),
-      size_(size),
-      x_pixmap_(0),
-      glx_pixmap_(0) {
-  DCHECK(!make_context_current_.is_null());
-};
-
-linked_ptr<VaapiVideoDecodeAccelerator::TFPPicture>
-VaapiVideoDecodeAccelerator::TFPPicture::Create(
-    const base::Callback<bool(void)>& make_context_current,
-    const GLXFBConfig& fb_config,
-    Display* x_display,
-    int32 picture_buffer_id,
-    uint32 texture_id,
-    gfx::Size size) {
-
-  linked_ptr<TFPPicture> tfp_picture(
-      new TFPPicture(make_context_current, x_display, picture_buffer_id,
-                     texture_id, size));
-
-  if (!tfp_picture->Initialize(fb_config))
-    tfp_picture.reset();
-
-  return tfp_picture;
-}
-
-bool VaapiVideoDecodeAccelerator::TFPPicture::Initialize(
-    const GLXFBConfig& fb_config) {
-  DCHECK(CalledOnValidThread());
-  if (!make_context_current_.Run())
-    return false;
-
-  XWindowAttributes win_attr;
-  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
-  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 false;
-  }
-
-  static const int pixmap_attr[] = {
-    GLX_TEXTURE_TARGET_EXT, GLX_TEXTURE_2D_EXT,
-    GLX_TEXTURE_FORMAT_EXT, GLX_TEXTURE_FORMAT_RGB_EXT,
-    GL_NONE,
-  };
-
-  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";
-    return false;
-  }
-
-  return true;
-}
-
-VaapiVideoDecodeAccelerator::TFPPicture::~TFPPicture() {
-  DCHECK(CalledOnValidThread());
-  // Unbind surface from texture and deallocate resources.
-  if (glx_pixmap_ && make_context_current_.Run()) {
-    glXReleaseTexImageEXT(x_display_, glx_pixmap_, GLX_FRONT_LEFT_EXT);
-    glXDestroyPixmap(x_display_, glx_pixmap_);
-  }
-
-  if (x_pixmap_)
-    XFreePixmap(x_display_, x_pixmap_);
-  XSync(x_display_, False);  // Needed to work around buggy vdpau-driver.
-}
-
-bool VaapiVideoDecodeAccelerator::TFPPicture::Bind() {
-  DCHECK(CalledOnValidThread());
-  DCHECK(x_pixmap_);
-  DCHECK(glx_pixmap_);
-  if (!make_context_current_.Run())
-    return false;
-
-  gfx::ScopedTextureBinder texture_binder(GL_TEXTURE_2D, texture_id_);
-  glXBindTexImageEXT(x_display_, glx_pixmap_, GLX_FRONT_LEFT_EXT, NULL);
-
-  return true;
-}
-
-VaapiVideoDecodeAccelerator::TFPPicture*
-    VaapiVideoDecodeAccelerator::TFPPictureById(int32 picture_buffer_id) {
-  TFPPictures::iterator it = tfp_pictures_.find(picture_buffer_id);
-  if (it == tfp_pictures_.end()) {
-    DVLOG(1) << "Picture id " << picture_buffer_id << " does not exist";
-    return NULL;
-  }
-
-  return it->second.get();
+VaapiPictureProvider::Picture* VaapiVideoDecodeAccelerator::PictureById(
+    int32 picture_buffer_id) {
+  VaapiPictureProvider::Picture* picture = pictures_.get(picture_buffer_id);
+  return picture;
 }
 
 VaapiVideoDecodeAccelerator::VaapiVideoDecodeAccelerator(
-    Display* x_display,
+    gfx::GLContext* gl_context,
     const base::Callback<bool(void)>& make_context_current)
-    : x_display_(x_display),
+    : gl_context_(gl_context),
       make_context_current_(make_context_current),
       state_(kUninitialized),
       input_ready_(&lock_),
@@ -249,35 +88,8 @@ VaapiVideoDecodeAccelerator::VaapiVideoDecodeAccelerator(
 
 VaapiVideoDecodeAccelerator::~VaapiVideoDecodeAccelerator() {
   DCHECK_EQ(message_loop_, base::MessageLoop::current());
-}
-
-class XFreeDeleter {
- public:
-  void operator()(void* x) const {
-    ::XFree(x);
-  }
-};
-
-bool VaapiVideoDecodeAccelerator::InitializeFBConfig() {
-  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)
-    return false;
-  if (!num_fbconfigs)
-    return false;
-
-  fb_config_ = glx_fb_configs.get()[0];
-  return true;
+  // Make sure pictures are destroyed before the picture provider is.
+  pictures_.clear();

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

Looks like each Picture should rather have a refptr to the provider.

[llandwerlin-old, 2014/10/02 14:14:23]

Right, thanks, the comment is misleading here (leftover from previous version of
the CL). But with the changes you asked for (every picture should be able to
destroy itself) no pointer to the VaapiPictureProvider is required.

I'll add a comment to mention that pictures need to be destroyed before the
VaapiWrapper otherwise their VADisplay might be invalid.

 }
 
 bool VaapiVideoDecodeAccelerator::Initialize(media::VideoCodecProfile profile,
@@ -291,18 +103,23 @@ bool VaapiVideoDecodeAccelerator::Initialize(media::VideoCodecProfile profile,
   DCHECK_EQ(state_, kUninitialized);
   DVLOG(2) << "Initializing VAVDA, profile: " << profile;
 
-  if (!make_context_current_.Run())
+#if defined(USE_X11)
+  if (gfx::GetGLImplementation() != gfx::kGLImplementationDesktopGL) {
+    DVLOG(1) << "HW video decode acceleration not available without "
+               "DesktopGL (GLX).";
     return false;
-
-  if (!InitializeFBConfig()) {
-    DVLOG(1) << "Could not get a usable FBConfig";
+  }
+#else
+  if (gfx::GetGLImplementation() != gfx::kGLImplementationEGLGLES2) {
+    DVLOG(1) << "HW video decode acceleration not available without "
+               "EGLGLES2.";
     return false;
   }
+#endif // USE_X11
 
   vaapi_wrapper_ = VaapiWrapper::Create(
       VaapiWrapper::kDecode,
       profile,
-      x_display_,
       base::Bind(&ReportToUMA, content::VaapiH264Decoder::VAAPI_ERROR));
 
   if (!vaapi_wrapper_.get()) {
@@ -310,6 +127,16 @@ bool VaapiVideoDecodeAccelerator::Initialize(media::VideoCodecProfile profile,
     return false;
   }
 
+  vaapi_picture_provider_ = VaapiPictureProvider::Create(
+      vaapi_wrapper_->GetDisplay(),

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

Could the provider get its own display? Or better, as mentioned before, use its
own VaapiWrapper?

[llandwerlin-old, 2014/10/02 14:14:23]

You need to have the all VA objects (VASurface/VAContext/etc...) allocated from
the same display object (parent object) in order to do operations across them
(VPP stuff or call vaPutSurface()). If it's not the case, you will experience
crashes in libva.

Would you like me to give the VaapiWrapper object instead?

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

Yes, please. I'd really prefer to use a common VaapiWrapper. Then we wouldn't
have to worry about ownership, lifetimes, synchronization and same display...?

+      gl_context_,
+      make_context_current_);
+
+  if (!vaapi_picture_provider_.get()) {
+    DVLOG(1) << "Failed initializing VAAPI Picture Provider";
+    return false;
+  }
+
   decoder_.reset(
       new VaapiH264Decoder(
           vaapi_wrapper_.get(),
@@ -344,10 +171,10 @@ void VaapiVideoDecodeAccelerator::SurfaceReady(
 void VaapiVideoDecodeAccelerator::OutputPicture(
     const scoped_refptr<VASurface>& va_surface,
     int32 input_id,
-    TFPPicture* tfp_picture) {
+    VaapiPictureProvider::Picture* picture) {
   DCHECK_EQ(message_loop_, base::MessageLoop::current());
 
-  int32 output_id  = tfp_picture->picture_buffer_id();
+  int32 output_id = picture->picture_buffer_id();
 
   TRACE_EVENT2("Video Decoder", "VAVDA::OutputSurface",
                "input_id", input_id,
@@ -356,15 +183,10 @@ void VaapiVideoDecodeAccelerator::OutputPicture(
   DVLOG(3) << "Outputting VASurface " << va_surface->id()
            << " into pixmap bound to picture buffer id " << output_id;
 
-  RETURN_AND_NOTIFY_ON_FAILURE(tfp_picture->Bind(),
-                               "Failed binding texture to pixmap",
-                               PLATFORM_FAILURE, );
-
   RETURN_AND_NOTIFY_ON_FAILURE(
-      vaapi_wrapper_->PutSurfaceIntoPixmap(va_surface->id(),
-                                           tfp_picture->x_pixmap(),
-                                           tfp_picture->size()),
-      "Failed putting surface into pixmap", PLATFORM_FAILURE, );
+      vaapi_picture_provider_->PutSurfaceIntoPicture(va_surface->id(), picture),
+      "Failed putting surface into pixmap",
+      PLATFORM_FAILURE, );
 
   // Notify the client a picture is ready to be displayed.
   ++num_frames_at_client_;
@@ -375,7 +197,7 @@ void VaapiVideoDecodeAccelerator::OutputPicture(
   // (crbug.com/402760).
   if (client_)
     client_->PictureReady(
-        media::Picture(output_id, input_id, gfx::Rect(tfp_picture->size())));
+        media::Picture(output_id, input_id, gfx::Rect(picture->size())));
 }
 
 void VaapiVideoDecodeAccelerator::TryOutputSurface() {
@@ -391,11 +213,11 @@ void VaapiVideoDecodeAccelerator::TryOutputSurface() {
   OutputCB output_cb = pending_output_cbs_.front();
   pending_output_cbs_.pop();
 
-  TFPPicture* tfp_picture = TFPPictureById(output_buffers_.front());
-  DCHECK(tfp_picture);
+  VaapiPictureProvider::Picture* picture = PictureById(output_buffers_.front());
+  DCHECK(picture);
   output_buffers_.pop();
 
-  output_cb.Run(tfp_picture);
+  output_cb.Run(picture);
 
   if (finish_flush_pending_ && pending_output_cbs_.empty())
     FinishFlush();
@@ -598,7 +420,7 @@ void VaapiVideoDecodeAccelerator::TryFinishSurfaceSetChange() {
     return;
 
   if (!pending_output_cbs_.empty() ||
-      tfp_pictures_.size() != available_va_surfaces_.size()) {
+      pictures_.size() != available_va_surfaces_.size()) {
     // Either:
     // 1. Not all pending pending output callbacks have been executed yet.
     // Wait for the client to return enough pictures and retry later.
@@ -616,13 +438,13 @@ void VaapiVideoDecodeAccelerator::TryFinishSurfaceSetChange() {
   available_va_surfaces_.clear();
   vaapi_wrapper_->DestroySurfaces();
 
-  for (TFPPictures::iterator iter = tfp_pictures_.begin();
-       iter != tfp_pictures_.end(); ++iter) {
+  for (Pictures::iterator iter = pictures_.begin(); iter != pictures_.end();
+       ++iter) {
     DVLOG(2) << "Dismissing picture id: " << iter->first;
     if (client_)
       client_->DismissPictureBuffer(iter->first);
   }
-  tfp_pictures_.clear();
+  pictures_.clear();
 
   // And ask for a new set as requested.
   DVLOG(1) << "Requesting " << requested_num_pics_ << " pictures of size: "
@@ -682,7 +504,7 @@ void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
   DCHECK_EQ(message_loop_, base::MessageLoop::current());
 
   base::AutoLock auto_lock(lock_);
-  DCHECK(tfp_pictures_.empty());
+  DCHECK(pictures_.empty());
 
   while (!output_buffers_.empty())
     output_buffers_.pop();
@@ -701,23 +523,27 @@ void VaapiVideoDecodeAccelerator::AssignPictureBuffers(
       "Failed creating VA Surfaces", PLATFORM_FAILURE, );
   DCHECK_EQ(va_surface_ids.size(), buffers.size());
 
+  RETURN_AND_NOTIFY_ON_FAILURE(
+      vaapi_picture_provider_->SetCodedSurfacesSize(requested_pic_size_),
+      "Failed to update VA picture provider surface size", PLATFORM_FAILURE, );
+
   for (size_t i = 0; i < buffers.size(); ++i) {
     DVLOG(2) << "Assigning picture id: " << buffers[i].id()
              << " to texture id: " << buffers[i].texture_id()
              << " VASurfaceID: " << va_surface_ids[i];
 
-    linked_ptr<TFPPicture> tfp_picture(
-        TFPPicture::Create(make_context_current_, fb_config_, x_display_,
-                           buffers[i].id(), buffers[i].texture_id(),
-                           requested_pic_size_));
+    scoped_ptr<VaapiPictureProvider::Picture> picture(
+        vaapi_picture_provider_->CreatePicture(buffers[i].id(),
+                                               buffers[i].texture_id(),
+                                               requested_pic_size_));
 
     RETURN_AND_NOTIFY_ON_FAILURE(
-        tfp_picture.get(), "Failed assigning picture buffer to a texture.",
+        picture.get(),
+        "Failed assigning picture buffer to a texture.",
         PLATFORM_FAILURE, );
 
-    bool inserted = tfp_pictures_.insert(std::make_pair(
-        buffers[i].id(), tfp_picture)).second;
-    DCHECK(inserted);
+    DCHECK(!pictures_.contains(buffers[i].id()));

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

I think this should be s/!// ?

Are you testing on a Debug build?

[llandwerlin-old, 2014/10/02 14:14:23]

I might be wrong, but the purpose here is to check that the caller didn't
provide 2 buffers with the same id. Therefore it should be "not contained".

I'm using the version 9999 of chromeos-chrome package, not sure if that enables
dchecks.

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

Ah you are right, sorry.
As for building chrome, I'd recommend
http://www.chromium.org/chromium-os/how-tos-and-troubleshooting/building-chro....

+    pictures_.set(buffers[i].id(), picture.Pass());
 
     output_buffers_.push(buffers[i].id());
     available_va_surfaces_.push_back(va_surface_ids[i]);