From 6f420e424885dab1d9f885365ea9abea5cc4a901 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Tue, 9 Jul 2019 20:06:30 +0200 Subject: [PATCH] Reland "Add ability to set RTCP sender ssrc at construction time" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a reland of 94c58fd815f0c7c6429aa53a79621ea9ef39c770 Patch set 1 is the original CL. Patch set 2 introduced a trivial fix. In RtcpSender::SetSSRC(), check if either current SSRC is 0 or if the SSRC is identical to the current one. If so, don't schedule an early report. This prevents a regression in which audio jitter became too low(?) Original change's description: > Add ability to set RTCP sender ssrc at construction time > > Bug: webrtc:10774 > Change-Id: Iaf5857e24359e9795434bcd0cdbe1658a2f9f5d3 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/144632 > Reviewed-by: Åsa Persson > Commit-Queue: Erik Språng > Cr-Commit-Position: refs/heads/master@{#28506} Bug: webrtc:10774 Change-Id: I103dfa48719aa41d6ab633cdac8b3a5c46b54843 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/144565 Commit-Queue: Erik Språng Reviewed-by: Åsa Persson Cr-Commit-Position: refs/heads/master@{#28520} --- modules/rtp_rtcp/source/rtcp_sender.cc | 32 +++--- modules/rtp_rtcp/source/rtcp_sender.h | 9 +- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 98 ++++++++++++++----- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 11 +-- .../rtp_rtcp/source/rtp_rtcp_impl_unittest.cc | 1 + video/video_send_stream_tests.cc | 25 +++-- 6 files changed, 112 insertions(+), 64 deletions(-) diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index af5cd270cd..8c5f8216b7 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -48,6 +48,8 @@ namespace { const uint32_t kRtcpAnyExtendedReports = kRtcpXrReceiverReferenceTime | kRtcpXrDlrrReportBlock | kRtcpXrTargetBitrate; +constexpr int32_t kDefaultVideoReportInterval = 1000; +constexpr int32_t kDefaultAudioReportInterval = 5000; } // namespace RTCPSender::FeedbackState::FeedbackState() @@ -112,29 +114,25 @@ class RTCPSender::RtcpContext { const int64_t now_us_; }; -RTCPSender::RTCPSender( - bool audio, - Clock* clock, - ReceiveStatisticsProvider* receive_statistics, - RtcpPacketTypeCounterObserver* packet_type_counter_observer, - RtcEventLog* event_log, - Transport* outgoing_transport, - int report_interval_ms) - : audio_(audio), - clock_(clock), +RTCPSender::RTCPSender(const RtpRtcp::Configuration& config) + : audio_(config.audio), + clock_(config.clock), random_(clock_->TimeInMicroseconds()), method_(RtcpMode::kOff), - event_log_(event_log), - transport_(outgoing_transport), - report_interval_ms_(report_interval_ms), + 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)), sending_(false), next_time_to_send_rtcp_(0), timestamp_offset_(0), last_rtp_timestamp_(0), last_frame_capture_time_ms_(-1), - ssrc_(0), + ssrc_(config.media_send_ssrc.value_or(0)), remote_ssrc_(0), - receive_statistics_(receive_statistics), + receive_statistics_(config.receive_statistics), sequence_number_fir_(0), @@ -150,7 +148,7 @@ RTCPSender::RTCPSender( app_length_(0), xr_send_receiver_reference_time_enabled_(false), - packet_type_counter_observer_(packet_type_counter_observer), + packet_type_counter_observer_(config.rtcp_packet_type_counter_observer), send_video_bitrate_allocation_(false), last_payload_type_(-1) { RTC_DCHECK(transport_ != nullptr); @@ -307,7 +305,7 @@ uint32_t RTCPSender::SSRC() const { void RTCPSender::SetSSRC(uint32_t ssrc) { rtc::CritScope lock(&critical_section_rtcp_sender_); - if (ssrc_ != 0) { + if (ssrc_ != 0 && ssrc != ssrc_) { // not first SetSSRC, probably due to a collision // schedule a new RTCP report // make sure that we send a RTP packet diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index 74f4cc17a6..628121ea4e 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -23,6 +23,7 @@ #include "modules/remote_bitrate_estimator/include/bwe_defines.h" #include "modules/remote_bitrate_estimator/include/remote_bitrate_estimator.h" #include "modules/rtp_rtcp/include/receive_statistics.h" +#include "modules/rtp_rtcp/include/rtp_rtcp.h" #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_nack_stats.h" #include "modules/rtp_rtcp/source/rtcp_packet.h" @@ -62,13 +63,7 @@ class RTCPSender { ModuleRtpRtcpImpl* module; }; - RTCPSender(bool audio, - Clock* clock, - ReceiveStatisticsProvider* receive_statistics, - RtcpPacketTypeCounterObserver* packet_type_counter_observer, - RtcEventLog* event_log, - Transport* outgoing_transport, - int report_interval_ms); + explicit RTCPSender(const RtpRtcp::Configuration& config); virtual ~RTCPSender(); RtcpMode Status() const; diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index 01101c0e4b..09cdff17a2 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -75,22 +75,25 @@ class RtcpSenderTest : public ::testing::Test { : clock_(1335900000), receive_statistics_(ReceiveStatistics::Create(&clock_)), retransmission_rate_limiter_(&clock_, 1000) { + RtpRtcp::Configuration configuration = GetDefaultConfig(); + rtp_rtcp_impl_.reset(new ModuleRtpRtcpImpl(configuration)); + rtcp_sender_.reset(new RTCPSender(configuration)); + rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); + rtcp_sender_->SetTimestampOffset(kStartRtpTimestamp); + rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds(), + /*payload_type=*/0); + } + + RtpRtcp::Configuration GetDefaultConfig() { RtpRtcp::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; - - rtp_rtcp_impl_.reset(new ModuleRtpRtcpImpl(configuration)); - rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), - nullptr, nullptr, &test_transport_, - configuration.rtcp_report_interval_ms)); - rtcp_sender_->SetSSRC(kSenderSsrc); - rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); - rtcp_sender_->SetTimestampOffset(kStartRtpTimestamp); - rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds(), - /*payload_type=*/0); + configuration.receive_statistics = receive_statistics_.get(); + configuration.media_send_ssrc = kSenderSsrc; + return configuration; } void InsertIncomingPacket(uint32_t remote_ssrc, uint16_t seq_num) { @@ -187,9 +190,13 @@ TEST_F(RtcpSenderTest, SendConsecutiveSrWithExactSlope) { } TEST_F(RtcpSenderTest, DoNotSendSrBeforeRtp) { - rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), - nullptr, nullptr, &test_transport_, 1000)); - rtcp_sender_->SetSSRC(kSenderSsrc); + RtpRtcp::Configuration config; + config.clock = &clock_; + config.receive_statistics = receive_statistics_.get(); + config.outgoing_transport = &test_transport_; + config.rtcp_report_interval_ms = 1000; + config.media_send_ssrc = kSenderSsrc; + rtcp_sender_.reset(new RTCPSender(config)); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); rtcp_sender_->SetSendingStatus(feedback_state(), true); @@ -205,9 +212,13 @@ TEST_F(RtcpSenderTest, DoNotSendSrBeforeRtp) { } TEST_F(RtcpSenderTest, DoNotSendCompundBeforeRtp) { - rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), - nullptr, nullptr, &test_transport_, 1000)); - rtcp_sender_->SetSSRC(kSenderSsrc); + RtpRtcp::Configuration config; + config.clock = &clock_; + config.receive_statistics = receive_statistics_.get(); + config.outgoing_transport = &test_transport_; + config.rtcp_report_interval_ms = 1000; + config.media_send_ssrc = kSenderSsrc; + rtcp_sender_.reset(new RTCPSender(config)); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); rtcp_sender_->SetSendingStatus(feedback_state(), true); @@ -551,9 +562,14 @@ TEST_F(RtcpSenderTest, TestNoXrRrtrSentIfNotEnabled) { TEST_F(RtcpSenderTest, TestRegisterRtcpPacketTypeObserver) { RtcpPacketTypeCounterObserverImpl observer; - rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), - &observer, nullptr, &test_transport_, - 1000)); + RtpRtcp::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; + rtcp_sender_.reset(new RTCPSender(config)); + rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpPli)); @@ -674,9 +690,14 @@ TEST_F(RtcpSenderTest, ByeMustBeLast) { })); // Re-configure rtcp_sender_ with mock_transport_ - rtcp_sender_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), - nullptr, nullptr, &mock_transport, 1000)); - rtcp_sender_->SetSSRC(kSenderSsrc); + RtpRtcp::Configuration config; + config.clock = &clock_; + config.receive_statistics = receive_statistics_.get(); + config.outgoing_transport = &mock_transport; + config.rtcp_report_interval_ms = 1000; + config.media_send_ssrc = kSenderSsrc; + rtcp_sender_.reset(new RTCPSender(config)); + rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); rtcp_sender_->SetTimestampOffset(kStartRtpTimestamp); rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds(), @@ -795,4 +816,37 @@ TEST_F(RtcpSenderTest, SendTargetBitrateExplicitZeroOnStreamRemoval) { EXPECT_EQ(bitrates[1].target_bitrate_kbps, 0u); } +TEST_F(RtcpSenderTest, DoesntSchedulesInitialReportWhenSsrcSetOnConstruction) { + rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); + rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); + // New report should not have been scheduled yet. + clock_.AdvanceTimeMilliseconds(100); + EXPECT_FALSE(rtcp_sender_->TimeToSendRTCPReport(false)); +} + +TEST_F(RtcpSenderTest, DoesntSchedulesInitialReportOnFirstSetSsrc) { + // Set up without first SSRC not set at construction. + RtpRtcp::Configuration configuration = GetDefaultConfig(); + configuration.media_send_ssrc = absl::nullopt; + + rtcp_sender_.reset(new RTCPSender(configuration)); + rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); + rtcp_sender_->SetTimestampOffset(kStartRtpTimestamp); + rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds(), + /*payload_type=*/0); + rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); + + // Set SSRC for the first time. New report should not be scheduled. + rtcp_sender_->SetSSRC(kSenderSsrc); + clock_.AdvanceTimeMilliseconds(100); + EXPECT_FALSE(rtcp_sender_->TimeToSendRTCPReport(false)); +} + +TEST_F(RtcpSenderTest, SchedulesReportOnSsrcChange) { + rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); + rtcp_sender_->SetSSRC(kSenderSsrc + 1); + clock_.AdvanceTimeMilliseconds(100); + EXPECT_TRUE(rtcp_sender_->TimeToSendRTCPReport(false)); +} + } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index aa50227b14..765f76f70f 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -61,16 +61,7 @@ RtpRtcp* RtpRtcp::CreateRtpRtcp(const RtpRtcp::Configuration& configuration) { } ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) - : rtcp_sender_(configuration.audio, - configuration.clock, - configuration.receive_statistics, - configuration.rtcp_packet_type_counter_observer, - configuration.event_log, - configuration.outgoing_transport, - configuration.rtcp_report_interval_ms > 0 - ? configuration.rtcp_report_interval_ms - : (configuration.audio ? kDefaultAudioReportInterval - : kDefaultVideoReportInterval)), + : rtcp_sender_(configuration), rtcp_receiver_(configuration.clock, configuration.receiver_only, configuration.rtcp_packet_type_counter_observer, diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc index 31f9b24965..a0690c1778 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc @@ -162,6 +162,7 @@ class RtpRtcpModule : public RtcpPacketTypeCounterObserver { config.rtcp_packet_type_counter_observer = this; config.rtt_stats = &rtt_stats_; config.rtcp_report_interval_ms = rtcp_report_interval_ms_; + config.media_send_ssrc = kSenderSsrc; impl_.reset(new ModuleRtpRtcpImpl(config)); impl_->SetRTCPStatus(RtcpMode::kCompound); diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc index 96da991077..75423e5f56 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -914,9 +914,11 @@ void VideoSendStreamTest::TestNackRetransmission( non_padding_sequence_numbers_.end() - kNackedPacketsAtOnceCount, non_padding_sequence_numbers_.end()); - RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(), nullptr, - nullptr, nullptr, transport_adapter_.get(), - kRtcpIntervalMs); + RtpRtcp::Configuration config; + config.clock = Clock::GetRealTimeClock(); + config.outgoing_transport = transport_adapter_.get(); + config.rtcp_report_interval_ms = kRtcpIntervalMs; + RTCPSender rtcp_sender(config); rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize); rtcp_sender.SetRemoteSSRC(kVideoSendSsrcs[0]); @@ -1127,9 +1129,12 @@ void VideoSendStreamTest::TestPacketFragmentationSize(VideoFormat format, kVideoSendSsrcs[0], header.sequenceNumber, packets_lost_, // Cumulative lost. loss_ratio); // Loss percent. - RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(), - &lossy_receive_stats, nullptr, nullptr, - transport_adapter_.get(), kRtcpIntervalMs); + RtpRtcp::Configuration config; + config.clock = Clock::GetRealTimeClock(); + config.receive_statistics = &lossy_receive_stats; + config.outgoing_transport = transport_adapter_.get(); + config.rtcp_report_interval_ms = kRtcpIntervalMs; + RTCPSender rtcp_sender(config); rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize); rtcp_sender.SetRemoteSSRC(kVideoSendSsrcs[0]); @@ -1375,8 +1380,12 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_) { FakeReceiveStatistics receive_stats(kVideoSendSsrcs[0], last_sequence_number_, rtp_count_, 0); - RTCPSender rtcp_sender(false, clock_, &receive_stats, nullptr, nullptr, - transport_adapter_.get(), kRtcpIntervalMs); + RtpRtcp::Configuration config; + config.clock = clock_; + config.receive_statistics = &receive_stats; + config.outgoing_transport = transport_adapter_.get(); + config.rtcp_report_interval_ms = kRtcpIntervalMs; + RTCPSender rtcp_sender(config); rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize); rtcp_sender.SetRemoteSSRC(kVideoSendSsrcs[0]);