Monitor VideoResourceUpdater reusing destructed resource in Debug mode.

cc/resources/video_resource_updater.cc

Index: cc/resources/video_resource_updater.cc
diff --git a/cc/resources/video_resource_updater.cc b/cc/resources/video_resource_updater.cc
index 46dbbc95e4e0cc6999f121afe6f9f7dff515444a..9aaae1a1c60150448e1968b8500e5ad74386b9f6 100644
--- a/cc/resources/video_resource_updater.cc
+++ b/cc/resources/video_resource_updater.cc
@@ -23,6 +23,11 @@
 #include "third_party/khronos/GLES2/gl2ext.h"
 #include "ui/gfx/geometry/size_conversions.h"
 
+#if !defined(NDEBUG)
+#include "base/single_thread_task_runner.h"
+#include "base/thread_task_runner_handle.h"
+#endif
+
 namespace cc {
 
 namespace {
@@ -112,6 +117,14 @@ class SyncTokenClientImpl : public media::VideoFrame::SyncTokenClient {
   gpu::SyncToken sync_token_;
 };
 
+#if !defined(NDEBUG)
+void OnVideoFrameDestruct(
+    const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
+    const base::Closure& task) {
+  task_runner->PostTask(FROM_HERE, task);
+}
+#endif
+
 }  // namespace
 
 VideoResourceUpdater::PlaneResource::PlaneResource(
@@ -125,6 +138,9 @@ VideoResourceUpdater::PlaneResource::PlaneResource(
       mailbox(mailbox),
       ref_count(0),
       frame_ptr(nullptr),
+#if !defined(NDEBUG)
+      destructed(false),
+#endif
       plane_index(0u) {
 }
 
@@ -135,9 +151,17 @@ bool VideoResourceUpdater::PlaneResourceMatchesUniqueID(
     const PlaneResource& plane_resource,
     const media::VideoFrame* video_frame,
     size_t plane_index) {
-  return plane_resource.frame_ptr == video_frame &&
-         plane_resource.plane_index == plane_index &&
-         plane_resource.timestamp == video_frame->timestamp();
+  bool matched = plane_resource.frame_ptr == video_frame &&
+                 plane_resource.plane_index == plane_index &&
+                 plane_resource.timestamp == video_frame->timestamp();
+#if !defined(NDEBUG)
+  if ((plane_index == 0) && matched) {
+    DCHECK(!plane_resource.destructed)

[danakj, 2016/03/14 18:52:41]

Can you maybe just add a Lock to the struct when dcheck is on, and avoid
introducing task runners to this class?

Compositor receives task runners from its embedders, and I don't think we should
use ThreadTaskRunnerHandle in here, we'd need to plumb those thru and that's
maybe unnecessary?

[xjz, 2016/03/15 21:04:00]

Using Lock needs to add Lock on |all_resource_| anywhere it is used. Does it
look good? 

Maybe better to get the compositor's task runners when DCHECK is on? Where to
get it?

+        << "ERROR: reused the destructed resource."
+        << " timestamp = " << plane_resource.timestamp;
+  }
+#endif
+  return matched;
 }
 
 void VideoResourceUpdater::SetPlaneResourceUniqueId(
@@ -147,6 +171,9 @@ void VideoResourceUpdater::SetPlaneResourceUniqueId(
   plane_resource->frame_ptr = video_frame;
   plane_resource->plane_index = plane_index;
   plane_resource->timestamp = video_frame->timestamp();
+#if !defined(NDEBUG)
+  plane_resource->destructed = false;
+#endif
 }
 
 VideoFrameExternalResources::VideoFrameExternalResources()
@@ -394,6 +421,14 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
       // by software.
       video_renderer_->Copy(video_frame, &canvas, media::Context3D());
       SetPlaneResourceUniqueId(video_frame.get(), 0, &plane_resource);
+#if !defined(NDEBUG)
+      // Add VideoFrame destructor callback.
+      video_frame->AddDestructionObserver(base::Bind(
+          &OnVideoFrameDestruct, base::ThreadTaskRunnerHandle::Get(),
+          base::Bind(&VideoResourceUpdater::MarkOldResource, AsWeakPtr(),
+                     static_cast<void*>(video_frame.get()),
+                     video_frame->timestamp())));
+#endif
     }
 
     external_resources.software_resources.push_back(plane_resource.resource_id);
@@ -489,6 +524,16 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
       resource_provider_->CopyToResource(plane_resource.resource_id, pixels,
                                          resource_size_pixels);
       SetPlaneResourceUniqueId(video_frame.get(), i, &plane_resource);
+#if !defined(NDEBUG)
+      // Add VideoFrame destructor callback.
+      if (i == 0) {
+        video_frame->AddDestructionObserver(base::Bind(

[danakj, 2016/03/14 18:52:41]

Since you're doing this at each call to SetPlaneResourceUniqueID, maybe just
call this inside there?

[xjz, 2016/03/15 21:04:00]

In that way, we have to add a parameter to SetPlaneResourceUniqueID() as it is a
static function, and the callback MarkOldResource() uses member from
VideoResourceUpdater. Since this change only takes effect when DCHECK is on, it
might be unnecessary to change the definition of SetPlaneResourceUniqueID()?

[danakj, 2016/03/17 00:00:34]

OK. I feel like there's some refactoring needs building up in here around
allocating, finding, reusing resources. We're getting a pretty long and
complicated single function here.

+            &OnVideoFrameDestruct, base::ThreadTaskRunnerHandle::Get(),
+            base::Bind(&VideoResourceUpdater::MarkOldResource, AsWeakPtr(),
+                       static_cast<void*>(video_frame.get()),
+                       video_frame->timestamp())));
+      }
+#endif
     }
 
     if (plane_resource.resource_format == LUMINANCE_F16) {
@@ -693,4 +738,26 @@ void VideoResourceUpdater::RecycleResource(
   DCHECK_GE(resource_it->ref_count, 0);
 }
 
+#if !defined(NDEBUG)
+// static
+void VideoResourceUpdater::MarkOldResource(
+    base::WeakPtr<VideoResourceUpdater> updater,
+    const void* video_frame_ptr,

[danakj, 2016/03/14 18:52:41]

why void?

[xjz, 2016/03/15 21:04:00]

Because it is not allowed to bine a ref counted ptr to the callback, and the
function only uses the raw pointer of the |video_frame| to compare. 

+    base::TimeDelta timestamp) {
+  if (!updater)
+    return;
+  const ResourceList::iterator resource_it = std::find_if(
+      updater->all_resources_.begin(), updater->all_resources_.end(),
+      [video_frame_ptr, timestamp](const PlaneResource& plane_resource) {
+        return plane_resource.frame_ptr == video_frame_ptr &&
+               plane_resource.timestamp == timestamp &&
+               plane_resource.plane_index == 0;
+      });
+  if (resource_it == updater->all_resources_.end())
+    return;
+
+  resource_it->destructed = true;
+}
+#endif
+
 }  // namespace cc