Rewrite WebContentsVideoCaptureDeviceTest.

content/browser/renderer_host/media/web_contents_video_capture_device.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.
 //
 // Implementation notes: This needs to work on a variety of hardware
 // configurations where the speed of the CPU and GPU greatly affect overall
 // performance.  Therefore, the process of capturing has been split up into a
 // pipeline of three stages.  Each stage executes on its own thread:
 //
 //   1. Capture: A bitmap is snapshotted/copied from the RenderView's backing
@@ skipping 55 matching lines @@
 #include "content/public/browser/web_contents_observer.h"
 #include "media/base/bind_to_loop.h"
 #include "media/base/video_frame.h"
 #include "media/video/capture/video_capture_types.h"
 #include "skia/ext/image_operations.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkColor.h"
 #include "ui/gfx/rect.h"
 #include "ui/gfx/skia_util.h"
 
-// Used to self-trampoline invocation of methods to the approprate thread.  This
+// Used to self-trampoline invocation of methods to the appropriate thread. This
 // should be used sparingly, only when it's not clear which thread is invoking a
 // method.
 #define ENSURE_INVOKED_ON_THREAD(thread, ...) {  \
   DCHECK(thread.IsRunning());  \
   if (MessageLoop::current() != thread.message_loop()) {  \
     thread.message_loop()->PostTask(FROM_HERE, base::Bind(__VA_ARGS__));  \
     return;  \
   }  \
 }
 
@@ skipping 52 matching lines @@
 }
 
 // Keeps track of the RenderView to be sourced, and executes copying of the
 // backing store on the UI BrowserThread.
 class BackingStoreCopier : public WebContentsObserver {
  public:
   BackingStoreCopier(int render_process_id, int render_view_id);
 
   virtual ~BackingStoreCopier();
 
-  // If non-NULL, use the given |override| to access the backing store.
-  // This is used for unit testing.
-  void SetRenderWidgetHostForTesting(RenderWidgetHost* override);
-
   // Starts the copy from the backing store. Must be run on the UI
   // BrowserThread. Resulting frame is conveyed back to |consumer|.
   void StartCopy(const scoped_refptr<CaptureMachine>& consumer,
                  int frame_number,
                  int desired_width,
                  int desired_height);
 
   // Stops observing an existing WebContents instance, if any.  This must be
   // called before BackingStoreCopier is destroyed.  Must be run on the UI
   // BrowserThread.
@@ skipping 35 matching lines @@
   const int render_process_id_;
   const int render_view_id_;
 
   // Routing ID of any active fullscreen render widget or MSG_ROUTING_NONE
   // otherwise.
   int fullscreen_widget_id_;
 
   // Last known RenderView size.
   gfx::Size last_view_size_;
 
-  // If the following is NULL (normal behavior), the implementation should
-  // access RenderWidgetHost via web_contents().
-  RenderWidgetHost* rwh_for_testing_;
-
   DISALLOW_COPY_AND_ASSIGN(BackingStoreCopier);
 };
 
 // Renders captures (from the backing store) into video frame buffers on a
 // separate thread.  Manages use of internally-owned video frame buffers.
 class VideoFrameRenderer {
  public:
   typedef base::Callback<void(const SkBitmap*)> DoneCB;
 
   VideoFrameRenderer();
@@ skipping 90 matching lines @@
   // deliver stage) whenever verbose logging is turned on.
   base::Time last_frame_rate_log_time_;
   int count_frames_rendered_;
   int last_frame_number_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoFrameDeliverer);
 };
 
 BackingStoreCopier::BackingStoreCopier(int render_process_id,
                                        int render_view_id)
