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

webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc

@@ -157,41 +157,6 @@ int32_t RTPPacketHistory::PutRTPPacket(const uint8_t* packet,
   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_) {

webrtc/modules/rtp_rtcp/source/rtp_packet_history.h

@@ -41,14 +41,6 @@ class RTPPacketHistory {
                        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.

webrtc/modules/rtp_rtcp/source/rtp_packet_history_unittest.cc

@@ -142,38 +142,6 @@ TEST_F(RtpPacketHistoryTest, GetRtpPacket) {
   }
 }
 
-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;

webrtc/modules/rtp_rtcp/source/rtp_sender.cc

@@ -802,18 +802,9 @@ bool RTPSender::PrepareAndSendPacket(uint8_t* buffer,
 
   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);
-  }
-
   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 @@ uint8_t RTPSender::BuildAbsoluteSendTimeExtension(uint8_t* data_buffer) const {
   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 @@ bool RTPSender::UpdateTransmissionTimeOffset(
   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 @@ bool RTPSender::UpdateTransmissionTimeOffset(
   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 @@ bool RTPSender::UpdateTransmissionTimeOffset(
           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 @@ bool RTPSender::UpdateAudioLevel(uint8_t *rtp_packet,
   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 @@ bool RTPSender::UpdateAbsoluteSendTime(
   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 @@ bool RTPSender::UpdateAbsoluteSendTime(
           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) {

webrtc/modules/rtp_rtcp/source/rtp_sender.h

@@ -158,19 +158,11 @@ class RTPSender : public RTPSenderInterface, public Bitrate::Observer {
   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 @@ class RTPSender : public RTPSenderInterface, public Bitrate::Observer {
 
   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,