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..f9d832f448dafb36d19d30b2fe8ae1aba6a3e641 100644
--- a/content/common/gpu/media/omx_video_decode_accelerator.cc
+++ b/content/common/gpu/media/omx_video_decode_accelerator.cc
@@ -20,6 +20,7 @@
 typedef std::pair<scoped_ptr<base::SharedMemory>, int32> SharedMemoryAndId;
 
 enum { kNumPictureBuffers = 8 };
+enum { kSyncTimeoutNanosec = 50000000 };
 
 void* omx_handle = NULL;
 
@@ -36,6 +37,21 @@ OMXGetComponentsOfRole omx_get_components_of_role = NULL;
 OMXFreeHandle omx_free_handle = NULL;
 OMXDeinit omx_deinit = NULL;
 
+static PFNEGLCREATESYNCKHRPROC egl_create_sync_khr =
+    reinterpret_cast<PFNEGLCREATESYNCKHRPROC>(
+        eglGetProcAddress("eglCreateSyncKHR"));

[piman, 2012/10/06 06:42:40]

No static initialization. Realize that this code runs in the browser too and is
doing I/O during startup. Realize as well that eglGetProcAddress doesn't exist
on GL - in fact we dynamically load EGL, so this may be NULL.

[Pawel Osciak, 2012/10/10 17:37:52]

Ok, I was inspired by Gles2TextureToEglImageTranslator, which does the same. I
guess we need to fix that one too.

