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

media/base/video_frame.cc

 // Copyright (c) 2012 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 "media/base/video_frame.h"
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 88 matching lines @@
     const ReadPixelsCB& read_pixels_cb) {
   scoped_refptr<VideoFrame> frame(new VideoFrame(NATIVE_TEXTURE,
                                                  coded_size,
                                                  visible_rect,
                                                  natural_size,
                                                  timestamp,
                                                  false));
   frame->mailbox_holder_ = mailbox_holder.Pass();
   frame->mailbox_holder_release_cb_ = mailbox_holder_release_cb;
   frame->read_pixels_cb_ = read_pixels_cb;
+#ifndef NDEBUG
+  frame->debug_initial_sync_point_ = frame->mailbox_holder_->sync_point;
+#endif
 
   return frame;
 }
 
 void VideoFrame::ReadPixelsFromNativeTexture(const SkBitmap& pixels) {
   DCHECK_EQ(format_, NATIVE_TEXTURE);
   if (!read_pixels_cb_.is_null())
     read_pixels_cb_.Run(pixels);
 }
 
@@ skipping 280 matching lines @@
 VideoFrame::VideoFrame(VideoFrame::Format format,
                        const gfx::Size& coded_size,
                        const gfx::Rect& visible_rect,
                        const gfx::Size& natural_size,
                        base::TimeDelta timestamp,
                        bool end_of_stream)
     : format_(format),
       coded_size_(coded_size),
       visible_rect_(visible_rect),
       natural_size_(natural_size),
+#ifndef NDEBUG
+      debug_initial_sync_point_(0),
+#endif
       shared_memory_handle_(base::SharedMemory::NULLHandle()),
       timestamp_(timestamp),
       end_of_stream_(end_of_stream) {
   memset(&strides_, 0, sizeof(strides_));
   memset(&data_, 0, sizeof(data_));
 }
 
 VideoFrame::~VideoFrame() {
   if (!mailbox_holder_release_cb_.is_null()) {
-    base::ResetAndReturn(&mailbox_holder_release_cb_)
-        .Run(mailbox_holder_.Pass());
+    std::vector<uint32> release_sync_points;
+    {
+      base::AutoLock locker(release_sync_point_lock_);

[Ami GONE FROM CHROMIUM, 2014/04/11 20:55:17]

locking in a dtor is a code-smell.  If anything can race with the dtor then
losing the race means it will read deleted memory.

Since VideoFrame is RefCountedThreadSafe I believe this lock (and the swap
below) is unnecessary.

[dshwang, 2014/04/22 19:16:51]

I reserve this code as I explained.
IMO ensuring thread visibility via piggy-back is more difficult to maintain.

+      release_sync_points_.swap(release_sync_points);
+    }
+#ifndef NDEBUG
+    // VideoFrame doesn't use |mailbox_holder_->sync_point| as release sync
+    // point because VideoFrame can have multiple clients. i.e. the compositor,
+    // webgl. So VideoFrame uses |release_sync_points_| unlike webgl.
+    // mailbox_holder_->sync_point must be not changed by clients.
+    DCHECK_EQ(debug_initial_sync_point_, mailbox_holder_->sync_point);

[Ami GONE FROM CHROMIUM, 2014/04/11 20:55:17]

This would be even easier to guarantee if nobody had the ability to modify
mailbox_holder_->sync_point, which would be the case if mailbox_holder() died. 
Can you delete that accessor in favor of direct (const!) accessors to
*mailbox_holder_'s members?

Then debug_initial_sync_point_ would no longer be needed (assuming I'm right
about l.436 below also being deletable because it's enforcing a contract that
nobody guarantees).

[dshwang, 2014/04/22 19:16:51]

That's good idea. I make mailbox_holder const. And this change requires changing
VideoFrame ctor little bit.

+    for (size_t i = 0; i < release_sync_points.size(); i++)
+      DCHECK_NE(debug_initial_sync_point_, release_sync_points[i]);

[Ami GONE FROM CHROMIUM, 2014/04/11 20:55:17]

What guarantees that wrap-around doesn't hit this?

+#endif
+    base::ResetAndReturn(&mailbox_holder_release_cb_).Run(release_sync_points);
   }
   if (!no_longer_needed_cb_.is_null())
     base::ResetAndReturn(&no_longer_needed_cb_).Run();
 }
 
 bool VideoFrame::IsValidPlane(size_t plane) const {
   return (plane < NumPlanes(format_));
 }
 
 int VideoFrame::stride(size_t plane) const {
@@ skipping 60 matching lines @@
 
 gpu::MailboxHolder* VideoFrame::mailbox_holder() const {
   DCHECK_EQ(format_, NATIVE_TEXTURE);
   return mailbox_holder_.get();
 }
 
 base::SharedMemoryHandle VideoFrame::shared_memory_handle() const {
   return shared_memory_handle_;
 }
 
+void VideoFrame::AppendReleaseSyncPoint(uint32 sync_point) {
+  DCHECK_EQ(format_, NATIVE_TEXTURE);
+  if (!sync_point)

[Ami GONE FROM CHROMIUM, 2014/04/11 20:55:17]

OOC where does it say that a sync_point of zero means "no sync point"?
(I'm a relative n00b to the world of mailboxes)

[danakj, 2014/04/14 13:19:31]

Hm, it's a convention we follow to return 0 when we want to say "didn't use it,
just use the old sync point"

Maybe we should add this to
./gpu/GLES2/extensions/CHROMIUM/CHROMIUM_sync_point.txt

[dshwang, 2014/04/22 19:16:51]

Thank you for kind explanation. I agree, so I reserve this code.

+    return;
+  base::AutoLock locker(release_sync_point_lock_);
+  release_sync_points_.push_back(sync_point);
+}
+
 void VideoFrame::HashFrameForTesting(base::MD5Context* context) {
   for (int plane = 0; plane < kMaxPlanes; ++plane) {
     if (!IsValidPlane(plane))
       break;
     for (int row = 0; row < rows(plane); ++row) {
       base::MD5Update(context, base::StringPiece(
           reinterpret_cast<char*>(data(plane) + stride(plane) * row),
           row_bytes(plane)));
     }
   }
 }
 
 }  // namespace media