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

content/renderer/media/renderer_gpu_video_accelerator_factories.cc

Index: content/renderer/media/renderer_gpu_video_accelerator_factories.cc
diff --git a/content/renderer/media/renderer_gpu_video_accelerator_factories.cc b/content/renderer/media/renderer_gpu_video_accelerator_factories.cc
index 02fd859d0cc1f3376cc971185f657ce9af98ff0c..826fdbfbbba1bbe23cf4da6939b7c4dc075af449 100644
--- a/content/renderer/media/renderer_gpu_video_accelerator_factories.cc
+++ b/content/renderer/media/renderer_gpu_video_accelerator_factories.cc
@@ -14,6 +14,7 @@
 #include "content/common/gpu/client/webgraphicscontext3d_command_buffer_impl.h"
 #include "content/renderer/render_thread_impl.h"
 #include "gpu/command_buffer/client/gles2_implementation.h"
+#include "gpu/command_buffer/common/mailbox_holder.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkPixelRef.h"
 
@@ -167,7 +168,7 @@ void RendererGpuVideoAcceleratorFactories::WaitSyncPoint(uint32 sync_point) {
 
 void RendererGpuVideoAcceleratorFactories::ReadPixels(
     uint32 texture_id,
-    const gfx::Rect& visible_rect,
+    const scoped_refptr<media::VideoFrame>& video_frame,

[danakj, 2014/03/06 19:31:31]

Is it a bit weird that this method takes a texture_id and a whole VideoFrame and
then makes an assumption that the video frame's backed by the given texture_id?
Shouldn't this assumption be done at another level that actually knows
this/generates the texture_id? And this method would just take a sync point if
needed?

[dshwang, 2014/03/06 19:51:37]

That's very good point. This method is used by gpu video decoder in the media
thread, which made the video frame.
It's why providing texture_id is possible.

Actually, this method does not need wait/insert sync point because it runs in
the media thread.

I'll rollback the change in this method, and add this explanation as comment.

     const SkBitmap& pixels) {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
@@ -176,6 +177,7 @@ void RendererGpuVideoAcceleratorFactories::ReadPixels(
     return;
 
   gpu::gles2::GLES2Implementation* gles2 = context->GetImplementation();
+  gles2->WaitSyncPointCHROMIUM(video_frame->mailbox_holder()->sync_point);
 
   GLuint tmp_texture;
   gles2->GenTextures(1, &tmp_texture);
@@ -202,6 +204,7 @@ void RendererGpuVideoAcceleratorFactories::ReadPixels(
 #else
 #error Unexpected Skia ARGB_8888 layout!
 #endif
+  gfx::Rect visible_rect = video_frame->visible_rect();

[danakj, 2014/03/06 19:31:31]

Unrelated change, do this separately?

   gles2->ReadPixels(visible_rect.x(),
                     visible_rect.y(),
                     visible_rect.width(),
@@ -211,6 +214,8 @@ void RendererGpuVideoAcceleratorFactories::ReadPixels(
                     pixels.pixelRef()->pixels());
   gles2->DeleteFramebuffers(1, &fb);
   gles2->DeleteTextures(1, &tmp_texture);
+  gles2->Flush();

[danakj, 2014/03/06 19:31:31]

Why is this flush needed?

[dshwang, 2014/03/06 19:51:37]

I use flush as idiom. As I mentioned above, I'll rollback this change in this
method.

+  video_frame->AppendReleaseSyncPoint(gles2->InsertSyncPointCHROMIUM());
   DCHECK_EQ(gles2->GetError(), static_cast<GLenum>(GL_NO_ERROR));
 }