Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
282523003:
Deliver video frames on libjingle worker thread to WebRtcVideoCapturerAdapter. (Closed)
|
Created:
6 years, 7 months ago by perkj_chrome Modified:
6 years, 7 months ago CC:
chromium-reviews, fischman+watch_chromium.org, jam, mcasas+watch_chromium.org, feature-media-reviews_chromium.org, darin-cc_chromium.org, wjia+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionDeliver video frames on libjingle worker thread to WebRtcVideoCapturerAdapter.
This change so that WebRtcVideoCapturerAdapter receives video frames on libjingle worker thread instead of the IO-thread.
BUG=371711, 350111
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=270048
Patch Set 1 #Patch Set 2 : #
Total comments: 16
Patch Set 3 : Addressed Tommis comments. #
Total comments: 20
Patch Set 4 : Addressed Amis comments. #
Total comments: 8
Patch Set 5 : Addressed the last round. #
Messages
Total messages: 19 (0 generated)
Hej- Here is a patch for posting all video frames to libjingle on the libjingle worker thread.
https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/m... File content/renderer/media/media_stream_dependency_factory.h (right): https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/m... content/renderer/media/media_stream_dependency_factory.h:134: scoped_refptr<base::MessageLoopProxy> worker_thread_proxy() { Can we call this GetWebRtcWorkerThread() and move the implementation into the cc file? Also, I'd like to have a thread check to protect access to the chrome_worker_thread_ member variable. https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:32: // OnFrameCaptured delivers video frames to libjingle. It should be called on nit: It must be called on (right?) https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:54: base::MessageLoop message_loop_; for ui or for io? https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:46: void OnVideoFrameOnWt(const scoped_refptr<media::VideoFrame>& frame, OnVideoFrameOnWorkerThread https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:56: // Used for posting frames to libjingles worker thread. Accessed on the libjingle's https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:79: // thread, we should release our reference on the render thread. This is not nit: s/should/must https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:81: // implements libjingle's thread Send method. 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) https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:82: if (!render_thread_message_loop_->BelongsToCurrentThread()) { 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.
PTAL https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/m... File content/renderer/media/media_stream_dependency_factory.h (right): https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/m... content/renderer/media/media_stream_dependency_factory.h:134: scoped_refptr<base::MessageLoopProxy> worker_thread_proxy() { On 2014/05/12 14:20:41, tommi wrote: > Can we call this GetWebRtcWorkerThread() and move the implementation into the cc > file? > > Also, I'd like to have a thread check to protect access to the > chrome_worker_thread_ member variable. Done. https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:32: // OnFrameCaptured delivers video frames to libjingle. It should be called on On 2014/05/12 14:20:41, tommi wrote: > nit: It must be called on > (right?) Done. https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:54: base::MessageLoop message_loop_; On 2014/05/12 14:20:41, tommi wrote: > for ui or for io? neither, its the libjingle worker thread proxy. https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:46: void OnVideoFrameOnWt(const scoped_refptr<media::VideoFrame>& frame, On 2014/05/12 14:20:41, tommi wrote: > OnVideoFrameOnWorkerThread Done. https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:56: // Used for posting frames to libjingles worker thread. Accessed on the On 2014/05/12 14:20:41, tommi wrote: > libjingle's Done. https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:81: // implements libjingle's thread Send method. On 2014/05/12 14:20:41, tommi wrote: > 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) 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. https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:82: if (!render_thread_message_loop_->BelongsToCurrentThread()) { On 2014/05/12 14:20:41, tommi wrote: > 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. 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. https://codereview.chromium.org/282523003/diff/20001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:97: libjingle_worker_thread_->PostTask( This is what might cause WebRtcVideoTrackAdapter::WebRtcVideoSourceAdapter to be deleted on the WT.
https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:54: base::MessageLoop message_loop_; unused? https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:85: render_thread_message_loop_->PostTask( You can just use render_thread_message_loop_->ReleaseSoon(). But I don't know if there is a way to avoid all this.
https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:54: base::MessageLoop message_loop_; On 2014/05/12 15:45:56, wuchengli wrote: > unused? needed by base::MessageLoopProxy::current() https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:85: render_thread_message_loop_->PostTask( On 2014/05/12 15:45:56, wuchengli wrote: > You can just use render_thread_message_loop_->ReleaseSoon(). But I don't know if > there is a way to avoid all this. 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);
https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:59: scoped_refptr<base::MessageLoopProxy> libjingle_worker_thread_; call this target_thread_? and call io_thread_checker_ source_thread_checker_? https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:133: factory->GetWebRtcWorkerThread(), I think we should expose this interface on |capture_adapter|, e.g. inside WebRtcVideoSourceAdapter, gets the thread |capture_adapter| wants it to use.
LGTM https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:54: base::MessageLoop message_loop_; On 2014/05/12 16:07:09, perkj wrote: > On 2014/05/12 15:45:56, wuchengli wrote: > > unused? > > needed by base::MessageLoopProxy::current() I don't understand. How is this needed? https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:85: render_thread_message_loop_->PostTask( On 2014/05/12 16:07:09, perkj wrote: > On 2014/05/12 15:45:56, wuchengli wrote: > > You can just use render_thread_message_loop_->ReleaseSoon(). But I don't know > if > > there is a way to avoid all this. > > 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); I see.
drive-by Should this also fix crbug.com/347548? And also help 350111 (but not fix since it doesn't address InitEncode/Release)? (add relevant bug(s) to BUG= line) https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:18: : worker_thread_proxy_(worker_thread_proxy), AFAICT worker_thread_proxy_ is only ever used to assert being on a particular thread, which sounds like a thread_checker_ to me. But thread_checker_'s DetachFromThread() is never called, meaning you could drop w_t_p_ and just use DCHECK(thread_checker_.CalledOnValidThread()); at each use-site of w_t_p_, right? https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:59: scoped_refptr<base::MessageLoopProxy> libjingle_worker_thread_; On 2014/05/12 16:09:12, Ronghua Wu wrote: > call this target_thread_? and call io_thread_checker_ source_thread_checker_? 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) https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:87: base::Bind(&ReleaseWebRtcSourceOnMainRenderThread, FWIW, base::Bind(&talk_base::RefCountedInterface::Release, base::Unretained(source)); should allow you to drop the helper.
PTAL https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:18: : worker_thread_proxy_(worker_thread_proxy), On 2014/05/12 18:13:33, Ami Fischman wrote: > AFAICT worker_thread_proxy_ is only ever used to assert being on a particular > thread, which sounds like a thread_checker_ to me. But thread_checker_'s > DetachFromThread() is never called, meaning you could drop w_t_p_ and just use > DCHECK(thread_checker_.CalledOnValidThread()); at each use-site of w_t_p_, > right? yes- that would be cleaner I think, I used thread_checker to make sure WebRtcVideoCapturerAdapter was deleted on the same thread that created it. But I will skip that and only check the usage here. https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter_unittest.cc:54: base::MessageLoop message_loop_; On 2014/05/12 16:36:50, wuchengli wrote: > On 2014/05/12 16:07:09, perkj wrote: > > On 2014/05/12 15:45:56, wuchengli wrote: > > > unused? > > > > needed by base::MessageLoopProxy::current() > I don't understand. How is this needed? After fixing Amis comments its no longer needed.... https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:59: scoped_refptr<base::MessageLoopProxy> libjingle_worker_thread_; On 2014/05/12 18:13:33, Ami Fischman wrote: > On 2014/05/12 16:09:12, Ronghua Wu wrote: > > call this target_thread_? and call io_thread_checker_ source_thread_checker_? > > 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) I would prefer to leave as it is. https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:87: base::Bind(&ReleaseWebRtcSourceOnMainRenderThread, On 2014/05/12 18:13:33, Ami Fischman wrote: > FWIW, > base::Bind(&talk_base::RefCountedInterface::Release, base::Unretained(source)); > > should allow you to drop the helper. 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, https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:133: factory->GetWebRtcWorkerThread(), On 2014/05/12 16:09:12, Ronghua Wu wrote: > I think we should expose this interface on |capture_adapter|, e.g. inside > WebRtcVideoSourceAdapter, gets the thread |capture_adapter| wants it to use. 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.
https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:87: base::Bind(&ReleaseWebRtcSourceOnMainRenderThread, On 2014/05/12 19:28:26, perkj wrote: > On 2014/05/12 18:13:33, Ami Fischman wrote: > > FWIW, > > base::Bind(&talk_base::RefCountedInterface::Release, > base::Unretained(source)); > > > > should allow you to drop the helper. > 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, > > 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. https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/m... File content/renderer/media/mock_media_stream_dependency_factory.cc (right): https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/m... content/renderer/media/mock_media_stream_dependency_factory.cc:127: MockRtcVideoCapturer( File can be reverted out of this CL? https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:16: bool is_screencast) \n unnecessary now https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:23: // PeerConnection access cricket::VideoCapturer from a libJingle worker thread. Can you add a line here that explains how it is guaranteed that PeerConnection doesn't call |this| (on the worker thread) after it has been deleted (on the render thread)? https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:49: private: dup w/ l.35
Here we go again - this will be the last from me tonight. https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:87: base::Bind(&ReleaseWebRtcSourceOnMainRenderThread, On 2014/05/12 19:46:27, Ami Fischman wrote: > On 2014/05/12 19:28:26, perkj wrote: > > On 2014/05/12 18:13:33, Ami Fischman wrote: > > > FWIW, > > > base::Bind(&talk_base::RefCountedInterface::Release, > > base::Unretained(source)); > > > > > > should allow you to drop the helper. > > 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, > > > > > > 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. 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. https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/m... File content/renderer/media/mock_media_stream_dependency_factory.cc (right): https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/m... content/renderer/media/mock_media_stream_dependency_factory.cc:127: MockRtcVideoCapturer( On 2014/05/12 19:46:28, Ami Fischman wrote: > File can be reverted out of this CL? Done. https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc (right): https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc:16: bool is_screencast) On 2014/05/12 19:46:28, Ami Fischman wrote: > \n unnecessary now Done. https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_capturer_adapter.h (right): https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:23: // PeerConnection access cricket::VideoCapturer from a libJingle worker thread. On 2014/05/12 19:46:28, Ami Fischman wrote: > Can you add a line here that explains how it is guaranteed that PeerConnection > doesn't call |this| (on the worker thread) after it has been deleted (on the > render thread)? Done. https://codereview.chromium.org/282523003/diff/60001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_capturer_adapter.h:49: private: On 2014/05/12 19:46:28, Ami Fischman wrote: > dup w/ l.35 Done.
LGTM https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:87: base::Bind(&ReleaseWebRtcSourceOnMainRenderThread, On 2014/05/12 20:35:11, perkj wrote: > On 2014/05/12 19:46:27, Ami Fischman wrote: > > On 2014/05/12 19:28:26, perkj wrote: > > > On 2014/05/12 18:13:33, Ami Fischman wrote: > > > > FWIW, > > > > base::Bind(&talk_base::RefCountedInterface::Release, > > > base::Unretained(source)); > > > > > > > > should allow you to drop the helper. > > > 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, > > > > > > > > > > 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. > > Humm, Can it be that Release returns an int in libjingle? Yeah; that'd do it. > Can I use base::Bind anyway? Yes; use base::IgnoreResult(&talk_base::RefCountInterface::Release) instead of just &talk_base::RefCountInterface::Release by itself. > If you don't mind I will not change this now but I would like to learn. np
lgtm
lgtm https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... File content/renderer/media/webrtc/webrtc_video_track_adapter.cc (right): https://codereview.chromium.org/282523003/diff/40001/content/renderer/media/w... content/renderer/media/webrtc/webrtc_video_track_adapter.cc:133: factory->GetWebRtcWorkerThread(), On 2014/05/12 19:28:26, perkj wrote: > On 2014/05/12 16:09:12, Ronghua Wu wrote: > > I think we should expose this interface on |capture_adapter|, e.g. inside > > WebRtcVideoSourceAdapter, gets the thread |capture_adapter| wants it to use. > > 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. In that case it makes sense to keep it in MediaStreamDependencyFactory.
The CQ bit was checked by perkj@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/perkj@chromium.org/282523003/80001
Message was sent while issue was closed.
Change committed as 270048
Message was sent while issue was closed.
On 2014/05/13 07:51:44, I haz the power (commit-bot) wrote:
Message was sent while issue was closed.
On 2014/05/13 07:51:44, I haz the power (commit-bot) wrote: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
