From 5f01bf6c8b786d7dada2547be365572d9e8b9772 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Wed, 16 Oct 2019 17:06:34 +0200 Subject: [PATCH] Refactor handling of TransportSequenceNumber in PacketRouter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The use of SetTransportWideSequenceNumber() and AllocateSequenceNumber() is gone from webrtc, but some downstream code still references them. This means we can do some simplifications. The member that stores the sequence number is now always accessed while holding the modules lock, so we can just use that and don't need to add atomic operations on top. SetTransportWideSequenceNumber() is only used to set the start sequence number, it would be nice to set that in the constructor instead. AllocateSequnceNumber() is now actually only used as a getter, so this can be replace by a proper const getter method instead. Bug: webrtc:11036 Change-Id: I69b06e613ca3361cf24ef835b92dd0a894cbd27e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/157167 Reviewed-by: Sebastian Jansson Commit-Queue: Erik Språng Cr-Commit-Position: refs/heads/master@{#29507} --- modules/pacing/packet_router.cc | 31 ++++++++++-------------- modules/pacing/packet_router.h | 7 +++++- modules/pacing/packet_router_unittest.cc | 18 +++++++++++--- 3 files changed, 33 insertions(+), 23 deletions(-) diff --git a/modules/pacing/packet_router.cc b/modules/pacing/packet_router.cc index 2946b5c139..56922b73a4 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); + rtc::CritScope lock(&modules_crit_); + transport_seq_ = (transport_seq_ + 1) & 0xFFFF; + return transport_seq_; +} - return new_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 85aa003696..3680bce3d9 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 3fd9882207..1239201a6c 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) {