From 7dc719a2ba72f8122ac02d9938b26b07a48c08c2 Mon Sep 17 00:00:00 2001 From: philipel Date: Fri, 29 Sep 2017 16:15:25 +0200 Subject: [PATCH] Remove duplicate packet check from webrtc::PacketQueue. Original CL by eladalon@ (https://codereview.chromium.org/2929213002/). Bug: webrtc:7786, webrtc:8287, webrtc:8288 Change-Id: I1eaabfbd26b04882b65a3f2a779dd43b953553a6 Reviewed-on: https://webrtc-review.googlesource.com/4721 Reviewed-by: Elad Alon Commit-Queue: Philip Eliasson Cr-Commit-Position: refs/heads/master@{#20070} --- modules/pacing/paced_sender_unittest.cc | 60 ++++--------------------- modules/pacing/packet_queue.cc | 25 ----------- modules/pacing/packet_queue.h | 5 --- 3 files changed, 9 insertions(+), 81 deletions(-) diff --git a/modules/pacing/paced_sender_unittest.cc b/modules/pacing/paced_sender_unittest.cc index 7ad3f9fdcb..e4adcd3f29 100644 --- a/modules/pacing/paced_sender_unittest.cc +++ b/modules/pacing/paced_sender_unittest.cc @@ -255,60 +255,18 @@ TEST_F(PacedSenderTest, PaceQueuedPackets) { EXPECT_EQ(1u, send_bucket_->QueueSizePackets()); } -TEST_F(PacedSenderTest, PaceQueuedPacketsWithDuplicates) { - uint32_t ssrc = 12345; - uint16_t sequence_number = 1234; - uint16_t queued_sequence_number; - - // Due to the multiplicative factor we can send 5 packets during a send - // interval. (network capacity * multiplier / (8 bits per byte * - // (packet size * #send intervals per second) - const size_t packets_to_send_per_interval = - kTargetBitrateBps * PacedSender::kDefaultPaceMultiplier / (8 * 250 * 200); - for (size_t i = 0; i < packets_to_send_per_interval; ++i) { - SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, - clock_.TimeInMilliseconds(), 250, false); - } - queued_sequence_number = sequence_number; - - for (size_t j = 0; j < packets_to_send_per_interval * 10; ++j) { - // Send in duplicate packets. - send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, - sequence_number, clock_.TimeInMilliseconds(), - 250, false); - send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, - sequence_number++, clock_.TimeInMilliseconds(), - 250, false); - } - EXPECT_CALL(callback_, TimeToSendPadding(_, _)).Times(0); - send_bucket_->Process(); - for (int k = 0; k < 10; ++k) { - EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); +TEST_F(PacedSenderTest, RepeatedRetransmissionsAllowed) { + // Send one packet, then two retransmissions of that packet. + for (size_t i = 0; i < 3; i++) { + constexpr uint32_t ssrc = 333; + constexpr uint16_t sequence_number = 444; + constexpr size_t bytes = 250; + bool is_retransmission = (i != 0); // Original followed by retransmissions. + SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number, + clock_.TimeInMilliseconds(), bytes, is_retransmission); clock_.AdvanceTimeMilliseconds(5); - - for (size_t i = 0; i < packets_to_send_per_interval; ++i) { - EXPECT_CALL(callback_, - TimeToSendPacket(ssrc, queued_sequence_number++, _, false, _)) - .Times(1) - .WillRepeatedly(Return(true)); - } - EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess()); - send_bucket_->Process(); } - EXPECT_EQ(5, send_bucket_->TimeUntilNextProcess()); - clock_.AdvanceTimeMilliseconds(5); - EXPECT_EQ(0, send_bucket_->TimeUntilNextProcess()); send_bucket_->Process(); - - for (size_t i = 0; i < packets_to_send_per_interval; ++i) { - SendAndExpectPacket(PacedSender::kNormalPriority, ssrc, sequence_number++, - clock_.TimeInMilliseconds(), 250, false); - } - send_bucket_->InsertPacket(PacedSender::kNormalPriority, ssrc, - sequence_number++, clock_.TimeInMilliseconds(), - 250, false); - send_bucket_->Process(); - EXPECT_EQ(1u, send_bucket_->QueueSizePackets()); } TEST_F(PacedSenderTest, CanQueuePacketsWithSameSequenceNumberOnDifferentSsrcs) { diff --git a/modules/pacing/packet_queue.cc b/modules/pacing/packet_queue.cc index 765e43fcf6..99f27c864f 100644 --- a/modules/pacing/packet_queue.cc +++ b/modules/pacing/packet_queue.cc @@ -56,9 +56,6 @@ PacketQueue::PacketQueue(const Clock* clock) PacketQueue::~PacketQueue() {} void PacketQueue::Push(const Packet& packet) { - if (!AddToDupeSet(packet)) - return; - UpdateQueueTime(packet.enqueue_time_ms); // Store packet in list, use pointers in priority queue for cheaper moves. @@ -82,7 +79,6 @@ void PacketQueue::CancelPop(const PacketQueue::Packet& packet) { } void PacketQueue::FinalizePop(const PacketQueue::Packet& packet) { - RemoveFromDupeSet(packet); bytes_ -= packet.bytes; int64_t packet_queue_time_ms = time_last_updated_ - packet.enqueue_time_ms; RTC_DCHECK_LE(packet.sum_paused_ms, packet_queue_time_ms); @@ -150,25 +146,4 @@ int64_t PacketQueue::AverageQueueTimeMs() const { return queue_time_sum_ / packet_list_.size(); } -bool PacketQueue::AddToDupeSet(const PacketQueue::Packet& packet) { - SsrcSeqNoMap::iterator it = dupe_map_.find(packet.ssrc); - if (it == dupe_map_.end()) { - // First for this ssrc, just insert. - dupe_map_[packet.ssrc].insert(packet.sequence_number); - return true; - } - - // Insert returns a pair, where second is a bool set to true if new element. - return it->second.insert(packet.sequence_number).second; -} - -void PacketQueue::RemoveFromDupeSet(const PacketQueue::Packet& packet) { - SsrcSeqNoMap::iterator it = dupe_map_.find(packet.ssrc); - RTC_DCHECK(it != dupe_map_.end()); - it->second.erase(packet.sequence_number); - if (it->second.empty()) { - dupe_map_.erase(it); - } -} - } // namespace webrtc diff --git a/modules/pacing/packet_queue.h b/modules/pacing/packet_queue.h index c6aa8433b4..5e970fb16d 100644 --- a/modules/pacing/packet_queue.h +++ b/modules/pacing/packet_queue.h @@ -12,9 +12,7 @@ #define MODULES_PACING_PACKET_QUEUE_H_ #include -#include #include -#include #include #include "modules/rtp_rtcp/include/rtp_rtcp_defines.h" @@ -96,9 +94,6 @@ class PacketQueue { std::priority_queue, Comparator> prio_queue_; // Total number of bytes in the queue. uint64_t bytes_; - // Map >, for checking duplicates. - typedef std::map > SsrcSeqNoMap; - SsrcSeqNoMap dupe_map_; const Clock* const clock_; int64_t queue_time_sum_; int64_t time_last_updated_;