diff --git a/webrtc/api/api_tests.gyp b/webrtc/api/api_tests.gyp index 53285f90df..bb706dcf0f 100644 --- a/webrtc/api/api_tests.gyp +++ b/webrtc/api/api_tests.gyp @@ -37,14 +37,13 @@ 'fakemetricsobserver.h', 'jsepsessiondescription_unittest.cc', 'localaudiosource_unittest.cc', - 'mediaconstraintsinterface_unittest.cc', + 'mediaconstraintsinterface_unittest.cc', 'mediastream_unittest.cc', 'peerconnection_unittest.cc', 'peerconnectionendtoend_unittest.cc', 'peerconnectionfactory_unittest.cc', 'peerconnectioninterface_unittest.cc', # 'peerconnectionproxy_unittest.cc', - 'remotevideocapturer_unittest.cc', 'rtpsenderreceiver_unittest.cc', 'statscollector_unittest.cc', 'test/fakeaudiocapturemodule.cc', diff --git a/webrtc/api/mediastreaminterface.h b/webrtc/api/mediastreaminterface.h index 54d3fd48c3..69a50c97a3 100644 --- a/webrtc/api/mediastreaminterface.h +++ b/webrtc/api/mediastreaminterface.h @@ -177,26 +177,6 @@ class VideoTrackInterface virtual VideoTrackSourceInterface* GetSource() const = 0; - // Return the track input sink. I.e., frames sent to this sink are - // propagated to all renderers registered with the track. The - // returned sink must not change between calls. Currently, this - // method is used for remote tracks (VideoRtpReceiver); further - // refactoring is planned for this path, it's unclear if this method - // belongs here long term. - - // We do this instead of simply implementing the - // VideoSourceInterface directly, because if we did the latter, we'd - // need an OnFrame method in VideoTrackProxy, with a thread jump on - // each call. - - // TODO(nisse): It has a default implementation so that mock - // objects, in particular, chrome's MockWebRtcVideoTrack, doesn't - // need to know about it. Consider removing the implementation (or - // this comment) after refactoring dust settles. - virtual rtc::VideoSinkInterface* GetSink() { - return nullptr; - }; - protected: virtual ~VideoTrackInterface() {} }; diff --git a/webrtc/api/mediastreamtrackproxy.h b/webrtc/api/mediastreamtrackproxy.h index ada82919b0..676ba70982 100644 --- a/webrtc/api/mediastreamtrackproxy.h +++ b/webrtc/api/mediastreamtrackproxy.h @@ -54,7 +54,6 @@ BEGIN_PROXY_MAP(VideoTrack) const rtc::VideoSinkWants&) PROXY_METHOD1(void, RemoveSink, rtc::VideoSinkInterface*) PROXY_CONSTMETHOD0(VideoTrackSourceInterface*, GetSource) - PROXY_METHOD0(rtc::VideoSinkInterface*, GetSink) PROXY_METHOD1(void, RegisterObserver, ObserverInterface*) PROXY_METHOD1(void, UnregisterObserver, ObserverInterface*) diff --git a/webrtc/api/peerconnection.cc b/webrtc/api/peerconnection.cc index c2039aeea4..700714ae44 100644 --- a/webrtc/api/peerconnection.cc +++ b/webrtc/api/peerconnection.cc @@ -381,10 +381,8 @@ namespace webrtc { // Factory class for creating remote MediaStreams and MediaStreamTracks. class RemoteMediaStreamFactory { public: - explicit RemoteMediaStreamFactory(rtc::Thread* signaling_thread, - rtc::Thread* worker_thread) - : signaling_thread_(signaling_thread), - worker_thread_(worker_thread) {} + explicit RemoteMediaStreamFactory(rtc::Thread* signaling_thread) + : signaling_thread_(signaling_thread) {} rtc::scoped_refptr CreateMediaStream( const std::string& stream_label) { @@ -400,15 +398,6 @@ class RemoteMediaStreamFactory { stream, track_id, RemoteAudioSource::Create(ssrc, provider)); } - VideoTrackInterface* AddVideoTrack(webrtc::MediaStreamInterface* stream, - const std::string& track_id) { - return AddTrack( - stream, track_id, - VideoCapturerTrackSource::Create( - worker_thread_, new RemoteVideoCapturer(), nullptr, true) - .get()); - } - private: template TI* AddTrack(MediaStreamInterface* stream, @@ -424,7 +413,6 @@ class RemoteMediaStreamFactory { } rtc::Thread* signaling_thread_; - rtc::Thread* worker_thread_; }; bool ExtractMediaSessionOptions( @@ -611,8 +599,8 @@ bool PeerConnection::Initialize( media_controller_.reset(factory_->CreateMediaController(media_config)); - remote_stream_factory_.reset(new RemoteMediaStreamFactory( - factory_->signaling_thread(), factory_->worker_thread())); + remote_stream_factory_.reset( + new RemoteMediaStreamFactory(factory_->signaling_thread())); session_.reset( new WebRtcSession(media_controller_.get(), factory_->signaling_thread(), @@ -1325,11 +1313,12 @@ void PeerConnection::CreateAudioReceiver(MediaStreamInterface* stream, } void PeerConnection::CreateVideoReceiver(MediaStreamInterface* stream, - VideoTrackInterface* video_track, + const std::string& track_id, uint32_t ssrc) { - receivers_.push_back(RtpReceiverProxy::Create( - signaling_thread(), - new VideoRtpReceiver(video_track, ssrc, session_.get()))); + VideoRtpReceiver* video_receiver = new VideoRtpReceiver( + stream, track_id, factory_->worker_thread(), ssrc, session_.get()); + receivers_.push_back( + RtpReceiverProxy::Create(signaling_thread(), video_receiver)); } // TODO(deadbeef): Keep RtpReceivers around even if track goes away in remote @@ -1677,9 +1666,7 @@ void PeerConnection::OnRemoteTrackSeen(const std::string& stream_label, ssrc, session_.get(), stream, track_id); CreateAudioReceiver(stream, audio_track, ssrc); } else if (media_type == cricket::MEDIA_TYPE_VIDEO) { - VideoTrackInterface* video_track = - remote_stream_factory_->AddVideoTrack(stream, track_id); - CreateVideoReceiver(stream, video_track, ssrc); + CreateVideoReceiver(stream, track_id, ssrc); } else { RTC_DCHECK(false && "Invalid media type"); } @@ -1702,8 +1689,9 @@ void PeerConnection::OnRemoteTrackRemoved(const std::string& stream_label, rtc::scoped_refptr video_track = stream->FindVideoTrack(track_id); if (video_track) { - video_track->set_state(webrtc::MediaStreamTrackInterface::kEnded); stream->RemoveTrack(video_track); + // Stopping or destroying a VideoRtpReceiver will end the + // VideoRtpReceiver::track(). DestroyVideoReceiver(stream, video_track); } } else { diff --git a/webrtc/api/peerconnection.h b/webrtc/api/peerconnection.h index e4fcdf075a..2e9d9b22b8 100644 --- a/webrtc/api/peerconnection.h +++ b/webrtc/api/peerconnection.h @@ -27,6 +27,7 @@ namespace webrtc { class MediaStreamObserver; class RemoteMediaStreamFactory; +class VideoRtpReceiver; // Populates |session_options| from |rtc_options|, and returns true if options // are valid. @@ -167,8 +168,9 @@ class PeerConnection : public PeerConnectionInterface, void CreateAudioReceiver(MediaStreamInterface* stream, AudioTrackInterface* audio_track, uint32_t ssrc); + void CreateVideoReceiver(MediaStreamInterface* stream, - VideoTrackInterface* video_track, + const std::string& track_id, uint32_t ssrc); void DestroyAudioReceiver(MediaStreamInterface* stream, AudioTrackInterface* audio_track); diff --git a/webrtc/api/remotevideocapturer.cc b/webrtc/api/remotevideocapturer.cc index 8c1ffccb47..177ba54641 100644 --- a/webrtc/api/remotevideocapturer.cc +++ b/webrtc/api/remotevideocapturer.cc @@ -10,69 +10,4 @@ #include "webrtc/api/remotevideocapturer.h" -#include "webrtc/base/logging.h" -#include "webrtc/media/base/videoframe.h" - -namespace webrtc { - -RemoteVideoCapturer::RemoteVideoCapturer() {} - -RemoteVideoCapturer::~RemoteVideoCapturer() {} - -cricket::CaptureState RemoteVideoCapturer::Start( - const cricket::VideoFormat& capture_format) { - if (capture_state() == cricket::CS_RUNNING) { - LOG(LS_WARNING) - << "RemoteVideoCapturer::Start called when it's already started."; - return capture_state(); - } - - LOG(LS_INFO) << "RemoteVideoCapturer::Start"; - SetCaptureFormat(&capture_format); - return cricket::CS_RUNNING; -} - -void RemoteVideoCapturer::Stop() { - if (capture_state() == cricket::CS_STOPPED) { - LOG(LS_WARNING) - << "RemoteVideoCapturer::Stop called when it's already stopped."; - return; - } - - LOG(LS_INFO) << "RemoteVideoCapturer::Stop"; - SetCaptureFormat(NULL); - SetCaptureState(cricket::CS_STOPPED); -} - -bool RemoteVideoCapturer::IsRunning() { - return capture_state() == cricket::CS_RUNNING; -} - -bool RemoteVideoCapturer::GetPreferredFourccs(std::vector* fourccs) { - if (!fourccs) - return false; - fourccs->push_back(cricket::FOURCC_I420); - return true; -} - -bool RemoteVideoCapturer::GetBestCaptureFormat( - const cricket::VideoFormat& desired, cricket::VideoFormat* best_format) { - if (!best_format) { - return false; - } - - // RemoteVideoCapturer does not support capability enumeration. - // Use the desired format as the best format. - best_format->width = desired.width; - best_format->height = desired.height; - best_format->fourcc = cricket::FOURCC_I420; - best_format->interval = desired.interval; - return true; -} - -bool RemoteVideoCapturer::IsScreencast() const { - // TODO(ronghuawu): what about remote screencast stream. - return false; -} - -} // namespace webrtc +// TODO(perkj): Remove this file once Chrome gyp file doesn't depend on it. diff --git a/webrtc/api/remotevideocapturer.h b/webrtc/api/remotevideocapturer.h index a0fd4b7971..774cb5ac56 100644 --- a/webrtc/api/remotevideocapturer.h +++ b/webrtc/api/remotevideocapturer.h @@ -11,38 +11,6 @@ #ifndef WEBRTC_API_REMOTEVIDEOCAPTURER_H_ #define WEBRTC_API_REMOTEVIDEOCAPTURER_H_ -#include - -#include "webrtc/api/mediastreaminterface.h" -#include "webrtc/media/base/videocapturer.h" -#include "webrtc/media/base/videorenderer.h" - -namespace webrtc { - -// RemoteVideoCapturer implements a simple cricket::VideoCapturer which -// gets decoded remote video frames from media channel. -// It's used as the remote video source's VideoCapturer so that the remote video -// can be used as a cricket::VideoCapturer and in that way a remote video stream -// can implement the MediaStreamSourceInterface. -class RemoteVideoCapturer : public cricket::VideoCapturer { - public: - RemoteVideoCapturer(); - virtual ~RemoteVideoCapturer(); - - // cricket::VideoCapturer implementation. - cricket::CaptureState Start( - const cricket::VideoFormat& capture_format) override; - void Stop() override; - bool IsRunning() override; - bool GetPreferredFourccs(std::vector* fourccs) override; - bool GetBestCaptureFormat(const cricket::VideoFormat& desired, - cricket::VideoFormat* best_format) override; - bool IsScreencast() const override; - - private: - RTC_DISALLOW_COPY_AND_ASSIGN(RemoteVideoCapturer); -}; - -} // namespace webrtc +// TODO(perkj): Remove this file once Chrome gyp file doesn't depend on it. #endif // WEBRTC_API_REMOTEVIDEOCAPTURER_H_ diff --git a/webrtc/api/remotevideocapturer_unittest.cc b/webrtc/api/remotevideocapturer_unittest.cc deleted file mode 100644 index 6478d214c4..0000000000 --- a/webrtc/api/remotevideocapturer_unittest.cc +++ /dev/null @@ -1,97 +0,0 @@ -/* - * Copyright 2013 The WebRTC project authors. All Rights Reserved. - * - * Use of this source code is governed by a BSD-style license - * that can be found in the LICENSE file in the root of the source - * tree. An additional intellectual property rights grant can be found - * in the file PATENTS. All contributing project authors may - * be found in the AUTHORS file in the root of the source tree. - */ - -#include - -#include "webrtc/api/remotevideocapturer.h" -#include "webrtc/base/gunit.h" -#include "webrtc/media/engine/webrtcvideoframe.h" - -using cricket::CaptureState; -using cricket::VideoCapturer; -using cricket::VideoFormat; -using cricket::VideoFormatPod; -using cricket::VideoFrame; - -static const int kMaxWaitMs = 1000; -static const VideoFormatPod kTestFormat = - {640, 480, FPS_TO_INTERVAL(30), cricket::FOURCC_ANY}; - -class RemoteVideoCapturerTest : public testing::Test, - public sigslot::has_slots<> { - protected: - RemoteVideoCapturerTest() - : captured_frame_num_(0), - capture_state_(cricket::CS_STOPPED) {} - - virtual void SetUp() { - capturer_.SignalStateChange.connect( - this, &RemoteVideoCapturerTest::OnStateChange); - } - - ~RemoteVideoCapturerTest() { - capturer_.SignalStateChange.disconnect(this); - } - - int captured_frame_num() const { - return captured_frame_num_; - } - - CaptureState capture_state() const { - return capture_state_; - } - - webrtc::RemoteVideoCapturer capturer_; - - private: - void OnStateChange(VideoCapturer* capturer, - CaptureState capture_state) { - EXPECT_EQ(&capturer_, capturer); - capture_state_ = capture_state; - } - - int captured_frame_num_; - CaptureState capture_state_; -}; - -TEST_F(RemoteVideoCapturerTest, StartStop) { - // Start - EXPECT_TRUE( - capturer_.StartCapturing(VideoFormat(kTestFormat))); - EXPECT_TRUE_WAIT((cricket::CS_RUNNING == capture_state()), kMaxWaitMs); - EXPECT_EQ(VideoFormat(kTestFormat), - *capturer_.GetCaptureFormat()); - EXPECT_TRUE(capturer_.IsRunning()); - - // Stop - capturer_.Stop(); - EXPECT_TRUE_WAIT((cricket::CS_STOPPED == capture_state()), kMaxWaitMs); - EXPECT_TRUE(NULL == capturer_.GetCaptureFormat()); -} - -TEST_F(RemoteVideoCapturerTest, GetPreferredFourccs) { - EXPECT_FALSE(capturer_.GetPreferredFourccs(NULL)); - - std::vector fourccs; - EXPECT_TRUE(capturer_.GetPreferredFourccs(&fourccs)); - EXPECT_EQ(1u, fourccs.size()); - EXPECT_EQ(cricket::FOURCC_I420, fourccs.at(0)); -} - -TEST_F(RemoteVideoCapturerTest, GetBestCaptureFormat) { - VideoFormat desired = VideoFormat(kTestFormat); - EXPECT_FALSE(capturer_.GetBestCaptureFormat(desired, NULL)); - - VideoFormat expected_format = VideoFormat(kTestFormat); - expected_format.fourcc = cricket::FOURCC_I420; - VideoFormat best_format; - EXPECT_TRUE(capturer_.GetBestCaptureFormat(desired, &best_format)); - EXPECT_EQ(expected_format, best_format); -} diff --git a/webrtc/api/rtpreceiver.cc b/webrtc/api/rtpreceiver.cc index 0ad5218faf..15906120f6 100644 --- a/webrtc/api/rtpreceiver.cc +++ b/webrtc/api/rtpreceiver.cc @@ -10,7 +10,8 @@ #include "webrtc/api/rtpreceiver.h" -#include "webrtc/api/videosourceinterface.h" +#include "webrtc/api/mediastreamtrackproxy.h" +#include "webrtc/api/videotrack.h" namespace webrtc { @@ -65,11 +66,27 @@ void AudioRtpReceiver::Reconfigure() { provider_->SetAudioPlayout(ssrc_, track_->enabled()); } -VideoRtpReceiver::VideoRtpReceiver(VideoTrackInterface* track, +VideoRtpReceiver::VideoRtpReceiver(MediaStreamInterface* stream, + const std::string& track_id, + rtc::Thread* worker_thread, uint32_t ssrc, VideoProviderInterface* provider) - : id_(track->id()), track_(track), ssrc_(ssrc), provider_(provider) { - provider_->SetVideoPlayout(ssrc_, true, track_->GetSink()); + : id_(track_id), + ssrc_(ssrc), + provider_(provider), + source_(new RefCountedObject(&broadcaster_, + worker_thread, + true /* remote */)), + track_(VideoTrackProxy::Create( + rtc::Thread::Current(), + VideoTrack::Create(track_id, source_.get()))) { + source_->SetState(MediaSourceInterface::kLive); + // TODO(perkj): It should be enough to set the source state. All tracks + // belonging to the same source should get its state from the source. + // I.e. if a track has been cloned from a remote source. + track_->set_state(webrtc::MediaStreamTrackInterface::kLive); + provider_->SetVideoPlayout(ssrc_, true, &broadcaster_); + stream->AddTrack(track_); } VideoRtpReceiver::~VideoRtpReceiver() { @@ -83,6 +100,11 @@ void VideoRtpReceiver::Stop() { if (!provider_) { return; } + source_->SetState(MediaSourceInterface::kEnded); + source_->OnSourceDestroyed(); + // TODO(perkj): It should be enough to set the source state. All tracks + // belonging to the same source should get its state from the source. + track_->set_state(MediaStreamTrackInterface::kEnded); provider_->SetVideoPlayout(ssrc_, false, nullptr); provider_ = nullptr; } diff --git a/webrtc/api/rtpreceiver.h b/webrtc/api/rtpreceiver.h index 43adc72946..a62ed19dbf 100644 --- a/webrtc/api/rtpreceiver.h +++ b/webrtc/api/rtpreceiver.h @@ -19,7 +19,9 @@ #include "webrtc/api/mediastreamprovider.h" #include "webrtc/api/rtpreceiverinterface.h" +#include "webrtc/api/videotracksource.h" #include "webrtc/base/basictypes.h" +#include "webrtc/media/base/videobroadcaster.h" namespace webrtc { @@ -60,12 +62,18 @@ class AudioRtpReceiver : public ObserverInterface, class VideoRtpReceiver : public rtc::RefCountedObject { public: - VideoRtpReceiver(VideoTrackInterface* track, + VideoRtpReceiver(MediaStreamInterface* stream, + const std::string& track_id, + rtc::Thread* worker_thread, uint32_t ssrc, VideoProviderInterface* provider); virtual ~VideoRtpReceiver(); + rtc::scoped_refptr video_track() const { + return track_.get(); + } + // RtpReceiverInterface implementation rtc::scoped_refptr track() const override { return track_.get(); @@ -77,9 +85,16 @@ class VideoRtpReceiver : public rtc::RefCountedObject { private: std::string id_; - rtc::scoped_refptr track_; uint32_t ssrc_; VideoProviderInterface* provider_; + // |broadcaster_| is needed since the decoder can only handle one sink. + // It might be better if the decoder can handle multiple sinks and consider + // the VideoSinkWants. + rtc::VideoBroadcaster broadcaster_; + // |source_| is held here to be able to change the state of the source when + // the VideoRtpReceiver is stopped. + rtc::scoped_refptr source_; + rtc::scoped_refptr track_; }; } // namespace webrtc diff --git a/webrtc/api/rtpsenderreceiver_unittest.cc b/webrtc/api/rtpsenderreceiver_unittest.cc index 0a7c408bf0..22fa14f661 100644 --- a/webrtc/api/rtpsenderreceiver_unittest.cc +++ b/webrtc/api/rtpsenderreceiver_unittest.cc @@ -150,12 +150,11 @@ class RtpSenderReceiverTest : public testing::Test { } void CreateVideoRtpReceiver() { - AddVideoTrack(true); - EXPECT_CALL(video_provider_, - SetVideoPlayout(kVideoSsrc, true, - video_track_->GetSink())); - video_rtp_receiver_ = new VideoRtpReceiver(stream_->GetVideoTracks()[0], - kVideoSsrc, &video_provider_); + EXPECT_CALL(video_provider_, SetVideoPlayout(kVideoSsrc, true, _)); + video_rtp_receiver_ = + new VideoRtpReceiver(stream_, kVideoTrackId, rtc::Thread::Current(), + kVideoSsrc, &video_provider_); + video_track_ = video_rtp_receiver_->video_track(); } void DestroyAudioRtpReceiver() { @@ -244,6 +243,20 @@ TEST_F(RtpSenderReceiverTest, LocalVideoTrackDisable) { DestroyVideoRtpSender(); } +TEST_F(RtpSenderReceiverTest, RemoteVideoTrackState) { + CreateVideoRtpReceiver(); + + EXPECT_EQ(webrtc::MediaStreamTrackInterface::kLive, video_track_->state()); + EXPECT_EQ(webrtc::MediaSourceInterface::kLive, + video_track_->GetSource()->state()); + + DestroyVideoRtpReceiver(); + + EXPECT_EQ(webrtc::MediaStreamTrackInterface::kEnded, video_track_->state()); + EXPECT_EQ(webrtc::MediaSourceInterface::kEnded, + video_track_->GetSource()->state()); +} + TEST_F(RtpSenderReceiverTest, RemoteVideoTrackDisable) { CreateVideoRtpReceiver(); diff --git a/webrtc/api/videocapturertracksource_unittest.cc b/webrtc/api/videocapturertracksource_unittest.cc index b183421153..0a0687080c 100644 --- a/webrtc/api/videocapturertracksource_unittest.cc +++ b/webrtc/api/videocapturertracksource_unittest.cc @@ -182,27 +182,6 @@ TEST_F(VideoCapturerTrackSourceTest, StopRestart) { source_->Stop(); } -// Test start stop with a remote VideoSource - the video source that has a -// RemoteVideoCapturer and takes video frames from FrameInput. -TEST_F(VideoCapturerTrackSourceTest, StartStopRemote) { - source_ = VideoCapturerTrackSource::Create( - rtc::Thread::Current(), new webrtc::RemoteVideoCapturer(), NULL, true); - - ASSERT_TRUE(source_.get() != NULL); - EXPECT_TRUE(NULL != source_->GetVideoCapturer()); - - state_observer_.reset(new StateObserver(source_)); - source_->RegisterObserver(state_observer_.get()); - source_->AddOrUpdateSink(&renderer_, rtc::VideoSinkWants()); - - EXPECT_EQ_WAIT(MediaSourceInterface::kLive, state_observer_->state(), - kMaxWaitMs); - - source_->GetVideoCapturer()->Stop(); - EXPECT_EQ_WAIT(MediaSourceInterface::kEnded, state_observer_->state(), - kMaxWaitMs); -} - // Test that a VideoSource transition to kEnded if the capture device // fails. TEST_F(VideoCapturerTrackSourceTest, CameraFailed) { diff --git a/webrtc/api/videotrack.cc b/webrtc/api/videotrack.cc index 36d256f1e4..b2ea1f9dc2 100644 --- a/webrtc/api/videotrack.cc +++ b/webrtc/api/videotrack.cc @@ -50,10 +50,6 @@ void VideoTrack::RemoveSink( renderers_.RemoveSink(sink); } -rtc::VideoSinkInterface* VideoTrack::GetSink() { - return &renderers_; -} - bool VideoTrack::set_enabled(bool enable) { renderers_.SetEnabled(enable); return MediaStreamTrack::set_enabled(enable); diff --git a/webrtc/api/videotrack.h b/webrtc/api/videotrack.h index 066aa90a36..3a59504a38 100644 --- a/webrtc/api/videotrack.h +++ b/webrtc/api/videotrack.h @@ -33,7 +33,6 @@ class VideoTrack : public MediaStreamTrack { virtual VideoTrackSourceInterface* GetSource() const { return video_source_.get(); } - rtc::VideoSinkInterface* GetSink() override; virtual bool set_enabled(bool enable); virtual std::string kind() const; diff --git a/webrtc/api/videotrack_unittest.cc b/webrtc/api/videotrack_unittest.cc index 9f44b81ea6..008ba2181f 100644 --- a/webrtc/api/videotrack_unittest.cc +++ b/webrtc/api/videotrack_unittest.cc @@ -10,42 +10,37 @@ #include -#include "webrtc/api/remotevideocapturer.h" #include "webrtc/api/test/fakevideotrackrenderer.h" #include "webrtc/api/videocapturertracksource.h" #include "webrtc/api/videotrack.h" #include "webrtc/base/gunit.h" #include "webrtc/base/scoped_ptr.h" +#include "webrtc/media/base/fakevideocapturer.h" #include "webrtc/media/base/fakemediaengine.h" #include "webrtc/media/engine/webrtcvideoframe.h" -#include "webrtc/pc/channelmanager.h" using webrtc::FakeVideoTrackRenderer; using webrtc::FakeVideoTrackRendererOld; -using webrtc::VideoCapturerTrackSource; +using webrtc::VideoTrackSource; using webrtc::VideoTrack; using webrtc::VideoTrackInterface; -namespace { - -class WebRtcVideoTestFrame : public cricket::WebRtcVideoFrame { - public: - using cricket::WebRtcVideoFrame::SetRotation; -}; - -} // namespace class VideoTrackTest : public testing::Test { public: VideoTrackTest() { static const char kVideoTrackId[] = "track_id"; video_track_ = VideoTrack::Create( - kVideoTrackId, VideoCapturerTrackSource::Create( - rtc::Thread::Current(), - new webrtc::RemoteVideoCapturer(), NULL, true)); + kVideoTrackId, + new rtc::RefCountedObject( + &capturer_, rtc::Thread::Current(), true /* remote */)); + capturer_.Start( + cricket::VideoFormat(640, 480, cricket::VideoFormat::FpsToInterval(30), + cricket::FOURCC_I420)); } protected: + cricket::FakeVideoCapturer capturer_; rtc::scoped_refptr video_track_; }; @@ -56,35 +51,18 @@ TEST_F(VideoTrackTest, RenderVideo) { rtc::scoped_ptr renderer_1( new FakeVideoTrackRenderer(video_track_.get())); - rtc::VideoSinkInterface* renderer_input = - video_track_->GetSink(); - ASSERT_FALSE(renderer_input == NULL); - - cricket::WebRtcVideoFrame frame; - frame.InitToBlack(123, 123, 0); - renderer_input->OnFrame(frame); + capturer_.CaptureFrame(); EXPECT_EQ(1, renderer_1->num_rendered_frames()); - EXPECT_EQ(123, renderer_1->width()); - EXPECT_EQ(123, renderer_1->height()); - // FakeVideoTrackRenderer register itself to |video_track_| rtc::scoped_ptr renderer_2( new FakeVideoTrackRenderer(video_track_.get())); - - renderer_input->OnFrame(frame); - - EXPECT_EQ(123, renderer_1->width()); - EXPECT_EQ(123, renderer_1->height()); - EXPECT_EQ(123, renderer_2->width()); - EXPECT_EQ(123, renderer_2->height()); - + capturer_.CaptureFrame(); EXPECT_EQ(2, renderer_1->num_rendered_frames()); EXPECT_EQ(1, renderer_2->num_rendered_frames()); video_track_->RemoveSink(renderer_1.get()); - renderer_input->OnFrame(frame); - + capturer_.CaptureFrame(); EXPECT_EQ(2, renderer_1->num_rendered_frames()); EXPECT_EQ(2, renderer_2->num_rendered_frames()); } @@ -97,35 +75,19 @@ TEST_F(VideoTrackTest, RenderVideoOld) { rtc::scoped_ptr renderer_1( new FakeVideoTrackRendererOld(video_track_.get())); - rtc::VideoSinkInterface* renderer_input = - video_track_->GetSink(); - ASSERT_FALSE(renderer_input == NULL); - - cricket::WebRtcVideoFrame frame; - frame.InitToBlack(123, 123, 0); - renderer_input->OnFrame(frame); + capturer_.CaptureFrame(); EXPECT_EQ(1, renderer_1->num_rendered_frames()); - EXPECT_EQ(123, renderer_1->width()); - EXPECT_EQ(123, renderer_1->height()); - // FakeVideoTrackRenderer register itself to |video_track_| rtc::scoped_ptr renderer_2( new FakeVideoTrackRenderer(video_track_.get())); - renderer_input->OnFrame(frame); - - EXPECT_EQ(123, renderer_1->width()); - EXPECT_EQ(123, renderer_1->height()); - EXPECT_EQ(123, renderer_2->width()); - EXPECT_EQ(123, renderer_2->height()); - + capturer_.CaptureFrame(); EXPECT_EQ(2, renderer_1->num_rendered_frames()); EXPECT_EQ(1, renderer_2->num_rendered_frames()); video_track_->RemoveRenderer(renderer_1.get()); - renderer_input->OnFrame(frame); - + capturer_.CaptureFrame(); EXPECT_EQ(2, renderer_1->num_rendered_frames()); EXPECT_EQ(2, renderer_2->num_rendered_frames()); } @@ -135,32 +97,17 @@ TEST_F(VideoTrackTest, DisableTrackBlackout) { rtc::scoped_ptr renderer( new FakeVideoTrackRenderer(video_track_.get())); - rtc::VideoSinkInterface* renderer_input = - video_track_->GetSink(); - ASSERT_FALSE(renderer_input == NULL); - - cricket::WebRtcVideoFrame frame; - frame.InitToBlack(100, 200, 0); - // Make it not all-black - frame.GetUPlane()[0] = 0; - - renderer_input->OnFrame(frame); + capturer_.CaptureFrame(); EXPECT_EQ(1, renderer->num_rendered_frames()); EXPECT_FALSE(renderer->black_frame()); - EXPECT_EQ(100, renderer->width()); - EXPECT_EQ(200, renderer->height()); video_track_->set_enabled(false); - renderer_input->OnFrame(frame); + capturer_.CaptureFrame(); EXPECT_EQ(2, renderer->num_rendered_frames()); EXPECT_TRUE(renderer->black_frame()); - EXPECT_EQ(100, renderer->width()); - EXPECT_EQ(200, renderer->height()); video_track_->set_enabled(true); - renderer_input->OnFrame(frame); + capturer_.CaptureFrame(); EXPECT_EQ(3, renderer->num_rendered_frames()); EXPECT_FALSE(renderer->black_frame()); - EXPECT_EQ(100, renderer->width()); - EXPECT_EQ(200, renderer->height()); } diff --git a/webrtc/api/videotracksource.cc b/webrtc/api/videotracksource.cc index 3b5e19a5b7..f8212d7a70 100644 --- a/webrtc/api/videotracksource.cc +++ b/webrtc/api/videotracksource.cc @@ -32,9 +32,16 @@ void VideoTrackSource::SetState(SourceState new_state) { } } +void VideoTrackSource::OnSourceDestroyed() { + source_ = nullptr; +} + void VideoTrackSource::AddOrUpdateSink( rtc::VideoSinkInterface* sink, const rtc::VideoSinkWants& wants) { + if (!source_) { + return; + } worker_thread_->Invoke(rtc::Bind( &rtc::VideoSourceInterface::AddOrUpdateSink, source_, sink, wants)); @@ -42,6 +49,9 @@ void VideoTrackSource::AddOrUpdateSink( void VideoTrackSource::RemoveSink( rtc::VideoSinkInterface* sink) { + if (!source_) { + return; + } worker_thread_->Invoke( rtc::Bind(&rtc::VideoSourceInterface::RemoveSink, source_, sink)); diff --git a/webrtc/api/videotracksource.h b/webrtc/api/videotracksource.h index 59dae42950..5e2437c6b0 100644 --- a/webrtc/api/videotracksource.h +++ b/webrtc/api/videotracksource.h @@ -24,21 +24,27 @@ class VideoTrackSource : public Notifier { VideoTrackSource(rtc::VideoSourceInterface* source, rtc::Thread* worker_thread, bool remote); - void SetState(SourceState new_state); + // OnSourceDestroyed clears this instance pointer to |source_|. It is useful + // when the underlying rtc::VideoSourceInterface is destroyed before the + // reference counted VideoTrackSource. + void OnSourceDestroyed(); + SourceState state() const override { return state_; } bool remote() const override { return remote_; } void Stop() override{}; void Restart() override{}; - virtual bool is_screencast() const { return false; }; - virtual bool needs_denoising() const { return false; }; + virtual bool is_screencast() const { return false; } + virtual bool needs_denoising() const { return false; } void AddOrUpdateSink(rtc::VideoSinkInterface* sink, const rtc::VideoSinkWants& wants) override; void RemoveSink(rtc::VideoSinkInterface* sink) override; + cricket::VideoCapturer* GetVideoCapturer() override { return nullptr; } + protected: rtc::Thread* worker_thread() { return worker_thread_; } diff --git a/webrtc/media/base/videobroadcaster.cc b/webrtc/media/base/videobroadcaster.cc index 5fa3db733b..015f5206b6 100644 --- a/webrtc/media/base/videobroadcaster.cc +++ b/webrtc/media/base/videobroadcaster.cc @@ -25,6 +25,7 @@ void VideoBroadcaster::AddOrUpdateSink( const VideoSinkWants& wants) { RTC_DCHECK(thread_checker_.CalledOnValidThread()); RTC_DCHECK(sink != nullptr); + rtc::CritScope cs(&sinks_and_wants_lock_); SinkPair* sink_pair = FindSinkPair(sink); if (!sink_pair) { @@ -39,8 +40,8 @@ void VideoBroadcaster::RemoveSink( VideoSinkInterface* sink) { RTC_DCHECK(thread_checker_.CalledOnValidThread()); RTC_DCHECK(sink != nullptr); + rtc::CritScope cs(&sinks_and_wants_lock_); RTC_DCHECK(FindSinkPair(sink)); - sinks_.erase(std::remove_if(sinks_.begin(), sinks_.end(), [sink](const SinkPair& sink_pair) { return sink_pair.sink == sink; @@ -51,16 +52,18 @@ void VideoBroadcaster::RemoveSink( bool VideoBroadcaster::frame_wanted() const { RTC_DCHECK(thread_checker_.CalledOnValidThread()); + rtc::CritScope cs(&sinks_and_wants_lock_); return !sinks_.empty(); } VideoSinkWants VideoBroadcaster::wants() const { RTC_DCHECK(thread_checker_.CalledOnValidThread()); + rtc::CritScope cs(&sinks_and_wants_lock_); return current_wants_; } void VideoBroadcaster::OnFrame(const cricket::VideoFrame& frame) { - RTC_DCHECK(thread_checker_.CalledOnValidThread()); + rtc::CritScope cs(&sinks_and_wants_lock_); for (auto& sink_pair : sinks_) { sink_pair.sink->OnFrame(frame); } diff --git a/webrtc/media/base/videobroadcaster.h b/webrtc/media/base/videobroadcaster.h index 89c9f3dd49..b4227eaba6 100644 --- a/webrtc/media/base/videobroadcaster.h +++ b/webrtc/media/base/videobroadcaster.h @@ -14,6 +14,7 @@ #include #include +#include "webrtc/base/criticalsection.h" #include "webrtc/base/thread_checker.h" #include "webrtc/media/base/videoframe.h" #include "webrtc/media/base/videosinkinterface.h" @@ -21,6 +22,12 @@ namespace rtc { +// VideoBroadcaster broadcast video frames to sinks and combines +// VideoSinkWants from its sinks. It does that by implementing +// rtc::VideoSourceInterface and rtc::VideoSinkInterface. +// Sinks must be added and removed on one and only one thread. +// Video frames can be broadcasted on any thread. I.e VideoBroadcaster::OnFrame +// can be called on any thread. class VideoBroadcaster : public VideoSourceInterface, public VideoSinkInterface { public: @@ -46,13 +53,15 @@ class VideoBroadcaster : public VideoSourceInterface, VideoSinkInterface* sink; VideoSinkWants wants; }; - SinkPair* FindSinkPair(const VideoSinkInterface* sink); - void UpdateWants(); + SinkPair* FindSinkPair(const VideoSinkInterface* sink) + EXCLUSIVE_LOCKS_REQUIRED(sinks_and_wants_lock_); + void UpdateWants() EXCLUSIVE_LOCKS_REQUIRED(sinks_and_wants_lock_); ThreadChecker thread_checker_; + rtc::CriticalSection sinks_and_wants_lock_; - VideoSinkWants current_wants_; - std::vector sinks_; + VideoSinkWants current_wants_ GUARDED_BY(sinks_and_wants_lock_); + std::vector sinks_ GUARDED_BY(sinks_and_wants_lock_); }; } // namespace rtc diff --git a/webrtc/media/engine/webrtcvideoengine2.cc b/webrtc/media/engine/webrtcvideoengine2.cc index 02de6b8e6b..daffc2fd34 100644 --- a/webrtc/media/engine/webrtcvideoengine2.cc +++ b/webrtc/media/engine/webrtcvideoengine2.cc @@ -1616,6 +1616,8 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::SetCapturer( } } capturer_ = capturer; + // |capturer_->AddOrUpdateSink| may not be called while holding |lock_| since + // that might cause a lock order inversion. capturer_->AddOrUpdateSink(this, sink_wants_); return true; } @@ -1631,6 +1633,8 @@ bool WebRtcVideoChannel2::WebRtcVideoSendStream::DisconnectCapturer() { return false; } + // |capturer_->RemoveSink| may not be called while holding |lock_| since + // that might cause a lock order inversion. capturer_->RemoveSink(this); capturer_ = NULL; // Reset |cpu_restricted_counter_| if the capturer is changed. It is not @@ -1755,46 +1759,54 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::SetCodec( void WebRtcVideoChannel2::WebRtcVideoSendStream::SetSendParameters( const ChangedSendParameters& params) { - rtc::CritScope cs(&lock_); - // |recreate_stream| means construction-time parameters have changed and the - // sending stream needs to be reset with the new config. - bool recreate_stream = false; - if (params.rtcp_mode) { - parameters_.config.rtp.rtcp_mode = *params.rtcp_mode; - recreate_stream = true; - } + { + rtc::CritScope cs(&lock_); + // |recreate_stream| means construction-time parameters have changed and the + // sending stream needs to be reset with the new config. + bool recreate_stream = false; + if (params.rtcp_mode) { + parameters_.config.rtp.rtcp_mode = *params.rtcp_mode; + recreate_stream = true; + } + if (params.rtp_header_extensions) { + parameters_.config.rtp.extensions = *params.rtp_header_extensions; + recreate_stream = true; + } + if (params.max_bandwidth_bps) { + // Max bitrate has changed, reconfigure encoder settings on the next frame + // or stream recreation. + parameters_.max_bitrate_bps = *params.max_bandwidth_bps; + pending_encoder_reconfiguration_ = true; + } + if (params.conference_mode) { + parameters_.conference_mode = *params.conference_mode; + } + if (params.options) + SetOptions(*params.options); + + // Set codecs and options. + if (params.codec) { + SetCodec(*params.codec); + return; + } else if (params.conference_mode && parameters_.codec_settings) { + SetCodec(*parameters_.codec_settings); + return; + } + if (recreate_stream) { + LOG(LS_INFO) + << "RecreateWebRtcStream (send) because of SetSendParameters"; + RecreateWebRtcStream(); + } + } // release |lock_| + + // |capturer_->AddOrUpdateSink| may not be called while holding |lock_| since + // that might cause a lock order inversion. if (params.rtp_header_extensions) { - parameters_.config.rtp.extensions = *params.rtp_header_extensions; sink_wants_.rotation_applied = !ContainsHeaderExtension( *params.rtp_header_extensions, kRtpVideoRotationHeaderExtension); if (capturer_) { capturer_->AddOrUpdateSink(this, sink_wants_); } - recreate_stream = true; - } - if (params.max_bandwidth_bps) { - // Max bitrate has changed, reconfigure encoder settings on the next frame - // or stream recreation. - parameters_.max_bitrate_bps = *params.max_bandwidth_bps; - pending_encoder_reconfiguration_ = true; - } - if (params.conference_mode) { - parameters_.conference_mode = *params.conference_mode; - } - if (params.options) - SetOptions(*params.options); - - // Set codecs and options. - if (params.codec) { - SetCodec(*params.codec); - return; - } else if (params.conference_mode && parameters_.codec_settings) { - SetCodec(*parameters_.codec_settings); - return; - } - if (recreate_stream) { - LOG(LS_INFO) << "RecreateWebRtcStream (send) because of SetSendParameters"; - RecreateWebRtcStream(); } } @@ -1961,6 +1973,8 @@ void WebRtcVideoChannel2::WebRtcVideoSendStream::OnLoadUpdate(Load load) { sink_wants_.max_pixel_count = max_pixel_count; sink_wants_.max_pixel_count_step_up = max_pixel_count_step_up; } + // |capturer_->AddOrUpdateSink| may not be called while holding |lock_| since + // that might cause a lock order inversion. capturer_->AddOrUpdateSink(this, sink_wants_); }