From e8a6bc3f25015691bf290a19502e6e517bbb0c34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Tue, 15 Oct 2019 11:54:23 +0000 Subject: [PATCH] Revert "Reland "RtpRtcp modules and below: Make media, RTX and FEC SSRCs const"" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit c9348218cfe0cff6d0d3a383f7d1d6cfce4b1262. Reason for revert: Downstream tests are relying on incorrect behavior which this CL explicitly checks... Original change's description: > Reland "RtpRtcp modules and below: Make media, RTX and FEC SSRCs const" > > This is a reland of 17608dc4592fe25c1effdd75bf856f4af251942e > > Downstream fixed, relanding. > > Original change's description: > > RtpRtcp modules and below: Make media, RTX and FEC SSRCs const > > > > Downstream usage of SetSsrc() / SetRtxSsrc() should now be gone. Let's > > remove them, make the members const, and remove now unnecessary locking. > > > > Bug: webrtc:10774 > > Change-Id: Ie4c1b3935508cf329c5553030f740c565d32e04b > > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/155660 > > Commit-Queue: Erik Språng > > Reviewed-by: Niels Moller > > Cr-Commit-Position: refs/heads/master@{#29475} > > TBR=nisse@webrtc.org > > Bug: webrtc:10774 > Change-Id: I759bed3ff1909857696c6d1b13df595a5e552f03 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/157049 > Reviewed-by: Erik Språng > Commit-Queue: Erik Språng > Cr-Commit-Position: refs/heads/master@{#29486} TBR=nisse@webrtc.org,sprang@webrtc.org Change-Id: I168fb3738a04dfdbd1581ddd8c3276ede9f72322 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:10774 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/157080 Reviewed-by: Erik Språng Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#29488} --- call/rtp_video_sender.cc | 2 +- modules/rtp_rtcp/include/rtp_rtcp.h | 11 +++- modules/rtp_rtcp/source/nack_rtx_unittest.cc | 2 +- modules/rtp_rtcp/source/rtcp_receiver.cc | 31 +++++---- modules/rtp_rtcp/source/rtcp_receiver.h | 5 +- modules/rtp_rtcp/source/rtcp_sender.cc | 19 +++++- modules/rtp_rtcp/source/rtcp_sender.h | 6 +- .../rtp_rtcp/source/rtcp_sender_unittest.cc | 25 +++++++ modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 32 +++++++++ modules/rtp_rtcp/source/rtp_rtcp_impl.h | 8 ++- modules/rtp_rtcp/source/rtp_sender.cc | 66 ++++++++++++++++--- modules/rtp_rtcp/source/rtp_sender.h | 21 +++--- .../rtp_rtcp/source/rtp_sender_unittest.cc | 28 ++++++++ video/video_send_stream_tests.cc | 3 - 14 files changed, 215 insertions(+), 44 deletions(-) diff --git a/call/rtp_video_sender.cc b/call/rtp_video_sender.cc index fbfdc09e7a..73e356d3e8 100644 --- a/call/rtp_video_sender.cc +++ b/call/rtp_video_sender.cc @@ -177,7 +177,7 @@ std::vector CreateRtpStreamSenders( bool enable_flexfec = flexfec_sender != nullptr && std::find(flexfec_protected_ssrcs.begin(), flexfec_protected_ssrcs.end(), - configuration.local_media_ssrc) != + *configuration.local_media_ssrc) != flexfec_protected_ssrcs.end(); configuration.flexfec_sender = enable_flexfec ? flexfec_sender : nullptr; auto playout_delay_oracle = std::make_unique(); diff --git a/modules/rtp_rtcp/include/rtp_rtcp.h b/modules/rtp_rtcp/include/rtp_rtcp.h index a046f640c1..69ca8f81b3 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp.h +++ b/modules/rtp_rtcp/include/rtp_rtcp.h @@ -122,7 +122,7 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { // SSRCs for media and retransmission, respectively. // FlexFec SSRC is fetched from |flexfec_sender|. - uint32_t local_media_ssrc; + absl::optional local_media_ssrc; absl::optional rtx_send_ssrc; private: @@ -200,6 +200,10 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { // Returns SSRC. uint32_t SSRC() const override = 0; + // Sets SSRC, default is a random number. + // TODO(bugs.webrtc.org/10774): Remove. + virtual void SetSSRC(uint32_t ssrc) = 0; + // Sets the value for sending in the RID (and Repaired) RTP header extension. // RIDs are used to identify an RTP stream if SSRCs are not negotiated. // If the RID and Repaired RID extensions are not registered, the RID will @@ -223,6 +227,11 @@ class RtpRtcp : public Module, public RtcpFeedbackSenderInterface { // a combination of values of the enumerator RtxMode. virtual int RtxSendStatus() const = 0; + // Sets the SSRC to use when sending RTX packets. This doesn't enable RTX, + // only the SSRC is set. + // TODO(bugs.webrtc.org/10774): Remove. + virtual void SetRtxSsrc(uint32_t ssrc) = 0; + // Sets the payload type to use when sending RTX packets. Note that this // doesn't enable RTX, only the payload type is set. virtual void SetRtxSendPayloadType(int payload_type, diff --git a/modules/rtp_rtcp/source/nack_rtx_unittest.cc b/modules/rtp_rtcp/source/nack_rtx_unittest.cc index 17601dd966..a75fd6e101 100644 --- a/modules/rtp_rtcp/source/nack_rtx_unittest.cc +++ b/modules/rtp_rtcp/source/nack_rtx_unittest.cc @@ -134,7 +134,6 @@ class RtpRtcpRtxNackTest : public ::testing::Test { configuration.outgoing_transport = &transport_; configuration.retransmission_rate_limiter = &retransmission_rate_limiter_; configuration.local_media_ssrc = kTestSsrc; - configuration.rtx_send_ssrc = kTestRtxSsrc; rtp_rtcp_module_ = RtpRtcp::Create(configuration); FieldTrialBasedConfig field_trials; RTPSenderVideo::Config video_config; @@ -201,6 +200,7 @@ class RtpRtcpRtxNackTest : public ::testing::Test { rtx_receiver_ = transport_.stream_receiver_controller_.CreateReceiver( kTestRtxSsrc, &rtx_stream_); rtp_rtcp_module_->SetRtxSendStatus(rtx_method); + rtp_rtcp_module_->SetRtxSsrc(kTestRtxSsrc); transport_.DropEveryNthPacket(loss); uint32_t timestamp = 3000; uint16_t nack_list[kVideoNackListSize]; diff --git a/modules/rtp_rtcp/source/rtcp_receiver.cc b/modules/rtp_rtcp/source/rtcp_receiver.cc index 6b64473eea..f06fd1c361 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.cc +++ b/modules/rtp_rtcp/source/rtcp_receiver.cc @@ -65,18 +65,6 @@ const size_t kMaxNumberOfStoredRrtrs = 200; constexpr int32_t kDefaultVideoReportInterval = 1000; constexpr int32_t kDefaultAudioReportInterval = 5000; - -std::set GetRegisteredSsrcs(const RtpRtcp::Configuration& config) { - std::set ssrcs; - ssrcs.insert(config.local_media_ssrc); - if (config.rtx_send_ssrc) { - ssrcs.insert(*config.rtx_send_ssrc); - } - if (config.flexfec_sender) { - ssrcs.insert(config.flexfec_sender->ssrc()); - } - return ssrcs; -} } // namespace struct RTCPReceiver::PacketInformation { @@ -138,8 +126,6 @@ RTCPReceiver::RTCPReceiver(const RtpRtcp::Configuration& config, : clock_(config.clock), receiver_only_(config.receiver_only), rtp_rtcp_(owner), - main_ssrc_(config.local_media_ssrc), - registered_ssrcs_(GetRegisteredSsrcs(config)), rtcp_bandwidth_observer_(config.bandwidth_callback), rtcp_intra_frame_observer_(config.intra_frame_callback), rtcp_loss_notification_observer_(config.rtcp_loss_notification_observer), @@ -151,6 +137,7 @@ RTCPReceiver::RTCPReceiver(const RtpRtcp::Configuration& config, : (config.audio ? kDefaultAudioReportInterval : kDefaultVideoReportInterval)), // TODO(bugs.webrtc.org/10774): Remove fallback. + main_ssrc_(config.local_media_ssrc.value_or(0)), remote_ssrc_(0), remote_sender_rtp_time_(0), xr_rrtr_status_(false), @@ -165,6 +152,15 @@ RTCPReceiver::RTCPReceiver(const RtpRtcp::Configuration& config, num_skipped_packets_(0), last_skipped_packets_warning_ms_(clock_->TimeInMilliseconds()) { RTC_DCHECK(owner); + if (config.local_media_ssrc) { + registered_ssrcs_.insert(*config.local_media_ssrc); + } + if (config.rtx_send_ssrc) { + registered_ssrcs_.insert(*config.rtx_send_ssrc); + } + if (config.flexfec_sender) { + registered_ssrcs_.insert(config.flexfec_sender->ssrc()); + } } RTCPReceiver::~RTCPReceiver() {} @@ -198,6 +194,13 @@ uint32_t RTCPReceiver::RemoteSSRC() const { return remote_ssrc_; } +void RTCPReceiver::SetSsrcs(uint32_t main_ssrc, + const std::set& registered_ssrcs) { + rtc::CritScope lock(&rtcp_receiver_lock_); + main_ssrc_ = main_ssrc; + registered_ssrcs_ = registered_ssrcs; +} + int32_t RTCPReceiver::RTT(uint32_t remote_ssrc, int64_t* last_rtt_ms, int64_t* avg_rtt_ms, diff --git a/modules/rtp_rtcp/source/rtcp_receiver.h b/modules/rtp_rtcp/source/rtcp_receiver.h index 5b92d55609..30567110a1 100644 --- a/modules/rtp_rtcp/source/rtcp_receiver.h +++ b/modules/rtp_rtcp/source/rtcp_receiver.h @@ -59,6 +59,7 @@ class RTCPReceiver { int64_t LastReceivedReportBlockMs() const; + void SetSsrcs(uint32_t main_ssrc, const std::set& registered_ssrcs); void SetRemoteSSRC(uint32_t ssrc); uint32_t RemoteSSRC() const; @@ -214,8 +215,6 @@ class RTCPReceiver { Clock* const clock_; const bool receiver_only_; ModuleRtpRtcp* const rtp_rtcp_; - const uint32_t main_ssrc_; - const std::set registered_ssrcs_; rtc::CriticalSection feedbacks_lock_; RtcpBandwidthObserver* const rtcp_bandwidth_observer_; @@ -227,7 +226,9 @@ class RTCPReceiver { const int report_interval_ms_; rtc::CriticalSection rtcp_receiver_lock_; + uint32_t main_ssrc_ RTC_GUARDED_BY(rtcp_receiver_lock_); uint32_t remote_ssrc_ RTC_GUARDED_BY(rtcp_receiver_lock_); + std::set registered_ssrcs_ RTC_GUARDED_BY(rtcp_receiver_lock_); // Received sender report. NtpTime remote_sender_ntp_time_ RTC_GUARDED_BY(rtcp_receiver_lock_); diff --git a/modules/rtp_rtcp/source/rtcp_sender.cc b/modules/rtp_rtcp/source/rtcp_sender.cc index fba9b45ac5..15325d1592 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.cc +++ b/modules/rtp_rtcp/source/rtcp_sender.cc @@ -150,7 +150,6 @@ class RTCPSender::RtcpContext { RTCPSender::RTCPSender(const RtpRtcp::Configuration& config) : audio_(config.audio), - ssrc_(config.local_media_ssrc), clock_(config.clock), random_(clock_->TimeInMicroseconds()), method_(RtcpMode::kOff), @@ -165,6 +164,7 @@ RTCPSender::RTCPSender(const RtpRtcp::Configuration& config) timestamp_offset_(0), last_rtp_timestamp_(0), last_frame_capture_time_ms_(-1), + ssrc_(config.local_media_ssrc.value_or(0)), remote_ssrc_(0), receive_statistics_(config.receive_statistics), @@ -331,6 +331,23 @@ void RTCPSender::SetRtpClockRate(int8_t payload_type, int rtp_clock_rate_hz) { rtp_clock_rates_khz_[payload_type] = rtp_clock_rate_hz / 1000; } +uint32_t RTCPSender::SSRC() const { + rtc::CritScope lock(&critical_section_rtcp_sender_); + return ssrc_; +} + +void RTCPSender::SetSSRC(uint32_t ssrc) { + rtc::CritScope lock(&critical_section_rtcp_sender_); + + 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 + next_time_to_send_rtcp_ = clock_->TimeInMilliseconds() + 100; + } + ssrc_ = ssrc; +} + void RTCPSender::SetRemoteSSRC(uint32_t ssrc) { rtc::CritScope lock(&critical_section_rtcp_sender_); remote_ssrc_ = ssrc; diff --git a/modules/rtp_rtcp/source/rtcp_sender.h b/modules/rtp_rtcp/source/rtcp_sender.h index 97b4b70919..6deee878a9 100644 --- a/modules/rtp_rtcp/source/rtcp_sender.h +++ b/modules/rtp_rtcp/source/rtcp_sender.h @@ -85,7 +85,9 @@ class RTCPSender { void SetRtpClockRate(int8_t payload_type, int rtp_clock_rate_hz); - uint32_t SSRC() const { return ssrc_; } + uint32_t SSRC() const; + + void SetSSRC(uint32_t ssrc); void SetRemoteSSRC(uint32_t ssrc); @@ -185,7 +187,6 @@ class RTCPSender { private: const bool audio_; - const uint32_t ssrc_; Clock* const clock_; Random random_ RTC_GUARDED_BY(critical_section_rtcp_sender_); RtcpMode method_ RTC_GUARDED_BY(critical_section_rtcp_sender_); @@ -204,6 +205,7 @@ class RTCPSender { uint32_t last_rtp_timestamp_ RTC_GUARDED_BY(critical_section_rtcp_sender_); int64_t last_frame_capture_time_ms_ RTC_GUARDED_BY(critical_section_rtcp_sender_); + uint32_t ssrc_ RTC_GUARDED_BY(critical_section_rtcp_sender_); // SSRC that we receive on our RTP channel uint32_t remote_ssrc_ RTC_GUARDED_BY(critical_section_rtcp_sender_); std::string cname_ RTC_GUARDED_BY(critical_section_rtcp_sender_); diff --git a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc index c732a35bd0..c3f3920d3e 100644 --- a/modules/rtp_rtcp/source/rtcp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtcp_sender_unittest.cc @@ -825,6 +825,31 @@ TEST_F(RtcpSenderTest, DoesntSchedulesInitialReportWhenSsrcSetOnConstruction) { EXPECT_FALSE(rtcp_sender_->TimeToSendRTCPReport(false)); } +TEST_F(RtcpSenderTest, DoesntSchedulesInitialReportOnFirstSetSsrc) { + // Set up without first SSRC not set at construction. + RtpRtcp::Configuration configuration = GetDefaultConfig(); + configuration.local_media_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)); +} + TEST_F(RtcpSenderTest, SendsCombinedRtcpPacket) { rtcp_sender_->SetRTCPStatus(RtcpMode::kReducedSize); diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 7938396d99..7d8e33868a 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -175,6 +175,10 @@ int ModuleRtpRtcpImpl::RtxSendStatus() const { return rtp_sender_ ? rtp_sender_->RtxStatus() : kRtxOff; } +void ModuleRtpRtcpImpl::SetRtxSsrc(uint32_t ssrc) { + rtp_sender_->SetRtxSsrc(ssrc); +} + void ModuleRtpRtcpImpl::SetRtxSendPayloadType(int payload_type, int associated_payload_type) { rtp_sender_->SetRtxPayloadType(payload_type, associated_payload_type); @@ -236,6 +240,18 @@ RtpState ModuleRtpRtcpImpl::GetRtxState() const { return rtp_sender_->GetRtxRtpState(); } +uint32_t ModuleRtpRtcpImpl::SSRC() const { + return rtcp_sender_.SSRC(); +} + +void ModuleRtpRtcpImpl::SetSSRC(const uint32_t ssrc) { + if (rtp_sender_) { + rtp_sender_->SetSSRC(ssrc); + } + rtcp_sender_.SetSSRC(ssrc); + SetRtcpReceiverSsrcs(ssrc); +} + void ModuleRtpRtcpImpl::SetRid(const std::string& rid) { if (rtp_sender_) { rtp_sender_->SetRid(rid); @@ -290,6 +306,11 @@ int32_t ModuleRtpRtcpImpl::SetSendingStatus(const bool sending) { if (rtcp_sender_.SetSendingStatus(GetFeedbackState(), sending) != 0) { RTC_LOG(LS_WARNING) << "Failed to send RTCP BYE"; } + if (sending && rtp_sender_) { + // Update Rtcp receiver config, to track Rtx config changes from + // the SetRtxStatus and SetRtxSsrc methods. + SetRtcpReceiverSsrcs(rtp_sender_->SSRC()); + } } return 0; } @@ -734,6 +755,17 @@ std::vector ModuleRtpRtcpImpl::BoundingSet(bool* tmmbr_owner) { return rtcp_receiver_.BoundingSet(tmmbr_owner); } +void ModuleRtpRtcpImpl::SetRtcpReceiverSsrcs(uint32_t main_ssrc) { + std::set ssrcs; + ssrcs.insert(main_ssrc); + if (RtxSendStatus() != kRtxOff) + ssrcs.insert(rtp_sender_->RtxSsrc()); + absl::optional flexfec_ssrc = FlexfecSsrc(); + if (flexfec_ssrc) + ssrcs.insert(*flexfec_ssrc); + rtcp_receiver_.SetSsrcs(main_ssrc, ssrcs); +} + void ModuleRtpRtcpImpl::set_rtt_ms(int64_t rtt_ms) { rtc::CritScope cs(&critical_section_rtt_); rtt_ms_ = rtt_ms; diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h index 312f9d63cc..9ec481c842 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h @@ -94,7 +94,10 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { RtpState GetRtpState() const override; RtpState GetRtxState() const override; - uint32_t SSRC() const override { return rtcp_sender_.SSRC(); } + uint32_t SSRC() const override; + + // Configure SSRC, default is a random number. + void SetSSRC(uint32_t ssrc) override; void SetRid(const std::string& rid) override; @@ -107,6 +110,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { void SetRtxSendStatus(int mode) override; int RtxSendStatus() const override; + void SetRtxSsrc(uint32_t ssrc) override; + void SetRtxSendPayloadType(int payload_type, int associated_payload_type) override; @@ -297,6 +302,7 @@ class ModuleRtpRtcpImpl : public RtpRtcp, public RTCPReceiver::ModuleRtpRtcp { private: FRIEND_TEST_ALL_PREFIXES(RtpRtcpImplTest, Rtt); FRIEND_TEST_ALL_PREFIXES(RtpRtcpImplTest, RttForReceiverOnly); + void SetRtcpReceiverSsrcs(uint32_t main_ssrc); void set_rtt_ms(int64_t rtt_ms); int64_t rtt_ms() const; diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 5aa707f1b1..c88e0e20b0 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -124,8 +124,6 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config) : clock_(config.clock), random_(clock_->TimeInMicroseconds()), audio_configured_(config.audio), - ssrc_(config.local_media_ssrc), - rtx_ssrc_(config.rtx_send_ssrc), flexfec_ssrc_(config.flexfec_sender ? absl::make_optional(config.flexfec_sender->ssrc()) : absl::nullopt), @@ -156,6 +154,7 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config) bitrate_callback_(config.send_bitrate_observer), // RTP variables sequence_number_forced_(false), + ssrc_(config.local_media_ssrc), ssrc_has_acked_(false), rtx_ssrc_has_acked_(false), last_rtp_timestamp_(0), @@ -165,6 +164,7 @@ RTPSender::RTPSender(const RtpRtcp::Configuration& config) last_packet_marker_bit_(false), csrcs_(), rtx_(kRtxOff), + ssrc_rtx_(config.rtx_send_ssrc), rtp_overhead_bytes_per_packet_(0), supports_bwe_extension_(false), retransmission_rate_limiter_(config.retransmission_rate_limiter), @@ -267,6 +267,17 @@ int RTPSender::RtxStatus() const { return rtx_; } +void RTPSender::SetRtxSsrc(uint32_t ssrc) { + rtc::CritScope lock(&send_critsect_); + ssrc_rtx_.emplace(ssrc); +} + +uint32_t RTPSender::RtxSsrc() const { + rtc::CritScope lock(&send_critsect_); + RTC_DCHECK(ssrc_rtx_); + return *ssrc_rtx_; +} + void RTPSender::SetRtxPayloadType(int payload_type, int associated_payload_type) { rtc::CritScope lock(&send_critsect_); @@ -417,7 +428,7 @@ bool RTPSender::TrySendPacket(RtpPacketToSend* packet, case RtpPacketToSend::Type::kPadding: // Both padding and retransmission must be on either the media or the // RTX stream. - if (packet_ssrc == rtx_ssrc_) { + if (packet_ssrc == ssrc_rtx_) { is_rtx = true; } else if (packet_ssrc != ssrc_) { return false; @@ -610,7 +621,7 @@ std::vector> RTPSender::GeneratePadding( } RTC_DCHECK(ssrc_); - padding_packet->SetSsrc(ssrc_); + padding_packet->SetSsrc(*ssrc_); padding_packet->SetPayloadType(last_payload_type_); padding_packet->SetSequenceNumber(sequence_number_++); } else { @@ -634,8 +645,8 @@ std::vector> RTPSender::GeneratePadding( padding_packet->set_capture_time_ms(padding_packet->capture_time_ms() + (now_ms - last_timestamp_time_ms_)); } - RTC_DCHECK(rtx_ssrc_); - padding_packet->SetSsrc(*rtx_ssrc_); + RTC_DCHECK(ssrc_rtx_); + padding_packet->SetSsrc(*ssrc_rtx_); padding_packet->SetSequenceNumber(sequence_number_rtx_++); padding_packet->SetPayloadType(rtx_payload_type_map_.begin()->second); } @@ -791,10 +802,17 @@ void RTPSender::ProcessBitrate() { if (!bitrate_callback_) return; int64_t now_ms = clock_->TimeInMilliseconds(); + uint32_t ssrc; + { + rtc::CritScope lock(&send_critsect_); + if (!ssrc_) + return; + ssrc = *ssrc_; + } rtc::CritScope lock(&statistics_crit_); bitrate_callback_->Notify(total_bitrate_sent_.Rate(now_ms).value_or(0), - nack_bitrate_sent_.Rate(now_ms).value_or(0), ssrc_); + nack_bitrate_sent_.Rate(now_ms).value_or(0), ssrc); } size_t RTPSender::RtpHeaderLength() const { @@ -832,7 +850,7 @@ std::unique_ptr RTPSender::AllocatePacket() const { auto packet = std::make_unique( &rtp_header_extension_map_, max_packet_size_ + kExtraCapacity); RTC_DCHECK(ssrc_); - packet->SetSsrc(ssrc_); + packet->SetSsrc(*ssrc_); packet->SetCsrcs(csrcs_); // Reserve extensions, if registered, RtpSender set in SendToNetwork. packet->ReserveExtension(); @@ -905,6 +923,30 @@ uint32_t RTPSender::TimestampOffset() const { return timestamp_offset_; } +void RTPSender::SetSSRC(uint32_t ssrc) { + { + rtc::CritScope lock(&send_critsect_); + if (ssrc_ == ssrc) { + return; // Since it's the same SSRC, don't reset anything. + } + + ssrc_.emplace(ssrc); + if (!sequence_number_forced_) { + sequence_number_ = random_.Rand(1, kMaxInitRtpSeqNumber); + } + } + + // Clear RTP packet history, since any packets there belong to the old SSRC + // and they may conflict with packets from the new one. + packet_history_.Clear(); +} + +uint32_t RTPSender::SSRC() const { + rtc::CritScope lock(&send_critsect_); + RTC_DCHECK(ssrc_); + return *ssrc_; +} + void RTPSender::SetRid(const std::string& rid) { // RID is used in simulcast scenario when multiple layers share the same mid. rtc::CritScope lock(&send_critsect_); @@ -919,6 +961,10 @@ void RTPSender::SetMid(const std::string& mid) { mid_ = mid; } +absl::optional RTPSender::FlexfecSsrc() const { + return flexfec_ssrc_; +} + void RTPSender::SetCsrcs(const std::vector& csrcs) { RTC_DCHECK_LE(csrcs.size(), kRtpCsrcSize); rtc::CritScope lock(&send_critsect_); @@ -1006,7 +1052,7 @@ std::unique_ptr RTPSender::BuildRtxPacket( if (!sending_media_) return nullptr; - RTC_DCHECK(rtx_ssrc_); + RTC_DCHECK(ssrc_rtx_); // Replace payload type. auto kv = rtx_payload_type_map_.find(packet.PayloadType()); @@ -1022,7 +1068,7 @@ std::unique_ptr RTPSender::BuildRtxPacket( rtx_packet->SetSequenceNumber(sequence_number_rtx_++); // Replace SSRC. - rtx_packet->SetSsrc(*rtx_ssrc_); + rtx_packet->SetSsrc(*ssrc_rtx_); CopyHeaderAndExtensionsToRtxPacket(packet, rtx_packet.get()); diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index 9194d441de..d0a8396973 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -67,6 +67,9 @@ class RTPSender { uint32_t TimestampOffset() const; void SetTimestampOffset(uint32_t timestamp); + // TODO(bugs.webrtc.org/10774): Remove. + void SetSSRC(uint32_t ssrc); + void SetRid(const std::string& rid); void SetMid(const std::string& mid); @@ -113,10 +116,10 @@ class RTPSender { // RTX. void SetRtxStatus(int mode); int RtxStatus() const; - uint32_t RtxSsrc() const { - RTC_DCHECK(rtx_ssrc_); - return *rtx_ssrc_; - } + uint32_t RtxSsrc() const; + + // TODO(bugs.webrtc.org/10774): Remove. + void SetRtxSsrc(uint32_t ssrc); void SetRtxPayloadType(int payload_type, int associated_payload_type); @@ -140,9 +143,9 @@ class RTPSender { // Including RTP headers. size_t MaxRtpPacketSize() const; - uint32_t SSRC() const { return ssrc_; } + uint32_t SSRC() const; - absl::optional FlexfecSsrc() const { return flexfec_ssrc_; } + absl::optional FlexfecSsrc() const; // Sends packet to |transport_| or to the pacer, depending on configuration. // TODO(bugs.webrtc.org/XXX): Remove in favor of EnqueuePackets(). @@ -222,8 +225,6 @@ class RTPSender { const bool audio_configured_; - const uint32_t ssrc_; - const absl::optional rtx_ssrc_; const absl::optional flexfec_ssrc_; const std::unique_ptr non_paced_packet_sender_; @@ -267,6 +268,9 @@ class RTPSender { bool sequence_number_forced_ RTC_GUARDED_BY(send_critsect_); uint16_t sequence_number_ RTC_GUARDED_BY(send_critsect_); uint16_t sequence_number_rtx_ RTC_GUARDED_BY(send_critsect_); + // Must be explicitly set by the application, use of absl::optional + // only to keep track of correct use. + absl::optional ssrc_ RTC_GUARDED_BY(send_critsect_); // RID value to send in the RID or RepairedRID header extension. std::string rid_ RTC_GUARDED_BY(send_critsect_); // MID value to send in the MID header extension. @@ -282,6 +286,7 @@ class RTPSender { bool last_packet_marker_bit_ RTC_GUARDED_BY(send_critsect_); std::vector csrcs_ RTC_GUARDED_BY(send_critsect_); int rtx_ RTC_GUARDED_BY(send_critsect_); + absl::optional ssrc_rtx_ RTC_GUARDED_BY(send_critsect_); // Mapping rtx_payload_type_map_[associated] = rtx. std::map rtx_payload_type_map_ RTC_GUARDED_BY(send_critsect_); size_t rtp_overhead_bytes_per_packet_ RTC_GUARDED_BY(send_critsect_); diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 0b2d48e9db..da7ba4f67b 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -2562,6 +2562,34 @@ TEST_P(RtpSenderTest, SetsCaptureTimeAndPopulatesTransmissionOffset) { EXPECT_EQ(*transmission_time_extension, 2 * kOffsetMs * kTimestampTicksPerMs); } +TEST_P(RtpSenderTestWithoutPacer, ClearHistoryOnSsrcChange) { + const int64_t kRtt = 10; + + rtp_sender_->SetSendingMediaStatus(true); + rtp_sender_->SetRtxStatus(kRtxRetransmitted | kRtxRedundantPayloads); + rtp_sender_->SetRtxPayloadType(kRtxPayload, kPayload); + rtp_sender_->SetStorePacketsStatus(true, 10); + rtp_sender_->SetRtt(kRtt); + + // Send a packet and record its sequence numbers. + SendGenericPacket(); + ASSERT_EQ(1u, transport_.sent_packets_.size()); + const uint16_t packet_seqence_number = + transport_.sent_packets_.back().SequenceNumber(); + + // Advance time and make sure it can be retransmitted, even if we try to set + // the ssrc the what it already is. + rtp_sender_->SetSSRC(kSsrc); + fake_clock_.AdvanceTimeMilliseconds(kRtt); + EXPECT_GT(rtp_sender_->ReSendPacket(packet_seqence_number), 0); + + // Change the SSRC, then move the time and try to retransmit again. The old + // packet should now be gone. + rtp_sender_->SetSSRC(kSsrc + 1); + fake_clock_.AdvanceTimeMilliseconds(kRtt); + EXPECT_EQ(rtp_sender_->ReSendPacket(packet_seqence_number), 0); +} + TEST_P(RtpSenderTestWithoutPacer, ClearHistoryOnSequenceNumberCange) { const int64_t kRtt = 10; diff --git a/video/video_send_stream_tests.cc b/video/video_send_stream_tests.cc index d769bfe9e4..0e4c11461c 100644 --- a/video/video_send_stream_tests.cc +++ b/video/video_send_stream_tests.cc @@ -933,7 +933,6 @@ void VideoSendStreamTest::TestNackRetransmission( config.clock = Clock::GetRealTimeClock(); config.outgoing_transport = transport_adapter_.get(); config.rtcp_report_interval_ms = kRtcpIntervalMs; - config.local_media_ssrc = kReceiverLocalVideoSsrc; RTCPSender rtcp_sender(config); rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize); @@ -1150,7 +1149,6 @@ void VideoSendStreamTest::TestPacketFragmentationSize(VideoFormat format, config.receive_statistics = &lossy_receive_stats; config.outgoing_transport = transport_adapter_.get(); config.rtcp_report_interval_ms = kRtcpIntervalMs; - config.local_media_ssrc = kVideoSendSsrcs[0]; RTCPSender rtcp_sender(config); rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize); @@ -1402,7 +1400,6 @@ TEST_F(VideoSendStreamTest, SuspendBelowMinBitrate) { config.receive_statistics = &receive_stats; config.outgoing_transport = transport_adapter_.get(); config.rtcp_report_interval_ms = kRtcpIntervalMs; - config.local_media_ssrc = kVideoSendSsrcs[0]; RTCPSender rtcp_sender(config); rtcp_sender.SetRTCPStatus(RtcpMode::kReducedSize);