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..413b0b54e2a3fec7d3673960cd6a2f493edf2c95 100644
--- a/content/renderer/media/webrtc/webrtc_video_track_adapter.cc
+++ b/content/renderer/media/webrtc/webrtc_video_track_adapter.cc
@@ -18,17 +18,25 @@ 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 {
 
 // Simple help class used for receiving video frames on the IO-thread from
 // a MediaStreamVideoTrack and forward the frames to a
-// WebRtcVideoCapturerAdapter that implements a video capturer for libjingle.
+// WebRtcVideoCapturerAdapter on libjingle's worker thread.
+// WebRtcVideoCapturerAdapter implements a video capturer for libjingle.
 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 +44,66 @@ class WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter
                         const media::VideoCaptureFormat& format);
 
  private:
+  void OnVideoFrameOnWorkerThread(const scoped_refptr<media::VideoFrame>& frame,
+                                  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 libjingle's worker thread. Accessed on the
+  // IO-thread.
+  scoped_refptr<base::MessageLoopProxy> libjingle_worker_thread_;

[Ronghua Wu (Left Chromium), 2014/05/12 16:09:12]

call this target_thread_? and call io_thread_checker_ source_thread_checker_?

[Ami GONE FROM CHROMIUM, 2014/05/12 18:13:33]

FWIW I weakly prefer io/worker/signaling/rendering terminology to target/source
for being easier to tell at a glance what's what without having to refer back to
other places.
(this is a totes optional comment)

[perkj_chrome, 2014/05/12 19:28:26]

I would prefer to leave as it is. 

+
   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, 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(

[wuchengli, 2014/05/12 15:45:56]

You can just use render_thread_message_loop_->ReleaseSoon(). But I don't know if
there is a way to avoid all this.

[perkj_chrome, 2014/05/12 16:07:09]

Nice to know. But this is not a Chrome class so this magic doesn't seem to
compile. Can we skip this? 


./base/sequenced_task_runner_helpers.h:51:5: error: member function 'Release'
not viable: 'this' argument has type 'const webrtc::VideoSourceInterface', but
function is not marked const
    reinterpret_cast<const T*>(object)->Release();
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../base/sequenced_task_runner_helpers.h:101:39: note: in instantiation of
member function 'base::ReleaseHelper<webrtc::VideoSourceInterface>::DoRelease'
requested here
        from_here, &ReleaseHelper<T>::DoRelease, object);

[wuchengli, 2014/05/12 16:36:50]

I see.

+        FROM_HERE,
+        base::Bind(&ReleaseWebRtcSourceOnMainRenderThread,

[Ami GONE FROM CHROMIUM, 2014/05/12 18:13:33]

FWIW, 
base::Bind(&talk_base::RefCountedInterface::Release, base::Unretained(source));

should allow you to drop the helper.

[perkj_chrome, 2014/05/12 19:28:26]

Good idea, but
talk_base::RefCountInterface::Release is a pure virtual. I guess I can get it to
work if I point at the actual implementation but that does not feel right since
|source| is a pointer to an interface. 

    render_thread_message_loop_->PostTask(
        FROM_HERE,
        base::Bind(&talk_base::RefCountInterface::Release,
                   base::Unretained(source)));

../../content/renderer/media/webrtc/webrtc_video_track_adapter.cc:87:9: error:
no viable conversion from 'Callback<typename internal::BindState<typename
internal::FunctorTraits<int (class RefCountInterface::*)(void)>::RunnableType,
typename internal::FunctorTraits<int (class
RefCountInterface::*)(void)>::RunType, void (typename
internal::CallbackParamTraits<class UnretainedWrapper<class
VideoSourceInterface> >::StorageType)>::UnboundRunType>' to 'const Callback<void
(void)>'
        base::Bind(&talk_base::RefCountInterface::Release,

[Ami GONE FROM CHROMIUM, 2014/05/12 19:46:27]

It sounds like you're saying that base::Bind() can't bind to a pure-virtual, but
that's not the case (e.g. https://gist.github.com/fischman/6403dd36b9a9a2697930
works fine for me).
Not a big deal, obvs.

[perkj_chrome, 2014/05/12 20:35:11]

Humm, Can it be that Release returns an int in libjingle? Can I use base::Bind
anyway? If you don't mind I will not change this now but I would like to learn.

[Ami GONE FROM CHROMIUM, 2014/05/12 21:24:38]

Yeah; that'd do it.
Yes; use base::IgnoreResult(&talk_base::RefCountInterface::Release) instead of
just &talk_base::RefCountInterface::Release by itself.
np

+                   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(
+      FROM_HERE,
+      base::Bind(&WebRtcVideoSourceAdapter::OnVideoFrameOnWorkerThread,
+                 this, frame, format));
+}
+
+void
+WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter::OnVideoFrameOnWorkerThread(
+    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->GetWebRtcWorkerThread(),

[Ronghua Wu (Left Chromium), 2014/05/12 16:09:12]

I think we should expose this interface on |capture_adapter|, e.g. inside
WebRtcVideoSourceAdapter, gets the thread |capture_adapter| wants it to use.

[perkj_chrome, 2014/05/12 19:28:26]

MediaStreamDependencyFactory (terrible name now by the way) creates and owns
libjingles worker thread. I think Ami is right that WebRtcVideoCapturerAdapter
doesn't really need it. Its only used for thread checks so a thread checker is
probably the way to go. 

[Ronghua Wu (Left Chromium), 2014/05/12 21:45:46]

In that case it makes sense to keep it in MediaStreamDependencyFactory.

+      video_source,
+      capture_adapter);
 
   AddToVideoTrack(
       this,