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..473c73929f38cb0d0ef6b7deff210b198275e68d 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,106 @@ void OmxVideoDecodeAccelerator::AssignPictureBuffers(
     return;
 }
 
+class OmxVideoDecodeAccelerator::PictureSyncObject {
+ public:
+  PictureSyncObject();
+  ~PictureSyncObject();
+
+  // Create a sync object and insert into the GPU command stream.
+  // Returns false on failure.
+  bool Create(EGLDisplay egl_display);

[Ami GONE FROM CHROMIUM, 2012/10/12 17:20:05]

All of the error conditions that can cause createsynckhr to fail are programming
errors (not runtime-variable conditions) so you can just inline this into the
ctor, and DCHECK that the resulting sync obj !=EGL_NO_SYNC_KHR.

[Pawel Osciak, 2012/10/12 17:50:50]

Not sure what you mean, egl_create_sync_khr may fail in runtime for
driver-specific reasons...

[Ami GONE FROM CHROMIUM, 2012/10/12 17:57:28]

Where does the spec permit that?
Do you mean s/reasons/bugs/?  That reasoning could apply to any interaction with
any other component, so isn't particularly convincing.

[Pawel Osciak, 2012/10/12 18:39:28]

Well, mostly things like out of memory or driver implementation-specific,
runtime errors. I guess not good enough to guard against it.

+  // Check the status of the sync object. Return true if synced or if an error
+  // occurred so as not to wait forever.
+  bool IsSynced();
+
+ private:
+  EGLSyncKHR egl_sync_obj_;
+  EGLDisplay egl_display_;
+};
+
+OmxVideoDecodeAccelerator::PictureSyncObject::PictureSyncObject()
+    : egl_sync_obj_(EGL_NO_SYNC_KHR),
+      egl_display_(EGL_NO_DISPLAY) {
+}
+
+OmxVideoDecodeAccelerator::PictureSyncObject::~PictureSyncObject() {
+  if (egl_sync_obj_ != EGL_NO_SYNC_KHR && egl_display_ != EGL_NO_SYNC_KHR)
+    egl_destroy_sync_khr(egl_display_, egl_sync_obj_);
+}
+
+bool OmxVideoDecodeAccelerator::PictureSyncObject::Create(
+    EGLDisplay egl_display) {
+
+  egl_display_ = egl_display;
+
+  // Create a sync object and insert it into the command stream.
+  egl_sync_obj_ = egl_create_sync_khr(egl_display_, EGL_SYNC_FENCE_KHR, 0);

[Ami GONE FROM CHROMIUM, 2012/10/12 17:20:05]

s/0/NULL/

[Pawel Osciak, 2012/10/12 18:39:28]

Done.

+  if (egl_sync_obj_ == EGL_NO_SYNC_KHR) {
+    DLOG(WARNING) << "Could not create EGL sync object, will not sync.";
+    return false;
+  }
+
+  return true;
+}
+
+bool OmxVideoDecodeAccelerator::PictureSyncObject::IsSynced() {
+  EGLint value;
+  EGLBoolean ret = egl_get_sync_attrib_khr(
+      egl_display_, egl_sync_obj_, EGL_SYNC_STATUS_KHR, &value);
+  if (ret == EGL_TRUE && value == EGL_UNSIGNALED_KHR)
+    return false;
+
+  // The object has been signalled or we got an error polling the state. If we
+  // got an error (ret == EGL_FALSE), we allow to continue as well. Worst case,
+  // we'll get some corruption, but the decoding can continue safely.
+  DCHECK(ret) << "EGL sync failed, continuing without syncing.";

[Ami GONE FROM CHROMIUM, 2012/10/12 17:20:05]

lolwat?
In Debug, ain't nuthin' continuing after a DCHECK.
In Release, this message is never printed.

But, as above, the only error path for get_sync_attrib is programming error, so
IMO you should DCHECK(ret) at l.439 and replace the rest of the method with
return value == EGL_SIGNALED_KHR;

(and also remove the verbiage about error from the method-level comment)

[Pawel Osciak, 2012/10/12 17:50:50]

lolled myself, not sure what I was thinking.
I'm not sure if I want any of those calls to kill us though, we could just
continue without syncing as we are doing now...

[Pawel Osciak, 2012/10/12 18:39:28]

Done.

+  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);
+
+
+  if (!egl_sync_obj->Create(egl_display_)) {
+    // If creating the sync object failed, just requeue it, we might
+    // get artifacts, but decoding will continue.
+    QueuePictureBuffer(picture_buffer_id);
+    return;
+  }
+
+  // Inserted the object successfully, start checking it 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 get cancelled if the decoder is being

[Ami GONE FROM CHROMIUM, 2012/10/12 17:20:05]

s/get cancelled/never run/

[Ami GONE FROM CHROMIUM, 2012/10/12 17:20:05]

s/decoder is destroyed/messageloop is stopped/

[Pawel Osciak, 2012/10/12 18:39:28]

Done.

[Pawel Osciak, 2012/10/12 18:39:28]

Done.

+  // destroyed. 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, base::Unretained(this),

[Ami GONE FROM CHROMIUM, 2012/10/12 17:20:05]

s/base::Unretained(this)/weak_this_/ because what guarantees that
(base::Unretained) |this| will point at a valid object 5ms from now???

[Pawel Osciak, 2012/10/12 18:39:28]

Another brain fart, must've still been in the sub-thread mindset when I wrote
this.

+        picture_buffer_id, base::Passed(egl_sync_obj.Pass())),

[Ami GONE FROM CHROMIUM, 2012/10/12 17:20:05]

replace
egl_sync_obj.Pass()
with
&egl_sync_obj

[Pawel Osciak, 2012/10/12 18:39:28]

Ok. FWIW got inspired by
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/common/extensions/e....

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

Huh; filed bug 155593 to eradicate that pattern.

+        base::TimeDelta::FromMilliseconds(5));
+    return;
+  }
+
+  // Synced successfully or getting sync status failed. Queue the buffer for

[Ami GONE FROM CHROMIUM, 2012/10/12 17:20:05]

drop "failed" clause

[Pawel Osciak, 2012/10/12 18:39:28]

Done.

+  // reuse, in the latter case we might get some corruption, but decoding can
+  // continue safely.
+  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 +498,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 +1196,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