Refactor RTPPacketHistory to use a packet struct.
Collects packet information within a struct instead of spreading it out
over different vectors. Adds a fixed-size buffer to the stored packet
instead of using vectors.
BUG=
R=stefan@webrtc.org
Review URL: https://codereview.webrtc.org/1340573002
Cr-Commit-Position: refs/heads/master@{#9926}
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc
index 8fb1835..cc0cc83 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc
@@ -25,12 +25,10 @@
static const int kMinPacketRequestBytes = 50;
RTPPacketHistory::RTPPacketHistory(Clock* clock)
- : clock_(clock),
- critsect_(CriticalSectionWrapper::CreateCriticalSection()),
- store_(false),
- prev_index_(0),
- max_packet_length_(0) {
-}
+ : clock_(clock),
+ critsect_(CriticalSectionWrapper::CreateCriticalSection()),
+ store_(false),
+ prev_index_(0) {}
RTPPacketHistory::~RTPPacketHistory() {
}
@@ -55,11 +53,6 @@
assert(number_to_store <= kMaxHistoryCapacity);
store_ = true;
stored_packets_.resize(number_to_store);
- stored_seq_nums_.resize(number_to_store);
- stored_lengths_.resize(number_to_store);
- stored_times_.resize(number_to_store);
- stored_send_times_.resize(number_to_store);
- stored_types_.resize(number_to_store);
}
void RTPPacketHistory::Free() {
@@ -67,21 +60,10 @@
return;
}
- std::vector<std::vector<uint8_t> >::iterator it;
- for (it = stored_packets_.begin(); it != stored_packets_.end(); ++it) {
- it->clear();
- }
-
stored_packets_.clear();
- stored_seq_nums_.clear();
- stored_lengths_.clear();
- stored_times_.clear();
- stored_send_times_.clear();
- stored_types_.clear();
store_ = false;
prev_index_ = 0;
- max_packet_length_ = 0;
}
bool RTPPacketHistory::StorePackets() const {
@@ -89,31 +71,8 @@
return store_;
}
-void RTPPacketHistory::VerifyAndAllocatePacketLength(size_t packet_length,
- uint32_t start_index) {
- assert(packet_length > 0);
- if (!store_) {
- return;
- }
-
- // If start_index > 0 this is a resize and we must check any new (empty)
- // packets created during the resize.
- if (start_index == 0 && packet_length <= max_packet_length_) {
- return;
- }
-
- max_packet_length_ = std::max(packet_length, max_packet_length_);
-
- std::vector<std::vector<uint8_t> >::iterator it;
- for (it = stored_packets_.begin() + start_index; it != stored_packets_.end();
- ++it) {
- it->resize(max_packet_length_);
- }
-}
-
int32_t RTPPacketHistory::PutRTPPacket(const uint8_t* packet,
size_t packet_length,
- size_t max_packet_length,
int64_t capture_time_ms,
StorageType type) {
if (type == kDontStore) {
@@ -128,9 +87,7 @@
assert(packet);
assert(packet_length > 3);
- VerifyAndAllocatePacketLength(max_packet_length, 0);
-
- if (packet_length > max_packet_length_) {
+ if (packet_length > IP_PACKET_SIZE) {
LOG(LS_WARNING) << "Failed to store RTP packet with length: "
<< packet_length;
return -1;
@@ -141,14 +98,13 @@
// If index we're about to overwrite contains a packet that has not
// yet been sent (probably pending in paced sender), we need to expand
// the buffer.
- if (stored_lengths_[prev_index_] > 0 &&
- stored_send_times_[prev_index_] == 0) {
+ if (stored_packets_[prev_index_].length > 0 &&
+ stored_packets_[prev_index_].send_time == 0) {
size_t current_size = static_cast<uint16_t>(stored_packets_.size());
if (current_size < kMaxHistoryCapacity) {
size_t expanded_size = std::max(current_size * 3 / 2, current_size + 1);
expanded_size = std::min(expanded_size, kMaxHistoryCapacity);
Allocate(expanded_size);
- VerifyAndAllocatePacketLength(max_packet_length, current_size);
// Causes discontinuity, but that's OK-ish. FindSeqNum() will still work,
// but may be slower - at least until buffer has wrapped around once.
prev_index_ = current_size;
@@ -156,21 +112,19 @@
}
// Store packet
- std::vector<std::vector<uint8_t> >::iterator it =
- stored_packets_.begin() + prev_index_;
// TODO(sprang): Overhaul this class and get rid of this copy step.
// (Finally introduce the RtpPacket class?)
- std::copy(packet, packet + packet_length, it->begin());
+ memcpy(stored_packets_[prev_index_].data, packet, packet_length);
+ stored_packets_[prev_index_].length = packet_length;
- stored_seq_nums_[prev_index_] = seq_num;
- stored_lengths_[prev_index_] = packet_length;
- stored_times_[prev_index_] = (capture_time_ms > 0) ? capture_time_ms :
- clock_->TimeInMilliseconds();
- stored_send_times_[prev_index_] = 0; // Packet not sent.
- stored_types_[prev_index_] = type;
+ stored_packets_[prev_index_].sequence_number = seq_num;
+ stored_packets_[prev_index_].time_ms =
+ (capture_time_ms > 0) ? capture_time_ms : clock_->TimeInMilliseconds();
+ stored_packets_[prev_index_].send_time = 0; // Packet not sent.
+ stored_packets_[prev_index_].storage_type = type;
++prev_index_;
- if (prev_index_ >= stored_seq_nums_.size()) {
+ if (prev_index_ >= stored_packets_.size()) {
prev_index_ = 0;
}
return 0;
@@ -188,8 +142,7 @@
return false;
}
- size_t length = stored_lengths_.at(index);
- if (length == 0 || length > max_packet_length_) {
+ if (stored_packets_[index].length == 0) {
// Invalid length.
return false;
}
@@ -209,11 +162,11 @@
}
// Send time already set.
- if (stored_send_times_[index] != 0) {
+ if (stored_packets_[index].send_time != 0) {
return false;
}
- stored_send_times_[index] = clock_->TimeInMilliseconds();
+ stored_packets_[index].send_time = clock_->TimeInMilliseconds();
return true;
}
@@ -224,7 +177,7 @@
size_t* packet_length,
int64_t* stored_time_ms) {
CriticalSectionScoped cs(critsect_.get());
- assert(*packet_length >= max_packet_length_);
+ assert(*packet_length >= IP_PACKET_SIZE);
if (!store_) {
return false;
}
@@ -236,8 +189,8 @@
return false;
}
- size_t length = stored_lengths_.at(index);
- assert(length <= max_packet_length_);
+ size_t length = stored_packets_[index].length;
+ assert(length <= IP_PACKET_SIZE);
if (length == 0) {
LOG(LS_WARNING) << "No match for getting seqNum " << sequence_number
<< ", len " << length;
@@ -247,16 +200,16 @@
// Verify elapsed time since last retrieve.
int64_t now = clock_->TimeInMilliseconds();
if (min_elapsed_time_ms > 0 &&
- ((now - stored_send_times_.at(index)) < min_elapsed_time_ms)) {
+ ((now - stored_packets_[index].send_time) < min_elapsed_time_ms)) {
return false;
}
- if (retransmit && stored_types_.at(index) == kDontRetransmit) {
+ if (retransmit && stored_packets_[index].storage_type == kDontRetransmit) {
// No bytes copied since this packet shouldn't be retransmitted or is
// of zero size.
return false;
}
- stored_send_times_[index] = clock_->TimeInMilliseconds();
+ stored_packets_[index].send_time = clock_->TimeInMilliseconds();
GetPacket(index, packet, packet_length, stored_time_ms);
return true;
}
@@ -266,13 +219,10 @@
size_t* packet_length,
int64_t* stored_time_ms) const {
// Get packet.
- size_t length = stored_lengths_.at(index);
- std::vector<std::vector<uint8_t> >::const_iterator it_found_packet =
- stored_packets_.begin() + index;
- std::copy(it_found_packet->begin(), it_found_packet->begin() + length,
- packet);
+ size_t length = stored_packets_[index].length;
+ memcpy(packet, stored_packets_[index].data, length);
*packet_length = length;
- *stored_time_ms = stored_times_.at(index);
+ *stored_time_ms = stored_packets_[index].time_ms;
}
bool RTPPacketHistory::GetBestFittingPacket(uint8_t* packet,
@@ -294,24 +244,24 @@
uint16_t temp_sequence_number = 0;
if (prev_index_ > 0) {
*index = prev_index_ - 1;
- temp_sequence_number = stored_seq_nums_[*index];
+ temp_sequence_number = stored_packets_[*index].sequence_number;
} else {
- *index = stored_seq_nums_.size() - 1;
- temp_sequence_number = stored_seq_nums_[*index]; // wrap
+ *index = stored_packets_.size() - 1;
+ temp_sequence_number = stored_packets_[*index].sequence_number; // wrap
}
int32_t idx = (prev_index_ - 1) - (temp_sequence_number - sequence_number);
- if (idx >= 0 && idx < static_cast<int>(stored_seq_nums_.size())) {
+ if (idx >= 0 && idx < static_cast<int>(stored_packets_.size())) {
*index = idx;
- temp_sequence_number = stored_seq_nums_[*index];
+ temp_sequence_number = stored_packets_[*index].sequence_number;
}
if (temp_sequence_number != sequence_number) {
// We did not found a match, search all.
- for (uint16_t m = 0; m < stored_seq_nums_.size(); m++) {
- if (stored_seq_nums_[m] == sequence_number) {
+ for (uint16_t m = 0; m < stored_packets_.size(); m++) {
+ if (stored_packets_[m].sequence_number == sequence_number) {
*index = m;
- temp_sequence_number = stored_seq_nums_[*index];
+ temp_sequence_number = stored_packets_[*index].sequence_number;
break;
}
}
@@ -324,15 +274,16 @@
}
int RTPPacketHistory::FindBestFittingPacket(size_t size) const {
- if (size < kMinPacketRequestBytes || stored_lengths_.empty())
+ if (size < kMinPacketRequestBytes || stored_packets_.empty())
return -1;
size_t min_diff = std::numeric_limits<size_t>::max();
int best_index = -1; // Returned unchanged if we don't find anything.
- for (size_t i = 0; i < stored_lengths_.size(); ++i) {
- if (stored_lengths_[i] == 0)
+ for (size_t i = 0; i < stored_packets_.size(); ++i) {
+ if (stored_packets_[i].length == 0)
continue;
- size_t diff = (stored_lengths_[i] > size) ?
- (stored_lengths_[i] - size) : (size - stored_lengths_[i]);
+ size_t diff = (stored_packets_[i].length > size)
+ ? (stored_packets_[i].length - size)
+ : (size - stored_packets_[i].length);
if (diff < min_diff) {
min_diff = diff;
best_index = static_cast<int>(i);
@@ -340,4 +291,7 @@
}
return best_index;
}
+
+RTPPacketHistory::StoredPacket::StoredPacket() {}
+
} // namespace webrtc
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h
index 212aa21..4a99e16 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h
+++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h
@@ -39,7 +39,6 @@
// Stores RTP packet.
int32_t PutRTPPacket(const uint8_t* packet,
size_t packet_length,
- size_t max_packet_length,
int64_t capture_time_ms,
StorageType type);
@@ -88,14 +87,18 @@
rtc::scoped_ptr<CriticalSectionWrapper> critsect_;
bool store_ GUARDED_BY(critsect_);
uint32_t prev_index_ GUARDED_BY(critsect_);
- size_t max_packet_length_ GUARDED_BY(critsect_);
- std::vector<std::vector<uint8_t> > stored_packets_ GUARDED_BY(critsect_);
- std::vector<uint16_t> stored_seq_nums_ GUARDED_BY(critsect_);
- std::vector<size_t> stored_lengths_ GUARDED_BY(critsect_);
- std::vector<int64_t> stored_times_ GUARDED_BY(critsect_);
- std::vector<int64_t> stored_send_times_ GUARDED_BY(critsect_);
- std::vector<StorageType> stored_types_ GUARDED_BY(critsect_);
+ struct StoredPacket {
+ StoredPacket();
+ uint16_t sequence_number = 0;
+ int64_t time_ms = 0;
+ int64_t send_time = 0;
+ StorageType storage_type = kDontStore;
+
+ uint8_t data[IP_PACKET_SIZE];
+ size_t length = 0;
+ };
+ std::vector<StoredPacket> stored_packets_ GUARDED_BY(critsect_);
};
} // namespace webrtc
#endif // WEBRTC_MODULES_RTP_RTCP_RTP_PACKET_HISTORY_H_
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc b/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
index fe33b01..f3b5556 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
@@ -70,8 +70,8 @@
size_t len = 0;
int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
CreateRtpPacket(kSeqNum, kSsrc, kPayload, kTimestamp, packet_, &len);
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- capture_time_ms, kAllowRetransmission));
+ EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, capture_time_ms,
+ kAllowRetransmission));
// Packet should not be stored.
len = kMaxPacketLength;
int64_t time;
@@ -84,8 +84,7 @@
size_t len = 0;
int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
CreateRtpPacket(kSeqNum, kSsrc, kPayload, kTimestamp, packet_, &len);
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- capture_time_ms, kDontStore));
+ EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, capture_time_ms, kDontStore));
// Packet should not be stored.
len = kMaxPacketLength;
@@ -97,11 +96,8 @@
TEST_F(RtpPacketHistoryTest, PutRtpPacket_TooLargePacketLength) {
hist_->SetStorePacketsStatus(true, 10);
int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
- EXPECT_EQ(-1, hist_->PutRTPPacket(packet_,
- kMaxPacketLength + 1,
- kMaxPacketLength,
- capture_time_ms,
- kAllowRetransmission));
+ EXPECT_EQ(-1, hist_->PutRTPPacket(packet_, kMaxPacketLength + 1,
+ capture_time_ms, kAllowRetransmission));
}
TEST_F(RtpPacketHistoryTest, GetRtpPacket_NotStored) {
@@ -119,8 +115,8 @@
EXPECT_FALSE(hist_->HasRTPPacket(kSeqNum));
int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- capture_time_ms, kAllowRetransmission));
+ EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, capture_time_ms,
+ kAllowRetransmission));
EXPECT_TRUE(hist_->HasRTPPacket(kSeqNum));
}
@@ -129,8 +125,8 @@
size_t len = 0;
int64_t capture_time_ms = 1;
CreateRtpPacket(kSeqNum, kSsrc, kPayload, kTimestamp, packet_, &len);
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- capture_time_ms, kAllowRetransmission));
+ EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, capture_time_ms,
+ kAllowRetransmission));
size_t len_out = kMaxPacketLength;
int64_t time;
@@ -149,8 +145,7 @@
fake_clock_.AdvanceTimeMilliseconds(1);
int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
CreateRtpPacket(kSeqNum, kSsrc, kPayload, kTimestamp, packet_, &len);
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- -1, kAllowRetransmission));
+ EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, -1, kAllowRetransmission));
size_t len_out = kMaxPacketLength;
int64_t time;
@@ -168,8 +163,8 @@
size_t len = 0;
int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
CreateRtpPacket(kSeqNum, kSsrc, kPayload, kTimestamp, packet_, &len);
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- capture_time_ms, kDontRetransmit));
+ EXPECT_EQ(
+ 0, hist_->PutRTPPacket(packet_, len, capture_time_ms, kDontRetransmit));
size_t len_out = kMaxPacketLength;
int64_t time;
@@ -184,8 +179,8 @@
size_t len = 0;
int64_t capture_time_ms = fake_clock_.TimeInMilliseconds();
CreateRtpPacket(kSeqNum, kSsrc, kPayload, kTimestamp, packet_, &len);
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- capture_time_ms, kAllowRetransmission));
+ EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, capture_time_ms,
+ kAllowRetransmission));
int64_t time;
len = kMaxPacketLength;
@@ -215,8 +210,8 @@
for (int i = 0; i < 4; ++i) {
len = 0;
CreateRtpPacket(kSeqNum + i, kSsrc, kPayload, kTimestamp, packet_, &len);
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- capture_time_ms, kAllowRetransmission));
+ EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, capture_time_ms,
+ kAllowRetransmission));
}
for (int i = 0; i < 4; ++i) {
len = kMaxPacketLength;
@@ -230,8 +225,8 @@
for (int i = 4; i < 20; ++i) {
len = 0;
CreateRtpPacket(kSeqNum + i, kSsrc, kPayload, kTimestamp, packet_, &len);
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- capture_time_ms, kAllowRetransmission));
+ EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, capture_time_ms,
+ kAllowRetransmission));
}
for (int i = 4; i < 20; ++i) {
len = kMaxPacketLength;
@@ -257,8 +252,8 @@
for (size_t i = 0; i < kMaxHistoryCapacity + 1; ++i) {
len = 0;
CreateRtpPacket(kSeqNum + i, kSsrc, kPayload, kTimestamp, packet_, &len);
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- capture_time_ms, kAllowRetransmission));
+ EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, capture_time_ms,
+ kAllowRetransmission));
}
fake_clock_.AdvanceTimeMilliseconds(100);
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
index cec7e17..bc31212 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
@@ -1026,8 +1026,7 @@
// Used for NACK and to spread out the transmission of packets.
if (packet_history_.PutRTPPacket(buffer, rtp_header_length + payload_length,
- max_payload_length_, capture_time_ms,
- storage) != 0) {
+ capture_time_ms, storage) != 0) {
return -1;
}
