Remove VCMProtectionCallback::SetRetransmissionMode() The method has no callers. Its only implementation, in RtpVideoSender, forwarded the mode to RTPSenderVideo::SetRetransmissionSetting() under mutex_ to serialize it with packetization of encoded frames, so removing it also removes one of the remaining reasons for the mutex. VCMProtectionCallback is public API, but there are no known callers or implementations outside of WebRTC, and the header asks other users not to use the API yet. Downstream implementations of VCMProtectionCallback need to drop their SetRetransmissionMode() override. Bug: webrtc:42223727 Change-Id: Ibef15509dd7d6317155a781a91c07d8deabeb117 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/506201 Reviewed-by: Rasmus Brandt <brandtr@webrtc.org> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#48793}
diff --git a/api/fec_controller.h b/api/fec_controller.h index a6cf769..337fa21 100644 --- a/api/fec_controller.h +++ b/api/fec_controller.h
@@ -34,10 +34,6 @@ uint32_t* sent_nack_rate_bps, uint32_t* sent_fec_rate_bps) = 0; - // 'retransmission_mode' is either a value of enum RetransmissionMode, or - // computed with bitwise operators on values of enum RetransmissionMode. - virtual void SetRetransmissionMode(int retransmission_mode) = 0; - protected: virtual ~VCMProtectionCallback() {} };
diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index c283d3f..306dc22 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc
@@ -968,13 +968,6 @@ return 0; } -void RtpVideoSender::SetRetransmissionMode(int retransmission_mode) { - MutexLock lock(&mutex_); - for (const RtpStreamSender& stream : rtp_streams_) { - stream.sender_video->SetRetransmissionSetting(retransmission_mode); - } -} - void RtpVideoSender::SetFecAllowed(bool fec_allowed) { // Called by the encoder, which may run on any thread. `fec_allowed_` is only // used on the transport queue, so apply the value there.
diff --git a/call/rtp_video_sender.h b/call/rtp_video_sender.h index 7a12287..bc61956 100644 --- a/call/rtp_video_sender.h +++ b/call/rtp_video_sender.h
@@ -42,7 +42,6 @@ #include "common_video/frame_counts.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtp_rtcp_impl2.h" -#include "modules/rtp_rtcp/source/rtp_sender.h" #include "modules/rtp_rtcp/source/rtp_sender_video.h" #include "modules/rtp_rtcp/source/rtp_sequence_number_map.h" #include "modules/rtp_rtcp/source/video_fec_generator.h" @@ -122,11 +121,6 @@ uint32_t* sent_fec_rate_bps) RTC_LOCKS_EXCLUDED(mutex_) override; - // 'retransmission_mode' is either a value of enum RetransmissionMode, or - // computed with bitwise operators on values of enum RetransmissionMode. - void SetRetransmissionMode(int retransmission_mode) - RTC_LOCKS_EXCLUDED(mutex_) override; - // Implements FecControllerOverride. void SetFecAllowed(bool fec_allowed) RTC_LOCKS_EXCLUDED(mutex_) override;
diff --git a/call/rtp_video_sender_unittest.cc b/call/rtp_video_sender_unittest.cc index 8406c2c..a2b0594 100644 --- a/call/rtp_video_sender_unittest.cc +++ b/call/rtp_video_sender_unittest.cc
@@ -59,7 +59,6 @@ #include "modules/rtp_rtcp/source/rtcp_packet/nack.h" #include "modules/rtp_rtcp/source/rtp_dependency_descriptor_extension.h" #include "modules/rtp_rtcp/source/rtp_packet.h" -#include "modules/rtp_rtcp/source/rtp_sender_video.h" #include "modules/video_coding/codecs/interface/common_constants.h" #include "modules/video_coding/fec_controller_default.h" #include "modules/video_coding/include/video_codec_interface.h" @@ -1801,84 +1800,6 @@ EXPECT_NE(sent_packets[0].Timestamp(), first_frame_timestamp); } -// Integration test verifying that when retransmission mode is set to -// kRetransmitBaseLayer,only base layer is retransmitted. -TEST(RtpVideoSenderTest, RetransmitsBaseLayerOnly) { - RtpVideoSenderTestFixture test({kSsrc1, kSsrc2}, {kRtxSsrc1, kRtxSsrc2}, - kPayloadType, {}); - test.SetSending(true); - - test.router()->SetRetransmissionMode(kRetransmitBaseLayer); - constexpr uint8_t kPayload = 'a'; - EncodedImage encoded_image; - encoded_image.SetRtpTimestamp(1); - encoded_image.capture_time_ms_ = 2; - encoded_image.set_frame_type(VideoFrameType::kVideoFrameKey); - encoded_image.SetEncodedData(EncodedImageBuffer::Create(&kPayload, 1)); - - // Send two tiny images, mapping to two RTP packets. Capture sequence numbers. - std::vector<uint16_t> rtp_sequence_numbers; - std::vector<uint16_t> transport_sequence_numbers; - std::vector<uint16_t> base_sequence_numbers; - EXPECT_CALL(test.transport(), SendRtp) - .Times(2) - .WillRepeatedly( - [&rtp_sequence_numbers, &transport_sequence_numbers]( - std::span<const uint8_t> packet, const PacketOptions& options) { - RtpPacket rtp_packet; - EXPECT_TRUE(rtp_packet.Parse(packet)); - rtp_sequence_numbers.push_back(rtp_packet.SequenceNumber()); - transport_sequence_numbers.push_back(options.packet_id); - return true; - }); - CodecSpecificInfo key_codec_info; - key_codec_info.codecType = kVideoCodecVP8; - key_codec_info.codecSpecific.VP8.temporalIdx = 0; - EXPECT_EQ( - EncodedImageCallback::Result::OK, - test.router()->OnEncodedImage(encoded_image, &key_codec_info).error); - encoded_image.SetRtpTimestamp(2); - encoded_image.capture_time_ms_ = 3; - encoded_image.set_frame_type(VideoFrameType::kVideoFrameDelta); - CodecSpecificInfo delta_codec_info; - delta_codec_info.codecType = kVideoCodecVP8; - delta_codec_info.codecSpecific.VP8.temporalIdx = 1; - EXPECT_EQ( - EncodedImageCallback::Result::OK, - test.router()->OnEncodedImage(encoded_image, &delta_codec_info).error); - - test.AdvanceTime(TimeDelta::Millis(33)); - - // Construct a NACK message for requesting retransmission of both packet. - rtcp::Nack nack; - nack.SetMediaSsrc(kSsrc1); - nack.SetPacketIds(rtp_sequence_numbers); - Buffer nack_buffer = nack.Build(); - - std::vector<uint16_t> retransmitted_rtp_sequence_numbers; - EXPECT_CALL(test.transport(), SendRtp) - .Times(1) - .WillRepeatedly([&retransmitted_rtp_sequence_numbers]( - std::span<const uint8_t> packet, - const PacketOptions& options) { - RtpPacket rtp_packet; - EXPECT_TRUE(rtp_packet.Parse(packet)); - EXPECT_EQ(rtp_packet.Ssrc(), kRtxSsrc1); - // Capture the retransmitted sequence number from the RTX header. - std::span<const uint8_t> payload = rtp_packet.payload(); - retransmitted_rtp_sequence_numbers.push_back( - ByteReader<uint16_t>::ReadBigEndian(payload.data())); - return true; - }); - test.router()->DeliverRtcp(nack_buffer); - test.AdvanceTime(TimeDelta::Millis(33)); - - // Verify that only base layer packet was retransmitted. - std::vector<uint16_t> base_rtp_sequence_numbers( - rtp_sequence_numbers.begin(), rtp_sequence_numbers.begin() + 1); - EXPECT_EQ(retransmitted_rtp_sequence_numbers, base_rtp_sequence_numbers); -} - TEST(RtpVideoSenderTest, PostTaskRaceDoesNotLeadToDanglingPointer) { NiceMock<MockTransport> transport; NiceMock<MockRtcpIntraFrameObserver> encoder_feedback;
diff --git a/modules/video_coding/fec_controller_unittest.cc b/modules/video_coding/fec_controller_unittest.cc index 5585446..44c8b18 100644 --- a/modules/video_coding/fec_controller_unittest.cc +++ b/modules/video_coding/fec_controller_unittest.cc
@@ -41,7 +41,6 @@ *sent_fec_rate_bps = fec_rate_bps_; return 0; } - void SetRetransmissionMode(int /* retransmission_mode */) override {} uint32_t fec_rate_bps_ = 0; uint32_t nack_rate_bps_ = 0;