HW Video: Make media::VideoFrame handle the sync point of the compositor as well as webgl

content/renderer/media/webmediaplayer_impl.cc

Index: content/renderer/media/webmediaplayer_impl.cc
diff --git a/content/renderer/media/webmediaplayer_impl.cc b/content/renderer/media/webmediaplayer_impl.cc
index 411b814757cd4c0f737f2fb995bd249ecee8962f..aeb6908c56f27b0f3c8b1f762a02379b9c437882 100644
--- a/content/renderer/media/webmediaplayer_impl.cc
+++ b/content/renderer/media/webmediaplayer_impl.cc
@@ -537,6 +537,11 @@ void WebMediaPlayerImpl::paint(WebCanvas* canvas,
   scoped_refptr<media::VideoFrame> video_frame = painter_.GetCurrentFrame(true);
   gfx::Rect gfx_rect(rect);
   skcanvas_video_renderer_.Paint(video_frame.get(), canvas, gfx_rect, alpha);

[danakj, 2014/02/28 17:49:30]

Should someone be waiting on the video frame's sync point here?

[dshwang, 2014/02/28 18:01:48]

we don't need to wait for the sync point. It's why I remove line 654 also.

GpuVideoDecoder doesn't insert a sync point when it creates the video frame.
  scoped_refptr<VideoFrame> frame(VideoFrame::WrapNativeTexture(
      make_scoped_ptr(new gpu::MailboxHolder(
          pb.texture_mailbox(), decoder_texture_target_, 0 /* sync_point */)),
      BindToCurrentLoop(base::Bind(&GpuVideoDecoder::ReusePictureBuffer,
                                   weak_this_,
                                   picture.picture_buffer_id())),
      pb.size(),
      visible_rect,
      natural_size,
      timestamp,
      base::Bind(&ReadPixelsSync, factories_, pb.texture_id(), visible_rect)));

As I mentioned in CL description, at the time to create the video frame, gpu
process doesn't have any pending GPU operations for the video frame, because the
video frame is originated from gpu process.
It's why GpuVideoDecoder doesn't insert a sync point. All clients can trust the
video frame exists in gpu process.

[danakj, 2014/02/28 18:41:19]

All GL is actually run on the gpu process so I'm not sure what you mean here.
The sync points are to synchronize between different command buffers using the
gpu process.

Also there is this sync point that needs to be waited on to ensure the mailbox
is valid/created:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/m...

+  if (video_frame &&
+      video_frame->format() == media::VideoFrame::NATIVE_TEXTURE) {
+    DCHECK(gpu_factories_);
+    video_frame->SetReleaseSyncPoint(gpu_factories_->InsertSyncPoint());
+  }
 }
 
 bool WebMediaPlayerImpl::hasSingleSecurityOrigin() const {
@@ -651,7 +656,6 @@ bool WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(
 
   uint32 source_texture = web_graphics_context->createTexture();
 
-  web_graphics_context->waitSyncPoint(mailbox_holder->sync_point);

[danakj, 2014/02/28 17:49:30]

Why is this gone? This sync point ensures the mailbox is valid when we try to
consume it here, and the contents of the texture are correct before we execute
the copy.

   web_graphics_context->bindTexture(GL_TEXTURE_2D, source_texture);
   web_graphics_context->consumeTextureCHROMIUM(GL_TEXTURE_2D,
                                                mailbox_holder->mailbox.name);
@@ -683,6 +687,12 @@ bool WebMediaPlayerImpl::copyVideoTextureToPlatformTexture(
   // The flush() operation is not necessary here. It is kept since the
   // performance will be better when it is added than not.
   web_graphics_context->flush();
+  // WebGL doesn't need to wait for the sync point of |video_frame| like
+  // paint(...) because |video_frame| is updated by the gpu process.
+  // However, it's necessary to insert a sync point because gpu video decoder
+  // in the render process must make sure executing all pending commands of
+  // WebGL before notifying reusable mailboxes to the gpu process.
+  video_frame->SetReleaseSyncPoint(web_graphics_context->insertSyncPoint());

[danakj, 2014/02/28 17:49:30]

While this solves the problem of two threads writing the same variable, I don't
see how this solves the problem of clobbering the sync point. Both cc/ and here
could write to this sync point variable, and in the end you'll only have 1 sync
point get waited on. But you need both to be waited on. If you're going to
overwrite it you have to wait on the existing one first (with a lock around both
operations).

[dshwang, 2014/02/28 18:01:48]

you're right. GpuVideoDecoder will wait for only one sync point. it's issue. let
me think..

[danakj, 2014/02/28 18:41:19]

That will work if and only if the person doing the swapping does an insert after
doing the wait. So you can't give a sync point at the time of calling swap.

   return true;
 }