Eliminate video capture thread in renderer

content/renderer/media/rtc_video_capture_delegate.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 "content/renderer/media/rtc_video_capture_delegate.h"
 
 #include "base/bind.h"
+#include "base/location.h"
+#include "content/renderer/media/video_capture_impl_manager.h"
+#include "content/renderer/render_thread_impl.h"
 #include "media/base/video_frame.h"
 
 namespace content {
 
 RtcVideoCaptureDelegate::RtcVideoCaptureDelegate(
-    const media::VideoCaptureSessionId id,
-    VideoCaptureImplManager* vc_manager)
+    const media::VideoCaptureSessionId id)
     : session_id_(id),
-      vc_manager_(vc_manager),
-      capture_engine_(NULL),
       got_first_frame_(false),
       error_occured_(false) {
   DVLOG(3) << " RtcVideoCaptureDelegate::ctor";
-  capture_engine_ = vc_manager_->AddDevice(session_id_, this);
+  capture_engine_ =
+      RenderThreadImpl::current()->video_capture_impl_manager()

[wjia(left Chromium), 2013/12/28 02:42:32]

Using RenderThreadImpl::current() will complicate unit test.

[Alpha Left Google, 2014/01/02 23:35:16]

I think this object is created on the render thread by design so I'm not sure
how would a test run it without a render thread.

And RenderThreadImpl is more than just a thread or message loop. It's a
process-wide object that provides a bunch of services. What is how it is used
here. You're supposed to access this object with a static accessor.

For example:
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/p...

[wjia(left Chromium), 2014/01/06 19:17:23]

That's not my understanding. There is no restriction on which thread this class
has to be instantiated. That might be the reason vc_manager is injected in ctor.
Need confirmation from the original author.

[Ami GONE FROM CHROMIUM, 2014/01/06 23:37:14]

My opinion: using RTI this way (instead of injecting the VCIM) _looks_ like a
reduction in testability but in fact does not reduce test coverage because the
"feature" of accepting a VCIM was never taken advantage of to write tests that
utilize it.
Since this class is in content/renderer it makes sense for tests for it to
assume a functional RTI is present.

[perkj_chrome, 2014/01/07 06:21:44]

Good to know when RTI is ok to use and when not. I would have preferred
dependency injection but since this also make the rest of the code cleaner I am
ok. 

+         ->UseDevice(session_id_);
 }
 
 RtcVideoCaptureDelegate::~RtcVideoCaptureDelegate() {

[Ami GONE FROM CHROMIUM, 2014/01/06 23:37:14]

This silently relies on class-member-destruction order.
Is it ok to clear captured_callback_ and state_callback_ before capture_engine_,
for example?

I suspect you either want to tear down the members explicitly here or else doco
in the header that the declaration order is important, hopefully with some
rationale commentary.

[Alpha Left Google, 2014/01/08 00:23:36]

I'm calling StopCapture() here such that it explicitly reset the callback and
stop listening to capture events.

VideoCaptureHandle will then do the cleanup implicitly.

   DVLOG(3) << " RtcVideoCaptureDelegate::dtor";
-  vc_manager_->RemoveDevice(session_id_, this);
 }
 
 void RtcVideoCaptureDelegate::StartCapture(
     const media::VideoCaptureParams& params,
     const FrameCapturedCallback& captured_callback,
     const StateChangeCallback& state_callback) {
   DVLOG(3) << " RtcVideoCaptureDelegate::StartCapture ";
   message_loop_proxy_ = base::MessageLoopProxy::current();
   captured_callback_ = captured_callback;
   state_callback_ = state_callback;
@@ skipping 78 matching lines @@
 }
 
 
 void RtcVideoCaptureDelegate::OnRemovedOnCaptureThread(
     media::VideoCapture* capture) {
   if (!error_occured_ && !state_callback_.is_null())
     state_callback_.Run(CAPTURE_STOPPED);
 }
 
 }  // namespace content