From 420b2567f38241099907d30d8bece1c4db50262d Mon Sep 17 00:00:00 2001 From: "stefan@webrtc.org" Date: Fri, 30 May 2014 12:17:15 +0000 Subject: [PATCH] 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 --- .../rtp_rtcp/source/rtp_packet_history.cc | 35 ---------------- .../rtp_rtcp/source/rtp_packet_history.h | 8 ---- .../source/rtp_packet_history_unittest.cc | 32 --------------- webrtc/modules/rtp_rtcp/source/rtp_sender.cc | 41 +++++++------------ webrtc/modules/rtp_rtcp/source/rtp_sender.h | 17 ++++---- 5 files changed, 24 insertions(+), 109 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc b/webrtc/modules/rtp_rtcp/source/rtp_packet_history.cc index fb43563a8..e3515f445 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 @@ 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 >::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 a657d4174..190e5057b 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 @@ 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. 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 1072518ae..7eb22ff69 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, 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; diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc b/webrtc/modules/rtp_rtcp/source/rtp_sender.cc index aacc188db..ccbfa63f0 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.cc +++ b/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); - } - + 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 @@ 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) { diff --git a/webrtc/modules/rtp_rtcp/source/rtp_sender.h b/webrtc/modules/rtp_rtcp/source/rtp_sender.h index fc2c8217f..8b6cab8e8 100644 --- a/webrtc/modules/rtp_rtcp/source/rtp_sender.h +++ b/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,