refactor VideoCaptureModuleImpl.

1. call StopCapture() in dtor to ensure VideoCaptureModuleImpl is deleted correctly.
2. fix racing condition by enforcing synchronous StopCapture between VideoCaputreModuleImpl and capture_engine. Without sync mode, it's possible that capture_engine would call OnStopped after VideoCaptureModuleImpl is destructed.
3. remove timestamp rebasing.

BUG=none
TEST=trybots
Review URL: http://codereview.chromium.org/8334034

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@108676 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 21 insertions, 45 deletions

diff --git a/content/renderer/media/video_capture_module_impl.cc b/content/renderer/media/video_capture_module_impl.cc
index e50e561..9231112 100644
--- a/content/renderer/media/video_capture_module_impl.cc
+++ b/content/renderer/media/video_capture_module_impl.cc
@@ -14,6 +14,7 @@ VideoCaptureModuleImpl::VideoCaptureModuleImpl(
     : webrtc::videocapturemodule::VideoCaptureImpl(id),
       session_id_(id),
       thread_("VideoCaptureModuleImpl"),
+      stopped_event_(false, false),
       vc_manager_(vc_manager),
       state_(media::VideoCapture::kStopped),
       width_(-1),
@@ -21,14 +22,18 @@ VideoCaptureModuleImpl::VideoCaptureModuleImpl(
       frame_rate_(-1),
       video_type_(webrtc::kVideoI420),
       capture_engine_(NULL),
-      pending_start_(false),
       ref_count_(0) {
   DCHECK(vc_manager_);
   Init();
 }
 
 VideoCaptureModuleImpl::~VideoCaptureModuleImpl() {
+  // Ensure to stop capture in case user doesn't do it.
+  StopCapture();
   vc_manager_->RemoveDevice(session_id_, this);
+  // Ensure capture stopped by |capture_engine_| since all tasks posted on this
+  // thread will be executed and VCMImpl is waiting for OnStopped signal from
+  // |capture_engine_|.
   thread_.Stop();
 }
 
@@ -39,15 +44,15 @@ void VideoCaptureModuleImpl::Init() {
 }
 
 int32_t VideoCaptureModuleImpl::AddRef() {
-  VLOG(1) << "VideoCaptureModuleImpl::AddRef()";
+  DVLOG(1) << "VideoCaptureModuleImpl::AddRef()";
   return base::subtle::Barrier_AtomicIncrement(&ref_count_, 1);
 }
 
 int32_t VideoCaptureModuleImpl::Release() {
-  VLOG(1) << "VideoCaptureModuleImpl::Release()";
+  DVLOG(1) << "VideoCaptureModuleImpl::Release()";
   int ret = base::subtle::Barrier_AtomicIncrement(&ref_count_, -1);
   if (ret == 0) {
-    VLOG(1) << "Reference count is zero, hence this object is now deleted.";
+    DVLOG(1) << "Reference count is zero, hence this object is now deleted.";
     delete this;
   }
   return ret;
@@ -89,11 +94,7 @@ void VideoCaptureModuleImpl::OnStarted(media::VideoCapture* capture) {
 }
 
 void VideoCaptureModuleImpl::OnStopped(media::VideoCapture* capture) {
-  message_loop_proxy_->PostTask(
-      FROM_HERE,
-      base::Bind(&VideoCaptureModuleImpl::OnStoppedOnCaptureThread,
-                 base::Unretained(this),
-                 capture));
+  stopped_event_.Signal();
 }
 
 void VideoCaptureModuleImpl::OnPaused(media::VideoCapture* capture) {
@@ -129,15 +130,8 @@ void VideoCaptureModuleImpl::StartCaptureOnCaptureThread(
   DCHECK(message_loop_proxy_->BelongsToCurrentThread());
   DCHECK_NE(state_, media::VideoCapture::kStarted);
 
-  if (state_ == media::VideoCapture::kStopping) {
-    VLOG(1) << "Got a new StartCapture in Stopping state!!! ";
-    pending_start_ = true;
-    pending_cap_ = capability;
-    return;
-  }
-
-  VLOG(1) << "StartCaptureOnCaptureThread: " << capability.width << ", "
-          << capability.height;
+  DVLOG(1) << "StartCaptureOnCaptureThread: " << capability.width << ", "
+           << capability.height;
 
   StartCaptureInternal(capability);
   return;
@@ -165,39 +159,22 @@ void VideoCaptureModuleImpl::StartCaptureInternal(
 void VideoCaptureModuleImpl::StopCaptureOnCaptureThread() {
   DCHECK(message_loop_proxy_->BelongsToCurrentThread());
 
-  if (pending_start_) {
-    VLOG(1) << "Got a StopCapture with one pending start!!! ";
-    pending_start_ = false;
-    return;
-  }
-
   if (state_ != media::VideoCapture::kStarted) {
-    VLOG(1) << "Got a StopCapture while not started!!! ";
+    DVLOG(1) << "Got a StopCapture while not started!!! ";
     return;
   }
 
-  VLOG(1) << "StopCaptureOnCaptureThread. ";
-  state_ = media::VideoCapture::kStopping;
-
+  DVLOG(1) << "StopCaptureOnCaptureThread. ";
   capture_engine_->StopCapture(this);
-  return;
-}
-
-void VideoCaptureModuleImpl::OnStoppedOnCaptureThread(
-    media::VideoCapture* capture) {
-  DCHECK(message_loop_proxy_->BelongsToCurrentThread());
-
-  VLOG(1) << "Capture Stopped!!! ";
+  // Need to use synchronous mode here to make sure |capture_engine_| will
+  // not call VCMImpl after this function returns. Otherwise, there could be
+  // use-after-free, especially when StopCapture() is called from dtor.
+  stopped_event_.Wait();
+  DVLOG(1) << "Capture Stopped!!! ";
   state_ = media::VideoCapture::kStopped;
   width_ = -1;
   height_ = -1;
   frame_rate_ = -1;
-
-  if (pending_start_) {
-    VLOG(1) << "restart pending start ";
-    pending_start_ = false;
-    StartCaptureInternal(pending_cap_);
-  }
 }
 
 void VideoCaptureModuleImpl::OnBufferReadyOnCaptureThread(
diff --git a/content/renderer/media/video_capture_module_impl.h b/content/renderer/media/video_capture_module_impl.h
index fb3fb64..ca56e85 100644
--- a/content/renderer/media/video_capture_module_impl.h
+++ b/content/renderer/media/video_capture_module_impl.h
@@ -6,6 +6,7 @@
 #define CONTENT_RENDERER_MEDIA_VIDEO_CAPTURE_MODULE_IMPL_H_
 
 #include "base/compiler_specific.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
 #include "media/video/capture/video_capture.h"
 #include "third_party/webrtc/common_types.h"
@@ -57,7 +58,6 @@ class VideoCaptureModuleImpl
   void StopCaptureOnCaptureThread();
   void StartCaptureInternal(const webrtc::VideoCaptureCapability& capability);
 
-  void OnStoppedOnCaptureThread(media::VideoCapture* capture);
   void OnBufferReadyOnCaptureThread(
       media::VideoCapture* capture,
       scoped_refptr<media::VideoCapture::VideoFrameBuffer> buf);
@@ -67,6 +67,7 @@ class VideoCaptureModuleImpl
   media::VideoCaptureSessionId session_id_;
   base::Thread thread_;
   scoped_refptr<base::MessageLoopProxy> message_loop_proxy_;
+  base::WaitableEvent stopped_event_;
   // The video capture manager handles open/close of video capture devices.
   scoped_refptr<VideoCaptureImplManager> vc_manager_;
   media::VideoCapture::State state_;
@@ -78,8 +79,6 @@ class VideoCaptureModuleImpl
   base::Time start_time_;
   // The video capture module generating raw frame data.
   media::VideoCapture* capture_engine_;
-  bool pending_start_;
-  webrtc::VideoCaptureCapability pending_cap_;
   int ref_count_;
 
   DISALLOW_COPY_AND_ASSIGN(VideoCaptureModuleImpl);