From 185f10c082b69daa912116115979b09b79547b3c Mon Sep 17 00:00:00 2001 From: Tommi Date: Tue, 2 Aug 2022 11:51:20 +0200 Subject: [PATCH] [WebRtcVideoReceiveStream] Add ability to config flexfec post init. Bug: webrtc:11993 Change-Id: I35d7e645e18b7cb4a86645ea52c8958063f13d3c Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/269243 Reviewed-by: Danil Chapovalov Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#37661} --- call/video_receive_stream.h | 7 +++++++ media/engine/fake_webrtc_call.h | 5 +++++ media/engine/webrtc_video_engine.cc | 6 +++--- media/engine/webrtc_video_engine_unittest.cc | 15 ++++++++------- video/rtp_video_stream_receiver2.cc | 11 +++++++++-- video/rtp_video_stream_receiver2.h | 8 ++++++++ video/video_receive_stream2.cc | 10 ++++++++++ video/video_receive_stream2.h | 1 + 8 files changed, 51 insertions(+), 12 deletions(-) diff --git a/call/video_receive_stream.h b/call/video_receive_stream.h index eff25bfa54..65b50ef75f 100644 --- a/call/video_receive_stream.h +++ b/call/video_receive_stream.h @@ -287,6 +287,13 @@ class VideoReceiveStreamInterface : public MediaReceiveStreamInterface { virtual void SetRtcpMode(RtcpMode mode) = 0; + // Sets or clears a flexfec RTP sink. This affects `rtp.packet_sink_` and + // `rtp.protected_by_flexfec` parts of the configuration. Must be called on + // the packet delivery thread. + // TODO(bugs.webrtc.org/11993): Packet delivery thread today means `worker + // thread` but will be `network thread`. + virtual void SetFlexFecProtection(RtpPacketSinkInterface* flexfec_sink) = 0; + protected: virtual ~VideoReceiveStreamInterface() {} }; diff --git a/media/engine/fake_webrtc_call.h b/media/engine/fake_webrtc_call.h index c8d56cbcdc..eef15bfd80 100644 --- a/media/engine/fake_webrtc_call.h +++ b/media/engine/fake_webrtc_call.h @@ -284,6 +284,11 @@ class FakeVideoReceiveStream final config_.rtp.rtcp_mode = mode; } + void SetFlexFecProtection(webrtc::RtpPacketSinkInterface* sink) override { + config_.rtp.packet_sink_ = sink; + config_.rtp.protected_by_flexfec = (sink != nullptr); + } + void Start() override; void Stop() override; diff --git a/media/engine/webrtc_video_engine.cc b/media/engine/webrtc_video_engine.cc index 1318f98e17..4f02eab82d 100644 --- a/media/engine/webrtc_video_engine.cc +++ b/media/engine/webrtc_video_engine.cc @@ -3060,9 +3060,9 @@ void WebRtcVideoChannel::WebRtcVideoReceiveStream::SetFlexFecPayload( flexfec_stream_->SetPayloadType(payload_type); if (payload_type == -1) { - // TODO(tommi): Delete `flexfec_stream_` and clear references to it from - // `stream_` without having to recreate `stream_`. - flexfec_needs_recreation = true; + stream_->SetFlexFecProtection(nullptr); + call_->DestroyFlexfecReceiveStream(flexfec_stream_); + flexfec_stream_ = nullptr; } } else if (payload_type != -1) { flexfec_config_.payload_type = payload_type; diff --git a/media/engine/webrtc_video_engine_unittest.cc b/media/engine/webrtc_video_engine_unittest.cc index 93c086b2e7..0e1f2467ba 100644 --- a/media/engine/webrtc_video_engine_unittest.cc +++ b/media/engine/webrtc_video_engine_unittest.cc @@ -4231,11 +4231,11 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, } // 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. +// flexfec enabled and then turn off flexfec. This will not result in the video +// stream being recreated. The flexfec stream pointer that's held by the video +// stream will be set/cleared as dictated by the configuration change. TEST_F(WebRtcVideoChannelFlexfecRecvTest, - DisablingFlexfecRecreatesVideoReceiveStream) { + DisablingFlexfecDoesNotRecreateVideoReceiveStream) { cricket::VideoRecvParameters recv_parameters; recv_parameters.codecs.push_back(GetEngineCodec("VP8")); recv_parameters.codecs.push_back(GetEngineCodec("flexfec-03")); @@ -4258,9 +4258,10 @@ TEST_F(WebRtcVideoChannelFlexfecRecvTest, recv_parameters.codecs.clear(); recv_parameters.codecs.push_back(GetEngineCodec("VP8")); ASSERT_TRUE(channel_->SetRecvParameters(recv_parameters)); - // 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()) + // The count of created streams should remain 2 since the video stream will + // have been reconfigured to not reference flexfec and not recreated on + // account of the flexfec stream being deleted. + EXPECT_EQ(2, fake_call_->GetNumCreatedReceiveStreams()) << "Disabling FlexFEC should not recreate VideoReceiveStreamInterface."; EXPECT_EQ(1U, fake_call_->GetVideoReceiveStreams().size()) << "Disabling FlexFEC should not destroy VideoReceiveStreamInterface."; diff --git a/video/rtp_video_stream_receiver2.cc b/video/rtp_video_stream_receiver2.cc index b516b677f7..2ac51697e4 100644 --- a/video/rtp_video_stream_receiver2.cc +++ b/video/rtp_video_stream_receiver2.cc @@ -229,6 +229,7 @@ RtpVideoStreamReceiver2::RtpVideoStreamReceiver2( ulpfec_receiver_(UlpfecReceiver::Create(config->rtp.remote_ssrc, this, config->rtp.extensions)), + packet_sink_(config->rtp.packet_sink_), receiving_(false), last_packet_log_ms_(-1), rtp_rtcp_(CreateRtpRtcpModule( @@ -661,8 +662,8 @@ void RtpVideoStreamReceiver2::OnRtpPacket(const RtpPacketReceived& packet) { rtp_receive_statistics_->OnRtpPacket(packet); } - if (config_.rtp.packet_sink_) { - config_.rtp.packet_sink_->OnRtpPacket(packet); + if (packet_sink_) { + packet_sink_->OnRtpPacket(packet); } } @@ -928,6 +929,12 @@ void RtpVideoStreamReceiver2::SetRtcpMode(RtcpMode mode) { rtp_rtcp_->SetRTCPStatus(mode); } +void RtpVideoStreamReceiver2::SetPacketSink( + RtpPacketSinkInterface* packet_sink) { + RTC_DCHECK_RUN_ON(&packet_sequence_checker_); + packet_sink_ = packet_sink; +} + absl::optional RtpVideoStreamReceiver2::LastReceivedPacketMs() const { RTC_DCHECK_RUN_ON(&packet_sequence_checker_); if (last_received_rtp_system_time_) { diff --git a/video/rtp_video_stream_receiver2.h b/video/rtp_video_stream_receiver2.h index 0d946146d1..d416afd700 100644 --- a/video/rtp_video_stream_receiver2.h +++ b/video/rtp_video_stream_receiver2.h @@ -187,6 +187,13 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender, // Forwards the call to set rtcp_sender_ to the RTCP mode of the rtcp sender. void SetRtcpMode(RtcpMode mode); + // Sets or clears the callback sink that gets called for RTP packets. Used for + // packet handlers such as FlexFec. Must be called on the packet delivery + // thread (same context as `OnRtpPacket` is called on). + // TODO(bugs.webrtc.org/11993): Packet delivery thread today means `worker + // thread` but will be `network thread`. + void SetPacketSink(RtpPacketSinkInterface* packet_sink); + absl::optional LastReceivedPacketMs() const; absl::optional LastReceivedKeyframePacketMs() const; @@ -318,6 +325,7 @@ class RtpVideoStreamReceiver2 : public LossNotificationSender, // that belong to the network thread. Once the packets are fully delivered // on the network thread, this comment will be deleted. RTC_NO_UNIQUE_ADDRESS SequenceChecker packet_sequence_checker_; + RtpPacketSinkInterface* packet_sink_ RTC_GUARDED_BY(packet_sequence_checker_); bool receiving_ RTC_GUARDED_BY(packet_sequence_checker_); int64_t last_packet_log_ms_ RTC_GUARDED_BY(packet_sequence_checker_); diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc index 7a53f34ddd..969e6511dd 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -507,6 +507,16 @@ void VideoReceiveStream2::SetRtcpMode(RtcpMode mode) { rtp_video_stream_receiver_.SetRtcpMode(mode); } +void VideoReceiveStream2::SetFlexFecProtection( + RtpPacketSinkInterface* flexfec_sink) { + RTC_DCHECK_RUN_ON(&packet_sequence_checker_); + rtp_video_stream_receiver_.SetPacketSink(flexfec_sink); + // TODO(tommi): Stop using the config struct for the internal state. + const_cast(config_.rtp.packet_sink_) = flexfec_sink; + const_cast(config_.rtp.protected_by_flexfec) = + (flexfec_sink != nullptr); +} + void VideoReceiveStream2::CreateAndRegisterExternalDecoder( const Decoder& decoder) { TRACE_EVENT0("webrtc", diff --git a/video/video_receive_stream2.h b/video/video_receive_stream2.h index e16c07741e..ca0db1e3ba 100644 --- a/video/video_receive_stream2.h +++ b/video/video_receive_stream2.h @@ -146,6 +146,7 @@ class VideoReceiveStream2 bool transport_cc() const override; void SetTransportCc(bool transport_cc) override; void SetRtcpMode(RtcpMode mode) override; + void SetFlexFecProtection(RtpPacketSinkInterface* flexfec_sink) override; webrtc::VideoReceiveStreamInterface::Stats GetStats() const override;