From 8b3e4e2d1166464f6b309f4fc533a29607d2771f Mon Sep 17 00:00:00 2001 From: Mirko Bonadei Date: Fri, 12 Jul 2019 08:38:36 +0000 Subject: [PATCH] Revert "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 reverts commit 6f420e424885dab1d9f885365ea9abea5cc4a901. Reason for revert: Speculative revert (some perf test are failing) Original change's description: > Reland "Add ability to set RTCP sender ssrc at construction time" > > 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} TBR=asapersson@webrtc.org,sprang@webrtc.org,ilnik@webrtc.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:10774 Change-Id: I39238d942b2bbe0a9c8ca752387a35ed9dd70650 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/145327 Commit-Queue: Mirko Bonadei Reviewed-by: Mirko Bonadei Cr-Commit-Position: refs/heads/master@{#28555} --- 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, 64 insertions(+), 112 deletions(-) diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index 8c5f8216b7..af5cd270cd 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -48,8 +48,6 @@ namespace { const uint32_t kRtcpAnyExtendedReports = kRtcpXrReceiverReferenceTime | kRtcpXrDlrrReportBlock | kRtcpXrTargetBitrate; -constexpr int32_t kDefaultVideoReportInterval = 1000; -constexpr int32_t kDefaultAudioReportInterval = 5000; } // namespace RTCPSender::FeedbackState::FeedbackState() @@ -114,25 +112,29 @@ class RTCPSender::RtcpContext { const int64_t now_us_; }; -RTCPSender::RTCPSender(const RtpRtcp::Configuration& config) - : audio_(config.audio), - clock_(config.clock), +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), random_(clock_->TimeInMicroseconds()), 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)), + event_log_(event_log), + transport_(outgoing_transport), + report_interval_ms_(report_interval_ms), sending_(false), next_time_to_send_rtcp_(0), timestamp_offset_(0), last_rtp_timestamp_(0), last_frame_capture_time_ms_(-1), - ssrc_(config.media_send_ssrc.value_or(0)), + ssrc_(0), remote_ssrc_(0), - receive_statistics_(config.receive_statistics), + receive_statistics_(receive_statistics), sequence_number_fir_(0), @@ -148,7 +150,7 @@ RTCPSender::RTCPSender(const RtpRtcp::Configuration& config) app_length_(0), xr_send_receiver_reference_time_enabled_(false), - packet_type_counter_observer_(config.rtcp_packet_type_counter_observer), + packet_type_counter_observer_(packet_type_counter_observer), send_video_bitrate_allocation_(false), last_payload_type_(-1) { RTC_DCHECK(transport_ != nullptr); @@ -305,7 +307,7 @@ uint32_t RTCPSender::SSRC() const { void RTCPSender::SetSSRC(uint32_t ssrc) { rtc::CritScope lock(&critical_section_rtcp_sender_); - if (ssrc_ != 0 && ssrc != ssrc_) { + if (ssrc_ != 0) { // 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 628121ea4e..74f4cc17a6 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -23,7 +23,6 @@ #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" @@ -63,7 +62,13 @@ class RTCPSender { ModuleRtpRtcpImpl* module; }; - explicit RTCPSender(const RtpRtcp::Configuration& config); + RTCPSender(bool audio, + Clock* clock, + ReceiveStatisticsProvider* receive_statistics, + RtcpPacketTypeCounterObserver* packet_type_counter_observer, + RtcEventLog* event_log, + Transport* outgoing_transport, + int report_interval_ms); 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 09cdff17a2..01101c0e4b 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -75,25 +75,22 @@ 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; - configuration.receive_statistics = receive_statistics_.get(); - configuration.media_send_ssrc = kSenderSsrc; - return configuration; + + 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); } void InsertIncomingPacket(uint32_t remote_ssrc, uint16_t seq_num) { @@ -190,13 +187,9 @@ TEST_F(RtcpSenderTest, SendConsecutiveSrWithExactSlope) { } TEST_F(RtcpSenderTest, DoNotSendSrBeforeRtp) { - 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_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), + nullptr, nullptr, &test_transport_, 1000)); + rtcp_sender_->SetSSRC(kSenderSsrc); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); rtcp_sender_->SetSendingStatus(feedback_state(), true); @@ -212,13 +205,9 @@ TEST_F(RtcpSenderTest, DoNotSendSrBeforeRtp) { } TEST_F(RtcpSenderTest, DoNotSendCompundBeforeRtp) { - 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_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), + nullptr, nullptr, &test_transport_, 1000)); + rtcp_sender_->SetSSRC(kSenderSsrc); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); rtcp_sender_->SetRTCPStatus(RtcpMode::kCompound); rtcp_sender_->SetSendingStatus(feedback_state(), true); @@ -562,14 +551,9 @@ TEST_F(RtcpSenderTest, TestNoXrRrtrSentIfNotEnabled) { TEST_F(RtcpSenderTest, TestRegisterRtcpPacketTypeObserver) { RtcpPacketTypeCounterObserverImpl observer; - 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_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), + &observer, nullptr, &test_transport_, + 1000)); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); EXPECT_EQ(0, rtcp_sender_->SendRTCP(feedback_state(), kRtcpPli)); @@ -690,14 +674,9 @@ TEST_F(RtcpSenderTest, ByeMustBeLast) { })); // Re-configure rtcp_sender_ with mock_transport_ - 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_.reset(new RTCPSender(false, &clock_, receive_statistics_.get(), + nullptr, nullptr, &mock_transport, 1000)); + rtcp_sender_->SetSSRC(kSenderSsrc); rtcp_sender_->SetRemoteSSRC(kRemoteSsrc); rtcp_sender_->SetTimestampOffset(kStartRtpTimestamp); rtcp_sender_->SetLastRtpTime(kRtpTimestamp, clock_.TimeInMilliseconds(), @@ -816,37 +795,4 @@ 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 765f76f70f..aa50227b14 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -61,7 +61,16 @@ RtpRtcp* RtpRtcp::CreateRtpRtcp(const RtpRtcp::Configuration& configuration) { } ModuleRtpRtcpImpl::ModuleRtpRtcpImpl(const Configuration& configuration) - : rtcp_sender_(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_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 a0690c1778..31f9b24965 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc @@ -162,7 +162,6 @@ 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 75423e5f56..96da991077 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -914,11 +914,9 @@ void VideoSendStreamTest::TestNackRetransmission( non_padding_sequence_numbers_.end() - kNackedPacketsAtOnceCount, non_padding_sequence_numbers_.end()); - RtpRtcp::Configuration config; - config.clock = Clock::GetRealTimeClock(); - config.outgoing_transport = transport_adapter_.get(); - config.rtcp_report_interval_ms = kRtcpIntervalMs; - RTCPSender rtcp_sender(config); + RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(), nullptr, + nullptr, nullptr, transport_adapter_.get(), + kRtcpIntervalMs); rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize); rtcp_sender.SetRemoteSSRC(kVideoSendSsrcs[0]); @@ -1129,12 +1127,9 @@ void VideoSendStreamTest::TestPacketFragmentationSize(VideoFormat format, kVideoSendSsrcs[0], header.sequenceNumber, packets_lost_, // Cumulative lost. loss_ratio); // Loss percent. - 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); + RTCPSender rtcp_sender(false, Clock::GetRealTimeClock(), + &lossy_receive_stats, nullptr, nullptr, + transport_adapter_.get(), kRtcpIntervalMs); rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize); rtcp_sender.SetRemoteSSRC(kVideoSendSsrcs[0]); @@ -1380,12 +1375,8 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_) { FakeReceiveStatistics receive_stats(kVideoSendSsrcs[0], last_sequence_number_, rtp_count_, 0); - 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); + RTCPSender rtcp_sender(false, clock_, &receive_stats, nullptr, nullptr, + transport_adapter_.get(), kRtcpIntervalMs); rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize); rtcp_sender.SetRemoteSSRC(kVideoSendSsrcs[0]);