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}
diff --git a/modules/rtp_rtcp/BUILD.gn b/modules/rtp_rtcp/BUILD.gn index 778baf6..9d78cf3 100644 --- a/modules/rtp_rtcp/BUILD.gn +++ b/modules/rtp_rtcp/BUILD.gn
@@ -304,7 +304,6 @@ "../../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 @@ "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",
diff --git a/modules/rtp_rtcp/source/packet_sequencer.cc b/modules/rtp_rtcp/source/packet_sequencer.cc index 03ea9b8..e12b20c 100644 --- a/modules/rtp_rtcp/source/packet_sequencer.cc +++ b/modules/rtp_rtcp/source/packet_sequencer.cc
@@ -23,7 +23,7 @@ } // 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 @@ last_timestamp_time_ms_(0), last_packet_marker_bit_(false) {} -bool 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; - } - +void PacketSequencer::Sequence(RtpPacketToSend& packet) { 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); + } + packet.SetSequenceNumber(rtx_sequence_number_++); + } else { + RTC_NOTREACHED() << "Unexpected ssrc " << packet.Ssrc(); } - - RTC_DCHECK_EQ(packet.Ssrc(), rtx_ssrc_); - packet.SetSequenceNumber(rtx_sequence_number_++); - return true; } void PacketSequencer::SetRtpState(const RtpState& state) { @@ -91,30 +92,20 @@ 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) { - 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; - } + RTC_DCHECK(CanSendPaddingOnMediaSsrc()); 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 @@ (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; }
diff --git a/modules/rtp_rtcp/source/packet_sequencer.h b/modules/rtp_rtcp/source/packet_sequencer.h index 6725516..5c3ca1a 100644 --- a/modules/rtp_rtcp/source/packet_sequencer.h +++ b/modules/rtp_rtcp/source/packet_sequencer.h
@@ -22,21 +22,19 @@ // 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 - // 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 Sequence(RtpPacketToSend& packet); void set_media_sequence_number(uint16_t sequence_number) { media_sequence_number_ = sequence_number; @@ -51,12 +49,16 @@ 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_;
diff --git a/modules/rtp_rtcp/source/packet_sequencer_unittest.cc b/modules/rtp_rtcp/source/packet_sequencer_unittest.cc new file mode 100644 index 0000000..b82e765 --- /dev/null +++ b/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
diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index 80c319f..100d123 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc
@@ -174,7 +174,7 @@ 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 @@ padding_packet->set_packet_type(RtpPacketMediaType::kPadding); padding_packet->SetMarker(false); if (rtx_ == kRtxOff) { - padding_packet->SetSsrc(ssrc_); - if (!sequencer_.Sequence(*padding_packet)) { + if (!sequencer_.CanSendPaddingOnMediaSsrc()) { 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 @@ RTC_DCHECK(rtx_ssrc_); padding_packet->SetSsrc(*rtx_ssrc_); padding_packet->SetPayloadType(rtx_payload_type_map_.begin()->second); - if (!sequencer_.Sequence(*padding_packet)) { - break; - } + sequencer_.Sequence(*padding_packet); } if (rtp_header_extension_map_.IsRegistered(TransportSequenceNumber::kId)) { @@ -577,7 +576,8 @@ MutexLock lock(&send_mutex_); if (!sending_media_) return false; - return sequencer_.Sequence(*packet); + sequencer_.Sequence(*packet); + return true; } bool RTPSender::AssignSequenceNumbersAndStoreLastPacketState(