diff --git a/call/video_receive_stream.h b/call/video_receive_stream.h index 7a6803d9e2..cc9020b5ed 100644 --- a/call/video_receive_stream.h +++ b/call/video_receive_stream.h @@ -215,6 +215,10 @@ class VideoReceiveStream { // Set if the stream is protected using FlexFEC. bool protected_by_flexfec = false; + // Optional callback sink to support additional packet handlsers such as + // FlexFec. + RtpPacketSinkInterface* packet_sink_ = nullptr; + // Map from rtx payload type -> media payload type. // For RTX to be enabled, both an SSRC and this mapping are needed. std::map rtx_associated_payload_types; @@ -277,13 +281,6 @@ class VideoReceiveStream { // TODO(pbos): Add info on currently-received codec to Stats. virtual Stats GetStats() const = 0; - // RtpDemuxer only forwards a given RTP packet to one sink. However, some - // sinks, such as FlexFEC, might wish to be informed of all of the packets - // a given sink receives (or any set of sinks). They may do so by registering - // themselves as secondary sinks. - virtual void AddSecondarySink(RtpPacketSinkInterface* sink) = 0; - virtual void RemoveSecondarySink(const RtpPacketSinkInterface* sink) = 0; - virtual std::vector GetSources() const = 0; // Sets a base minimum for the playout delay. Base minimum delay sets lower @@ -324,6 +321,16 @@ class VideoReceiveStream { virtual ~VideoReceiveStream() {} }; +class DEPRECATED_VideoReceiveStream : public VideoReceiveStream { + public: + // RtpDemuxer only forwards a given RTP packet to one sink. However, some + // sinks, such as FlexFEC, might wish to be informed of all of the packets + // a given sink receives (or any set of sinks). They may do so by registering + // themselves as secondary sinks. + virtual void AddSecondarySink(RtpPacketSinkInterface* sink) = 0; + virtual void RemoveSecondarySink(const RtpPacketSinkInterface* sink) = 0; +}; + } // namespace webrtc #endif // CALL_VIDEO_RECEIVE_STREAM_H_ diff --git a/media/engine/fake_webrtc_call.cc b/media/engine/fake_webrtc_call.cc index e320880b2d..77b5a5dcfe 100644 --- a/media/engine/fake_webrtc_call.cc +++ b/media/engine/fake_webrtc_call.cc @@ -326,10 +326,7 @@ void FakeVideoSendStream::InjectVideoSinkWants( FakeVideoReceiveStream::FakeVideoReceiveStream( webrtc::VideoReceiveStream::Config config) - : config_(std::move(config)), - receiving_(false), - num_added_secondary_sinks_(0), - num_removed_secondary_sinks_(0) {} + : config_(std::move(config)), receiving_(false) {} const webrtc::VideoReceiveStream::Config& FakeVideoReceiveStream::GetConfig() const { @@ -361,24 +358,6 @@ void FakeVideoReceiveStream::SetStats( stats_ = stats; } -void FakeVideoReceiveStream::AddSecondarySink( - webrtc::RtpPacketSinkInterface* sink) { - ++num_added_secondary_sinks_; -} - -void FakeVideoReceiveStream::RemoveSecondarySink( - const webrtc::RtpPacketSinkInterface* sink) { - ++num_removed_secondary_sinks_; -} - -int FakeVideoReceiveStream::GetNumAddedSecondarySinks() const { - return num_added_secondary_sinks_; -} - -int FakeVideoReceiveStream::GetNumRemovedSecondarySinks() const { - return num_removed_secondary_sinks_; -} - FakeFlexfecReceiveStream::FakeFlexfecReceiveStream( const webrtc::FlexfecReceiveStream::Config& config) : config_(config) {} diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h index b0d797b8e8..25d43da62f 100644 --- a/media/engine/fake_webrtc_call.h +++ b/media/engine/fake_webrtc_call.h @@ -219,12 +219,6 @@ class FakeVideoReceiveStream final : public webrtc::VideoReceiveStream { void SetStats(const webrtc::VideoReceiveStream::Stats& stats); - void AddSecondarySink(webrtc::RtpPacketSinkInterface* sink) override; - void RemoveSecondarySink(const webrtc::RtpPacketSinkInterface* sink) override; - - int GetNumAddedSecondarySinks() const; - int GetNumRemovedSecondarySinks() const; - std::vector GetSources() const override { return std::vector(); } @@ -267,9 +261,6 @@ class FakeVideoReceiveStream final : public webrtc::VideoReceiveStream { webrtc::VideoReceiveStream::Stats stats_; int base_mininum_playout_delay_ms_ = 0; - - int num_added_secondary_sinks_; - int num_removed_secondary_sinks_; }; class FakeFlexfecReceiveStream final : public webrtc::FlexfecReceiveStream { diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index d402b5ce69..6c8b72bc44 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -626,11 +626,11 @@ WebRtcVideoEngine::WebRtcVideoEngine( : decoder_factory_(std::move(video_decoder_factory)), encoder_factory_(std::move(video_encoder_factory)), trials_(trials) { - RTC_LOG(LS_INFO) << "WebRtcVideoEngine::WebRtcVideoEngine()"; + RTC_DLOG(LS_INFO) << "WebRtcVideoEngine::WebRtcVideoEngine()"; } WebRtcVideoEngine::~WebRtcVideoEngine() { - RTC_LOG(LS_INFO) << "WebRtcVideoEngine::~WebRtcVideoEngine"; + RTC_DLOG(LS_INFO) << "WebRtcVideoEngine::~WebRtcVideoEngine"; } VideoMediaChannel* WebRtcVideoEngine::CreateMediaChannel( @@ -1214,25 +1214,25 @@ bool WebRtcVideoChannel::GetChangedRecvParameters( bool WebRtcVideoChannel::SetRecvParameters(const VideoRecvParameters& params) { RTC_DCHECK_RUN_ON(&thread_checker_); TRACE_EVENT0("webrtc", "WebRtcVideoChannel::SetRecvParameters"); - RTC_LOG(LS_INFO) << "SetRecvParameters: " << params.ToString(); + RTC_DLOG(LS_INFO) << "SetRecvParameters: " << params.ToString(); ChangedRecvParameters changed_params; if (!GetChangedRecvParameters(params, &changed_params)) { return false; } if (changed_params.flexfec_payload_type) { - RTC_LOG(LS_INFO) << "Changing FlexFEC payload type (recv) from " - << recv_flexfec_payload_type_ << " to " - << *changed_params.flexfec_payload_type; + RTC_DLOG(LS_INFO) << "Changing FlexFEC payload type (recv) from " + << recv_flexfec_payload_type_ << " to " + << *changed_params.flexfec_payload_type; recv_flexfec_payload_type_ = *changed_params.flexfec_payload_type; } if (changed_params.rtp_header_extensions) { recv_rtp_extensions_ = *changed_params.rtp_header_extensions; } if (changed_params.codec_settings) { - RTC_LOG(LS_INFO) << "Changing recv codecs from " - << CodecSettingsVectorToString(recv_codecs_) << " to " - << CodecSettingsVectorToString( - *changed_params.codec_settings); + RTC_DLOG(LS_INFO) << "Changing recv codecs from " + << CodecSettingsVectorToString(recv_codecs_) << " to " + << CodecSettingsVectorToString( + *changed_params.codec_settings); recv_codecs_ = *changed_params.codec_settings; } @@ -2782,17 +2782,14 @@ WebRtcVideoChannel::WebRtcVideoReceiveStream::WebRtcVideoReceiveStream( estimated_remote_start_ntp_time_ms_(0) { config_.renderer = this; ConfigureCodecs(recv_codecs); - ConfigureFlexfecCodec(flexfec_config.payload_type); - MaybeRecreateWebRtcFlexfecStream(); + flexfec_config_.payload_type = flexfec_config.payload_type; RecreateWebRtcVideoStream(); } WebRtcVideoChannel::WebRtcVideoReceiveStream::~WebRtcVideoReceiveStream() { - if (flexfec_stream_) { - MaybeDissociateFlexfecFromVideo(); - call_->DestroyFlexfecReceiveStream(flexfec_stream_); - } call_->DestroyVideoReceiveStream(stream_); + if (flexfec_stream_) + call_->DestroyFlexfecReceiveStream(flexfec_stream_); } const std::vector& @@ -2862,11 +2859,6 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::ConfigureCodecs( } } -void WebRtcVideoChannel::WebRtcVideoReceiveStream::ConfigureFlexfecCodec( - int flexfec_payload_type) { - flexfec_config_.payload_type = flexfec_payload_type; -} - void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetLocalSsrc( uint32_t local_ssrc) { // TODO(pbos): Consider turning this sanity check into a RTC_DCHECK. You @@ -2885,7 +2877,6 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetLocalSsrc( RTC_LOG(LS_INFO) << "RecreateWebRtcVideoStream (recv) because of SetLocalSsrc; local_ssrc=" << local_ssrc; - MaybeRecreateWebRtcFlexfecStream(); RecreateWebRtcVideoStream(); } @@ -2917,14 +2908,12 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFeedbackParameters( RTC_LOG(LS_INFO) << "RecreateWebRtcVideoStream (recv) because of " "SetFeedbackParameters; nack=" << nack_enabled << ", transport_cc=" << transport_cc_enabled; - MaybeRecreateWebRtcFlexfecStream(); RecreateWebRtcVideoStream(); } void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetRecvParameters( const ChangedRecvParameters& params) { bool video_needs_recreation = false; - bool flexfec_needs_recreation = false; if (params.codec_settings) { ConfigureCodecs(*params.codec_settings); video_needs_recreation = true; @@ -2933,20 +2922,16 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetRecvParameters( config_.rtp.extensions = *params.rtp_header_extensions; flexfec_config_.rtp_header_extensions = *params.rtp_header_extensions; video_needs_recreation = true; - flexfec_needs_recreation = true; } if (params.flexfec_payload_type) { - ConfigureFlexfecCodec(*params.flexfec_payload_type); - flexfec_needs_recreation = true; - } - if (flexfec_needs_recreation) { - RTC_LOG(LS_INFO) << "MaybeRecreateWebRtcFlexfecStream (recv) because of " - "SetRecvParameters"; - MaybeRecreateWebRtcFlexfecStream(); + flexfec_config_.payload_type = *params.flexfec_payload_type; + // TODO(tommi): See if it is better to always have a flexfec stream object + // configured and instead of recreating the video stream, reconfigure the + // flexfec object from within the rtp callback (soon to be on the network + // thread). + video_needs_recreation = true; } if (video_needs_recreation) { - RTC_LOG(LS_INFO) - << "RecreateWebRtcVideoStream (recv) because of SetRecvParameters"; RecreateWebRtcVideoStream(); } } @@ -2959,12 +2944,22 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::RecreateWebRtcVideoStream() { recording_state = stream_->SetAndGetRecordingState( webrtc::VideoReceiveStream::RecordingState(), /*generate_key_frame=*/false); - MaybeDissociateFlexfecFromVideo(); call_->DestroyVideoReceiveStream(stream_); stream_ = nullptr; } + + if (flexfec_stream_) { + call_->DestroyFlexfecReceiveStream(flexfec_stream_); + flexfec_stream_ = nullptr; + } + + if (flexfec_config_.IsCompleteAndEnabled()) { + flexfec_stream_ = call_->CreateFlexfecReceiveStream(flexfec_config_); + } + webrtc::VideoReceiveStream::Config config = config_.Copy(); config.rtp.protected_by_flexfec = (flexfec_stream_ != nullptr); + config.rtp.packet_sink_ = flexfec_stream_; config.stream_id = stream_params_.id; stream_ = call_->CreateVideoReceiveStream(std::move(config)); if (base_minimum_playout_delay_ms) { @@ -2975,7 +2970,7 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::RecreateWebRtcVideoStream() { stream_->SetAndGetRecordingState(std::move(*recording_state), /*generate_key_frame=*/false); } - MaybeAssociateFlexfecWithVideo(); + stream_->Start(); if (IsEnabled(call_->trials(), "WebRTC-Video-BufferPacketsWithUnknownSsrc")) { @@ -2983,33 +2978,6 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::RecreateWebRtcVideoStream() { } } -void WebRtcVideoChannel::WebRtcVideoReceiveStream:: - MaybeRecreateWebRtcFlexfecStream() { - if (flexfec_stream_) { - MaybeDissociateFlexfecFromVideo(); - call_->DestroyFlexfecReceiveStream(flexfec_stream_); - flexfec_stream_ = nullptr; - } - if (flexfec_config_.IsCompleteAndEnabled()) { - flexfec_stream_ = call_->CreateFlexfecReceiveStream(flexfec_config_); - MaybeAssociateFlexfecWithVideo(); - } -} - -void WebRtcVideoChannel::WebRtcVideoReceiveStream:: - MaybeAssociateFlexfecWithVideo() { - if (stream_ && flexfec_stream_) { - stream_->AddSecondarySink(flexfec_stream_); - } -} - -void WebRtcVideoChannel::WebRtcVideoReceiveStream:: - MaybeDissociateFlexfecFromVideo() { - if (stream_ && flexfec_stream_) { - stream_->RemoveSecondarySink(flexfec_stream_); - } -} - void WebRtcVideoChannel::WebRtcVideoReceiveStream::OnFrame( const webrtc::VideoFrame& frame) { webrtc::MutexLock lock(&sink_lock_); diff --git a/media/engine/webrtc_video_engine.h b/media/engine/webrtc_video_engine.h index 5a3cd81c17..f278897eb5 100644 --- a/media/engine/webrtc_video_engine.h +++ b/media/engine/webrtc_video_engine.h @@ -483,13 +483,8 @@ class WebRtcVideoChannel : public VideoMediaChannel, private: void RecreateWebRtcVideoStream(); - void MaybeRecreateWebRtcFlexfecStream(); - - void MaybeAssociateFlexfecWithVideo(); - void MaybeDissociateFlexfecFromVideo(); void ConfigureCodecs(const std::vector& recv_codecs); - void ConfigureFlexfecCodec(int flexfec_payload_type); std::string GetCodecNameFromPayloadType(int payload_type); diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 7a8c1de7f3..1f7a0eee62 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -4081,8 +4081,10 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, SetDefaultRecvCodecsWithoutSsrc) { fake_call_->GetVideoReceiveStreams(); ASSERT_EQ(1U, video_streams.size()); const FakeVideoReceiveStream& video_stream = *video_streams.front(); - EXPECT_EQ(0, video_stream.GetNumAddedSecondarySinks()); - EXPECT_EQ(0, video_stream.GetNumRemovedSecondarySinks()); + const webrtc::VideoReceiveStream::Config& video_config = + video_stream.GetConfig(); + EXPECT_FALSE(video_config.rtp.protected_by_flexfec); + EXPECT_EQ(video_config.rtp.packet_sink_, nullptr); } TEST_F(WebRtcVideoChannelFlexfecRecvTest, SetDefaultRecvCodecsWithSsrc) { @@ -4103,14 +4105,17 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, SetDefaultRecvCodecsWithSsrc) { fake_call_->GetVideoReceiveStreams(); ASSERT_EQ(1U, video_streams.size()); const FakeVideoReceiveStream& video_stream = *video_streams.front(); - EXPECT_EQ(1, video_stream.GetNumAddedSecondarySinks()); const webrtc::VideoReceiveStream::Config& video_config = video_stream.GetConfig(); EXPECT_TRUE(video_config.rtp.protected_by_flexfec); + EXPECT_NE(video_config.rtp.packet_sink_, nullptr); } +// Test changing the configuration after a video stream has been created and +// turn on flexfec. This will result in the video stream being recreated because +// the flexfec stream pointer is injected to the video stream at construction. TEST_F(WebRtcVideoChannelFlexfecRecvTest, - EnablingFlexfecDoesNotRecreateVideoReceiveStream) { + EnablingFlexfecRecreatesVideoReceiveStream) { cricket::VideoRecvParameters recv_parameters; recv_parameters.codecs.push_back(GetEngineCodec("VP8")); ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); @@ -4121,25 +4126,37 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, const std::vector& video_streams = fake_call_->GetVideoReceiveStreams(); ASSERT_EQ(1U, video_streams.size()); - const FakeVideoReceiveStream& video_stream = *video_streams.front(); - EXPECT_EQ(0, video_stream.GetNumAddedSecondarySinks()); - EXPECT_EQ(0, video_stream.GetNumRemovedSecondarySinks()); + const FakeVideoReceiveStream* video_stream = video_streams.front(); + const webrtc::VideoReceiveStream::Config* video_config = + &video_stream->GetConfig(); + EXPECT_FALSE(video_config->rtp.protected_by_flexfec); + EXPECT_EQ(video_config->rtp.packet_sink_, nullptr); // Enable FlexFEC. recv_parameters.codecs.push_back(GetEngineCodec("flexfec-03")); ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); - EXPECT_EQ(2, fake_call_->GetNumCreatedReceiveStreams()) + + // Now the count of created streams will be 3 since the video stream was + // recreated and a flexfec stream was created. + EXPECT_EQ(3, fake_call_->GetNumCreatedReceiveStreams()) << "Enabling FlexFEC should create FlexfecReceiveStream."; + EXPECT_EQ(1U, fake_call_->GetVideoReceiveStreams().size()) << "Enabling FlexFEC should not create VideoReceiveStream."; EXPECT_EQ(1U, fake_call_->GetFlexfecReceiveStreams().size()) << "Enabling FlexFEC should create a single FlexfecReceiveStream."; - EXPECT_EQ(1, video_stream.GetNumAddedSecondarySinks()); - EXPECT_EQ(0, video_stream.GetNumRemovedSecondarySinks()); + video_stream = video_streams.front(); + video_config = &video_stream->GetConfig(); + EXPECT_TRUE(video_config->rtp.protected_by_flexfec); + EXPECT_NE(video_config->rtp.packet_sink_, nullptr); } +// Test changing the configuration after a video stream has been created with +// flexfec enabled and then turn off flexfec. This will result in the video +// stream being recreated because the flexfec stream pointer is injected to the +// video stream at construction and that config needs to be torn down. TEST_F(WebRtcVideoChannelFlexfecRecvTest, - DisablingFlexfecDoesNotRecreateVideoReceiveStream) { + DisablingFlexfecRecreatesVideoReceiveStream) { cricket::VideoRecvParameters recv_parameters; recv_parameters.codecs.push_back(GetEngineCodec("VP8")); recv_parameters.codecs.push_back(GetEngineCodec("flexfec-03")); @@ -4152,22 +4169,28 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, const std::vector& video_streams = fake_call_->GetVideoReceiveStreams(); ASSERT_EQ(1U, video_streams.size()); - const FakeVideoReceiveStream& video_stream = *video_streams.front(); - EXPECT_EQ(1, video_stream.GetNumAddedSecondarySinks()); - EXPECT_EQ(0, video_stream.GetNumRemovedSecondarySinks()); + const FakeVideoReceiveStream* video_stream = video_streams.front(); + const webrtc::VideoReceiveStream::Config* video_config = + &video_stream->GetConfig(); + EXPECT_TRUE(video_config->rtp.protected_by_flexfec); + EXPECT_NE(video_config->rtp.packet_sink_, nullptr); // Disable FlexFEC. recv_parameters.codecs.clear(); recv_parameters.codecs.push_back(GetEngineCodec("VP8")); ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); - EXPECT_EQ(2, fake_call_->GetNumCreatedReceiveStreams()) + // Now the count of created streams will be 3 since the video stream had to + // be recreated on account of the flexfec stream being deleted. + EXPECT_EQ(3, fake_call_->GetNumCreatedReceiveStreams()) << "Disabling FlexFEC should not recreate VideoReceiveStream."; EXPECT_EQ(1U, fake_call_->GetVideoReceiveStreams().size()) << "Disabling FlexFEC should not destroy VideoReceiveStream."; EXPECT_TRUE(fake_call_->GetFlexfecReceiveStreams().empty()) << "Disabling FlexFEC should destroy FlexfecReceiveStream."; - EXPECT_EQ(1, video_stream.GetNumAddedSecondarySinks()); - EXPECT_EQ(1, video_stream.GetNumRemovedSecondarySinks()); + video_stream = video_streams.front(); + video_config = &video_stream->GetConfig(); + EXPECT_FALSE(video_config->rtp.protected_by_flexfec); + EXPECT_EQ(video_config->rtp.packet_sink_, nullptr); } TEST_F(WebRtcVideoChannelFlexfecRecvTest, DuplicateFlexfecCodecIsDropped) { diff --git a/test/call_test.cc b/test/call_test.cc index dd7c576ef9..0ba947ce08 100644 --- a/test/call_test.cc +++ b/test/call_test.cc @@ -447,8 +447,10 @@ void CallTest::CreateMatchingFecConfig( config.remote_ssrc = send_config.rtp.flexfec.ssrc; config.protected_media_ssrcs = send_config.rtp.flexfec.protected_media_ssrcs; config.local_ssrc = kReceiverLocalVideoSsrc; - if (!video_receive_configs_.empty()) + if (!video_receive_configs_.empty()) { video_receive_configs_[0].rtp.protected_by_flexfec = true; + video_receive_configs_[0].rtp.packet_sink_ = this; + } flexfec_receive_configs_.push_back(config); } @@ -510,8 +512,6 @@ void CallTest::CreateVideoStreams() { video_receive_streams_.push_back(receiver_call_->CreateVideoReceiveStream( video_receive_configs_[i].Copy())); } - - AssociateFlexfecStreamsWithVideoStreams(); } void CallTest::CreateVideoSendStreams() { @@ -572,8 +572,6 @@ void CallTest::CreateFlexfecStreams() { receiver_call_->CreateFlexfecReceiveStream( flexfec_receive_configs_[i])); } - - AssociateFlexfecStreamsWithVideoStreams(); } void CallTest::ConnectVideoSourcesToStreams() { @@ -582,23 +580,6 @@ void CallTest::ConnectVideoSourcesToStreams() { degradation_preference_); } -void CallTest::AssociateFlexfecStreamsWithVideoStreams() { - // All FlexFEC streams protect all of the video streams. - for (FlexfecReceiveStream* flexfec_recv_stream : flexfec_receive_streams_) { - for (VideoReceiveStream* video_recv_stream : video_receive_streams_) { - video_recv_stream->AddSecondarySink(flexfec_recv_stream); - } - } -} - -void CallTest::DissociateFlexfecStreamsFromVideoStreams() { - for (FlexfecReceiveStream* flexfec_recv_stream : flexfec_receive_streams_) { - for (VideoReceiveStream* video_recv_stream : video_receive_streams_) { - video_recv_stream->RemoveSecondarySink(flexfec_recv_stream); - } - } -} - void CallTest::Start() { StartVideoStreams(); if (audio_send_stream_) { @@ -632,8 +613,6 @@ void CallTest::StopVideoStreams() { } void CallTest::DestroyStreams() { - DissociateFlexfecStreamsFromVideoStreams(); - if (audio_send_stream_) sender_call_->DestroyAudioSendStream(audio_send_stream_); audio_send_stream_ = nullptr; @@ -691,6 +670,12 @@ FlexfecReceiveStream::Config* CallTest::GetFlexFecConfig() { return &flexfec_receive_configs_[0]; } +void CallTest::OnRtpPacket(const RtpPacketReceived& packet) { + // All FlexFEC streams protect all of the video streams. + for (FlexfecReceiveStream* flexfec_recv_stream : flexfec_receive_streams_) + flexfec_recv_stream->OnRtpPacket(packet); +} + absl::optional CallTest::GetRtpExtensionByUri( const std::string& uri) const { for (const auto& extension : rtp_extensions_) { diff --git a/test/call_test.h b/test/call_test.h index 4b26097b6c..adb21dd7f0 100644 --- a/test/call_test.h +++ b/test/call_test.h @@ -38,7 +38,7 @@ namespace test { class BaseTest; -class CallTest : public ::testing::Test { +class CallTest : public ::testing::Test, public RtpPacketSinkInterface { public: CallTest(); virtual ~CallTest(); @@ -156,9 +156,6 @@ class CallTest : public ::testing::Test { void ConnectVideoSourcesToStreams(); - void AssociateFlexfecStreamsWithVideoStreams(); - void DissociateFlexfecStreamsFromVideoStreams(); - void Start(); void StartVideoStreams(); void Stop(); @@ -177,6 +174,9 @@ class CallTest : public ::testing::Test { FlexfecReceiveStream::Config* GetFlexFecConfig(); TaskQueueBase* task_queue() { return task_queue_.get(); } + // RtpPacketSinkInterface implementation. + void OnRtpPacket(const RtpPacketReceived& packet) override; + test::RunLoop loop_; Clock* const clock_; diff --git a/video/rtp_video_stream_receiver2.cc b/video/rtp_video_stream_receiver2.cc index 38c10b907f..9834e064ab 100644 --- a/video/rtp_video_stream_receiver2.cc +++ b/video/rtp_video_stream_receiver2.cc @@ -316,8 +316,6 @@ RtpVideoStreamReceiver2::RtpVideoStreamReceiver2( } RtpVideoStreamReceiver2::~RtpVideoStreamReceiver2() { - RTC_DCHECK(secondary_sinks_.empty()); - process_thread_->DeRegisterModule(rtp_rtcp_.get()); if (packet_router_) @@ -675,8 +673,8 @@ void RtpVideoStreamReceiver2::OnRtpPacket(const RtpPacketReceived& packet) { rtp_receive_statistics_->OnRtpPacket(packet); } - for (RtpPacketSinkInterface* secondary_sink : secondary_sinks_) { - secondary_sink->OnRtpPacket(packet); + if (config_.rtp.packet_sink_) { + config_.rtp.packet_sink_->OnRtpPacket(packet); } } @@ -923,26 +921,6 @@ absl::optional RtpVideoStreamReceiver2::LastReceivedKeyframePacketMs() return packet_buffer_.LastReceivedKeyframePacketMs(); } -void RtpVideoStreamReceiver2::AddSecondarySink(RtpPacketSinkInterface* sink) { - RTC_DCHECK_RUN_ON(&worker_task_checker_); - RTC_DCHECK(!absl::c_linear_search(secondary_sinks_, sink)); - secondary_sinks_.push_back(sink); -} - -void RtpVideoStreamReceiver2::RemoveSecondarySink( - const RtpPacketSinkInterface* sink) { - RTC_DCHECK_RUN_ON(&worker_task_checker_); - auto it = absl::c_find(secondary_sinks_, sink); - if (it == secondary_sinks_.end()) { - // We might be rolling-back a call whose setup failed mid-way. In such a - // case, it's simpler to remove "everything" rather than remember what - // has already been added. - RTC_LOG(LS_WARNING) << "Removal of unknown sink."; - return; - } - secondary_sinks_.erase(it); -} - void RtpVideoStreamReceiver2::ManageFrame( std::unique_ptr frame) { RTC_DCHECK_RUN_ON(&worker_task_checker_); diff --git a/video/rtp_video_stream_receiver2.h b/video/rtp_video_stream_receiver2.h index 053e96366f..616aeafeb7 100644 --- a/video/rtp_video_stream_receiver2.h +++ b/video/rtp_video_stream_receiver2.h @@ -175,13 +175,6 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender, absl::optional LastReceivedPacketMs() const; absl::optional LastReceivedKeyframePacketMs() const; - // RtpDemuxer only forwards a given RTP packet to one sink. However, some - // sinks, such as FlexFEC, might wish to be informed of all of the packets - // a given sink receives (or any set of sinks). They may do so by registering - // themselves as secondary sinks. - void AddSecondarySink(RtpPacketSinkInterface* sink); - void RemoveSecondarySink(const RtpPacketSinkInterface* sink); - private: // Implements RtpVideoFrameReceiver. void ManageFrame( @@ -339,9 +332,6 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender, bool has_received_frame_ RTC_GUARDED_BY(worker_task_checker_); - std::vector secondary_sinks_ - RTC_GUARDED_BY(worker_task_checker_); - absl::optional last_received_rtp_timestamp_ RTC_GUARDED_BY(worker_task_checker_); absl::optional last_received_rtp_system_time_ms_ diff --git a/video/rtp_video_stream_receiver2_unittest.cc b/video/rtp_video_stream_receiver2_unittest.cc index 185cdbbb9c..9684b4ac3a 100644 --- a/video/rtp_video_stream_receiver2_unittest.cc +++ b/video/rtp_video_stream_receiver2_unittest.cc @@ -145,11 +145,11 @@ class MockRtpPacketSink : public RtpPacketSinkInterface { }; constexpr uint32_t kSsrc = 111; -constexpr uint16_t kSequenceNumber = 222; constexpr int kPayloadType = 100; constexpr int kRedPayloadType = 125; std::unique_ptr CreateRtpPacketReceived() { + constexpr uint16_t kSequenceNumber = 222; auto packet = std::make_unique(); packet->SetSsrc(kSsrc); packet->SetSequenceNumber(kSequenceNumber); @@ -164,7 +164,8 @@ MATCHER_P(SamePacketAs, other, "") { } // namespace -class RtpVideoStreamReceiver2Test : public ::testing::Test { +class RtpVideoStreamReceiver2Test : public ::testing::Test, + public RtpPacketSinkInterface { public: RtpVideoStreamReceiver2Test() : RtpVideoStreamReceiver2Test("") {} explicit RtpVideoStreamReceiver2Test(std::string field_trials) @@ -228,12 +229,18 @@ class RtpVideoStreamReceiver2Test : public ::testing::Test { h264.nalus[h264.nalus_length++] = info; } + void OnRtpPacket(const RtpPacketReceived& packet) override { + if (test_packet_sink_) + test_packet_sink_->OnRtpPacket(packet); + } + protected: - static VideoReceiveStream::Config CreateConfig() { + VideoReceiveStream::Config CreateConfig() { VideoReceiveStream::Config config(nullptr); config.rtp.remote_ssrc = 1111; config.rtp.local_ssrc = 2222; config.rtp.red_payload_type = kRedPayloadType; + config.rtp.packet_sink_ = this; return config; } @@ -249,6 +256,7 @@ class RtpVideoStreamReceiver2Test : public ::testing::Test { std::unique_ptr process_thread_; std::unique_ptr rtp_receive_statistics_; std::unique_ptr rtp_video_stream_receiver_; + RtpPacketSinkInterface* test_packet_sink_ = nullptr; }; TEST_F(RtpVideoStreamReceiver2Test, CacheColorSpaceFromLastPacketOfKeyframe) { @@ -755,83 +763,36 @@ TEST_F(RtpVideoStreamReceiver2Test, RequestKeyframeWhenPacketBufferGetsFull) { video_header); } -TEST_F(RtpVideoStreamReceiver2Test, SecondarySinksGetRtpNotifications) { +TEST_F(RtpVideoStreamReceiver2Test, SinkGetsRtpNotifications) { rtp_video_stream_receiver_->StartReceive(); - MockRtpPacketSink secondary_sink_1; - MockRtpPacketSink secondary_sink_2; - - rtp_video_stream_receiver_->AddSecondarySink(&secondary_sink_1); - rtp_video_stream_receiver_->AddSecondarySink(&secondary_sink_2); + MockRtpPacketSink test_sink; + test_packet_sink_ = &test_sink; auto rtp_packet = CreateRtpPacketReceived(); - EXPECT_CALL(secondary_sink_1, OnRtpPacket(SamePacketAs(*rtp_packet))); - EXPECT_CALL(secondary_sink_2, OnRtpPacket(SamePacketAs(*rtp_packet))); + EXPECT_CALL(test_sink, OnRtpPacket(SamePacketAs(*rtp_packet))); rtp_video_stream_receiver_->OnRtpPacket(*rtp_packet); // Test tear-down. rtp_video_stream_receiver_->StopReceive(); - rtp_video_stream_receiver_->RemoveSecondarySink(&secondary_sink_1); - rtp_video_stream_receiver_->RemoveSecondarySink(&secondary_sink_2); + test_packet_sink_ = nullptr; } -TEST_F(RtpVideoStreamReceiver2Test, - RemovedSecondarySinksGetNoRtpNotifications) { - rtp_video_stream_receiver_->StartReceive(); - - MockRtpPacketSink secondary_sink; - - rtp_video_stream_receiver_->AddSecondarySink(&secondary_sink); - rtp_video_stream_receiver_->RemoveSecondarySink(&secondary_sink); - - auto rtp_packet = CreateRtpPacketReceived(); - - EXPECT_CALL(secondary_sink, OnRtpPacket(_)).Times(0); - - rtp_video_stream_receiver_->OnRtpPacket(*rtp_packet); - - // Test tear-down. - rtp_video_stream_receiver_->StopReceive(); -} - -TEST_F(RtpVideoStreamReceiver2Test, - OnlyRemovedSecondarySinksExcludedFromNotifications) { - rtp_video_stream_receiver_->StartReceive(); - - MockRtpPacketSink kept_secondary_sink; - MockRtpPacketSink removed_secondary_sink; - - rtp_video_stream_receiver_->AddSecondarySink(&kept_secondary_sink); - rtp_video_stream_receiver_->AddSecondarySink(&removed_secondary_sink); - rtp_video_stream_receiver_->RemoveSecondarySink(&removed_secondary_sink); - - auto rtp_packet = CreateRtpPacketReceived(); - EXPECT_CALL(kept_secondary_sink, OnRtpPacket(SamePacketAs(*rtp_packet))); - - rtp_video_stream_receiver_->OnRtpPacket(*rtp_packet); - - // Test tear-down. - rtp_video_stream_receiver_->StopReceive(); - rtp_video_stream_receiver_->RemoveSecondarySink(&kept_secondary_sink); -} - -TEST_F(RtpVideoStreamReceiver2Test, - SecondariesOfNonStartedStreamGetNoNotifications) { +TEST_F(RtpVideoStreamReceiver2Test, NonStartedStreamGetsNoRtpCallbacks) { // Explicitly showing that the stream is not in the |started| state, // regardless of whether streams start out |started| or |stopped|. rtp_video_stream_receiver_->StopReceive(); - MockRtpPacketSink secondary_sink; - rtp_video_stream_receiver_->AddSecondarySink(&secondary_sink); + MockRtpPacketSink test_sink; + test_packet_sink_ = &test_sink; auto rtp_packet = CreateRtpPacketReceived(); - EXPECT_CALL(secondary_sink, OnRtpPacket(_)).Times(0); + EXPECT_CALL(test_sink, OnRtpPacket(_)).Times(0); rtp_video_stream_receiver_->OnRtpPacket(*rtp_packet); - // Test tear-down. - rtp_video_stream_receiver_->RemoveSecondarySink(&secondary_sink); + test_packet_sink_ = nullptr; } TEST_F(RtpVideoStreamReceiver2Test, ParseGenericDescriptorOnePacket) { @@ -1178,20 +1139,6 @@ TEST_F(RtpVideoStreamReceiver2DependencyDescriptorTest, InjectPacketWith(stream_structure2, deltaframe_descriptor); } -#if RTC_DCHECK_IS_ON && GTEST_HAS_DEATH_TEST && !defined(WEBRTC_ANDROID) -using RtpVideoStreamReceiver2DeathTest = RtpVideoStreamReceiver2Test; -TEST_F(RtpVideoStreamReceiver2DeathTest, RepeatedSecondarySinkDisallowed) { - MockRtpPacketSink secondary_sink; - - rtp_video_stream_receiver_->AddSecondarySink(&secondary_sink); - EXPECT_DEATH(rtp_video_stream_receiver_->AddSecondarySink(&secondary_sink), - ""); - - // Test tear-down. - rtp_video_stream_receiver_->RemoveSecondarySink(&secondary_sink); -} -#endif - TEST_F(RtpVideoStreamReceiver2Test, TransformFrame) { rtc::scoped_refptr mock_frame_transformer = new rtc::RefCountedObject>(); diff --git a/video/video_receive_stream.h b/video/video_receive_stream.h index ce409618d7..8c63e323e0 100644 --- a/video/video_receive_stream.h +++ b/video/video_receive_stream.h @@ -45,7 +45,7 @@ class VCMTiming; namespace internal { -class VideoReceiveStream : public webrtc::VideoReceiveStream, +class VideoReceiveStream : public webrtc::DEPRECATED_VideoReceiveStream, public rtc::VideoSinkInterface, public NackSender, public video_coding::OnCompleteFrameCallback, diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc index 227dff7cfd..3ecbdeecaa 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -471,15 +471,6 @@ void VideoReceiveStream2::UpdateHistograms() { stats_proxy_.UpdateHistograms(fraction_lost, rtp_stats, nullptr); } -void VideoReceiveStream2::AddSecondarySink(RtpPacketSinkInterface* sink) { - rtp_video_stream_receiver_.AddSecondarySink(sink); -} - -void VideoReceiveStream2::RemoveSecondarySink( - const RtpPacketSinkInterface* sink) { - rtp_video_stream_receiver_.RemoveSecondarySink(sink); -} - bool VideoReceiveStream2::SetBaseMinimumPlayoutDelayMs(int delay_ms) { RTC_DCHECK_RUN_ON(&worker_sequence_checker_); if (delay_ms < kMinBaseMinimumDelayMs || delay_ms > kMaxBaseMinimumDelayMs) { diff --git a/video/video_receive_stream2.h b/video/video_receive_stream2.h index b8e3ba51d9..5cc074b207 100644 --- a/video/video_receive_stream2.h +++ b/video/video_receive_stream2.h @@ -110,9 +110,6 @@ class VideoReceiveStream2 : public webrtc::VideoReceiveStream, webrtc::VideoReceiveStream::Stats GetStats() const override; - void AddSecondarySink(RtpPacketSinkInterface* sink) override; - void RemoveSecondarySink(const RtpPacketSinkInterface* sink) override; - // SetBaseMinimumPlayoutDelayMs and GetBaseMinimumPlayoutDelayMs are called // from webrtc/api level and requested by user code. For e.g. blink/js layer // in Chromium.