Monitor VideoResourceUpdater reusing destructed resource in Debug mode.

VideoResourceUpdater software planes use VideoFrame raw pointer and
video timestamp as the unique ID for each VideoFrame. However, if the
VideoFrame is destructed and the pointer is reused by a new VideoFrame,
and if the timestamp is not correctly set, the resource maybe
incorrectly reused. This CL marks the resource as destructed when the
original VideoFrame is destructed, and monitors if the destructed
resource is re-used. The CL only takes effect in Debug mode.

BUG=581480
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Review URL: https://codereview.chromium.org/1688033005

Cr-Commit-Position: refs/heads/master@{#381739}

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 DCHECK_IS_ON()
+#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 DCHECK_IS_ON()
+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 DCHECK_IS_ON()
+      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 DCHECK_IS_ON()
+  if ((plane_index == 0) && matched) {
+    DCHECK(!plane_resource.destructed)
+        << "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 DCHECK_IS_ON()
+  plane_resource->destructed = false;
+#endif
 }
 
 VideoFrameExternalResources::VideoFrameExternalResources()
@@ -393,6 +420,14 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
       // by software.
       video_renderer_->Copy(video_frame, &canvas, media::Context3D());
       SetPlaneResourceUniqueId(video_frame.get(), 0, &plane_resource);
+#if DCHECK_IS_ON()
+      // Add VideoFrame destructor callback.
+      video_frame->AddDestructionObserver(base::Bind(
+          &OnVideoFrameDestruct, base::ThreadTaskRunnerHandle::Get(),
+          base::Bind(&VideoResourceUpdater::MarkOldResource, AsWeakPtr(),
+                     base::Unretained(video_frame.get()),
+                     video_frame->timestamp())));
+#endif
     }
 
     external_resources.software_resources.push_back(plane_resource.resource_id);
@@ -488,6 +523,16 @@ VideoFrameExternalResources VideoResourceUpdater::CreateForSoftwarePlanes(
       resource_provider_->CopyToResource(plane_resource.resource_id, pixels,
                                          resource_size_pixels);
       SetPlaneResourceUniqueId(video_frame.get(), i, &plane_resource);
+#if DCHECK_IS_ON()
+      // Add VideoFrame destructor callback.
+      if (i == 0) {
+        video_frame->AddDestructionObserver(base::Bind(
+            &OnVideoFrameDestruct, base::ThreadTaskRunnerHandle::Get(),
+            base::Bind(&VideoResourceUpdater::MarkOldResource, AsWeakPtr(),
+                       base::Unretained(video_frame.get()),
+                       video_frame->timestamp())));
+      }
+#endif
     }
 
     if (plane_resource.resource_format == LUMINANCE_F16) {
@@ -696,4 +741,26 @@ void VideoResourceUpdater::RecycleResource(
   DCHECK_GE(resource_it->ref_count, 0);
 }
 
+#if DCHECK_IS_ON()
+// static
+void VideoResourceUpdater::MarkOldResource(
+    base::WeakPtr<VideoResourceUpdater> updater,
+    const media::VideoFrame* video_frame_ptr,
+    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

cc/resources/video_resource_updater.h

@@ -99,6 +99,14 @@ class CC_EXPORT VideoResourceUpdater
     // frame pointer will only be used for pointer comparison, i.e. the
     // underlying data will not be accessed.
     const void* frame_ptr;
+#if DCHECK_IS_ON()
+    // This is marked true when the orginal VideoFrame is destructed. It is
+    // used to detect clients that are not setting the VideoFrame's timestamp
+    // field correctly, as required. The memory allocator can and will re-use
+    // the same pointer for new VideoFrame instances, so a destruction observer
+    // is used to detect that.
+    bool destructed;
+#endif
     size_t plane_index;
     base::TimeDelta timestamp;
 
@@ -143,6 +151,12 @@ class CC_EXPORT VideoResourceUpdater
                             const gpu::SyncToken& sync_token,
                             bool lost_resource,
                             BlockingTaskRunner* main_thread_task_runner);
+#if DCHECK_IS_ON()
+  // Mark the |destructed| as true when the orginal VideoFrame is destructed.
+  static void MarkOldResource(base::WeakPtr<VideoResourceUpdater> updater,
+                              const media::VideoFrame* video_frame_ptr,
+                              base::TimeDelta timestamp);
+#endif
 
   ContextProvider* context_provider_;
   ResourceProvider* resource_provider_;