diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index 2946b5c1394..56922b73a45 100644 --- a/modules/pacing/packet_router.cc +++ b/modules/pacing/packet_router.cc @@ -21,7 +21,6 @@ #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" #include "modules/rtp_rtcp/source/rtcp_packet.h" #include "modules/rtp_rtcp/source/rtcp_packet/transport_feedback.h" -#include "rtc_base/atomic_ops.h" #include "rtc_base/checks.h" #include "rtc_base/logging.h" #include "rtc_base/time_utils.h" @@ -33,14 +32,16 @@ constexpr int kRembSendIntervalMs = 200; } // namespace -PacketRouter::PacketRouter() +PacketRouter::PacketRouter() : PacketRouter(0) {} + +PacketRouter::PacketRouter(uint16_t start_transport_seq) : last_send_module_(nullptr), last_remb_time_ms_(rtc::TimeMillis()), last_send_bitrate_bps_(0), bitrate_bps_(0), max_bitrate_bps_(std::numeric_limits::max()), active_remb_module_(nullptr), - transport_seq_(0) {} + transport_seq_(start_transport_seq) {} PacketRouter::~PacketRouter() { RTC_DCHECK(rtp_send_modules_.empty()); @@ -185,25 +186,19 @@ std::vector> PacketRouter::GeneratePadding( } void PacketRouter::SetTransportWideSequenceNumber(uint16_t sequence_number) { - rtc::AtomicOps::ReleaseStore(&transport_seq_, sequence_number); + rtc::CritScope lock(&modules_crit_); + transport_seq_ = sequence_number; } uint16_t PacketRouter::AllocateSequenceNumber() { - int prev_seq = rtc::AtomicOps::AcquireLoad(&transport_seq_); - int desired_prev_seq; - int new_seq; - do { - desired_prev_seq = prev_seq; - new_seq = (desired_prev_seq + 1) & 0xFFFF; - // Note: CompareAndSwap returns the actual value of transport_seq at the - // time the CAS operation was executed. Thus, if prev_seq is returned, the - // operation was successful - otherwise we need to retry. Saving the - // return value saves us a load on retry. - prev_seq = rtc::AtomicOps::CompareAndSwap(&transport_seq_, desired_prev_seq, - new_seq); - } while (prev_seq != desired_prev_seq); - - return new_seq; + rtc::CritScope lock(&modules_crit_); + transport_seq_ = (transport_seq_ + 1) & 0xFFFF; + return transport_seq_; +} + +uint16_t PacketRouter::CurrentTransportSequenceNumber() const { + rtc::CritScope lock(&modules_crit_); + return transport_seq_; } void PacketRouter::OnReceiveBitrateChanged(const std::vector& ssrcs, diff --git a/modules/pacing/packet_router.h b/modules/pacing/packet_router.h index 85aa003696e..3680bce3d9f 100644 --- a/modules/pacing/packet_router.h +++ b/modules/pacing/packet_router.h @@ -41,6 +41,7 @@ class PacketRouter : public RemoteBitrateObserver, public TransportFeedbackSenderInterface { public: PacketRouter(); + explicit PacketRouter(uint16_t start_transport_seq); ~PacketRouter() override; void AddSendRtpModule(RtpRtcp* rtp_module, bool remb_candidate); @@ -56,9 +57,13 @@ class PacketRouter : public RemoteBitrateObserver, virtual std::vector> GeneratePadding( size_t target_size_bytes); + // TODO(bugs.webrtc.org/11036): Remove when downstream usage is gone. void SetTransportWideSequenceNumber(uint16_t sequence_number); + // TODO(bugs.webrtc.org/11036): Make private when downstream usage is gone. uint16_t AllocateSequenceNumber(); + uint16_t CurrentTransportSequenceNumber() const; + // Called every time there is a new bitrate estimate for a receive channel // group. This call will trigger a new RTCP REMB packet if the bitrate // estimate has decreased or if no RTCP REMB packet has been sent for @@ -126,7 +131,7 @@ class PacketRouter : public RemoteBitrateObserver, RtcpFeedbackSenderInterface* active_remb_module_ RTC_GUARDED_BY(modules_crit_); - volatile int transport_seq_; + int transport_seq_ RTC_GUARDED_BY(modules_crit_); RTC_DISALLOW_COPY_AND_ASSIGN(PacketRouter); }; diff --git a/modules/pacing/packet_router_unittest.cc b/modules/pacing/packet_router_unittest.cc index 3fd98822074..1239201a6c5 100644 --- a/modules/pacing/packet_router_unittest.cc +++ b/modules/pacing/packet_router_unittest.cc @@ -226,17 +226,27 @@ TEST_F(PacketRouterTest, PadsOnLastActiveMediaStream) { packet_router_.RemoveSendRtpModule(&rtp_3); } -TEST_F(PacketRouterTest, AllocateSequenceNumbers) { +TEST_F(PacketRouterTest, AllocatesTransportSequenceNumbers) { const uint16_t kStartSeq = 0xFFF0; const size_t kNumPackets = 32; + const uint16_t kSsrc1 = 1234; - packet_router_.SetTransportWideSequenceNumber(kStartSeq - 1); + PacketRouter packet_router(kStartSeq - 1); + NiceMock rtp_1; + EXPECT_CALL(rtp_1, SSRC()).WillRepeatedly(Return(kSsrc1)); + EXPECT_CALL(rtp_1, TrySendPacket).WillRepeatedly(Return(true)); + packet_router.AddSendRtpModule(&rtp_1, false); for (size_t i = 0; i < kNumPackets; ++i) { - uint16_t seq = packet_router_.AllocateSequenceNumber(); + auto packet = BuildRtpPacket(kSsrc1); + EXPECT_TRUE(packet->ReserveExtension()); + packet_router.SendPacket(std::move(packet), PacedPacketInfo()); uint32_t expected_unwrapped_seq = static_cast(kStartSeq) + i; - EXPECT_EQ(static_cast(expected_unwrapped_seq & 0xFFFF), seq); + EXPECT_EQ(static_cast(expected_unwrapped_seq & 0xFFFF), + packet_router.CurrentTransportSequenceNumber()); } + + packet_router.RemoveSendRtpModule(&rtp_1); } TEST_F(PacketRouterTest, SendTransportFeedback) {