OVDA: Perform an egl sync before reusing a texture.

content/common/gpu/media/omx_video_decode_accelerator.cc

 // Copyright (c) 2012 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/omx_video_decode_accelerator.h"
 
 #include "base/bind.h"
 #include "base/debug/trace_event.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
@@ skipping 18 matching lines @@
 typedef OMX_ERRORTYPE (*OMXGetComponentsOfRole)(OMX_STRING, OMX_U32*, OMX_U8**);
 typedef OMX_ERRORTYPE (*OMXFreeHandle)(OMX_HANDLETYPE);
 typedef OMX_ERRORTYPE (*OMXDeinit)();
 
 OMXInit omx_init = NULL;
 OMXGetHandle omx_gethandle = NULL;
 OMXGetComponentsOfRole omx_get_components_of_role = NULL;
 OMXFreeHandle omx_free_handle = NULL;
 OMXDeinit omx_deinit = NULL;
 
+static PFNEGLCREATESYNCKHRPROC egl_create_sync_khr = NULL;
+static PFNEGLGETSYNCATTRIBKHRPROC egl_get_sync_attrib_khr = NULL;
+static PFNEGLDESTROYSYNCKHRPROC egl_destroy_sync_khr = NULL;
+
 // Maps h264-related Profile enum values to OMX_VIDEO_AVCPROFILETYPE values.
 static OMX_U32 MapH264ProfileToOMXAVCProfile(uint32 profile) {
   switch (profile) {
     case media::H264PROFILE_BASELINE:
       return OMX_VIDEO_AVCProfileBaseline;
     case media::H264PROFILE_MAIN:
       return OMX_VIDEO_AVCProfileMain;
     case media::H264PROFILE_EXTENDED:
       return OMX_VIDEO_AVCProfileExtended;
     case media::H264PROFILE_HIGH:
@@ skipping 330 matching lines @@
     return;  // get all the buffers first.
   DCHECK_EQ(pictures_.size(), kNumPictureBuffers);
 
   // These do their own RETURN_ON_FAILURE dances.
   if (!AllocateOutputBuffers())
     return;
   if (!SendCommandToPort(OMX_CommandPortEnable, output_port_))
     return;
 }
 
+class OmxVideoDecodeAccelerator::PictureSyncObject {

[piman, 2012/10/12 19:37:10]

nit: we usually don't define inner classes in the middle of defining the methods
of the outer class. Mind moving this before the OmxVideoDecodeAccelerator
constructor?

[Pawel Osciak, 2012/10/12 20:33:08]

Ok, if that's the convention. (The idea was to have it near the only use site.)

+ public:
+  // Create a sync object and insert into the GPU command stream.
+  PictureSyncObject(EGLDisplay egl_display);
+  ~PictureSyncObject();
+
+  // Check the status of the sync object. Return true if synced.

[Ami GONE FROM CHROMIUM, 2012/10/12 19:34:46]

Style-guide-sanctioned comment style would be:
// Returns true if object is synced.
which is probably not adding any information content beyond the signature of the
function, so I'd just drop it.

[Pawel Osciak, 2012/10/12 20:33:08]

Done.

+  bool IsSynced();
+
+ private:
+  EGLSyncKHR egl_sync_obj_;
+  EGLDisplay egl_display_;
+};
+
+OmxVideoDecodeAccelerator::PictureSyncObject::PictureSyncObject(
+    EGLDisplay egl_display)
+    : egl_display_(egl_display) {
+  DCHECK(egl_display_ != EGL_NO_DISPLAY);
+
+  egl_sync_obj_ = egl_create_sync_khr(egl_display_, EGL_SYNC_FENCE_KHR, NULL);
+  DCHECK(egl_sync_obj_ != EGL_NO_SYNC_KHR)
+      << "Failed creating a sync object, will not sync.";

[Ami GONE FROM CHROMIUM, 2012/10/12 19:34:46]

msg is still bogus (not only will the process not sync, it's crashing!)
I'd just drop the message entirely since the body of the assertion (which is
emitted on crash) is informative.

[Pawel Osciak, 2012/10/12 20:33:08]

Heh. Somehow my brain won't acknowledge that this an assert.

+}
+
+OmxVideoDecodeAccelerator::PictureSyncObject::~PictureSyncObject() {
+  if (egl_sync_obj_ != EGL_NO_SYNC_KHR)

[Ami GONE FROM CHROMIUM, 2012/10/12 19:34:46]

Not possible for this to be false anymore, so can drop the if.

[Pawel Osciak, 2012/10/12 20:33:08]

Done.

+    egl_destroy_sync_khr(egl_display_, egl_sync_obj_);
+}
+
+bool OmxVideoDecodeAccelerator::PictureSyncObject::IsSynced() {
+  EGLint value;

[piman, 2012/10/12 19:37:10]

nit: = 0

[Ami GONE FROM CHROMIUM, 2012/10/12 19:43:56]

OOC: Why?

[Pawel Osciak, 2012/10/12 20:33:08]

Are you worried the call would exit early before overwriting it? But then I
think it'd be safe to assume ret would be != EGL_TRUE.

[Pawel Osciak, 2012/10/12 20:34:54]

Oh, but that's only only in debug builds. I really need more caffeine today.

+  EGLBoolean ret = egl_get_sync_attrib_khr(
+      egl_display_, egl_sync_obj_, EGL_SYNC_STATUS_KHR, &value);
+  DCHECK(ret) << "Failed getting sync object state.";
+
+  if (value == EGL_UNSIGNALED_KHR)

[Ami GONE FROM CHROMIUM, 2012/10/12 19:34:46]

l.428-431 is equiv to
return value == EGL_SIGNALED_KHR;

[Pawel Osciak, 2012/10/12 20:33:08]

Done.

+    return false;
+
+  return true;
+}
+
 void OmxVideoDecodeAccelerator::ReusePictureBuffer(int32 picture_buffer_id) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
   TRACE_EVENT1("Video Decoder", "OVDA::ReusePictureBuffer",
                "Picture id", picture_buffer_id);
+  scoped_ptr<PictureSyncObject> egl_sync_obj(
+      new PictureSyncObject(egl_display_));
+
+  // Start checking sync status periodically.
+  CheckPictureStatus(picture_buffer_id, egl_sync_obj.Pass());
+}
+
+void OmxVideoDecodeAccelerator::CheckPictureStatus(
+    int32 picture_buffer_id,
+    scoped_ptr<PictureSyncObject> egl_sync_obj) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
+
+  // It's possible for this task to never run if the message loop is
+  // stopped. In that case we may never call QueuePictureBuffer().
+  // This is fine though, because all pictures, irrespective of their state,
+  // are in pictures_ map and that's what will be used to do the clean up.
+  // 5ms feels like a good compromise, allowing some decoding ahead
+  // (up to 3 frames/vsync) to compensate for more difficult frames.
+  if (!egl_sync_obj->IsSynced()) {
+    MessageLoop::current()->PostDelayedTask(FROM_HERE, base::Bind(
+        &OmxVideoDecodeAccelerator::CheckPictureStatus, weak_this_,
+        picture_buffer_id, base::Passed(&egl_sync_obj)),
+        base::TimeDelta::FromMilliseconds(5));

[piman, 2012/10/12 19:37:10]

nit: can you make this a constant in global scope (e.g. kSyncPollDelayMs)

[Pawel Osciak, 2012/10/12 20:33:08]

Done.

+    return;
+  }
+
+  // Synced successfully. Queue the buffer for reuse.
+  QueuePictureBuffer(picture_buffer_id);
+}
+
+void OmxVideoDecodeAccelerator::QueuePictureBuffer(int32 picture_buffer_id) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
   // During port-flushing, do not call OMX FillThisBuffer.
   if (current_state_change_ == RESETTING) {
     queued_picture_buffer_ids_.push_back(picture_buffer_id);
     return;
   }
 
