Make sure webrtc::VideoSource is released when WebRtcVideoTrackAdapter is destroyed.

content/renderer/media/webrtc/webrtc_video_track_adapter.cc

Index: content/renderer/media/webrtc/webrtc_video_track_adapter.cc
diff --git a/content/renderer/media/webrtc/webrtc_video_track_adapter.cc b/content/renderer/media/webrtc/webrtc_video_track_adapter.cc
index 810f8bad7e5ea650ba793ff20b0ed75917623279..828a477d8bc83ebe4b4ebeca29910e27a6700f30 100644
--- a/content/renderer/media/webrtc/webrtc_video_track_adapter.cc
+++ b/content/renderer/media/webrtc/webrtc_video_track_adapter.cc
@@ -5,6 +5,7 @@
 #include "content/renderer/media/webrtc/webrtc_video_track_adapter.h"
 
 #include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/lock.h"
 #include "content/common/media/media_stream_options.h"
 #include "content/renderer/media/media_stream_video_source.h"
 #include "content/renderer/media/media_stream_video_track.h"
@@ -18,12 +19,6 @@ bool ConstraintKeyExists(const blink::WebMediaConstraints& constraints,
       constraints.getOptionalConstraintValue(name, value_str);
 }
 
-// Used to make sure |source| is released on the main render thread.
-void ReleaseWebRtcSourceOnMainRenderThread(
-    webrtc::VideoSourceInterface* source) {
-  source->Release();
-}
-
 }  // anonymouse namespace
 
 namespace content {
@@ -40,6 +35,8 @@ class WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter
       const scoped_refptr<webrtc::VideoSourceInterface>& source,
       WebRtcVideoCapturerAdapter* capture_adapter);
 
+  void ReleaseSourceOnMainThread();

[tommi (sloooow) - chröme, 2014/05/30 11:15:22]

Can you add some notes on why this is needed?

[perkj_chrome, 2014/05/30 12:16:33]

Done.

+
   void OnVideoFrameOnIO(const scoped_refptr<media::VideoFrame>& frame,
                         const media::VideoCaptureFormat& format);
 
@@ -51,6 +48,8 @@ class WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter
 
   scoped_refptr<base::MessageLoopProxy> render_thread_message_loop_;
 
+  // |render_thread_checker_| is bound to the main render thread.
+  base::ThreadChecker render_thread_checker_;
   // Used to DCHECK that frames are called on the IO-thread.
   base::ThreadChecker io_thread_checker_;
 
@@ -59,6 +58,12 @@ class WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter
   scoped_refptr<base::MessageLoopProxy> libjingle_worker_thread_;
 
   scoped_refptr<webrtc::VideoSourceInterface> video_source_;
+
+  // Used to protect |capture_adapter_|. It is taken by libjingle's worker
+  // thread for each video frame that is delivered but only taken on the
+  // main render thread in ReleaseSourceOnMainThread() when
+  // the owning WebRtcVideoTrackAdapter is being destroyed.
+  base::Lock capture_adapter_stop_lock_;
   // |capture_adapter_| is owned by |video_source_|
   WebRtcVideoCapturerAdapter* capture_adapter_;
 };
@@ -75,18 +80,26 @@ WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter::WebRtcVideoSourceAdapter(
 }
 
 WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter::~WebRtcVideoSourceAdapter() {
+  DVLOG(3) << "~WebRtcVideoSourceAdapter()";
+  DCHECK(!capture_adapter_);
+  // This object can be destroyed on the main render thread or libjingles
+  // worker thread since it posts video frames on that thread. But
+  // |video_source_| must be released on the main render thread before the
+  // PeerConnectionFactory has been destroyed. The only way to ensure that is
+  // to make sure |video_source_| is released when WebRtcVideoTrackAdapter() is
+  // destroyed.
+}
+
+void WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter::
+ReleaseSourceOnMainThread() {
+  DCHECK(render_thread_checker_.CalledOnValidThread());
   // Since frames are posted to the worker thread, this object might be deleted
   // on that thread. However, since |video_source_| was created on the render
   // thread, it should be released on the render thread.
-  if (!render_thread_message_loop_->BelongsToCurrentThread()) {
-    webrtc::VideoSourceInterface* source = video_source_.get();
-    source->AddRef();
-    video_source_ = NULL;
-    render_thread_message_loop_->PostTask(
-        FROM_HERE,
-        base::Bind(&ReleaseWebRtcSourceOnMainRenderThread,
-                   base::Unretained(source)));
-  }
+  base::AutoLock auto_lock(capture_adapter_stop_lock_);
+  // |video_source| owns |capture_adapter_|.
+  capture_adapter_ = NULL;
+  video_source_ = NULL;
 }
 
 void WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter::OnVideoFrameOnIO(
@@ -104,7 +117,9 @@ WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter::OnVideoFrameOnWorkerThread(
     const scoped_refptr<media::VideoFrame>& frame,
     const media::VideoCaptureFormat& format) {
   DCHECK(libjingle_worker_thread_->BelongsToCurrentThread());
-  capture_adapter_->OnFrameCaptured(frame);
+  base::AutoLock auto_lock(capture_adapter_stop_lock_);
+  if (capture_adapter_)
+    capture_adapter_->OnFrameCaptured(frame);
 }
 
 WebRtcVideoTrackAdapter::WebRtcVideoTrackAdapter(
@@ -148,6 +163,7 @@ WebRtcVideoTrackAdapter::~WebRtcVideoTrackAdapter() {
   DCHECK(thread_checker_.CalledOnValidThread());
   DVLOG(3) << "WebRtcVideoTrackAdapter dtor().";
   RemoveFromVideoTrack(this, web_track_);
+  source_adapter_->ReleaseSourceOnMainThread();
 }
 
 void WebRtcVideoTrackAdapter::OnEnabledChanged(bool enabled) {