Monitor VideoResourceUpdater reusing destructed resource in Debug mode.

cc/resources/video_resource_updater.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 "cc/resources/video_resource_updater.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <algorithm>
 
 #include "base/bind.h"
 #include "base/bit_cast.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/base/math_util.h"
 #include "cc/output/gl_renderer.h"
 #include "cc/resources/resource_provider.h"
 #include "gpu/GLES2/gl2extchromium.h"
 #include "gpu/command_buffer/client/gles2_interface.h"
 #include "media/base/video_frame.h"
 #include "media/renderers/skcanvas_video_renderer.h"
 #include "third_party/khronos/GLES2/gl2.h"
 #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 {
 
 const ResourceFormat kRGBResourceFormat = RGBA_8888;
 
 VideoFrameExternalResources::ResourceType ResourceTypeForVideoFrame(
     media::VideoFrame* video_frame) {
   switch (video_frame->format()) {
     case media::PIXEL_FORMAT_ARGB:
@@ skipping 69 matching lines @@
         sync_token_.Clear();
       }
     }
   }
 
  private:
   gpu::gles2::GLES2Interface* gl_;
   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(
     unsigned int resource_id,
     const gfx::Size& resource_size,
     ResourceFormat resource_format,
     gpu::Mailbox mailbox)
     : resource_id(resource_id),
       resource_size(resource_size),
       resource_format(resource_format),
       mailbox(mailbox),
       ref_count(0),
       frame_ptr(nullptr),
+#if !defined(NDEBUG)
+      destructed(false),
+#endif
       plane_index(0u) {
 }
 
 VideoResourceUpdater::PlaneResource::PlaneResource(const PlaneResource& other) =
     default;
 
 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(
     const media::VideoFrame* video_frame,
     size_t plane_index,
     PlaneResource* plane_resource) {
   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()
     : type(NONE),
       read_lock_fences_enabled(false),
       offset(0.0f),
       multiplier(1.0f) {}
 
 VideoFrameExternalResources::VideoFrameExternalResources(
     const VideoFrameExternalResources& other) = default;
@@ skipping 227 matching lines @@
       if (!video_renderer_)
         video_renderer_.reset(new media::SkCanvasVideoRenderer);
 
       ResourceProvider::ScopedWriteLockSoftware lock(
           resource_provider_, plane_resource.resource_id);
       SkCanvas canvas(lock.sk_bitmap());
       // This is software path, so canvas and video_frame are always backed
       // 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);
     external_resources.software_release_callback =
         base::Bind(&RecycleResource, AsWeakPtr(), plane_resource.resource_id);
     external_resources.type = VideoFrameExternalResources::SOFTWARE_RESOURCE;
     return external_resources;
   }
 
   for (size_t i = 0; i < plane_resources.size(); ++i) {
@@ skipping 75 matching lines @@
                 video_frame->data(i) + (video_stride_bytes * row);
             memcpy(dst, src, bytes_per_row);
           }
         }
         pixels = &upload_pixels_[0];
       }
 
       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) {
       // By OR-ing with 0x3800, 10-bit numbers become half-floats in the
       // range [0.5..1) and 9-bit numbers get the range [0.5..0.75).
       //
       // Half-floats are evaluated as:
       // float value = pow(2.0, exponent - 25) * (0x400 + fraction);
       //
       // In our case the exponent is 14 (since we or with 0x3800) and
@@ skipping 184 matching lines @@
   if (lost_resource) {
     resource_it->ref_count = 0;
     updater->DeleteResource(resource_it);
     return;
   }
 
   --resource_it->ref_count;
   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