Deliver video frames on libjingle worker thread to WebRtcVideoCapturerAdapter.

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 211013dc9e57f11af45ccda9270e6987478ca19f..1f22b1420702d070ed150f0090620c0be5808c77 100644
--- a/content/renderer/media/webrtc/webrtc_video_track_adapter.cc
+++ b/content/renderer/media/webrtc/webrtc_video_track_adapter.cc
@@ -18,6 +18,12 @@ 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 {
@@ -29,6 +35,7 @@ class WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter
     : public base::RefCountedThreadSafe<WebRtcVideoSourceAdapter> {
  public:
   WebRtcVideoSourceAdapter(
+      const scoped_refptr<base::MessageLoopProxy>& libjingle_worker_thread,
       const scoped_refptr<webrtc::VideoSourceInterface>& source,
       WebRtcVideoCapturerAdapter* capture_adapter);
 
@@ -36,31 +43,67 @@ class WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter
                         const media::VideoCaptureFormat& format);
 
  private:
+  void OnVideoFrameOnWt(const scoped_refptr<media::VideoFrame>& frame,

[tommi (sloooow) - chröme, 2014/05/12 14:20:41]

OnVideoFrameOnWorkerThread

[perkj_chrome, 2014/05/12 15:39:51]

Done.

+                        const media::VideoCaptureFormat& format);
   friend class base::RefCountedThreadSafe<WebRtcVideoSourceAdapter>;
   virtual ~WebRtcVideoSourceAdapter();
 
+  scoped_refptr<base::MessageLoopProxy> render_thread_message_loop_;
+
   // Used to DCHECK that frames are called on the IO-thread.
   base::ThreadChecker io_thread_checker_;
+
+  // Used for posting frames to libjingles worker thread. Accessed on the

[tommi (sloooow) - chröme, 2014/05/12 14:20:41]

libjingle's

[perkj_chrome, 2014/05/12 15:39:51]

Done.

+  // IO-thread.
+  scoped_refptr<base::MessageLoopProxy> libjingle_worker_thread_;
+
   scoped_refptr<webrtc::VideoSourceInterface> video_source_;
   // |capture_adapter_| is owned by |video_source_|
   WebRtcVideoCapturerAdapter* capture_adapter_;
 };
 
 WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter::WebRtcVideoSourceAdapter(
+    const scoped_refptr<base::MessageLoopProxy>& libjingle_worker_thread,
     const scoped_refptr<webrtc::VideoSourceInterface>& source,
     WebRtcVideoCapturerAdapter* capture_adapter)
-    : video_source_(source),
+    : render_thread_message_loop_(base::MessageLoopProxy::current()),
+      libjingle_worker_thread_(libjingle_worker_thread),
+      video_source_(source),
       capture_adapter_(capture_adapter) {
   io_thread_checker_.DetachFromThread();
 }
 
 WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter::~WebRtcVideoSourceAdapter() {
+  // 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, we should release our reference on the render thread. This is not

[tommi (sloooow) - chröme, 2014/05/12 14:20:41]

nit: s/should/must

+  // strictly necessary since both the main render thread and the worker thread
+  // implements libjingle's thread Send method.

[tommi (sloooow) - chröme, 2014/05/12 14:20:41]

nit: implement
(no s)

After reading why this isn't strictly necessary, I still don't understand why it
isn't strictly necessary.  Is it because you have proxy to the source at this
point and the proxy will do the right thing somehow? (I guess I'm not groking
how a couple of threads implementing Send() somehow make this particular case
safe)

[perkj_chrome, 2014/05/12 15:39:51]

ok- I removed the whole comment instead since we don't want to rely on it
anyway. It is ok since each libjingle has a proxy that ensures that all methods
are called on the signaling thread. Including destruction. But for that to work,
the originating thread must implement Send. 

+  if (!render_thread_message_loop_->BelongsToCurrentThread()) {

[tommi (sloooow) - chröme, 2014/05/12 14:20:41]

I think that if at all possible to avoid this, we should.

Having an inconsistent teardown sequence can be very hard to troubleshoot.  I'd
like to keep the object being deleted on the same thread and avoid checks like
these.

[perkj_chrome, 2014/05/12 15:39:51]

Suggestions? We have 3 - threads here. Render thread that sets it all up. IO
thread where video frames are received on. WT where we want to deliver frames
on. 

All objects should be released on the render thread
video_source_ own the capturer that received frames on the WT. So we must make
sure no frames are posted to the WT after
WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter is destroyed.

+    webrtc::VideoSourceInterface* source = video_source_.get();
+    source->AddRef();
+    video_source_ = NULL;
+    render_thread_message_loop_->PostTask(
+        FROM_HERE,
+        base::Bind(&ReleaseWebRtcSourceOnMainRenderThread,
+                   base::Unretained(source)));
+  }
 }
 
 void WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter::OnVideoFrameOnIO(
     const scoped_refptr<media::VideoFrame>& frame,
     const media::VideoCaptureFormat& format) {
   DCHECK(io_thread_checker_.CalledOnValidThread());
+  libjingle_worker_thread_->PostTask(

[perkj_chrome, 2014/05/12 15:39:51]

This is what might cause WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter to be
deleted on the WT.

+      FROM_HERE,
+      base::Bind(&WebRtcVideoSourceAdapter::OnVideoFrameOnWt,
+                 this, frame, format));
+}
+
+void WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter::OnVideoFrameOnWt(
+    const scoped_refptr<media::VideoFrame>& frame,
+    const media::VideoCaptureFormat& format) {
+  DCHECK(libjingle_worker_thread_->BelongsToCurrentThread());
   capture_adapter_->OnFrameCaptured(frame);
 }
 
@@ -86,8 +129,10 @@ WebRtcVideoTrackAdapter::WebRtcVideoTrackAdapter(
 
   video_track_->set_enabled(web_track_.isEnabled());
 
-  source_adapter_ = new WebRtcVideoSourceAdapter(video_source,
-                                                 capture_adapter);
+  source_adapter_ = new WebRtcVideoSourceAdapter(
+      factory->worker_thread_proxy(),
+      video_source,
+      capture_adapter);
 
   AddToVideoTrack(
       this,