OVDA: Perform an egl sync before reusing a texture.

content/common/gpu/media/omx_video_decode_accelerator.cc

Index: content/common/gpu/media/omx_video_decode_accelerator.cc
diff --git a/content/common/gpu/media/omx_video_decode_accelerator.cc b/content/common/gpu/media/omx_video_decode_accelerator.cc
index a72756012b6c73c58183afb348725da7a437cd6e..31af10fa7934689dc7b71fd8aaf0980beddd8ed1 100644
--- a/content/common/gpu/media/omx_video_decode_accelerator.cc
+++ b/content/common/gpu/media/omx_video_decode_accelerator.cc
@@ -36,6 +36,10 @@ 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) {
@@ -386,9 +390,82 @@ void OmxVideoDecodeAccelerator::AssignPictureBuffers(
     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.
@@ -397,7 +474,11 @@ void OmxVideoDecodeAccelerator::ReusePictureBuffer(int32 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(),
@@ -1091,8 +1172,17 @@ bool OmxVideoDecodeAccelerator::PostSandboxInitialization() {
       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