Refactor handling of TransportSequenceNumber in PacketRouter

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 <srte@webrtc.org>
Commit-Queue: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29507}

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<decltype(max_bitrate_bps_)>::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<std::unique_ptr<RtpPacketToSend>> 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<uint32_t>& ssrcs,

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<std::unique_ptr<RtpPacketToSend>> 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);
 };

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<MockRtpRtcp> 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<TransportSequenceNumber>());
+    packet_router.SendPacket(std::move(packet), PacedPacketInfo());
     uint32_t expected_unwrapped_seq = static_cast<uint32_t>(kStartSeq) + i;
-    EXPECT_EQ(static_cast<uint16_t>(expected_unwrapped_seq & 0xFFFF), seq);
+    EXPECT_EQ(static_cast<uint16_t>(expected_unwrapped_seq & 0xFFFF),
+              packet_router.CurrentTransportSequenceNumber());
   }
+
+  packet_router.RemoveSendRtpModule(&rtp_1);
 }
 
 TEST_F(PacketRouterTest, SendTransportFeedback) {