Refactor PacketSequencer in preparation for deferred sequencing.

This CL is extracted from
https://webrtc-review.googlesource.com/c/src/+/208584
PacketSequencer now has its own unit tests. They are maybe somewhat
redundant with a few RtpSender unit tests, but will defer cleanup to
a later CL.

Bug: webrtc:11340, webrtc:12470
Change-Id: I1c31004b85ae075ddc696bdf1100d2a5044d4ef5
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/227343
Commit-Queue: Erik Språng <sprang@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34638}

modules/rtp_rtcp/BUILD.gn

@@ -304,7 +304,6 @@ rtc_library("rtp_rtcp") {
     "../../rtc_base/task_utils:repeating_task",
     "../../rtc_base/task_utils:to_queued_task",
     "../../rtc_base/time:timestamp_extrapolator",
-    "../../rtc_base/containers:flat_map",
     "../../system_wrappers",
     "../../system_wrappers:metrics",
     "../remote_bitrate_estimator",
@@ -499,6 +498,7 @@ if (rtc_include_tests) {
       "source/flexfec_sender_unittest.cc",
       "source/nack_rtx_unittest.cc",
       "source/packet_loss_stats_unittest.cc",
+      "source/packet_sequencer_unittest.cc",
       "source/receive_statistics_unittest.cc",
       "source/remote_ntp_time_estimator_unittest.cc",
       "source/rtcp_nack_stats_unittest.cc",

modules/rtp_rtcp/source/packet_sequencer.cc

@@ -23,7 +23,7 @@ constexpr uint32_t kTimestampTicksPerMs = 90;
 }  // namespace
 
 PacketSequencer::PacketSequencer(uint32_t media_ssrc,
-                                 uint32_t rtx_ssrc,
+                                 absl::optional<uint32_t> rtx_ssrc,
                                  bool require_marker_before_media_padding,
                                  Clock* clock)
     : media_ssrc_(media_ssrc),
@@ -38,25 +38,26 @@ PacketSequencer::PacketSequencer(uint32_t media_ssrc,
       last_timestamp_time_ms_(0),
       last_packet_marker_bit_(false) {}
 
-bool PacketSequencer::Sequence(RtpPacketToSend& packet) {
+void PacketSequencer::Sequence(RtpPacketToSend& packet) {
-  if (packet.packet_type() == RtpPacketMediaType::kPadding &&
-      !PopulatePaddingFields(packet)) {
-    // This padding packet can't be sent with current state, return without
-    // updating the sequence number.
-    return false;
-  }
-
   if (packet.Ssrc() == media_ssrc_) {
+    if (packet.packet_type() == RtpPacketMediaType::kRetransmission) {
+      // Retransmission of an already sequenced packet, ignore.
+      return;
+    } else if (packet.packet_type() == RtpPacketMediaType::kPadding) {
+      PopulatePaddingFields(packet);
+    }
     packet.SetSequenceNumber(media_sequence_number_++);
     if (packet.packet_type() != RtpPacketMediaType::kPadding) {
       UpdateLastPacketState(packet);
     }
-    return true;
+  } else if (packet.Ssrc() == rtx_ssrc_) {
+    if (packet.packet_type() == RtpPacketMediaType::kPadding) {
+      PopulatePaddingFields(packet);
     }
-
-  RTC_DCHECK_EQ(packet.Ssrc(), rtx_ssrc_);
     packet.SetSequenceNumber(rtx_sequence_number_++);
-  return true;
+  } else {
+    RTC_NOTREACHED() << "Unexpected ssrc " << packet.Ssrc();
+  }
 }
 
 void PacketSequencer::SetRtpState(const RtpState& state) {
@@ -91,30 +92,20 @@ void PacketSequencer::UpdateLastPacketState(const RtpPacketToSend& packet) {
   last_capture_time_ms_ = packet.capture_time_ms();
 }
 
-bool PacketSequencer::PopulatePaddingFields(RtpPacketToSend& packet) {
+void PacketSequencer::PopulatePaddingFields(RtpPacketToSend& packet) {
   if (packet.Ssrc() == media_ssrc_) {
-    if (last_payload_type_ == -1) {
+    RTC_DCHECK(CanSendPaddingOnMediaSsrc());
-      return false;
-    }
-
-    // Without RTX we can't send padding in the middle of frames.
-    // For audio marker bits doesn't mark the end of a frame and frames
-    // are usually a single packet, so for now we don't apply this rule
-    // for audio.
-    if (require_marker_before_media_padding_ && !last_packet_marker_bit_) {
-      return false;
-    }
 
     packet.SetTimestamp(last_rtp_timestamp_);
     packet.set_capture_time_ms(last_capture_time_ms_);
     packet.SetPayloadType(last_payload_type_);
-    return true;
+    return;
   }
 
-  RTC_DCHECK_EQ(packet.Ssrc(), rtx_ssrc_);
+  RTC_DCHECK(packet.Ssrc() == rtx_ssrc_);
   if (packet.payload_size() > 0) {
     // This is payload padding packet, don't update timestamp fields.
-    return true;
+    return;
   }
 
   packet.SetTimestamp(last_rtp_timestamp_);
@@ -133,6 +124,20 @@ bool PacketSequencer::PopulatePaddingFields(RtpPacketToSend& packet) {
                                  (now_ms - last_timestamp_time_ms_));
     }
   }
+}
+
+bool PacketSequencer::CanSendPaddingOnMediaSsrc() const {
+  if (last_payload_type_ == -1) {
+    return false;
+  }
+
+  // Without RTX we can't send padding in the middle of frames.
+  // For audio marker bits doesn't mark the end of a frame and frames
+  // are usually a single packet, so for now we don't apply this rule
+  // for audio.
+  if (require_marker_before_media_padding_ && !last_packet_marker_bit_) {
+    return false;
+  }
 
   return true;
 }

modules/rtp_rtcp/source/packet_sequencer.h

@@ -22,21 +22,19 @@ namespace webrtc {
 // This class is not thread safe, the caller must provide that.
 class PacketSequencer {
  public:
-  // If |require_marker_before_media_padding_| is true, padding packets on the
+  // If `require_marker_before_media_padding_` is true, padding packets on the
   // media ssrc is not allowed unless the last sequenced media packet had the
   // marker bit set (i.e. don't insert padding packets between the first and
   // last packets of a video frame).
+  // Packets with unknown SSRCs will be ignored.
   PacketSequencer(uint32_t media_ssrc,
-                  uint32_t rtx_ssrc,
+                  absl::optional<uint32_t> rtx_ssrc,
                   bool require_marker_before_media_padding,
                   Clock* clock);
 
   // Assigns sequence number, and in the case of non-RTX padding also timestamps
   // and payload type.
-  // Returns false if sequencing failed, which it can do for instance if the
+  void Sequence(RtpPacketToSend& packet);
-  // packet to squence is padding on the media ssrc, but the media is mid frame
-  // (the last marker bit is false).
-  bool Sequence(RtpPacketToSend& packet);
 
   void set_media_sequence_number(uint16_t sequence_number) {
     media_sequence_number_ = sequence_number;
@@ -51,12 +49,16 @@ class PacketSequencer {
   uint16_t media_sequence_number() const { return media_sequence_number_; }
   uint16_t rtx_sequence_number() const { return rtx_sequence_number_; }
 
+  // Checks whether it is allowed to send padding on the media SSRC at this
+  // time, e.g. that we don't send padding in the middle of a video frame.
+  bool CanSendPaddingOnMediaSsrc() const;
+
  private:
   void UpdateLastPacketState(const RtpPacketToSend& packet);
-  bool PopulatePaddingFields(RtpPacketToSend& packet);
+  void PopulatePaddingFields(RtpPacketToSend& packet);
 
   const uint32_t media_ssrc_;
-  const uint32_t rtx_ssrc_;
+  const absl::optional<uint32_t> rtx_ssrc_;
   const bool require_marker_before_media_padding_;
   Clock* const clock_;
 

modules/rtp_rtcp/source/packet_sequencer_unittest.cc

@@ -0,0 +1,250 @@
+/*
+ *  Copyright (c) 2021 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "modules/rtp_rtcp/source/packet_sequencer.h"
+
+#include "api/units/timestamp.h"
+#include "modules/rtp_rtcp/source/rtp_packet_to_send.h"
+#include "system_wrappers/include/clock.h"
+#include "test/gmock.h"
+#include "test/gtest.h"
+
+namespace webrtc {
+namespace {
+constexpr Timestamp kStartTime = Timestamp::Millis(98765);
+constexpr uint32_t kMediaSsrc = 123456;
+constexpr uint32_t kRtxSsrc = 123457;
+constexpr uint8_t kMediaPayloadType = 42;
+constexpr uint16_t kMediaStartSequenceNumber = 123;
+constexpr uint16_t kRtxStartSequenceNumber = 234;
+constexpr uint16_t kDefaultSequenceNumber = 0x1234;
+constexpr uint32_t kStartRtpTimestamp = 798;
+
+class PacketSequencerTest : public ::testing::Test {
+ public:
+  PacketSequencerTest()
+      : clock_(kStartTime),
+        sequencer_(kMediaSsrc,
+                   kRtxSsrc,
+                   /*require_marker_before_media_padding=*/true,
+                   &clock_) {}
+
+  RtpPacketToSend CreatePacket(RtpPacketMediaType type, uint32_t ssrc) {
+    RtpPacketToSend packet(/*extension_manager=*/nullptr);
+    packet.set_packet_type(type);
+    packet.SetSsrc(ssrc);
+    packet.SetSequenceNumber(kDefaultSequenceNumber);
+    packet.set_capture_time_ms(clock_.TimeInMilliseconds());
+    packet.SetTimestamp(
+        kStartRtpTimestamp +
+        static_cast<uint32_t>(packet.capture_time_ms() - kStartTime.ms()));
+    return packet;
+  }
+
+ protected:
+  SimulatedClock clock_;
+  PacketSequencer sequencer_;
+};
+
+TEST_F(PacketSequencerTest, IgnoresMediaSsrcRetransmissions) {
+  RtpPacketToSend packet =
+      CreatePacket(RtpPacketMediaType::kRetransmission, kMediaSsrc);
+  sequencer_.set_media_sequence_number(kMediaStartSequenceNumber);
+  sequencer_.Sequence(packet);
+  EXPECT_EQ(packet.SequenceNumber(), kDefaultSequenceNumber);
+  EXPECT_EQ(sequencer_.media_sequence_number(), kMediaStartSequenceNumber);
+}
+
+TEST_F(PacketSequencerTest, SequencesAudio) {
+  RtpPacketToSend packet = CreatePacket(RtpPacketMediaType::kAudio, kMediaSsrc);
+  sequencer_.set_media_sequence_number(kMediaStartSequenceNumber);
+  sequencer_.Sequence(packet);
+  EXPECT_EQ(packet.SequenceNumber(), kMediaStartSequenceNumber);
+  EXPECT_EQ(sequencer_.media_sequence_number(), kMediaStartSequenceNumber + 1);
+}
+
+TEST_F(PacketSequencerTest, SequencesVideo) {
+  RtpPacketToSend packet = CreatePacket(RtpPacketMediaType::kVideo, kMediaSsrc);
+  sequencer_.set_media_sequence_number(kMediaStartSequenceNumber);
+  sequencer_.Sequence(packet);
+  EXPECT_EQ(packet.SequenceNumber(), kMediaStartSequenceNumber);
+  EXPECT_EQ(sequencer_.media_sequence_number(), kMediaStartSequenceNumber + 1);
+}
+
+TEST_F(PacketSequencerTest, SequencesUlpFec) {
+  RtpPacketToSend packet =
+      CreatePacket(RtpPacketMediaType::kForwardErrorCorrection, kMediaSsrc);
+  sequencer_.set_media_sequence_number(kMediaStartSequenceNumber);
+  sequencer_.Sequence(packet);
+  EXPECT_EQ(packet.SequenceNumber(), kMediaStartSequenceNumber);
+  EXPECT_EQ(sequencer_.media_sequence_number(), kMediaStartSequenceNumber + 1);
+}
+
+TEST_F(PacketSequencerTest, SequencesRtxRetransmissions) {
+  RtpPacketToSend packet =
+      CreatePacket(RtpPacketMediaType::kRetransmission, kRtxSsrc);
+  sequencer_.set_rtx_sequence_number(kRtxStartSequenceNumber);
+  sequencer_.Sequence(packet);
+  EXPECT_EQ(packet.SequenceNumber(), kRtxStartSequenceNumber);
+  EXPECT_EQ(sequencer_.rtx_sequence_number(), kRtxStartSequenceNumber + 1);
+}
+
+TEST_F(PacketSequencerTest, ProhibitsPaddingWithinVideoFrame) {
+  // Send a video packet with the marker bit set to false (indicating it is not
+  // the last packet of a frame).
+  RtpPacketToSend media_packet =
+      CreatePacket(RtpPacketMediaType::kVideo, kMediaSsrc);
+  media_packet.SetPayloadType(kMediaPayloadType);
+  media_packet.SetMarker(false);
+  sequencer_.Sequence(media_packet);
+
+  // Sending padding on the media SSRC should not be allowed at this point.
+  EXPECT_FALSE(sequencer_.CanSendPaddingOnMediaSsrc());
+
+  // Send a video packet with marker set to true, padding should be allowed
+  // again.
+  media_packet.SetMarker(true);
+  sequencer_.Sequence(media_packet);
+  EXPECT_TRUE(sequencer_.CanSendPaddingOnMediaSsrc());
+}
+
+TEST_F(PacketSequencerTest, AllowsPaddingAtAnyTimeIfSoConfigured) {
+  PacketSequencer packet_sequencer(
+      kMediaSsrc, kRtxSsrc,
+      /*require_marker_before_media_padding=*/false, &clock_);
+
+  // Send an audio packet with the marker bit set to false.
+  RtpPacketToSend media_packet =
+      CreatePacket(RtpPacketMediaType::kAudio, kMediaSsrc);
+  media_packet.SetPayloadType(kMediaPayloadType);
+  media_packet.SetMarker(false);
+  packet_sequencer.Sequence(media_packet);
+
+  // Sending padding on the media SSRC should be allowed despite no marker bit.
+  EXPECT_TRUE(packet_sequencer.CanSendPaddingOnMediaSsrc());
+}
+
+TEST_F(PacketSequencerTest, UpdatesPaddingBasedOnLastMediaPacket) {
+  // First send a media packet.
+  RtpPacketToSend media_packet =
+      CreatePacket(RtpPacketMediaType::kVideo, kMediaSsrc);
+  media_packet.SetPayloadType(kMediaPayloadType);
+  media_packet.SetMarker(true);
+  // Advance time so current time doesn't exactly match timestamp.
+  clock_.AdvanceTime(TimeDelta::Millis(5));
+  sequencer_.set_media_sequence_number(kMediaStartSequenceNumber);
+  sequencer_.Sequence(media_packet);
+
+  // Next send a padding packet and verify the media packet's timestamps and
+  // payload type is transferred to the padding packet.
+  RtpPacketToSend padding_packet =
+      CreatePacket(RtpPacketMediaType::kPadding, kMediaSsrc);
+  padding_packet.SetPadding(/*padding_size=*/100);
+  sequencer_.Sequence(padding_packet);
+
+  EXPECT_EQ(padding_packet.SequenceNumber(), kMediaStartSequenceNumber + 1);
+  EXPECT_EQ(padding_packet.PayloadType(), kMediaPayloadType);
+  EXPECT_EQ(padding_packet.Timestamp(), media_packet.Timestamp());
+  EXPECT_EQ(padding_packet.capture_time_ms(), media_packet.capture_time_ms());
+}
+
+TEST_F(PacketSequencerTest, UpdatesPaddingBasedOnLastRedPacket) {
+  // First send a media packet.
+  RtpPacketToSend media_packet =
+      CreatePacket(RtpPacketMediaType::kVideo, kMediaSsrc);
+  media_packet.SetPayloadType(kMediaPayloadType);
+  // Simulate a packet with RED encapsulation;
+  media_packet.set_is_red(true);
+  uint8_t* payload_buffer = media_packet.SetPayloadSize(1);
+  payload_buffer[0] = kMediaPayloadType + 1;
+
+  media_packet.SetMarker(true);
+  // Advance time so current time doesn't exactly match timestamp.
+  clock_.AdvanceTime(TimeDelta::Millis(5));
+  sequencer_.set_media_sequence_number(kMediaStartSequenceNumber);
+  sequencer_.Sequence(media_packet);
+
+  // Next send a padding packet and verify the media packet's timestamps and
+  // payload type is transferred to the padding packet.
+  RtpPacketToSend padding_packet =
+      CreatePacket(RtpPacketMediaType::kPadding, kMediaSsrc);
+  padding_packet.SetPadding(100);
+  sequencer_.Sequence(padding_packet);
+
+  EXPECT_EQ(padding_packet.SequenceNumber(), kMediaStartSequenceNumber + 1);
+  EXPECT_EQ(padding_packet.PayloadType(), kMediaPayloadType + 1);
+  EXPECT_EQ(padding_packet.Timestamp(), media_packet.Timestamp());
+  EXPECT_EQ(padding_packet.capture_time_ms(), media_packet.capture_time_ms());
+}
+
+TEST_F(PacketSequencerTest, DoesNotUpdateFieldsOnPayloadPadding) {
+  // First send a media packet.
+  RtpPacketToSend media_packet =
+      CreatePacket(RtpPacketMediaType::kVideo, kMediaSsrc);
+  media_packet.SetPayloadType(kMediaPayloadType);
+  media_packet.SetMarker(true);
+  // Advance time so current time doesn't exactly match timestamp.
+  clock_.AdvanceTime(TimeDelta::Millis(5));
+  sequencer_.set_media_sequence_number(kMediaStartSequenceNumber);
+  sequencer_.Sequence(media_packet);
+
+  // Simulate a payload padding packet on the RTX SSRC.
+  RtpPacketToSend padding_packet =
+      CreatePacket(RtpPacketMediaType::kPadding, kRtxSsrc);
+  padding_packet.SetPayloadSize(100);
+  padding_packet.SetPayloadType(kMediaPayloadType + 1);
+  padding_packet.SetTimestamp(kStartRtpTimestamp + 1);
+  padding_packet.set_capture_time_ms(kStartTime.ms() + 1);
+  sequencer_.set_rtx_sequence_number(kRtxStartSequenceNumber);
+  sequencer_.Sequence(padding_packet);
+
+  // The sequence number should be updated, but timestamps kept.
+  EXPECT_EQ(padding_packet.SequenceNumber(), kRtxStartSequenceNumber);
+  EXPECT_EQ(padding_packet.PayloadType(), kMediaPayloadType + 1);
+  EXPECT_EQ(padding_packet.Timestamp(), kStartRtpTimestamp + 1);
+  EXPECT_EQ(padding_packet.capture_time_ms(), kStartTime.ms() + 1);
+}
+
+TEST_F(PacketSequencerTest, UpdatesRtxPaddingBasedOnLastMediaPacket) {
+  constexpr uint32_t kTimestampTicksPerMs = 90;
+
+  // First send a media packet.
+  RtpPacketToSend media_packet =
+      CreatePacket(RtpPacketMediaType::kVideo, kMediaSsrc);
+  media_packet.SetPayloadType(kMediaPayloadType);
+  media_packet.SetMarker(true);
+  sequencer_.set_media_sequence_number(kMediaStartSequenceNumber);
+  sequencer_.Sequence(media_packet);
+
+  // Advance time, this time delta will be used to interpolate padding
+  // timestamps.
+  constexpr TimeDelta kTimeDelta = TimeDelta::Millis(10);
+  clock_.AdvanceTime(kTimeDelta);
+
+  RtpPacketToSend padding_packet =
+      CreatePacket(RtpPacketMediaType::kPadding, kRtxSsrc);
+  padding_packet.SetPadding(100);
+  padding_packet.SetPayloadType(kMediaPayloadType + 1);
+  sequencer_.set_rtx_sequence_number(kRtxStartSequenceNumber);
+  sequencer_.Sequence(padding_packet);
+
+  // Assigned RTX sequence number, but payload type unchanged in this case.
+  EXPECT_EQ(padding_packet.SequenceNumber(), kRtxStartSequenceNumber);
+  EXPECT_EQ(padding_packet.PayloadType(), kMediaPayloadType + 1);
+  // Timestamps are offset realtive to last media packet.
+  EXPECT_EQ(
+      padding_packet.Timestamp(),
+      media_packet.Timestamp() + (kTimeDelta.ms() * kTimestampTicksPerMs));
+  EXPECT_EQ(padding_packet.capture_time_ms(),
+            media_packet.capture_time_ms() + kTimeDelta.ms());
+}
+
+}  // namespace
+}  // namespace webrtc

modules/rtp_rtcp/source/rtp_sender.cc

@@ -174,7 +174,7 @@ RTPSender::RTPSender(const RtpRtcpInterface::Configuration& config,
       rtp_header_extension_map_(config.extmap_allow_mixed),
       // RTP variables
       sequencer_(config.local_media_ssrc,
-                 config.rtx_send_ssrc.value_or(config.local_media_ssrc),
+                 rtx_ssrc_,
                  /*require_marker_before_media_padding_=*/!config.audio,
                  config.clock),
       always_send_mid_and_rid_(config.always_send_mid_and_rid),
@@ -446,10 +446,11 @@ std::vector<std::unique_ptr<RtpPacketToSend>> RTPSender::GeneratePadding(
     padding_packet->set_packet_type(RtpPacketMediaType::kPadding);
     padding_packet->SetMarker(false);
     if (rtx_ == kRtxOff) {
-      padding_packet->SetSsrc(ssrc_);
+      if (!sequencer_.CanSendPaddingOnMediaSsrc()) {
-      if (!sequencer_.Sequence(*padding_packet)) {
         break;
       }
+      padding_packet->SetSsrc(ssrc_);
+      sequencer_.Sequence(*padding_packet);
     } else {
       // Without abs-send-time or transport sequence number a media packet
       // must be sent before padding so that the timestamps used for
@@ -464,9 +465,7 @@ std::vector<std::unique_ptr<RtpPacketToSend>> RTPSender::GeneratePadding(
       RTC_DCHECK(rtx_ssrc_);
       padding_packet->SetSsrc(*rtx_ssrc_);
       padding_packet->SetPayloadType(rtx_payload_type_map_.begin()->second);
-      if (!sequencer_.Sequence(*padding_packet)) {
+      sequencer_.Sequence(*padding_packet);
-        break;
-      }
     }
 
     if (rtp_header_extension_map_.IsRegistered(TransportSequenceNumber::kId)) {
@@ -577,7 +576,8 @@ bool RTPSender::AssignSequenceNumber(RtpPacketToSend* packet) {
   MutexLock lock(&send_mutex_);
   if (!sending_media_)
     return false;
-  return sequencer_.Sequence(*packet);
+  sequencer_.Sequence(*packet);
+  return true;
 }
 
 bool RTPSender::AssignSequenceNumbersAndStoreLastPacketState(