+static PFNEGLCLIENTWAITSYNCKHRPROC egl_client_wait_sync_khr =
+    reinterpret_cast<PFNEGLCLIENTWAITSYNCKHRPROC>(
+        eglGetProcAddress("eglClientWaitSyncKHR"));
+static PFNEGLDESTROYSYNCKHRPROC egl_destroy_sync_khr =
+    reinterpret_cast<PFNEGLDESTROYSYNCKHRPROC>(
+        eglGetProcAddress("eglDestroySyncKHR"));
+
+static bool AreEGLExtensionsInitialized() {
+  return egl_create_sync_khr && egl_client_wait_sync_khr &&
+         egl_destroy_sync_khr;
+}
+
 // Maps h264-related Profile enum values to OMX_VIDEO_AVCPROFILETYPE values.
 static OMX_U32 MapH264ProfileToOMXAVCProfile(uint32 profile) {
   switch (profile) {
@@ -111,7 +127,10 @@ OmxVideoDecodeAccelerator::OmxVideoDecodeAccelerator(
       client_(client),
       codec_(UNKNOWN),
       h264_profile_(OMX_VIDEO_AVCProfileMax),
-      component_name_is_nvidia_h264ext_(false) {
+      component_name_is_nvidia_h264ext_(false),
+      sync_thread_("OVDASyncThread") {
+  RETURN_ON_FAILURE(AreEGLExtensionsInitialized(),
+                    "Failed to load EGL extensions", PLATFORM_FAILURE,);
   static bool omx_functions_initialized = PostSandboxInitialization();
   RETURN_ON_FAILURE(omx_functions_initialized,
                     "Failed to load openmax library", PLATFORM_FAILURE,);
@@ -150,6 +169,8 @@ bool OmxVideoDecodeAccelerator::Initialize(media::VideoCodecProfile profile) {
                       INVALID_ARGUMENT, false);
   }
 
+  CHECK(sync_thread_.Start());
+
   if (!CreateComponent())  // Does its own RETURN_ON_FAILURE dances.
     return false;
 
@@ -387,8 +408,50 @@ void OmxVideoDecodeAccelerator::AssignPictureBuffers(
 }
 
 void OmxVideoDecodeAccelerator::ReusePictureBuffer(int32 picture_buffer_id) {
+  DCHECK_EQ(message_loop_, MessageLoop::current());
   TRACE_EVENT1("Video Decoder", "OVDA::ReusePictureBuffer",
                "Picture id", picture_buffer_id);
+
+  // Create a sync object and insert it into the command stream.
+  EGLSyncKHR egl_sync_obj = egl_create_sync_khr(egl_display_,

[sheu, 2012/10/06 21:54:53]

I'd be concerned about this leaking an EGLSyncKHR if tasks get cancelled
somehow.  This can't happen now, since we let the class destructor destroy the
sync_thread_ (race conditions as piman points out), and all callbacks will
eventually be called.  If you eventually make these callbacks though, you'll
have to wrap the EGLSyncKHR in some kind of ownership-tracked struct to make
sure we don't leak them.

[sheu, 2012/10/06 21:56:00]

I meant: "if you eventually make these callbacks cancelable though..."

[Pawel Osciak, 2012/10/10 17:37:52]

Yes, not happening now, but if we go with PostDelayedTask, we'll need
refcounting.

+                                                EGL_SYNC_FENCE_KHR,
+                                                0);
+  if (egl_sync_obj == EGL_NO_SYNC_KHR) {
+    DLOG(WARNING) << "Could not create EGL sync object.";
+    // If creating the sync object failed, just requeue it, we will lose
+    // the synchronization and could possible have artifacts, but decoding
+    // will continue. QueuePictureBuffer(picture_buffer_id);
+    return;
+  }
+
+  // Inserted the object successfully, wait on the sync_thread_ for it to
+  // become signalled.
+  sync_thread_.message_loop()->PostTask(FROM_HERE, base::Bind(
+        &OmxVideoDecodeAccelerator::WaitForPictureSync, weak_this_,

[piman, 2012/10/06 06:42:40]

You can't pass a weak pointer to another thread. It cannot be made thread-safe
(note: it'll assert in debug).
What you can do is put a pointer to the weak pointer into the closure, and call
a free function on the thread (as well as other params you need in there). That
function can post a task to the main thread where you can then dereference the
weak pointer before calling back into the class.

[Pawel Osciak, 2012/10/10 17:37:52]

I'll just use Unretained and weakptr back. This is always valid while there are
any tasks on sync_thread.

+        picture_buffer_id, egl_sync_obj));
+}
+
+void OmxVideoDecodeAccelerator::WaitForPictureSync(int32 picture_buffer_id,
+                                                   EGLSyncKHR egl_sync_obj) {
+  DCHECK_EQ(sync_thread_.message_loop(), MessageLoop::current());
+
+  EGLint ret = egl_client_wait_sync_khr(egl_display_, egl_sync_obj,

[piman, 2012/10/06 06:42:40]

How do you know it's safe to access egl_display_ without a lock? What if this
gets deleted while this function is running?

[Pawel Osciak, 2012/10/10 17:37:52]

The thread is a member of this, so this can't be deleted with tasks
running/pending.

+                                        EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
+                                        kSyncTimeoutNanosec);

[piman, 2012/10/06 06:42:40]

Why using a timeout at all?
A lock with a timeout is like no lock - imagine if you attach the debugger, or
if the thread isn't scheduled in time because the CPU was busy doing other
things, or whatever. You create races that are hard to repro/debug.
If we're worried about blocking the thread for too long, then we should post a
task to try again, not return success after the failure.

[Pawel Osciak, 2012/10/10 17:37:52]

Well, the idea was to not block the whole thing forever in case something
strange happened. Because otherwise we'd leak/wait forever. But maybe it's a bit
too defensive.
Of course, but I thought it negligible, because then it's just that texture
contents would possibly be overwritten. We didn't care about it until now and
still don't care about it in other VDAs.

On the other hand though I was worried of never returning the buffer to the
decoder and leaking/hanging. But that would happen if the sync object somehow
never got signaled. I wonder if it even makes sense to guard against it. Is it
possible? Could we somehow drop the command stream's contents thus never
signalling this?

+  // If the sync fails, ignore it. Worst case we'll get some artifacts, but
+  // will continue decoding.
+  if (ret != EGL_CONDITION_SATISFIED_KHR)
+    DLOG(WARNING) << "Failed EGL sync.";
+
+  egl_destroy_sync_khr(egl_display_, egl_sync_obj);
+
+  // Post back onto main thread to queue the picture for future use.
+  message_loop_->PostTask(FROM_HERE, base::Bind(

[piman, 2012/10/06 06:42:40]

Same here, accessing message_loop_ isn't safe. Instead, pass a message loop
proxy to the function.

[Pawel Osciak, 2012/10/10 17:37:52]

Passing weak pointer to sync_thread is wrong and you were right to point this
out above, but here it is correct, we want to be passing a weak pointer back to
the main thread (the one it's been created on). This will be valid under all
circumstances (see my previous comment).

+        &OmxVideoDecodeAccelerator::QueuePictureBuffer, weak_this_,
+        picture_buffer_id));
+}
+
+void OmxVideoDecodeAccelerator::QueuePictureBuffer(int32 picture_buffer_id) {
   DCHECK_EQ(message_loop_, MessageLoop::current());
 
   // During port-flushing, do not call OMX FillThisBuffer.