Android media: VideoFrame should not store so many sync points.

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 632 matching lines @@
                        scoped_ptr<gpu::MailboxHolder> mailbox_holder,
                        base::TimeDelta timestamp,
                        bool end_of_stream)
     : format_(format),
       coded_size_(coded_size),
       visible_rect_(visible_rect),
       natural_size_(natural_size),
       mailbox_holder_(mailbox_holder.Pass()),
       shared_memory_handle_(base::SharedMemory::NULLHandle()),
       timestamp_(timestamp),
+      release_sync_point_(0),
       end_of_stream_(end_of_stream) {
   DCHECK(IsValidConfig(format_, coded_size_, visible_rect_, natural_size_));
 
   memset(&strides_, 0, sizeof(strides_));
   memset(&data_, 0, sizeof(data_));
 }
 
 VideoFrame::~VideoFrame() {
   if (!mailbox_holder_release_cb_.is_null()) {
-    std::vector<uint32> release_sync_points;
+    uint32 release_sync_point;
     {
+      // To ensure |release_sync_point_| is visible on this thread.
       base::AutoLock locker(release_sync_point_lock_);

[no sievers, 2014/07/10 18:52:29]

Why do we need a lock in the destructor? Somebody might access this object while
it's being destructed?

[danakj, 2014/07/10 19:15:57]

Thread barrier, someone may have changed the value on another thread that you're
running the destructor on, and you need to get that value. The lock ensures you
do.

[dshwang, 2014/07/10 19:23:11]

Thank you for answer!

[no sievers, 2014/07/10 19:27:47]

Ah thanks. I guess the comment says that already. If you change it to something
like "To ensure that changes to |release_sync_point_| are visible on this thread
(imply a memory barrier)" then even I will understand it :)

-      release_sync_points_.swap(release_sync_points);
+      release_sync_point = release_sync_point_;
     }
-    base::ResetAndReturn(&mailbox_holder_release_cb_).Run(release_sync_points);
+    base::ResetAndReturn(&mailbox_holder_release_cb_).Run(release_sync_point);
   }
   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 131 matching lines @@
 
 const 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) {
+void VideoFrame::UpdateReleaseSyncPoint(SyncPointClient* client) {
   DCHECK_EQ(format_, NATIVE_TEXTURE);
-  if (!sync_point)
-    return;
   base::AutoLock locker(release_sync_point_lock_);
-  release_sync_points_.push_back(sync_point);
+  // Must wait the previous sync point before inserting a new sync point so that
+  // |mailbox_holder_release_cb_| guarantees the previous sync point occurred
+  // when it waits |release_sync_point_|.
+  if (release_sync_point_)
+    client->WaitSyncPoint(release_sync_point_);
+  release_sync_point_ = client->InsertSyncPoint();
 }
 
 #if defined(OS_POSIX)
 int VideoFrame::dmabuf_fd(size_t plane) const {
   return dmabuf_fds_[plane].get();
 }
 #endif
 
 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