-    : render_process_id_(render_process_id), render_view_id_(render_view_id),
-      fullscreen_widget_id_(MSG_ROUTING_NONE), rwh_for_testing_(NULL) {}
+    : render_process_id_(render_process_id),
+      render_view_id_(render_view_id),
+      fullscreen_widget_id_(MSG_ROUTING_NONE) {}
 
 BackingStoreCopier::~BackingStoreCopier() {
   DCHECK(!web_contents());
 }
 
 void BackingStoreCopier::StopObservingWebContents() {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
   if (web_contents()) {
     web_contents()->DecrementCapturerCount();
@@ skipping 28 matching lines @@
   Observe(rvh ? WebContents::FromRenderViewHost(rvh) : NULL);
   WebContentsImpl* contents = static_cast<WebContentsImpl*>(web_contents());
   if (contents) {
     contents->IncrementCapturerCount();
     fullscreen_widget_id_ = contents->GetFullscreenWidgetRoutingID();
   } else {
     DVLOG(1) << "WebContents::FromRenderViewHost(" << rvh << ") returned NULL.";
   }
 }
 
-void BackingStoreCopier::SetRenderWidgetHostForTesting(
-    RenderWidgetHost* override) {
-  rwh_for_testing_ = override;
-}
-
 VideoFrameRenderer::VideoFrameRenderer()
     : render_thread_("WebContentsVideo_RenderThread") {
   output_[0].in_use = false;
   output_[1].in_use = false;
   render_thread_.Start();
 }
 
 void VideoFrameRenderer::Render(int frame_number,
                                 const SkBitmap& capture,
                                 int frame_width, int frame_height,
@@ skipping 279 matching lines @@
 class CaptureMachine
     : public base::RefCountedThreadSafe<CaptureMachine, CaptureMachine> {
  public:
   enum SnapshotError {
     NO_SOURCE,
     TRANSIENT_ERROR
   };
 
   CaptureMachine(int render_process_id, int render_view_id);
 
-  // Sets the capture source to the given |override| for unit testing.
-  // Also, |destroy_cb| will be invoked after CaptureMachine is fully destroyed
-  // (to synchronize tear-down).
-  void InitializeForTesting(RenderWidgetHost* override,

[miu, 2013/02/20 06:45:10]

For the tests, how can you be sure CaptureMachine has finished using the "UI
thread" before you destroy it in WCVCDTest::TearDown()?

[ncarter (slow), 2013/02/22 02:16:11]

This is moot now, but the old code did reliably run DoShutdownTasksOnUIThread()
(it's posted to the UI thread and thus would be there when we pumped the message
queue).  What it didn't do reliably, was to actually join the threads and free
the CaptureMachine and its components before test-case TearDown().

So I've restored the destroy_cb, though it uses a RunLoop now instead of the
ConditionVar.

-                            const base::Closure& destroy_cb);
-
   // Synchronously sets/unsets the consumer.  Pass |consumer| as NULL to remove
   // the reference to the consumer; then, once this method returns,
   // CaptureMachine will no longer invoke callbacks on the old consumer from any
   // thread.
   void SetConsumer(media::VideoCaptureDevice::EventHandler* consumer);
 
   // Asynchronous requests to change CaptureMachine state.
   void Allocate(int width, int height, int frame_rate);
   void Start();
   void Stop();
@@ skipping 74 matching lines @@
   base::TimeDelta capture_period_;  // Time between frames.
 
   bool is_snapshotting_;  // True while taking a snapshot with copier_.
   int num_renders_pending_;  // The number of renders enqueued.
 
   // The three pipeline stages.
   BackingStoreCopier copier_;
   VideoFrameRenderer renderer_;
   VideoFrameDeliverer deliverer_;
 
-  base::Closure destroy_cb_;  // Invoked once CaptureMachine is destroyed.
-
   DISALLOW_COPY_AND_ASSIGN(CaptureMachine);
 };
 
 CaptureMachine::CaptureMachine(int render_process_id, int render_view_id)
     : manager_thread_("WebContentsVideo_ManagerThread"),
       state_(kIdle),
       is_snapshotting_(false),
       num_renders_pending_(0),
       copier_(render_process_id, render_view_id),
       deliverer_(&consumer_) {
   manager_thread_.Start();
 }
 
-void CaptureMachine::InitializeForTesting(RenderWidgetHost* override,
-                                          const base::Closure& destroy_cb) {
-  copier_.SetRenderWidgetHostForTesting(override);
-  destroy_cb_ = destroy_cb;
-}
-
 void CaptureMachine::SetConsumer(
     media::VideoCaptureDevice::EventHandler* consumer) {
   consumer_.SetConsumer(consumer);
 }
 
 void CaptureMachine::Allocate(int width, int height, int frame_rate) {
   ENSURE_INVOKED_ON_THREAD(manager_thread_,
                            &CaptureMachine::Allocate, this,
                            width, height, frame_rate);
 
@@ skipping 91 matching lines @@
   // ~CaptureMachine() is called, it will attempt to join with the
   // CaptureMachine-owned threads, including itself.  Since it's illegal for a
   // thread to join with itself, we need to trampoline the destructor call to
   // another thread.
   BrowserThread::PostBlockingPoolTask(
       FROM_HERE, base::Bind(&DeleteFromOutsideThread, x));
 }
 
 // static
 void CaptureMachine::DeleteFromOutsideThread(const CaptureMachine* x) {
-  const base::Closure run_after_delete = x->destroy_cb_;
-  // Note: Thread joins are about to happen here (in ~CaptureThread()).

[miu, 2013/02/20 06:45:10]

This comment should be retained (it's not related to the destroy_cb_ stuff).  It
might not be clear that thread joining will happen on the call to delete.

[ncarter (slow), 2013/02/22 02:16:11]

Done.

   delete x;
-  if (!run_after_delete.is_null()) {
-    run_after_delete.Run();
-  }
 }
 
 void CaptureMachine::TransitionStateTo(State next_state) {
   DCHECK_EQ(manager_thread_.message_loop(), MessageLoop::current());
 
 #ifndef NDEBUG
   static const char* kStateNames[] = {
     "Idle", "Allocated", "Capturing", "Error", "Destroyed"
   };
   DVLOG(1) << "State change: " << kStateNames[state_]
@@ skipping 170 matching lines @@
   copier_.StopObservingWebContents();
 }
 
 void BackingStoreCopier::StartCopy(
     const scoped_refptr<CaptureMachine>& consumer,
     int frame_number,
     int desired_width,
     int desired_height) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
 
-  RenderWidgetHost* rwh;
-  if (rwh_for_testing_) {
-    rwh = rwh_for_testing_;
-  } else {
-    if (!web_contents()) {  // No source yet.
-      LookUpAndObserveWebContents();
-      if (!web_contents()) {  // No source ever.
-        consumer->OnSnapshotFailed(CaptureMachine::NO_SOURCE, frame_number);
-        return;
-      }
-    }
-
-    if (fullscreen_widget_id_ != MSG_ROUTING_NONE) {
-      RenderProcessHost* process = web_contents()->GetRenderProcessHost();
-      rwh = process ? process->GetRenderWidgetHostByID(fullscreen_widget_id_)
-                    : NULL;
-    } else {
-      rwh = web_contents()->GetRenderViewHost();
-    }
-
-    if (!rwh) {
-      // Transient failure state (e.g., a RenderView is being replaced).
-      consumer->OnSnapshotFailed(CaptureMachine::TRANSIENT_ERROR, frame_number);
+  if (!web_contents()) {  // No source yet.
+    LookUpAndObserveWebContents();
+    if (!web_contents()) {  // No source ever.
+      consumer->OnSnapshotFailed(CaptureMachine::NO_SOURCE, frame_number);
       return;
     }
   }
 
+  RenderWidgetHost* rwh;
+  if (fullscreen_widget_id_ != MSG_ROUTING_NONE) {
+    RenderProcessHost* process = web_contents()->GetRenderProcessHost();
+    rwh = process ? process->GetRenderWidgetHostByID(fullscreen_widget_id_)
+                  : NULL;
+  } else {
+    rwh = web_contents()->GetRenderViewHost();
+  }
+
+  if (!rwh) {
+    // Transient failure state (e.g., a RenderView is being replaced).
+    consumer->OnSnapshotFailed(CaptureMachine::TRANSIENT_ERROR, frame_number);
+    return;
+  }
+
   RenderWidgetHostViewPort* view =
       RenderWidgetHostViewPort::FromRWHV(rwh->GetView());
 
   gfx::Size fitted_size;
   if (view) {
     gfx::Size view_size = view->GetViewBounds().size();
     if (!view_size.IsEmpty()) {
       CalculateFittedSize(view_size.width(), view_size.height(),
                           desired_width, desired_height,
                           &fitted_size);
@@ skipping 84 matching lines @@
         CaptureMachine::TRANSIENT_ERROR, frame_number);
   }
 }
 
 WebContentsVideoCaptureDevice::WebContentsVideoCaptureDevice(
     const media::VideoCaptureDevice::Name& name,
     int render_process_id, int render_view_id)
     : device_name_(name),
       capturer_(new CaptureMachine(render_process_id, render_view_id)) {}
 
-WebContentsVideoCaptureDevice::WebContentsVideoCaptureDevice(
-    RenderWidgetHost* test_source, const base::Closure& destroy_cb)
-    : capturer_(new CaptureMachine(-1, -1)) {
-  device_name_.device_name = "WebContentsForTesting";
-  device_name_.unique_id = "-1:-1";
-  capturer_->InitializeForTesting(test_source, destroy_cb);
-}
-
 WebContentsVideoCaptureDevice::~WebContentsVideoCaptureDevice() {
   DVLOG(2) << "WebContentsVideoCaptureDevice@" << this << " destroying.";
 }
 
 // static
 media::VideoCaptureDevice* WebContentsVideoCaptureDevice::Create(
     const std::string& device_id) {
   // Parse device_id into render_process_id and render_view_id.
   int render_process_id = -1;
   int render_view_id = -1;
   if (!WebContentsCaptureUtil::ExtractTabCaptureTarget(device_id,
                                                        &render_process_id,
                                                        &render_view_id))
     return NULL;
 
   media::VideoCaptureDevice::Name name;
   base::SStringPrintf(&name.device_name,
                       "WebContents[%.*s]",
                       static_cast<int>(device_id.size()), device_id.data());
   name.unique_id = device_id;
 
   return new WebContentsVideoCaptureDevice(
       name, render_process_id, render_view_id);
 }
 
-// static
-media::VideoCaptureDevice* WebContentsVideoCaptureDevice::CreateForTesting(
-    RenderWidgetHost* test_source, const base::Closure& destroy_cb) {
-  return new WebContentsVideoCaptureDevice(test_source, destroy_cb);
-}
-
 void WebContentsVideoCaptureDevice::Allocate(
     int width, int height, int frame_rate,
     VideoCaptureDevice::EventHandler* consumer) {
   DCHECK(capturer_);
   capturer_->SetConsumer(consumer);
   capturer_->Allocate(width, height, frame_rate);
 }
 
 void WebContentsVideoCaptureDevice::Start() {
   DCHECK(capturer_);
@@ skipping 10 matching lines @@
   capturer_->SetConsumer(NULL);
   capturer_->DeAllocate();
 }
 
 const media::VideoCaptureDevice::Name&
 WebContentsVideoCaptureDevice::device_name() {
   return device_name_;
 }
 
 }  // namespace content