From 2e3edc1da90a6c050af0bd29ef631073b128da07 Mon Sep 17 00:00:00 2001 From: Markus Handell Date: Fri, 18 Jun 2021 13:44:13 +0200 Subject: [PATCH] RTCPSender: migrate to own configuration struct. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The class depends on RtcRtcpInterface::Configuration which adds an unneeded dependency, and inhibits well-manored changes to the constructor interface. Fix this so that RTCPSender uses it's own configuration struct which can be extended in future CLs. Also add a legacy constructor while downstream dependencies are updated. Bug: webrtc:11581 Change-Id: I8d166ab8253b27c08fcbe6aa7c7adde92688b7dc Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/222322 Commit-Queue: Markus Handell Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#34343} --- modules/rtp_rtcp/source/rtcp_sender.cc | 36 ++++++++++--- modules/rtp_rtcp/source/rtcp_sender.h | 30 +++++++++++ .../rtp_rtcp/source/rtcp_sender_unittest.cc | 52 +++++++++++-------- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 5 +- modules/rtp_rtcp/source/rtp_rtcp_impl2.cc | 3 +- video/video_send_stream_tests.cc | 8 +-- 6 files changed, 100 insertions(+), 34 deletions(-) diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index bacb7675fd..0e40d88e67 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -17,6 +17,7 @@ #include #include "api/rtc_event_log/rtc_event_log.h" +#include "api/units/time_delta.h" #include "api/units/timestamp.h" #include "logging/rtc_event_log/events/rtc_event_rtcp_packet_outgoing.h" #include "modules/rtp_rtcp/source/rtcp_packet/app.h" @@ -35,6 +36,7 @@ #include "modules/rtp_rtcp/source/rtcp_packet/tmmbr.h" #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" #include "modules/rtp_rtcp/source/rtp_rtcp_impl2.h" +#include "modules/rtp_rtcp/source/rtp_rtcp_interface.h" #include "modules/rtp_rtcp/source/time_util.h" #include "modules/rtp_rtcp/source/tmmbr_help.h" #include "rtc_base/checks.h" @@ -50,7 +52,6 @@ const uint32_t kRtcpAnyExtendedReports = kRtcpXrReceiverReferenceTime | kRtcpXrTargetBitrate; constexpr int32_t kDefaultVideoReportInterval = 1000; constexpr int32_t kDefaultAudioReportInterval = 5000; - } // namespace // Helper to put several RTCP packets into lower layer datagram RTCP packet. @@ -116,7 +117,26 @@ class RTCPSender::RtcpContext { const Timestamp now_; }; -RTCPSender::RTCPSender(const RtpRtcpInterface::Configuration& config) +RTCPSender::Configuration RTCPSender::Configuration::FromRtpRtcpConfiguration( + const RtpRtcpInterface::Configuration& configuration) { + RTCPSender::Configuration result; + result.audio = configuration.audio; + result.local_media_ssrc = configuration.local_media_ssrc; + result.clock = configuration.clock; + result.outgoing_transport = configuration.outgoing_transport; + result.non_sender_rtt_measurement = configuration.non_sender_rtt_measurement; + result.event_log = configuration.event_log; + if (configuration.rtcp_report_interval_ms) { + result.rtcp_report_interval = + TimeDelta::Millis(configuration.rtcp_report_interval_ms); + } + result.receive_statistics = configuration.receive_statistics; + result.rtcp_packet_type_counter_observer = + configuration.rtcp_packet_type_counter_observer; + return result; +} + +RTCPSender::RTCPSender(const Configuration& config) : audio_(config.audio), ssrc_(config.local_media_ssrc), clock_(config.clock), @@ -124,10 +144,11 @@ RTCPSender::RTCPSender(const RtpRtcpInterface::Configuration& config) method_(RtcpMode::kOff), event_log_(config.event_log), transport_(config.outgoing_transport), - report_interval_ms_(config.rtcp_report_interval_ms > 0 - ? config.rtcp_report_interval_ms - : (config.audio ? kDefaultAudioReportInterval - : kDefaultVideoReportInterval)), + report_interval_ms_(config.rtcp_report_interval + .value_or(TimeDelta::Millis( + config.audio ? kDefaultAudioReportInterval + : kDefaultVideoReportInterval)) + .ms()), sending_(false), next_time_to_send_rtcp_(0), timestamp_offset_(0), @@ -165,6 +186,9 @@ RTCPSender::RTCPSender(const RtpRtcpInterface::Configuration& config) builders_[kRtcpAnyExtendedReports] = &RTCPSender::BuildExtendedReports; } +RTCPSender::RTCPSender(const RtpRtcpInterface::Configuration& config) + : RTCPSender(Configuration::FromRtpRtcpConfiguration(config)) {} + RTCPSender::~RTCPSender() {} RtcpMode RTCPSender::Status() const { diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index 67200eef36..aa13156a94 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -19,6 +19,7 @@ #include "absl/types/optional.h" #include "api/call/transport.h" +#include "api/units/time_delta.h" #include "api/video/video_bitrate_allocation.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "modules/rtp_rtcp/include/receive_statistics.h" @@ -42,6 +43,32 @@ class RtcEventLog; class RTCPSender final { public: + struct Configuration { + // TODO(bugs.webrtc.org/11581): Remove this temporary conversion utility + // once rtc_rtcp_impl.cc/h are gone. + static Configuration FromRtpRtcpConfiguration( + const RtpRtcpInterface::Configuration& config); + + // True for a audio version of the RTP/RTCP module object false will create + // a video version. + bool audio = false; + // SSRCs for media and retransmission, respectively. + // FlexFec SSRC is fetched from |flexfec_sender|. + uint32_t local_media_ssrc = 0; + // The clock to use to read time. If nullptr then system clock will be used. + Clock* clock = nullptr; + // Transport object that will be called when packets are ready to be sent + // out on the network. + Transport* outgoing_transport = nullptr; + // Estimate RTT as non-sender as described in + // https://tools.ietf.org/html/rfc3611#section-4.4 and #section-4.5 + bool non_sender_rtt_measurement = false; + + RtcEventLog* event_log = nullptr; + absl::optional rtcp_report_interval; + ReceiveStatisticsProvider* receive_statistics = nullptr; + RtcpPacketTypeCounterObserver* rtcp_packet_type_counter_observer = nullptr; + }; struct FeedbackState { FeedbackState(); FeedbackState(const FeedbackState&); @@ -63,6 +90,9 @@ class RTCPSender final { RTCPReceiver* receiver; }; + explicit RTCPSender(const Configuration& config); + // TODO(bugs.webrtc.org/11581): delete this temporary compatibility helper + // once downstream dependencies migrates. explicit RTCPSender(const RtpRtcpInterface::Configuration& config); RTCPSender() = delete; diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index 8ad20b2928..706b804d19 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -14,6 +14,7 @@ #include #include "absl/base/macros.h" +#include "api/units/time_delta.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet/bye.h" #include "modules/rtp_rtcp/source/rtcp_packet/common_header.h" @@ -71,7 +72,7 @@ static const uint32_t kStartRtpTimestamp = 0x34567; static const uint32_t kRtpTimestamp = 0x45678; std::unique_ptr CreateRtcpSender( - const RtpRtcpInterface::Configuration& config, + const RTCPSender::Configuration& config, bool init_timestamps = true) { auto rtcp_sender = std::make_unique(config); rtcp_sender->SetRemoteSSRC(kRemoteSsrc); @@ -83,31 +84,39 @@ std::unique_ptr CreateRtcpSender( } return rtcp_sender; } - } // namespace class RtcpSenderTest : public ::testing::Test { protected: RtcpSenderTest() : clock_(1335900000), - receive_statistics_(ReceiveStatistics::Create(&clock_)), - retransmission_rate_limiter_(&clock_, 1000) { - RtpRtcpInterface::Configuration configuration = GetDefaultConfig(); - rtp_rtcp_impl_.reset(new ModuleRtpRtcpImpl2(configuration)); + receive_statistics_(ReceiveStatistics::Create(&clock_)) { + rtp_rtcp_impl_.reset(new ModuleRtpRtcpImpl2(GetDefaultRtpRtcpConfig())); } - RtpRtcpInterface::Configuration GetDefaultConfig() { - RtpRtcpInterface::Configuration configuration; + RTCPSender::Configuration GetDefaultConfig() { + RTCPSender::Configuration configuration; configuration.audio = false; configuration.clock = &clock_; configuration.outgoing_transport = &test_transport_; - configuration.retransmission_rate_limiter = &retransmission_rate_limiter_; - configuration.rtcp_report_interval_ms = 1000; + configuration.rtcp_report_interval = TimeDelta::Millis(1000); configuration.receive_statistics = receive_statistics_.get(); configuration.local_media_ssrc = kSenderSsrc; return configuration; } + RtpRtcpInterface::Configuration GetDefaultRtpRtcpConfig() { + RTCPSender::Configuration config = GetDefaultConfig(); + RtpRtcpInterface::Configuration result; + result.audio = config.audio; + result.clock = config.clock; + result.outgoing_transport = config.outgoing_transport; + result.rtcp_report_interval_ms = config.rtcp_report_interval->ms(); + result.receive_statistics = config.receive_statistics; + result.local_media_ssrc = config.local_media_ssrc; + return result; + } + void InsertIncomingPacket(uint32_t remote_ssrc, uint16_t seq_num) { RtpPacketReceived packet; packet.SetSsrc(remote_ssrc); @@ -127,7 +136,6 @@ class RtcpSenderTest : public ::testing::Test { TestTransport test_transport_; std::unique_ptr receive_statistics_; std::unique_ptr rtp_rtcp_impl_; - RateLimiter retransmission_rate_limiter_; }; TEST_F(RtcpSenderTest, SetRtcpStatus) { @@ -206,11 +214,11 @@ TEST_F(RtcpSenderTest, SendConsecutiveSrWithExactSlope) { } TEST_F(RtcpSenderTest, DoNotSendSrBeforeRtp) { - RtpRtcpInterface::Configuration config; + RTCPSender::Configuration config; config.clock = &clock_; config.receive_statistics = receive_statistics_.get(); config.outgoing_transport = &test_transport_; - config.rtcp_report_interval_ms = 1000; + config.rtcp_report_interval = TimeDelta::Millis(1000); config.local_media_ssrc = kSenderSsrc; auto rtcp_sender = CreateRtcpSender(config, /*init_timestamps=*/false); rtcp_sender->SetRTCPStatus(RtcpMode::kReducedSize); @@ -227,11 +235,11 @@ TEST_F(RtcpSenderTest, DoNotSendSrBeforeRtp) { } TEST_F(RtcpSenderTest, DoNotSendCompundBeforeRtp) { - RtpRtcpInterface::Configuration config; + RTCPSender::Configuration config; config.clock = &clock_; config.receive_statistics = receive_statistics_.get(); config.outgoing_transport = &test_transport_; - config.rtcp_report_interval_ms = 1000; + config.rtcp_report_interval = TimeDelta::Millis(1000); config.local_media_ssrc = kSenderSsrc; auto rtcp_sender = CreateRtcpSender(config, /*init_timestamps=*/false); rtcp_sender->SetRTCPStatus(RtcpMode::kCompound); @@ -510,7 +518,7 @@ TEST_F(RtcpSenderTest, SendXrWithMultipleDlrrSubBlocks) { } TEST_F(RtcpSenderTest, SendXrWithRrtr) { - RtpRtcpInterface::Configuration config = GetDefaultConfig(); + RTCPSender::Configuration config = GetDefaultConfig(); config.non_sender_rtt_measurement = true; auto rtcp_sender = CreateRtcpSender(config); rtcp_sender->SetRTCPStatus(RtcpMode::kCompound); @@ -525,7 +533,7 @@ TEST_F(RtcpSenderTest, SendXrWithRrtr) { } TEST_F(RtcpSenderTest, TestNoXrRrtrSentIfSending) { - RtpRtcpInterface::Configuration config = GetDefaultConfig(); + RTCPSender::Configuration config = GetDefaultConfig(); config.non_sender_rtt_measurement = true; auto rtcp_sender = CreateRtcpSender(config); rtcp_sender->SetRTCPStatus(RtcpMode::kCompound); @@ -535,7 +543,7 @@ TEST_F(RtcpSenderTest, TestNoXrRrtrSentIfSending) { } TEST_F(RtcpSenderTest, TestNoXrRrtrSentIfNotEnabled) { - RtpRtcpInterface::Configuration config = GetDefaultConfig(); + RTCPSender::Configuration config = GetDefaultConfig(); config.non_sender_rtt_measurement = false; auto rtcp_sender = CreateRtcpSender(config); rtcp_sender->SetRTCPStatus(RtcpMode::kCompound); @@ -546,12 +554,12 @@ TEST_F(RtcpSenderTest, TestNoXrRrtrSentIfNotEnabled) { TEST_F(RtcpSenderTest, TestRegisterRtcpPacketTypeObserver) { RtcpPacketTypeCounterObserverImpl observer; - RtpRtcpInterface::Configuration config; + RTCPSender::Configuration config; config.clock = &clock_; config.receive_statistics = receive_statistics_.get(); config.outgoing_transport = &test_transport_; config.rtcp_packet_type_counter_observer = &observer; - config.rtcp_report_interval_ms = 1000; + config.rtcp_report_interval = TimeDelta::Millis(1000); auto rtcp_sender = CreateRtcpSender(config); rtcp_sender->SetRTCPStatus(RtcpMode::kReducedSize); EXPECT_EQ(0, rtcp_sender->SendRTCP(feedback_state(), kRtcpPli)); @@ -643,11 +651,11 @@ TEST_F(RtcpSenderTest, ByeMustBeLast) { })); // Re-configure rtcp_sender with mock_transport_ - RtpRtcpInterface::Configuration config; + RTCPSender::Configuration config; config.clock = &clock_; config.receive_statistics = receive_statistics_.get(); config.outgoing_transport = &mock_transport; - config.rtcp_report_interval_ms = 1000; + config.rtcp_report_interval = TimeDelta::Millis(1000); config.local_media_ssrc = kSenderSsrc; auto rtcp_sender = CreateRtcpSender(config); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 54316a85b5..8d5b3ac754 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -21,7 +21,9 @@ #include "api/transport/field_trial_based_config.h" #include "modules/rtp_rtcp/source/rtcp_packet/dlrr.h" +#include "modules/rtp_rtcp/source/rtcp_sender.h" #include "modules/rtp_rtcp/source/rtp_rtcp_config.h" +#include "modules/rtp_rtcp/source/rtp_rtcp_interface.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" #include "system_wrappers/include/ntp_time.h" @@ -58,7 +60,8 @@ std::unique_ptr RtpRtcp::DEPRECATED_Create( } ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) - : rtcp_sender_(configuration), + : rtcp_sender_( + RTCPSender::Configuration::FromRtpRtcpConfiguration(configuration)), rtcp_receiver_(configuration, this), clock_(configuration.clock), last_bitrate_process_time_(clock_->TimeInMilliseconds()), diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc index 55e4a74817..3a2ec81fdc 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl2.cc @@ -55,7 +55,8 @@ void ModuleRtpRtcpImpl2::RtpSenderContext::AssignSequenceNumber( ModuleRtpRtcpImpl2::ModuleRtpRtcpImpl2(const Configuration& configuration) : worker_queue_(TaskQueueBase::Current()), - rtcp_sender_(configuration), + rtcp_sender_( + RTCPSender::Configuration::FromRtpRtcpConfiguration(configuration)), rtcp_receiver_(configuration, this), clock_(configuration.clock), last_rtt_process_time_(clock_->TimeInMilliseconds()), diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc index 78265cc7dc..4e94d8fc77 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -948,10 +948,10 @@ void VideoSendStreamTest::TestNackRetransmission( non_padding_sequence_numbers_.end() - kNackedPacketsAtOnceCount, non_padding_sequence_numbers_.end()); - RtpRtcpInterface::Configuration config; + RTCPSender::Configuration config; config.clock = Clock::GetRealTimeClock(); config.outgoing_transport = transport_adapter_.get(); - config.rtcp_report_interval_ms = kRtcpIntervalMs; + config.rtcp_report_interval = TimeDelta::Millis(kRtcpIntervalMs); config.local_media_ssrc = kReceiverLocalVideoSsrc; RTCPSender rtcp_sender(config); @@ -1164,11 +1164,11 @@ void VideoSendStreamTest::TestPacketFragmentationSize(VideoFormat format, kVideoSendSsrcs[0], rtp_packet.SequenceNumber(), packets_lost_, // Cumulative lost. loss_ratio); // Loss percent. - RtpRtcpInterface::Configuration config; + RTCPSender::Configuration config; config.clock = Clock::GetRealTimeClock(); config.receive_statistics = &lossy_receive_stats; config.outgoing_transport = transport_adapter_.get(); - config.rtcp_report_interval_ms = kRtcpIntervalMs; + config.rtcp_report_interval = TimeDelta::Millis(kRtcpIntervalMs); config.local_media_ssrc = kVideoSendSsrcs[0]; RTCPSender rtcp_sender(config);