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

content/renderer/media/webmediaplayer_impl.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/webmediaplayer_impl.h"
 
 #include <algorithm>
 #include <limits>
 #include <string>
 #include <vector>
@@ skipping 518 matching lines @@
     // video frames are still rendered to a canvas.
     UMA_HISTOGRAM_BOOLEAN(
         "Media.AcceleratedCompositingActive",
         frame_->view()->isAcceleratedCompositingActive());
   }
 
 
   TRACE_EVENT0("media", "WebMediaPlayerImpl:paint");
   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 {
   if (data_source_)
     return data_source_->HasSingleOrigin();
   return true;
 }
 
 bool WebMediaPlayerImpl::didPassCORSAccessCheck() const {
   if (data_source_)
@@ skipping 94 matching lines @@
   // bound when this method is called, and only verify this fact when
   // DCHECK_IS_ON.
   if (DCHECK_IS_ON()) {
     GLint bound_texture = 0;
     web_graphics_context->getIntegerv(GL_TEXTURE_BINDING_2D, &bound_texture);
     DCHECK_EQ(static_cast<GLuint>(bound_texture), texture);
   }
 
   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);
 
   // The video is stored in a unmultiplied format, so premultiply
   // if necessary.
   web_graphics_context->pixelStorei(GL_UNPACK_PREMULTIPLY_ALPHA_CHROMIUM,
                                     premultiply_alpha);
   // Application itself needs to take care of setting the right flip_y
   // value down to get the expected result.
@@ skipping 11 matching lines @@
                                     false);
 
   // Restore the state for TEXTURE_2D binding point as mentioned above.
   web_graphics_context->bindTexture(GL_TEXTURE_2D, texture);
 
   web_graphics_context->deleteTexture(source_texture);
 
   // 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;
 }
 
 // Helper functions to report media EME related stats to UMA. They follow the
 // convention of more commonly used macros UMA_HISTOGRAM_ENUMERATION and
 // UMA_HISTOGRAM_COUNTS. The reason that we cannot use those macros directly is
 // that UMA_* macros require the names to be constant throughout the process'
 // lifetime.
 static void EmeUMAHistogramEnumeration(const std::string& key_system,
                                        const std::string& method,
@@ skipping 620 matching lines @@
 
   if (web_cdm_) {
     decryptor_ready_cb.Run(web_cdm_->GetDecryptor());
     return;
   }
 
   decryptor_ready_cb_ = decryptor_ready_cb;
 }
 
 }  // namespace content