-  RETURN_ON_FAILURE(CanFillBuffer(), "Can't fill buffer", ILLEGAL_STATE,);
+  // We might have started destroying while waiting for the picture. It's safe
+  // to drop it here, because we will free all the pictures regardless of their
+  // state using the pictures_ map.
+  if (!CanFillBuffer())
+    return;
 
   OutputPictureById::iterator it = pictures_.find(picture_buffer_id);
   RETURN_ON_FAILURE(it != pictures_.end(),
                     "Missing picture buffer id: " << picture_buffer_id,
                     INVALID_ARGUMENT,);
   OutputPicture& output_picture = it->second;
 
   ++output_buffers_at_component_;
   OMX_ERRORTYPE result =
       OMX_FillThisBuffer(component_handle_, output_picture.omx_buffer_header);
@@ skipping 673 matching lines @@
   omx_init = reinterpret_cast<OMXInit>(dlsym(omx_handle, "OMX_Init"));
   omx_gethandle =
       reinterpret_cast<OMXGetHandle>(dlsym(omx_handle, "OMX_GetHandle"));
   omx_get_components_of_role =
       reinterpret_cast<OMXGetComponentsOfRole>(
           dlsym(omx_handle, "OMX_GetComponentsOfRole"));
   omx_free_handle =
       reinterpret_cast<OMXFreeHandle>(dlsym(omx_handle, "OMX_FreeHandle"));
   omx_deinit =
       reinterpret_cast<OMXDeinit>(dlsym(omx_handle, "OMX_Deinit"));
+
+  egl_create_sync_khr = reinterpret_cast<PFNEGLCREATESYNCKHRPROC>(
+      eglGetProcAddress("eglCreateSyncKHR"));
+  egl_get_sync_attrib_khr = reinterpret_cast<PFNEGLGETSYNCATTRIBKHRPROC>(
+      eglGetProcAddress("eglGetSyncAttribKHR"));
+  egl_destroy_sync_khr = reinterpret_cast<PFNEGLDESTROYSYNCKHRPROC>(
+      eglGetProcAddress("eglDestroySyncKHR"));
+
   return (omx_init && omx_gethandle && omx_get_components_of_role &&
-          omx_free_handle && omx_deinit);
+          omx_free_handle && omx_deinit && egl_create_sync_khr &&
+          egl_get_sync_attrib_khr && egl_destroy_sync_khr);
 }
 
 // static
 OMX_ERRORTYPE OmxVideoDecodeAccelerator::EventHandler(OMX_HANDLETYPE component,
                                                       OMX_PTR priv_data,
                                                       OMX_EVENTTYPE event,
                                                       OMX_U32 data1,
                                                       OMX_U32 data2,
                                                       OMX_PTR event_data) {
   // Called on the OMX thread.
@@ skipping 54 matching lines @@
 
 bool OmxVideoDecodeAccelerator::SendCommandToPort(
     OMX_COMMANDTYPE cmd, int port_index) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
   OMX_ERRORTYPE result = OMX_SendCommand(component_handle_,
                                          cmd, port_index, 0);
   RETURN_ON_OMX_FAILURE(result, "SendCommand() failed" << cmd,
                         PLATFORM_FAILURE, false);
   return true;
 }