Fix bug where RTP headers in the packet history were replaced with the RTX wrapped headers.
This caused only the first retransmission to be successful.
Introduced with https://code.google.com/p/webrtc/source/detail?r=5728.
BUG=1811
R=asapersson@webrtc.org, mflodman@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/12609005
git-svn-id: http://webrtc.googlecode.com/svn/trunk@6284 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc
index fb43563..e3515f4 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc
@@ -157,41 +157,6 @@
return 0;
}
-int32_t RTPPacketHistory::ReplaceRTPHeader(const uint8_t* packet,
- uint16_t sequence_number,
- uint16_t rtp_header_length) {
- CriticalSectionScoped cs(critsect_);
- if (!store_) {
- return 0;
- }
-
- assert(packet);
- assert(rtp_header_length > 3);
- assert(rtp_header_length <= max_packet_length_);
-
- int32_t index = 0;
- bool found = FindSeqNum(sequence_number, &index);
- if (!found) {
- LOG(LS_WARNING)
- << "Failed to replace RTP packet due to missing sequence number.";
- return -1;
- }
-
- uint16_t length = stored_lengths_.at(index);
- if (length == 0 || length > max_packet_length_) {
- LOG(LS_WARNING) << "No match for getting seqNum " << sequence_number
- << ", len " << length;
- return -1;
- }
- assert(stored_seq_nums_[index] == sequence_number);
-
- // Update RTP header.
- std::vector<std::vector<uint8_t> >::iterator it =
- stored_packets_.begin() + index;
- std::copy(packet, packet + rtp_header_length, it->begin());
- return 0;
-}
-
bool RTPPacketHistory::HasRTPPacket(uint16_t sequence_number) const {
CriticalSectionScoped cs(critsect_);
if (!store_) {
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h
index a657d41..190e505 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h
+++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.h
@@ -41,14 +41,6 @@
int64_t capture_time_ms,
StorageType type);
- // Replaces the stored RTP packet with matching sequence number with the
- // RTP header of the provided packet.
- // Note: Calling this function assumes that the RTP header length should not
- // have changed since the packet was stored.
- int32_t ReplaceRTPHeader(const uint8_t* packet,
- uint16_t sequence_number,
- uint16_t rtp_header_length);
-
// Gets stored RTP packet corresponding to the input sequence number.
// The packet is copied to the buffer pointed to by ptr_rtp_packet.
// The rtp_packet_length should show the available buffer size.
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 1072518..7eb22ff 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc
@@ -142,38 +142,6 @@
}
}
-TEST_F(RtpPacketHistoryTest, ReplaceRtpHeader) {
- hist_->SetStorePacketsStatus(true, 10);
-
- uint16_t len = 0;
- int64_t capture_time_ms = 1;
- CreateRtpPacket(kSeqNum, kSsrc, kPayload, kTimestamp, packet_, &len);
-
- // Replace should fail, packet is not stored.
- EXPECT_EQ(0, hist_->PutRTPPacket(packet_, len, kMaxPacketLength,
- capture_time_ms, kAllowRetransmission));
-
- // Create modified packet and replace.
- len = 0;
- CreateRtpPacket(kSeqNum, kSsrc + 1, kPayload + 2, kTimestamp, packet_, &len);
- EXPECT_EQ(0, hist_->ReplaceRTPHeader(packet_, kSeqNum, len));
-
- uint16_t len_out = kMaxPacketLength;
- int64_t time;
- EXPECT_TRUE(hist_->GetPacketAndSetSendTime(kSeqNum, 0, false, packet_out_,
- &len_out, &time));
- EXPECT_EQ(len, len_out);
- EXPECT_EQ(capture_time_ms, time);
- for (int i = 0; i < len; i++) {
- EXPECT_EQ(packet_[i], packet_out_[i]);
- }
-
- // Replace should fail, packet is not stored.
- len = 0;
- CreateRtpPacket(kSeqNum + 1, kSsrc, kPayload, kTimestamp, packet_, &len);
- EXPECT_EQ(-1, hist_->ReplaceRTPHeader(packet_, kSeqNum + 1, len));
-}
-
TEST_F(RtpPacketHistoryTest, NoCaptureTime) {
hist_->SetStorePacketsStatus(true, 10);
uint16_t len = 0;
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
index aacc188..ccbfa63 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc
@@ -802,18 +802,9 @@
int64_t now_ms = clock_->TimeInMilliseconds();
int64_t diff_ms = now_ms - capture_time_ms;
- bool updated_transmission_time_offset =
- UpdateTransmissionTimeOffset(buffer_to_send_ptr, length, rtp_header,
- diff_ms);
- bool updated_abs_send_time =
- UpdateAbsoluteSendTime(buffer_to_send_ptr, length, rtp_header, now_ms);
- if (updated_transmission_time_offset || updated_abs_send_time) {
- // Update stored packet in case of receiving a re-transmission request.
- packet_history_.ReplaceRTPHeader(buffer_to_send_ptr,
- rtp_header.sequenceNumber,
- rtp_header.headerLength);
- }
-
+ UpdateTransmissionTimeOffset(buffer_to_send_ptr, length, rtp_header,
+ diff_ms);
+ UpdateAbsoluteSendTime(buffer_to_send_ptr, length, rtp_header, now_ms);
bool ret = SendPacketToNetwork(buffer_to_send_ptr, length);
UpdateRtpStats(buffer_to_send_ptr, length, rtp_header, send_over_rtx,
is_retransmit);
@@ -1230,7 +1221,7 @@
return kAbsoluteSendTimeLength;
}
-bool RTPSender::UpdateTransmissionTimeOffset(
+void RTPSender::UpdateTransmissionTimeOffset(
uint8_t *rtp_packet, const uint16_t rtp_packet_length,
const RTPHeader &rtp_header, const int64_t time_diff_ms) const {
CriticalSectionScoped cs(send_critsect_);
@@ -1239,7 +1230,7 @@
if (rtp_header_extension_map_.GetId(kRtpExtensionTransmissionTimeOffset,
&id) != 0) {
// Not registered.
- return false;
+ return;
}
// Get length until start of header extension block.
int extension_block_pos =
@@ -1248,7 +1239,7 @@
if (extension_block_pos < 0) {
LOG(LS_WARNING)
<< "Failed to update transmission time offset, not registered.";
- return false;
+ return;
}
int block_pos = 12 + rtp_header.numCSRCs + extension_block_pos;
if (rtp_packet_length < block_pos + kTransmissionTimeOffsetLength ||
@@ -1256,25 +1247,24 @@
block_pos + kTransmissionTimeOffsetLength) {
LOG(LS_WARNING)
<< "Failed to update transmission time offset, invalid length.";
- return false;
+ return;
}
// Verify that header contains extension.
if (!((rtp_packet[12 + rtp_header.numCSRCs] == 0xBE) &&
(rtp_packet[12 + rtp_header.numCSRCs + 1] == 0xDE))) {
LOG(LS_WARNING) << "Failed to update transmission time offset, hdr "
"extension not found.";
- return false;
+ return;
}
// Verify first byte in block.
const uint8_t first_block_byte = (id << 4) + 2;
if (rtp_packet[block_pos] != first_block_byte) {
LOG(LS_WARNING) << "Failed to update transmission time offset.";
- return false;
+ return;
}
// Update transmission offset field (converting to a 90 kHz timestamp).
ModuleRTPUtility::AssignUWord24ToBuffer(rtp_packet + block_pos + 1,
time_diff_ms * 90); // RTP timestamp.
- return true;
}
bool RTPSender::UpdateAudioLevel(uint8_t *rtp_packet,
@@ -1320,7 +1310,7 @@
return true;
}
-bool RTPSender::UpdateAbsoluteSendTime(
+void RTPSender::UpdateAbsoluteSendTime(
uint8_t *rtp_packet, const uint16_t rtp_packet_length,
const RTPHeader &rtp_header, const int64_t now_ms) const {
CriticalSectionScoped cs(send_critsect_);
@@ -1330,7 +1320,7 @@
if (rtp_header_extension_map_.GetId(kRtpExtensionAbsoluteSendTime,
&id) != 0) {
// Not registered.
- return false;
+ return;
}
// Get length until start of header extension block.
int extension_block_pos =
@@ -1338,32 +1328,31 @@
kRtpExtensionAbsoluteSendTime);
if (extension_block_pos < 0) {
// The feature is not enabled.
- return false;
+ return;
}
int block_pos = 12 + rtp_header.numCSRCs + extension_block_pos;
if (rtp_packet_length < block_pos + kAbsoluteSendTimeLength ||
rtp_header.headerLength < block_pos + kAbsoluteSendTimeLength) {
LOG(LS_WARNING) << "Failed to update absolute send time, invalid length.";
- return false;
+ return;
}
// Verify that header contains extension.
if (!((rtp_packet[12 + rtp_header.numCSRCs] == 0xBE) &&
(rtp_packet[12 + rtp_header.numCSRCs + 1] == 0xDE))) {
LOG(LS_WARNING)
<< "Failed to update absolute send time, hdr extension not found.";
- return false;
+ return;
}
// Verify first byte in block.
const uint8_t first_block_byte = (id << 4) + 2;
if (rtp_packet[block_pos] != first_block_byte) {
LOG(LS_WARNING) << "Failed to update absolute send time.";
- return false;
+ return;
}
// Update absolute send time field (convert ms to 24-bit unsigned with 18 bit
// fractional part).
ModuleRTPUtility::AssignUWord24ToBuffer(rtp_packet + block_pos + 1,
((now_ms << 18) / 1000) & 0x00ffffff);
- return true;
}
void RTPSender::SetSendingStatus(bool enabled) {
diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.h b/webrtc/modules/rtp_rtcp/source/rtp_sender.h
index fc2c821..8b6cab8 100644
--- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h
+++ b/webrtc/modules/rtp_rtcp/source/rtp_sender.h
@@ -158,19 +158,11 @@
uint8_t BuildAudioLevelExtension(uint8_t* data_buffer) const;
uint8_t BuildAbsoluteSendTimeExtension(uint8_t* data_buffer) const;
- bool UpdateTransmissionTimeOffset(uint8_t *rtp_packet,
- const uint16_t rtp_packet_length,
- const RTPHeader &rtp_header,
- const int64_t time_diff_ms) const;
bool UpdateAudioLevel(uint8_t *rtp_packet,
const uint16_t rtp_packet_length,
const RTPHeader &rtp_header,
const bool is_voiced,
const uint8_t dBov) const;
- bool UpdateAbsoluteSendTime(uint8_t *rtp_packet,
- const uint16_t rtp_packet_length,
- const RTPHeader &rtp_header,
- const int64_t now_ms) const;
bool TimeToSendPacket(uint16_t sequence_number, int64_t capture_time_ms,
bool retransmission);
@@ -319,6 +311,15 @@
void UpdateDelayStatistics(int64_t capture_time_ms, int64_t now_ms);
+ void UpdateTransmissionTimeOffset(uint8_t *rtp_packet,
+ const uint16_t rtp_packet_length,
+ const RTPHeader &rtp_header,
+ const int64_t time_diff_ms) const;
+ void UpdateAbsoluteSendTime(uint8_t *rtp_packet,
+ const uint16_t rtp_packet_length,
+ const RTPHeader &rtp_header,
+ const int64_t now_ms) const;
+
void UpdateRtpStats(const uint8_t* buffer,
uint32_t size,
const RTPHeader& header,
