From 70768f4a8e106b5889c940a5ce4a024ba31360a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Erik=20Spr=C3=A5ng?= Date: Tue, 27 Aug 2019 18:16:26 +0200 Subject: [PATCH] Remove usage of StorageType enum MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Previously the kDontRetransmit value was used to indicate packets that should not be retransmitted but were put in the RtpPacketHistory anyway as a temporary storage while waiting for a callback from PacedSender. Since PacedSender now always owns the delayed packets directly, we can remove all usage of StorageTye in RtpPacketHistory, and only put packets there after pacing if RtpPacketToSend::allow_retransmission() returns true. Bug: webrtc:10633 Change-Id: I003b76ba43bd87658cc2a39e908fd28ebcd403f7 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/150521 Commit-Queue: Erik Språng Reviewed-by: Åsa Persson Cr-Commit-Position: refs/heads/master@{#28974} --- modules/rtp_rtcp/include/rtp_rtcp_defines.h | 1 + modules/rtp_rtcp/source/rtp_packet_history.cc | 56 ++---- modules/rtp_rtcp/source/rtp_packet_history.h | 15 +- .../source/rtp_packet_history_unittest.cc | 112 ++++-------- modules/rtp_rtcp/source/rtp_sender.cc | 7 +- modules/rtp_rtcp/source/rtp_sender.h | 10 +- modules/rtp_rtcp/source/rtp_sender_audio.cc | 12 +- modules/rtp_rtcp/source/rtp_sender_audio.h | 3 +- .../rtp_rtcp/source/rtp_sender_unittest.cc | 29 +-- modules/rtp_rtcp/source/rtp_sender_video.cc | 52 +++--- modules/rtp_rtcp/source/rtp_sender_video.h | 14 +- .../source/rtp_sender_video_unittest.cc | 171 ++++++++---------- 12 files changed, 201 insertions(+), 281 deletions(-) diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index d3435166d7..839b13cb16 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -45,6 +45,7 @@ const int kBogusRtpRateForAudioRtcp = 8000; // Minimum RTP header size in bytes. const uint8_t kRtpHeaderSize = 12; +// TODO(bugs.webrtc.org/10633): Remove once downstream usage is gone. enum StorageType { kDontRetransmit, kAllowRetransmission }; bool IsLegalMidName(absl::string_view name); diff --git a/modules/rtp_rtcp/source/rtp_packet_history.cc b/modules/rtp_rtcp/source/rtp_packet_history.cc index e6542a9ce0..85689f9637 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history.cc @@ -33,7 +33,6 @@ RtpPacketHistory::PacketState::~PacketState() = default; RtpPacketHistory::StoredPacket::StoredPacket( std::unique_ptr packet, - StorageType storage_type, absl::optional send_time_ms, uint64_t insert_order) : send_time_ms_(send_time_ms), @@ -42,7 +41,6 @@ RtpPacketHistory::StoredPacket::StoredPacket( // be put in the pacer queue and later retrieved via // GetPacketAndSetSendTime(). pending_transmission_(!send_time_ms.has_value()), - storage_type_(storage_type), insert_order_(insert_order), times_retransmitted_(0) {} @@ -57,9 +55,6 @@ void RtpPacketHistory::StoredPacket::IncrementTimesRetransmitted( // it before updating |times_retransmitted_| since that is used in sorting, // and then add it back. const bool in_priority_set = priority_set->erase(this) > 0; - RTC_DCHECK_EQ(in_priority_set, - storage_type_ == StorageType::kAllowRetransmission) - << "ERROR: All retransmittable packets should be in priority set."; ++times_retransmitted_; if (in_priority_set) { auto it = priority_set->insert(this); @@ -88,7 +83,7 @@ RtpPacketHistory::RtpPacketHistory(Clock* clock) number_to_store_(0), mode_(StorageMode::kDisabled), rtt_ms_(-1), - retransmittable_packets_inserted_(0) {} + packets_inserted_(0) {} RtpPacketHistory::~RtpPacketHistory() {} @@ -122,7 +117,6 @@ void RtpPacketHistory::SetRtt(int64_t rtt_ms) { } void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, - StorageType type, absl::optional send_time_ms) { RTC_DCHECK(packet); rtc::CritScope cs(&lock_); @@ -131,27 +125,24 @@ void RtpPacketHistory::PutRtpPacket(std::unique_ptr packet, return; } + RTC_DCHECK(packet->allow_retransmission()); CullOldPackets(now_ms); // Store packet. const uint16_t rtp_seq_no = packet->SequenceNumber(); - auto it = packet_history_.emplace( - rtp_seq_no, StoredPacket(std::move(packet), type, send_time_ms, - type != StorageType::kDontRetransmit - ? retransmittable_packets_inserted_++ - : 0)); - RTC_DCHECK(it.second) << "Failed to insert packet in history."; - StoredPacket& stored_packet = it.first->second; + auto packet_it = packet_history_.emplace( + rtp_seq_no, + StoredPacket(std::move(packet), send_time_ms, packets_inserted_++)); + RTC_DCHECK(packet_it.second) << "Failed to insert packet in history."; + StoredPacket& stored_packet = packet_it.first->second; if (!start_seqno_) { start_seqno_ = rtp_seq_no; } // Store the sequence number of the last send packet with this size. - if (type != StorageType::kDontRetransmit) { - auto it = padding_priority_.insert(&stored_packet); - RTC_DCHECK(it.second) << "Failed to insert packet into prio set."; - } + auto prio_it = padding_priority_.insert(&stored_packet); + RTC_DCHECK(prio_it.second) << "Failed to insert packet into prio set."; } std::unique_ptr RtpPacketHistory::GetPacketAndSetSendTime( @@ -172,8 +163,7 @@ std::unique_ptr RtpPacketHistory::GetPacketAndSetSendTime( return nullptr; } - if (packet.storage_type() != StorageType::kDontRetransmit && - packet.send_time_ms_) { + if (packet.send_time_ms_) { packet.IncrementTimesRetransmitted(&padding_priority_); } @@ -181,13 +171,7 @@ std::unique_ptr RtpPacketHistory::GetPacketAndSetSendTime( packet.send_time_ms_ = now_ms; packet.pending_transmission_ = false; - if (packet.storage_type() == StorageType::kDontRetransmit) { - // Non retransmittable packet, so call must come from paced sender. - // Remove from history and return actual packet instance. - return RemovePacket(rtp_it); - } - - // Return copy of packet instance since it may need to be retransmitted. + // Return copy of packet instance since it may need to be retransmitted again. return absl::make_unique(*packet.packet_); } @@ -215,7 +199,6 @@ std::unique_ptr RtpPacketHistory::GetPacketAndMarkAsPending( } StoredPacket& packet = rtp_it->second; - RTC_DCHECK(packet.storage_type() != StorageType::kDontRetransmit); if (packet.pending_transmission_) { // Packet already in pacer queue, ignore this request. @@ -250,7 +233,6 @@ void RtpPacketHistory::MarkPacketAsSent(uint16_t sequence_number) { } StoredPacket& packet = rtp_it->second; - RTC_CHECK(packet.storage_type() != StorageType::kDontRetransmit); RTC_DCHECK(packet.send_time_ms_); // Update send-time, mark as no longer in pacer queue, and increment @@ -423,15 +405,13 @@ std::unique_ptr RtpPacketHistory::RemovePacket( const bool is_first_packet = packet_it->first == start_seqno_; // Erase from padding priority set, if eligible. - if (packet_it->second.storage_type() != StorageType::kDontRetransmit) { - size_t num_erased = padding_priority_.erase(&packet_it->second); - RTC_DCHECK_EQ(num_erased, 1) - << "Failed to remove one packet from prio set, got " << num_erased; - if (num_erased != 1) { - RTC_LOG(LS_ERROR) << "RtpPacketHistory in inconsistent state, resetting."; - Reset(); - return nullptr; - } + size_t num_erased = padding_priority_.erase(&packet_it->second); + RTC_DCHECK_EQ(num_erased, 1) + << "Failed to remove one packet from prio set, got " << num_erased; + if (num_erased != 1) { + RTC_LOG(LS_ERROR) << "RtpPacketHistory in inconsistent state, resetting."; + Reset(); + return nullptr; } // Erase the packet from the map, and capture iterator to the next one. diff --git a/modules/rtp_rtcp/source/rtp_packet_history.h b/modules/rtp_rtcp/source/rtp_packet_history.h index 54c774e663..4850c7538c 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history.h +++ b/modules/rtp_rtcp/source/rtp_packet_history.h @@ -74,7 +74,6 @@ class RtpPacketHistory { // If |send_time| is set, packet was sent without using pacer, so state will // be set accordingly. void PutRtpPacket(std::unique_ptr packet, - StorageType type, absl::optional send_time_ms); // Gets stored RTP packet corresponding to the input |sequence number|. @@ -141,14 +140,12 @@ class RtpPacketHistory { class StoredPacket { public: StoredPacket(std::unique_ptr packet, - StorageType storage_type, absl::optional send_time_ms, uint64_t insert_order); StoredPacket(StoredPacket&&); StoredPacket& operator=(StoredPacket&&); ~StoredPacket(); - StorageType storage_type() const { return storage_type_; } uint64_t insert_order() const { return insert_order_; } size_t times_retransmitted() const { return times_retransmitted_; } void IncrementTimesRetransmitted(PacketPrioritySet* priority_set); @@ -163,10 +160,6 @@ class RtpPacketHistory { bool pending_transmission_; private: - // Storing a packet with |storage_type| = kDontRetransmit indicates this is - // only used as temporary storage until sent by the pacer sender. - StorageType storage_type_; - // Unique number per StoredPacket, incremented by one for each added // packet. Used to sort on insert order. uint64_t insert_order_; @@ -202,10 +195,10 @@ class RtpPacketHistory { // Map from rtp sequence numbers to stored packet. std::map packet_history_ RTC_GUARDED_BY(lock_); - // Total number of packets with StorageType::kAllowsRetransmission inserted. - uint64_t retransmittable_packets_inserted_ RTC_GUARDED_BY(lock_); - // Retransmittable objects from |packet_history_| ordered by - // "most likely to be useful", used in GetPayloadPaddingPacket(). + // Total number of packets with inserted. + uint64_t packets_inserted_ RTC_GUARDED_BY(lock_); + // Objects from |packet_history_| ordered by "most likely to be useful", used + // in GetPayloadPaddingPacket(). PacketPrioritySet padding_priority_ RTC_GUARDED_BY(lock_); // The earliest packet in the history. This might not be the lowest sequence diff --git a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc index 97e8bc3976..0523ed2ba9 100644 --- a/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc @@ -45,6 +45,7 @@ class RtpPacketHistoryTest : public ::testing::Test { std::unique_ptr packet(new RtpPacketToSend(nullptr)); packet->SetSequenceNumber(seq_num); packet->set_capture_time_ms(fake_clock_.TimeInMilliseconds()); + packet->set_allow_retransmission(true); return packet; } }; @@ -62,8 +63,7 @@ TEST_F(RtpPacketHistoryTest, SetStoreStatus) { TEST_F(RtpPacketHistoryTest, ClearsHistoryAfterSetStoreStatus) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); // Store a packet, but with send-time. It should then not be removed. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, - absl::nullopt); + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), absl::nullopt); EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); // Changing store status, even to the current one, will clear the history. @@ -74,8 +74,7 @@ TEST_F(RtpPacketHistoryTest, ClearsHistoryAfterSetStoreStatus) { TEST_F(RtpPacketHistoryTest, StartSeqResetAfterReset) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); // Store a packet, but with send-time. It should then not be removed. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, - absl::nullopt); + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), absl::nullopt); EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); // Changing store status, to clear the history. @@ -83,8 +82,7 @@ TEST_F(RtpPacketHistoryTest, StartSeqResetAfterReset) { EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); // Add a new packet. - hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 1)), - kAllowRetransmission, absl::nullopt); + hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 1)), absl::nullopt); EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); // Advance time past where packet expires. @@ -93,8 +91,7 @@ TEST_F(RtpPacketHistoryTest, StartSeqResetAfterReset) { RtpPacketHistory::kMinPacketDurationMs); // Add one more packet and verify no state left from packet before reset. - hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 2)), - kAllowRetransmission, absl::nullopt); + hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 2)), absl::nullopt); EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2))); @@ -103,7 +100,7 @@ TEST_F(RtpPacketHistoryTest, StartSeqResetAfterReset) { TEST_F(RtpPacketHistoryTest, NoStoreStatus) { EXPECT_EQ(StorageMode::kDisabled, hist_.GetStorageMode()); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, absl::nullopt); + hist_.PutRtpPacket(std::move(packet), absl::nullopt); // Packet should not be stored. EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } @@ -118,7 +115,7 @@ TEST_F(RtpPacketHistoryTest, PutRtpPacket) { std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, absl::nullopt); + hist_.PutRtpPacket(std::move(packet), absl::nullopt); EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); } @@ -128,7 +125,7 @@ TEST_F(RtpPacketHistoryTest, GetRtpPacket) { std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); packet->set_capture_time_ms(capture_time_ms); rtc::CopyOnWriteBuffer buffer = packet->Buffer(); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, absl::nullopt); + hist_.PutRtpPacket(std::move(packet), absl::nullopt); std::unique_ptr packet_out = hist_.GetPacketAndSetSendTime(kStartSeqNum); @@ -137,25 +134,6 @@ TEST_F(RtpPacketHistoryTest, GetRtpPacket) { EXPECT_EQ(capture_time_ms, packet_out->capture_time_ms()); } -TEST_F(RtpPacketHistoryTest, DontRetransmit) { - hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 10); - int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); - std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); - rtc::CopyOnWriteBuffer buffer = packet->Buffer(); - hist_.PutRtpPacket(std::move(packet), kDontRetransmit, absl::nullopt); - - // Get the packet and verify data. - std::unique_ptr packet_out; - packet_out = hist_.GetPacketAndSetSendTime(kStartSeqNum); - ASSERT_TRUE(packet_out); - EXPECT_EQ(buffer.size(), packet_out->size()); - EXPECT_EQ(capture_time_ms, packet_out->capture_time_ms()); - - // Non-retransmittable packets are immediately removed, so getting in again - // should fail. - EXPECT_FALSE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); -} - TEST_F(RtpPacketHistoryTest, PacketStateIsCorrect) { const uint32_t kSsrc = 92384762; const int64_t kRttMs = 100; @@ -166,8 +144,7 @@ TEST_F(RtpPacketHistoryTest, PacketStateIsCorrect) { packet->SetPayloadSize(1234); const size_t packet_size = packet->size(); - hist_.PutRtpPacket(std::move(packet), StorageType::kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); absl::optional state = hist_.GetPacketState(kStartSeqNum); @@ -196,7 +173,7 @@ TEST_F(RtpPacketHistoryTest, MinResendTimeWithPacer) { int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); size_t len = packet->size(); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, absl::nullopt); + hist_.PutRtpPacket(std::move(packet), absl::nullopt); // First transmission: TimeToSendPacket() call from pacer. EXPECT_TRUE(hist_.GetPacketAndSetSendTime(kStartSeqNum)); @@ -237,8 +214,7 @@ TEST_F(RtpPacketHistoryTest, MinResendTimeWithoutPacer) { int64_t capture_time_ms = fake_clock_.TimeInMilliseconds(); std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); size_t len = packet->size(); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); // First retransmission - allow early retransmission. fake_clock_.AdvanceTimeMilliseconds(1); @@ -270,8 +246,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldestSentPacketWhenAtMaxSize) { std::unique_ptr packet = CreateRtpPacket(To16u(kStartSeqNum + i)); // Immediate mark packet as sent. - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); fake_clock_.AdvanceTimeMilliseconds(kPacketIntervalMs); } @@ -281,8 +256,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldestSentPacketWhenAtMaxSize) { // History is full, oldest one should be overwritten. std::unique_ptr packet = CreateRtpPacket(To16u(kStartSeqNum + kMaxNumPackets)); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); // Oldest packet should be gone, but packet after than one still present. EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); @@ -301,7 +275,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldestPacketWhenAtMaxCapacity) { std::unique_ptr packet = CreateRtpPacket(To16u(kStartSeqNum + i)); // Don't mark packets as sent, preventing them from being removed. - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, absl::nullopt); + hist_.PutRtpPacket(std::move(packet), absl::nullopt); } // First packet should still be there. @@ -310,8 +284,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldestPacketWhenAtMaxCapacity) { // History is full, oldest one should be overwritten. std::unique_ptr packet = CreateRtpPacket(To16u(kStartSeqNum + kMaxNumPackets)); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); // Oldest packet should be gone, but packet after than one still present. EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); @@ -325,8 +298,7 @@ TEST_F(RtpPacketHistoryTest, DontRemoveUnsentPackets) { // Add packets until the buffer is full. for (size_t i = 0; i < kMaxNumPackets; ++i) { // Mark packets as unsent. - hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + i)), - kAllowRetransmission, absl::nullopt); + hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + i)), absl::nullopt); } fake_clock_.AdvanceTimeMilliseconds(RtpPacketHistory::kMinPacketDurationMs); @@ -335,7 +307,7 @@ TEST_F(RtpPacketHistoryTest, DontRemoveUnsentPackets) { // History is full, but old packets not sent, so allow expansion. hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + kMaxNumPackets)), - kAllowRetransmission, fake_clock_.TimeInMilliseconds()); + fake_clock_.TimeInMilliseconds()); EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); // Set all packet as sent and advance time past min packet duration time, @@ -346,7 +318,7 @@ TEST_F(RtpPacketHistoryTest, DontRemoveUnsentPackets) { fake_clock_.AdvanceTimeMilliseconds(RtpPacketHistory::kMinPacketDurationMs); // Add a new packet, this means the two oldest ones will be culled. hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + kMaxNumPackets + 1)), - kAllowRetransmission, fake_clock_.TimeInMilliseconds()); + fake_clock_.TimeInMilliseconds()); EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); EXPECT_FALSE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 2))); @@ -357,21 +329,21 @@ TEST_F(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPackets) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); // Add a packet, marked as send, and advance time to just before removal time. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.TimeInMilliseconds()); fake_clock_.AdvanceTimeMilliseconds(RtpPacketHistory::kMinPacketDurationMs - 1); // Add a new packet to trigger culling. hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 1)), - kAllowRetransmission, fake_clock_.TimeInMilliseconds()); + fake_clock_.TimeInMilliseconds()); // First packet should still be there. EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); // Advance time to where packet will be eligible for removal and try again. fake_clock_.AdvanceTimeMilliseconds(1); hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 2)), - kAllowRetransmission, fake_clock_.TimeInMilliseconds()); + fake_clock_.TimeInMilliseconds()); // First packet should no be gone, but next one still there. EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); @@ -387,20 +359,20 @@ TEST_F(RtpPacketHistoryTest, DontRemoveTooRecentlyTransmittedPacketsHighRtt) { hist_.SetRtt(kRttMs); // Add a packet, marked as send, and advance time to just before removal time. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.TimeInMilliseconds()); fake_clock_.AdvanceTimeMilliseconds(kPacketTimeoutMs - 1); // Add a new packet to trigger culling. hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 1)), - kAllowRetransmission, fake_clock_.TimeInMilliseconds()); + fake_clock_.TimeInMilliseconds()); // First packet should still be there. EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum)); // Advance time to where packet will be eligible for removal and try again. fake_clock_.AdvanceTimeMilliseconds(1); hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 2)), - kAllowRetransmission, fake_clock_.TimeInMilliseconds()); + fake_clock_.TimeInMilliseconds()); // First packet should no be gone, but next one still there. EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); EXPECT_TRUE(hist_.GetPacketState(To16u(kStartSeqNum + 1))); @@ -411,7 +383,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldWithCulling) { // Enable culling. Even without feedback, this can trigger early removal. hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, kMaxNumPackets); - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.TimeInMilliseconds()); int64_t kMaxPacketDurationMs = RtpPacketHistory::kMinPacketDurationMs * @@ -424,7 +396,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldWithCulling) { // Advance to where packet can be culled, even if buffer is not full. fake_clock_.AdvanceTimeMilliseconds(1); hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 1)), - kAllowRetransmission, fake_clock_.TimeInMilliseconds()); + fake_clock_.TimeInMilliseconds()); EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } @@ -436,7 +408,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldWithCullingHighRtt) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, kMaxNumPackets); hist_.SetRtt(kRttMs); - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.TimeInMilliseconds()); int64_t kMaxPacketDurationMs = kRttMs * @@ -450,7 +422,7 @@ TEST_F(RtpPacketHistoryTest, RemovesOldWithCullingHighRtt) { // Advance to where packet can be culled, even if buffer is not full. fake_clock_.AdvanceTimeMilliseconds(1); hist_.PutRtpPacket(CreateRtpPacket(To16u(kStartSeqNum + 1)), - kAllowRetransmission, fake_clock_.TimeInMilliseconds()); + fake_clock_.TimeInMilliseconds()); EXPECT_FALSE(hist_.GetPacketState(kStartSeqNum)); } @@ -465,20 +437,17 @@ TEST_F(RtpPacketHistoryTest, CullWithAcks) { // Insert three packets 33ms apart, immediately mark them as sent. std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); packet->SetPayloadSize(50); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); hist_.GetPacketAndSetSendTime(kStartSeqNum); fake_clock_.AdvanceTimeMilliseconds(33); packet = CreateRtpPacket(To16u(kStartSeqNum + 1)); packet->SetPayloadSize(50); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); hist_.GetPacketAndSetSendTime(To16u(kStartSeqNum + 1)); fake_clock_.AdvanceTimeMilliseconds(33); packet = CreateRtpPacket(To16u(kStartSeqNum + 2)); packet->SetPayloadSize(50); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMilliseconds()); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMilliseconds()); hist_.GetPacketAndSetSendTime(To16u(kStartSeqNum + 2)); EXPECT_TRUE(hist_.GetPacketState(kStartSeqNum).has_value()); @@ -519,7 +488,7 @@ TEST_F(RtpPacketHistoryTest, SetsPendingTransmissionState) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); // Add a packet, without send time, indicating it's in pacer queue. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), /* send_time_ms = */ absl::nullopt); // Packet is pending transmission. @@ -561,7 +530,7 @@ TEST_F(RtpPacketHistoryTest, GetPacketAndSetSent) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); // Add a sent packet to the history. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.TimeInMicroseconds()); // Retransmission request, first retransmission is allowed immediately. @@ -591,8 +560,7 @@ TEST_F(RtpPacketHistoryTest, GetPacketWithEncapsulation) { // Add a sent packet to the history, with a set SSRC. std::unique_ptr packet = CreateRtpPacket(kStartSeqNum); packet->SetSsrc(kSsrc); - hist_.PutRtpPacket(std::move(packet), kAllowRetransmission, - fake_clock_.TimeInMicroseconds()); + hist_.PutRtpPacket(std::move(packet), fake_clock_.TimeInMicroseconds()); // Retransmission request, simulate an RTX-like encapsulation, were the packet // is sent on a different SSRC. @@ -611,7 +579,7 @@ TEST_F(RtpPacketHistoryTest, GetPacketWithEncapsulation) { TEST_F(RtpPacketHistoryTest, GetPacketWithEncapsulationAbortOnNullptr) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.TimeInMicroseconds()); // Retransmission request, but the encapsulator determines that this packet is @@ -635,7 +603,7 @@ TEST_F(RtpPacketHistoryTest, DontRemovePendingTransmissions) { hist_.SetRtt(kRttMs); // Add a sent packet. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.TimeInMilliseconds()); // Advance clock to just before packet timeout. @@ -662,11 +630,11 @@ TEST_F(RtpPacketHistoryTest, PrioritizedPayloadPadding) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); // Add two sent packets, one millisecond apart. - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.TimeInMilliseconds()); fake_clock_.AdvanceTimeMilliseconds(1); - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum + 1), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum + 1), fake_clock_.TimeInMilliseconds()); fake_clock_.AdvanceTimeMilliseconds(1); @@ -698,7 +666,7 @@ TEST_F(RtpPacketHistoryTest, PrioritizedPayloadPadding) { TEST_F(RtpPacketHistoryTest, NoPendingPacketAsPadding) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.TimeInMilliseconds()); fake_clock_.AdvanceTimeMilliseconds(1); @@ -716,7 +684,7 @@ TEST_F(RtpPacketHistoryTest, NoPendingPacketAsPadding) { TEST_F(RtpPacketHistoryTest, PayloadPaddingWithEncapsulation) { hist_.SetStorePacketsStatus(StorageMode::kStoreAndCull, 1); - hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), kAllowRetransmission, + hist_.PutRtpPacket(CreateRtpPacket(kStartSeqNum), fake_clock_.TimeInMilliseconds()); fake_clock_.AdvanceTimeMilliseconds(1); diff --git a/modules/rtp_rtcp/source/rtp_sender.cc b/modules/rtp_rtcp/source/rtp_sender.cc index a61a2cbe0d..b7e1761af8 100644 --- a/modules/rtp_rtcp/source/rtp_sender.cc +++ b/modules/rtp_rtcp/source/rtp_sender.cc @@ -495,7 +495,7 @@ bool RTPSender::TrySendPacket(RtpPacketToSend* packet, // actual sending fails. if (is_media && packet->allow_retransmission()) { packet_history_.PutRtpPacket(absl::make_unique(*packet), - StorageType::kAllowRetransmission, now_ms); + now_ms); } else if (packet->retransmitted_sequence_number()) { packet_history_.MarkPacketAsSent(*packet->retransmitted_sequence_number()); } @@ -667,8 +667,7 @@ std::vector> RTPSender::GeneratePadding( return padding_packets; } -bool RTPSender::SendToNetwork(std::unique_ptr packet, - StorageType storage) { +bool RTPSender::SendToNetwork(std::unique_ptr packet) { RTC_DCHECK(packet); int64_t now_ms = clock_->TimeInMilliseconds(); @@ -679,8 +678,6 @@ bool RTPSender::SendToNetwork(std::unique_ptr packet, packet->set_capture_time_ms(now_ms); } - packet->set_allow_retransmission(storage == - StorageType::kAllowRetransmission); paced_sender_->EnqueuePacket(std::move(packet)); return true; diff --git a/modules/rtp_rtcp/source/rtp_sender.h b/modules/rtp_rtcp/source/rtp_sender.h index dfade3dd34..f7b48bfb3f 100644 --- a/modules/rtp_rtcp/source/rtp_sender.h +++ b/modules/rtp_rtcp/source/rtp_sender.h @@ -147,8 +147,16 @@ class RTPSender { absl::optional FlexfecSsrc() const; // Sends packet to |transport_| or to the pacer, depending on configuration. + bool SendToNetwork(std::unique_ptr packet); + + // TODO(bugs.webrtc.org/10633): Remove once StorageType is gone. bool SendToNetwork(std::unique_ptr packet, - StorageType storage); + StorageType storage) { + if (storage == StorageType::kAllowRetransmission) { + packet->set_allow_retransmission(true); + } + return SendToNetwork(std::move(packet)); + } // Called on update of RTP statistics. void RegisterRtpStatisticsCallback(StreamDataCountersCallback* callback); diff --git a/modules/rtp_rtcp/source/rtp_sender_audio.cc b/modules/rtp_rtcp/source/rtp_sender_audio.cc index f3e742a5ed..f93715addd 100644 --- a/modules/rtp_rtcp/source/rtp_sender_audio.cc +++ b/modules/rtp_rtcp/source/rtp_sender_audio.cc @@ -257,8 +257,8 @@ bool RTPSenderAudio::SendAudio(AudioFrameType frame_type, packet->Timestamp(), "seqnum", packet->SequenceNumber()); packet->set_packet_type(RtpPacketToSend::Type::kAudio); - bool send_result = - LogAndSendToNetwork(std::move(packet), kAllowRetransmission); + packet->set_allow_retransmission(true); + bool send_result = LogAndSendToNetwork(std::move(packet)); if (first_packet_sent_()) { RTC_LOG(LS_INFO) << "First audio RTP packet sent to pacer"; } @@ -342,7 +342,8 @@ bool RTPSenderAudio::SendTelephoneEventPacket(bool ended, ByteWriter::WriteBigEndian(dtmfbuffer + 2, duration); packet->set_packet_type(RtpPacketToSend::Type::kAudio); - result = LogAndSendToNetwork(std::move(packet), kAllowRetransmission); + packet->set_allow_retransmission(true); + result = LogAndSendToNetwork(std::move(packet)); send_count--; } while (send_count > 0 && result); @@ -350,8 +351,7 @@ bool RTPSenderAudio::SendTelephoneEventPacket(bool ended, } bool RTPSenderAudio::LogAndSendToNetwork( - std::unique_ptr packet, - StorageType storage) { + std::unique_ptr packet) { #if BWE_TEST_LOGGING_COMPILE_TIME_ENABLE int64_t now_ms = clock_->TimeInMilliseconds(); BWE_TEST_LOGGING_PLOT_WITH_SSRC(1, "AudioTotBitrate_kbps", now_ms, @@ -361,7 +361,7 @@ bool RTPSenderAudio::LogAndSendToNetwork( rtp_sender_->NackOverheadRate() / 1000, packet->Ssrc()); #endif - return rtp_sender_->SendToNetwork(std::move(packet), storage); + return rtp_sender_->SendToNetwork(std::move(packet)); } } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_sender_audio.h b/modules/rtp_rtcp/source/rtp_sender_audio.h index d8a61fda21..c846d81daf 100644 --- a/modules/rtp_rtcp/source/rtp_sender_audio.h +++ b/modules/rtp_rtcp/source/rtp_sender_audio.h @@ -63,8 +63,7 @@ class RTPSenderAudio { bool MarkerBit(AudioFrameType frame_type, int8_t payload_type); private: - bool LogAndSendToNetwork(std::unique_ptr packet, - StorageType storage); + bool LogAndSendToNetwork(std::unique_ptr packet); Clock* const clock_ = nullptr; RTPSender* const rtp_sender_ = nullptr; diff --git a/modules/rtp_rtcp/source/rtp_sender_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_unittest.cc index 2ae5891a85..125a0b8994 100644 --- a/modules/rtp_rtcp/source/rtp_sender_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_unittest.cc @@ -268,10 +268,11 @@ class RtpSenderTest : public ::testing::TestWithParam { auto packet = BuildRtpPacket(kPayload, kMarkerBit, timestamp, capture_time_ms); packet->AllocatePayload(payload_length); + packet->set_allow_retransmission(true); // Packet should be stored in a send bucket. EXPECT_TRUE(rtp_sender_->SendToNetwork( - absl::make_unique(*packet), kAllowRetransmission)); + absl::make_unique(*packet))); return packet; } @@ -739,10 +740,11 @@ TEST_P(RtpSenderTest, WritesPacerExitToTimingExtension) { const int kStoredTimeInMs = 100; packet->set_packet_type(RtpPacketToSend::Type::kVideo); + packet->set_allow_retransmission(true); EXPECT_CALL(mock_paced_sender_, EnqueuePacket(Pointee(Property(&RtpPacketToSend::Ssrc, kSsrc)))); - EXPECT_TRUE(rtp_sender_->SendToNetwork( - absl::make_unique(*packet), kAllowRetransmission)); + EXPECT_TRUE( + rtp_sender_->SendToNetwork(absl::make_unique(*packet))); fake_clock_.AdvanceTimeMilliseconds(kStoredTimeInMs); rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()); EXPECT_EQ(1, transport_.packets_sent()); @@ -774,11 +776,12 @@ TEST_P(RtpSenderTest, WritesNetwork2ToTimingExtensionWithPacer) { const int kStoredTimeInMs = 100; packet->set_packet_type(RtpPacketToSend::Type::kVideo); + packet->set_allow_retransmission(true); EXPECT_CALL( mock_paced_sender_, EnqueuePacket(Pointee(Property(&RtpPacketToSend::Ssrc, kSsrc)))); EXPECT_TRUE(rtp_sender_->SendToNetwork( - absl::make_unique(*packet), kAllowRetransmission)); + absl::make_unique(*packet))); fake_clock_.AdvanceTimeMilliseconds(kStoredTimeInMs); rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()); @@ -801,14 +804,14 @@ TEST_P(RtpSenderTest, WritesNetwork2ToTimingExtensionWithoutPacer) { packet->set_capture_time_ms(fake_clock_.TimeInMilliseconds()); const VideoSendTiming kVideoTiming = {0u, 0u, 0u, 0u, 0u, 0u, true}; packet->SetExtension(kVideoTiming); + packet->set_allow_retransmission(true); EXPECT_TRUE(rtp_sender_->AssignSequenceNumber(packet.get())); packet->set_packet_type(RtpPacketToSend::Type::kVideo); const int kPropagateTimeMs = 10; fake_clock_.AdvanceTimeMilliseconds(kPropagateTimeMs); - EXPECT_TRUE( - rtp_sender_->SendToNetwork(std::move(packet), kAllowRetransmission)); + EXPECT_TRUE(rtp_sender_->SendToNetwork(std::move(packet))); EXPECT_EQ(1, transport_.packets_sent()); absl::optional video_timing = @@ -840,8 +843,9 @@ TEST_P(RtpSenderTest, TrafficSmoothingWithExtensions) { Pointee(Property(&RtpPacketToSend::Ssrc, kSsrc)), Pointee(Property(&RtpPacketToSend::SequenceNumber, kSeqNum))))); packet->set_packet_type(RtpPacketToSend::Type::kVideo); + packet->set_allow_retransmission(true); EXPECT_TRUE(rtp_sender_->SendToNetwork( - absl::make_unique(*packet), kAllowRetransmission)); + absl::make_unique(*packet))); EXPECT_EQ(0, transport_.packets_sent()); fake_clock_.AdvanceTimeMilliseconds(kStoredTimeInMs); rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()); @@ -885,7 +889,7 @@ TEST_P(RtpSenderTest, TrafficSmoothingRetransmits) { packet->set_packet_type(RtpPacketToSend::Type::kVideo); packet->set_allow_retransmission(true); EXPECT_TRUE(rtp_sender_->SendToNetwork( - absl::make_unique(*packet), kAllowRetransmission)); + absl::make_unique(*packet))); // Immediately process send bucket and send packet. rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()); @@ -964,7 +968,7 @@ TEST_P(RtpSenderTest, SendPadding) { packet->set_packet_type(RtpPacketToSend::Type::kVideo); packet->set_allow_retransmission(true); EXPECT_TRUE(rtp_sender_->SendToNetwork( - absl::make_unique(*packet), kAllowRetransmission)); + absl::make_unique(*packet))); EXPECT_EQ(total_packets_sent, transport_.packets_sent()); fake_clock_.AdvanceTimeMilliseconds(kStoredTimeInMs); rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()); @@ -1011,13 +1015,14 @@ TEST_P(RtpSenderTest, SendPadding) { packet_size = packet->size(); packet->set_packet_type(RtpPacketToSend::Type::kVideo); + packet->set_allow_retransmission(true); EXPECT_CALL( mock_paced_sender_, EnqueuePacket(AllOf( Pointee(Property(&RtpPacketToSend::Ssrc, kSsrc)), Pointee(Property(&RtpPacketToSend::SequenceNumber, seq_num))))); EXPECT_TRUE(rtp_sender_->SendToNetwork( - absl::make_unique(*packet), kAllowRetransmission)); + absl::make_unique(*packet))); rtp_sender_->TrySendPacket(packet.get(), PacedPacketInfo()); // Process send bucket. @@ -2465,8 +2470,8 @@ TEST_P(RtpSenderTest, SetsCaptureTimeAndPopulatesTransmissionOffset) { packet_to_pace = std::move(packet); }); - EXPECT_TRUE( - rtp_sender_->SendToNetwork(std::move(packet), kAllowRetransmission)); + packet->set_allow_retransmission(true); + EXPECT_TRUE(rtp_sender_->SendToNetwork(std::move(packet))); fake_clock_.AdvanceTimeMilliseconds(kOffsetMs); diff --git a/modules/rtp_rtcp/source/rtp_sender_video.cc b/modules/rtp_rtcp/source/rtp_sender_video.cc index 51441f6ab8..d5cad467d1 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video.cc @@ -265,13 +265,12 @@ void RTPSenderVideo::RegisterPayloadType(int8_t payload_type, } } -void RTPSenderVideo::SendVideoPacket(std::unique_ptr packet, - StorageType storage) { +void RTPSenderVideo::SendVideoPacket(std::unique_ptr packet) { // Remember some values about the packet before sending it away. size_t packet_size = packet->size(); uint16_t seq_num = packet->SequenceNumber(); packet->set_packet_type(RtpPacketToSend::Type::kVideo); - if (!LogAndSendToNetwork(std::move(packet), storage)) { + if (!LogAndSendToNetwork(std::move(packet))) { RTC_LOG(LS_WARNING) << "Failed to send video packet " << seq_num; return; } @@ -281,7 +280,6 @@ void RTPSenderVideo::SendVideoPacket(std::unique_ptr packet, void RTPSenderVideo::SendVideoPacketAsRedMaybeWithUlpfec( std::unique_ptr media_packet, - StorageType media_packet_storage, bool protect_media_packet) { uint16_t media_seq_num = media_packet->SequenceNumber(); @@ -331,7 +329,8 @@ void RTPSenderVideo::SendVideoPacketAsRedMaybeWithUlpfec( // Send |red_packet| instead of |packet| for allocated sequence number. size_t red_packet_size = red_packet->size(); red_packet->set_packet_type(RtpPacketToSend::Type::kVideo); - if (LogAndSendToNetwork(std::move(red_packet), media_packet_storage)) { + red_packet->set_allow_retransmission(media_packet->allow_retransmission()); + if (LogAndSendToNetwork(std::move(red_packet))) { rtc::CritScope cs(&stats_crit_); video_bitrate_.Update(red_packet_size, clock_->TimeInMilliseconds()); } else { @@ -346,7 +345,8 @@ void RTPSenderVideo::SendVideoPacketAsRedMaybeWithUlpfec( rtp_packet->set_capture_time_ms(media_packet->capture_time_ms()); rtp_packet->set_packet_type(RtpPacketToSend::Type::kForwardErrorCorrection); uint16_t fec_sequence_number = rtp_packet->SequenceNumber(); - if (LogAndSendToNetwork(std::move(rtp_packet), kDontRetransmit)) { + rtp_packet->set_allow_retransmission(false); + if (LogAndSendToNetwork(std::move(rtp_packet))) { rtc::CritScope cs(&stats_crit_); fec_bitrate_.Update(fec_packet->length(), clock_->TimeInMilliseconds()); } else { @@ -358,14 +358,13 @@ void RTPSenderVideo::SendVideoPacketAsRedMaybeWithUlpfec( void RTPSenderVideo::SendVideoPacketWithFlexfec( std::unique_ptr media_packet, - StorageType media_packet_storage, bool protect_media_packet) { RTC_DCHECK(flexfec_sender_); if (protect_media_packet) flexfec_sender_->AddRtpPacketAndGenerateFec(*media_packet); - SendVideoPacket(std::move(media_packet), media_packet_storage); + SendVideoPacket(std::move(media_packet)); if (flexfec_sender_->FecAvailable()) { std::vector> fec_packets = @@ -375,7 +374,8 @@ void RTPSenderVideo::SendVideoPacketWithFlexfec( uint16_t seq_num = fec_packet->SequenceNumber(); fec_packet->set_packet_type( RtpPacketToSend::Type::kForwardErrorCorrection); - if (LogAndSendToNetwork(std::move(fec_packet), kDontRetransmit)) { + fec_packet->set_allow_retransmission(false); + if (LogAndSendToNetwork(std::move(fec_packet))) { rtc::CritScope cs(&stats_crit_); fec_bitrate_.Update(packet_length, clock_->TimeInMilliseconds()); } else { @@ -386,8 +386,7 @@ void RTPSenderVideo::SendVideoPacketWithFlexfec( } bool RTPSenderVideo::LogAndSendToNetwork( - std::unique_ptr packet, - StorageType storage) { + std::unique_ptr packet) { #if BWE_TEST_LOGGING_COMPILE_TIME_ENABLE int64_t now_ms = clock_->TimeInMilliseconds(); BWE_TEST_LOGGING_PLOT_WITH_SSRC(1, "VideoTotBitrate_kbps", now_ms, @@ -399,7 +398,7 @@ bool RTPSenderVideo::LogAndSendToNetwork( rtp_sender_->NackOverheadRate() / 1000, packet->Ssrc()); #endif - return rtp_sender_->SendToNetwork(std::move(packet), storage); + return rtp_sender_->SendToNetwork(std::move(packet)); } void RTPSenderVideo::SetUlpfecConfig(int red_payload_type, @@ -665,11 +664,11 @@ bool RTPSenderVideo::SendVideo( // TODO(bugs.webrtc.org/10714): retransmission_settings_ should generally be // replaced by expected_retransmission_time_ms.has_value(). For now, though, // only VP8 with an injected frame buffer controller actually controls it. - const StorageType storage = + const bool allow_retransmission = expected_retransmission_time_ms.has_value() - ? GetStorageType(temporal_id, retransmission_settings, - expected_retransmission_time_ms.value()) - : StorageType::kDontRetransmit; + ? AllowRetransmission(temporal_id, retransmission_settings, + expected_retransmission_time_ms.value()) + : false; const size_t num_packets = packetizer->NumPackets(); size_t unpacketized_payload_size; @@ -727,6 +726,8 @@ bool RTPSenderVideo::SendVideo( // No FEC protection for upper temporal layers, if used. bool protect_packet = temporal_id == 0 || temporal_id == kNoTemporalIdx; + packet->set_allow_retransmission(allow_retransmission); + // Put packetization finish timestamp into extension. if (packet->HasExtension()) { packet->set_packetization_finish_time_ms(clock_->TimeInMilliseconds()); @@ -742,12 +743,11 @@ bool RTPSenderVideo::SendVideo( if (flexfec_enabled()) { // TODO(brandtr): Remove the FlexFEC code path when FlexfecSender // is wired up to PacedSender instead. - SendVideoPacketWithFlexfec(std::move(packet), storage, protect_packet); + SendVideoPacketWithFlexfec(std::move(packet), protect_packet); } else if (red_enabled) { - SendVideoPacketAsRedMaybeWithUlpfec(std::move(packet), storage, - protect_packet); + SendVideoPacketAsRedMaybeWithUlpfec(std::move(packet), protect_packet); } else { - SendVideoPacket(std::move(packet), storage); + SendVideoPacket(std::move(packet)); } if (first_frame) { @@ -827,12 +827,12 @@ std::vector RTPSenderVideo::GetSentRtpPacketInfos( return results; } -StorageType RTPSenderVideo::GetStorageType( +bool RTPSenderVideo::AllowRetransmission( uint8_t temporal_id, int32_t retransmission_settings, int64_t expected_retransmission_time_ms) { if (retransmission_settings == kRetransmitOff) - return StorageType::kDontRetransmit; + return false; rtc::CritScope cs(&stats_crit_); // Media packet storage. @@ -843,15 +843,15 @@ StorageType RTPSenderVideo::GetStorageType( } if (temporal_id == kNoTemporalIdx) - return kAllowRetransmission; + return true; if ((retransmission_settings & kRetransmitBaseLayer) && temporal_id == 0) - return kAllowRetransmission; + return true; if ((retransmission_settings & kRetransmitHigherLayers) && temporal_id > 0) - return kAllowRetransmission; + return true; - return kDontRetransmit; + return false; } uint8_t RTPSenderVideo::GetTemporalId(const RTPVideoHeader& header) { diff --git a/modules/rtp_rtcp/source/rtp_sender_video.h b/modules/rtp_rtcp/source/rtp_sender_video.h index 2505fe544b..9b9d157f9a 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video.h +++ b/modules/rtp_rtcp/source/rtp_sender_video.h @@ -110,9 +110,9 @@ class RTPSenderVideo { protected: static uint8_t GetTemporalId(const RTPVideoHeader& header); - StorageType GetStorageType(uint8_t temporal_id, - int32_t retransmission_settings, - int64_t expected_retransmission_time_ms); + bool AllowRetransmission(uint8_t temporal_id, + int32_t retransmission_settings, + int64_t expected_retransmission_time_ms); private: struct TemporalLayerStats { @@ -128,22 +128,18 @@ class RTPSenderVideo { size_t CalculateFecPacketOverhead() const RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_); - void SendVideoPacket(std::unique_ptr packet, - StorageType storage); + void SendVideoPacket(std::unique_ptr packet); void SendVideoPacketAsRedMaybeWithUlpfec( std::unique_ptr media_packet, - StorageType media_packet_storage, bool protect_media_packet); // TODO(brandtr): Remove the FlexFEC functions when FlexfecSender has been // moved to PacedSender. void SendVideoPacketWithFlexfec(std::unique_ptr media_packet, - StorageType media_packet_storage, bool protect_media_packet); - bool LogAndSendToNetwork(std::unique_ptr packet, - StorageType storage); + bool LogAndSendToNetwork(std::unique_ptr packet); bool red_enabled() const RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_) { return red_payload_type_ >= 0; diff --git a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc index 54210c73e7..946b62e9cd 100644 --- a/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_sender_video_unittest.cc @@ -109,12 +109,12 @@ class TestRtpSenderVideo : public RTPSenderVideo { field_trials) {} ~TestRtpSenderVideo() override {} - StorageType GetStorageType(const RTPVideoHeader& header, - int32_t retransmission_settings, - int64_t expected_retransmission_time_ms) { - return RTPSenderVideo::GetStorageType(GetTemporalId(header), - retransmission_settings, - expected_retransmission_time_ms); + bool AllowRetransmission(const RTPVideoHeader& header, + int32_t retransmission_settings, + int64_t expected_retransmission_time_ms) { + return RTPSenderVideo::AllowRetransmission(GetTemporalId(header), + retransmission_settings, + expected_retransmission_time_ms); } PlayoutDelayOracle playout_delay_oracle_; }; @@ -323,19 +323,15 @@ TEST_P(RtpSenderVideoTest, RetransmissionTypesGeneric) { RTPVideoHeader header; header.codec = kVideoCodecGeneric; - EXPECT_EQ(kDontRetransmit, - rtp_sender_video_.GetStorageType( - header, kRetransmitOff, kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, rtp_sender_video_.GetStorageType( - header, kRetransmitBaseLayer, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, rtp_sender_video_.GetStorageType( - header, kRetransmitHigherLayers, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, - rtp_sender_video_.GetStorageType( - header, kConditionallyRetransmitHigherLayers, - kDefaultExpectedRetransmissionTimeMs)); + EXPECT_FALSE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitOff, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitBaseLayer, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitHigherLayers, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kConditionallyRetransmitHigherLayers, + kDefaultExpectedRetransmissionTimeMs)); } TEST_P(RtpSenderVideoTest, RetransmissionTypesH264) { @@ -345,27 +341,22 @@ TEST_P(RtpSenderVideoTest, RetransmissionTypesH264) { header.codec = kVideoCodecH264; header.frame_marking.temporal_id = kNoTemporalIdx; - EXPECT_EQ(kDontRetransmit, - rtp_sender_video_.GetStorageType( - header, kRetransmitOff, kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, rtp_sender_video_.GetStorageType( - header, kRetransmitBaseLayer, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, rtp_sender_video_.GetStorageType( - header, kRetransmitHigherLayers, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, - rtp_sender_video_.GetStorageType( - header, kConditionallyRetransmitHigherLayers, - kDefaultExpectedRetransmissionTimeMs)); + EXPECT_FALSE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitOff, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitBaseLayer, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitHigherLayers, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kConditionallyRetransmitHigherLayers, + kDefaultExpectedRetransmissionTimeMs)); // Test higher level retransmit. for (int tid = 0; tid <= kMaxTemporalStreams; ++tid) { header.frame_marking.temporal_id = tid; - EXPECT_EQ(kAllowRetransmission, - rtp_sender_video_.GetStorageType( - header, kRetransmitHigherLayers | kRetransmitBaseLayer, - kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitHigherLayers | kRetransmitBaseLayer, + kDefaultExpectedRetransmissionTimeMs)); } } @@ -375,27 +366,21 @@ TEST_P(RtpSenderVideoTest, RetransmissionTypesVP8BaseLayer) { auto& vp8_header = header.video_type_header.emplace(); vp8_header.temporalIdx = 0; - EXPECT_EQ(kDontRetransmit, - rtp_sender_video_.GetStorageType( - header, kRetransmitOff, kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, rtp_sender_video_.GetStorageType( - header, kRetransmitBaseLayer, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kDontRetransmit, rtp_sender_video_.GetStorageType( - header, kRetransmitHigherLayers, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, - rtp_sender_video_.GetStorageType( - header, kRetransmitHigherLayers | kRetransmitBaseLayer, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kDontRetransmit, rtp_sender_video_.GetStorageType( - header, kConditionallyRetransmitHigherLayers, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ( - kAllowRetransmission, - rtp_sender_video_.GetStorageType( - header, kRetransmitBaseLayer | kConditionallyRetransmitHigherLayers, - kDefaultExpectedRetransmissionTimeMs)); + EXPECT_FALSE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitOff, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitBaseLayer, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_FALSE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitHigherLayers, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitHigherLayers | kRetransmitBaseLayer, + kDefaultExpectedRetransmissionTimeMs)); + EXPECT_FALSE(rtp_sender_video_.AllowRetransmission( + header, kConditionallyRetransmitHigherLayers, + kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitBaseLayer | kConditionallyRetransmitHigherLayers, + kDefaultExpectedRetransmissionTimeMs)); } TEST_P(RtpSenderVideoTest, RetransmissionTypesVP8HigherLayers) { @@ -406,19 +391,15 @@ TEST_P(RtpSenderVideoTest, RetransmissionTypesVP8HigherLayers) { for (int tid = 1; tid <= kMaxTemporalStreams; ++tid) { vp8_header.temporalIdx = tid; - EXPECT_EQ(kDontRetransmit, rtp_sender_video_.GetStorageType( - header, kRetransmitOff, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kDontRetransmit, rtp_sender_video_.GetStorageType( - header, kRetransmitBaseLayer, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, rtp_sender_video_.GetStorageType( - header, kRetransmitHigherLayers, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, - rtp_sender_video_.GetStorageType( - header, kRetransmitHigherLayers | kRetransmitBaseLayer, - kDefaultExpectedRetransmissionTimeMs)); + EXPECT_FALSE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitOff, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_FALSE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitBaseLayer, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitHigherLayers, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitHigherLayers | kRetransmitBaseLayer, + kDefaultExpectedRetransmissionTimeMs)); } } @@ -430,19 +411,15 @@ TEST_P(RtpSenderVideoTest, RetransmissionTypesVP9) { for (int tid = 1; tid <= kMaxTemporalStreams; ++tid) { vp9_header.temporal_idx = tid; - EXPECT_EQ(kDontRetransmit, rtp_sender_video_.GetStorageType( - header, kRetransmitOff, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kDontRetransmit, rtp_sender_video_.GetStorageType( - header, kRetransmitBaseLayer, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, rtp_sender_video_.GetStorageType( - header, kRetransmitHigherLayers, - kDefaultExpectedRetransmissionTimeMs)); - EXPECT_EQ(kAllowRetransmission, - rtp_sender_video_.GetStorageType( - header, kRetransmitHigherLayers | kRetransmitBaseLayer, - kDefaultExpectedRetransmissionTimeMs)); + EXPECT_FALSE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitOff, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_FALSE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitBaseLayer, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitHigherLayers, kDefaultExpectedRetransmissionTimeMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission( + header, kRetransmitHigherLayers | kRetransmitBaseLayer, + kDefaultExpectedRetransmissionTimeMs)); } } @@ -464,7 +441,7 @@ TEST_P(RtpSenderVideoTest, ConditionalRetransmit) { auto& vp8_header = header.video_type_header.emplace(); for (size_t i = 0; i < arraysize(kPattern) * kNumRepetitions; ++i) { vp8_header.temporalIdx = kPattern[i % arraysize(kPattern)]; - rtp_sender_video_.GetStorageType(header, kSettings, kRttMs); + rtp_sender_video_.AllowRetransmission(header, kSettings, kRttMs); fake_clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); } @@ -473,35 +450,32 @@ TEST_P(RtpSenderVideoTest, ConditionalRetransmit) { // acknowledging that it did not arrive, which means this frame and the next // will not be retransmitted. vp8_header.temporalIdx = 1; - EXPECT_EQ(StorageType::kDontRetransmit, - rtp_sender_video_.GetStorageType(header, kSettings, kRttMs)); + EXPECT_FALSE( + rtp_sender_video_.AllowRetransmission(header, kSettings, kRttMs)); fake_clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); - EXPECT_EQ(StorageType::kDontRetransmit, - rtp_sender_video_.GetStorageType(header, kSettings, kRttMs)); + EXPECT_FALSE( + rtp_sender_video_.AllowRetransmission(header, kSettings, kRttMs)); fake_clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); // The TL0 frame did not arrive. So allow retransmission. - EXPECT_EQ(StorageType::kAllowRetransmission, - rtp_sender_video_.GetStorageType(header, kSettings, kRttMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission(header, kSettings, kRttMs)); fake_clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); // Insert a frame for TL2. We just had frame in TL1, so the next one there is // in three frames away. TL0 is still too far in the past. So, allow // retransmission. vp8_header.temporalIdx = 2; - EXPECT_EQ(StorageType::kAllowRetransmission, - rtp_sender_video_.GetStorageType(header, kSettings, kRttMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission(header, kSettings, kRttMs)); fake_clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); // Another TL2, next in TL1 is two frames away. Allow again. - EXPECT_EQ(StorageType::kAllowRetransmission, - rtp_sender_video_.GetStorageType(header, kSettings, kRttMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission(header, kSettings, kRttMs)); fake_clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); // Yet another TL2, next in TL1 is now only one frame away, so don't store // for retransmission. - EXPECT_EQ(StorageType::kDontRetransmit, - rtp_sender_video_.GetStorageType(header, kSettings, kRttMs)); + EXPECT_FALSE( + rtp_sender_video_.AllowRetransmission(header, kSettings, kRttMs)); } TEST_P(RtpSenderVideoTest, ConditionalRetransmitLimit) { @@ -523,7 +497,7 @@ TEST_P(RtpSenderVideoTest, ConditionalRetransmitLimit) { for (size_t i = 0; i < arraysize(kPattern) * kNumRepetitions; ++i) { vp8_header.temporalIdx = kPattern[i % arraysize(kPattern)]; - rtp_sender_video_.GetStorageType(header, kSettings, kRttMs); + rtp_sender_video_.AllowRetransmission(header, kSettings, kRttMs); fake_clock_.AdvanceTimeMilliseconds(kFrameIntervalMs); } @@ -533,8 +507,7 @@ TEST_P(RtpSenderVideoTest, ConditionalRetransmitLimit) { // layer, but that last frame in TL1 was a long time ago in absolute terms, // so allow retransmission anyway. vp8_header.temporalIdx = 1; - EXPECT_EQ(StorageType::kAllowRetransmission, - rtp_sender_video_.GetStorageType(header, kSettings, kRttMs)); + EXPECT_TRUE(rtp_sender_video_.AllowRetransmission(header, kSettings, kRttMs)); } void RtpSenderVideoTest::PopulateGenericFrameDescriptor(int version) {