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

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/renderer_gpu_video_accelerator_factories.h"
 
 #include <GLES2/gl2.h>
 #include <GLES2/gl2ext.h>
 
 #include "base/bind.h"
 #include "content/child/child_thread.h"
 #include "content/common/gpu/client/context_provider_command_buffer.h"
 #include "content/common/gpu/client/gpu_channel_host.h"
 #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"
 
 namespace content {
 
 // static
 scoped_refptr<RendererGpuVideoAcceleratorFactories>
 RendererGpuVideoAcceleratorFactories::Create(
     GpuChannelHost* gpu_channel_host,
     const scoped_refptr<base::MessageLoopProxy>& message_loop_proxy,
@@ skipping 133 matching lines @@
   gpu::gles2::GLES2Implementation* gles2 = context->GetImplementation();
   gles2->WaitSyncPointCHROMIUM(sync_point);
 
   // Callers expect the WaitSyncPoint to affect the next IPCs. Make sure to
   // flush the command buffers to ensure that.
   gles2->ShallowFlushCHROMIUM();
 }
 
 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());
 
   WebGraphicsContext3DCommandBufferImpl* context = GetContext3d();
   if (!context)
     return;
 
   gpu::gles2::GLES2Implementation* gles2 = context->GetImplementation();
+  gles2->WaitSyncPointCHROMIUM(video_frame->mailbox_holder()->sync_point);
 
   GLuint tmp_texture;
   gles2->GenTextures(1, &tmp_texture);
   gles2->BindTexture(GL_TEXTURE_2D, tmp_texture);
   gles2->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MIN_FILTER, GL_LINEAR);
   gles2->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_MAG_FILTER, GL_LINEAR);
   gles2->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE);
   gles2->TexParameteri(GL_TEXTURE_2D, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE);
   context->copyTextureCHROMIUM(
       GL_TEXTURE_2D, texture_id, tmp_texture, 0, GL_RGBA, GL_UNSIGNED_BYTE);
 
   GLuint fb;
   gles2->GenFramebuffers(1, &fb);
   gles2->BindFramebuffer(GL_FRAMEBUFFER, fb);
   gles2->FramebufferTexture2D(
       GL_FRAMEBUFFER, GL_COLOR_ATTACHMENT0, GL_TEXTURE_2D, tmp_texture, 0);
   gles2->PixelStorei(GL_PACK_ALIGNMENT, 4);
 #if SK_B32_SHIFT == 0 && SK_G32_SHIFT == 8 && SK_R32_SHIFT == 16 && \
     SK_A32_SHIFT == 24
   GLenum skia_format = GL_BGRA_EXT;
 #elif SK_R32_SHIFT == 0 && SK_G32_SHIFT == 8 && SK_B32_SHIFT == 16 && \
     SK_A32_SHIFT == 24
   GLenum skia_format = GL_RGBA;
 #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(),
                     visible_rect.height(),
                     skia_format,
                     GL_UNSIGNED_BYTE,
                     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));
 }
 
 base::SharedMemory* RendererGpuVideoAcceleratorFactories::CreateSharedMemory(
     size_t size) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   return ChildThread::AllocateSharedMemory(size, thread_safe_sender_.get());
 }
 
 scoped_refptr<base::SingleThreadTaskRunner>
 RendererGpuVideoAcceleratorFactories::GetTaskRunner() {
   return task_runner_;
 }
 
 }  // namespace content