Fix mismatch between different NACK list lengths and packet buffers.

This is a second version of http://review.webrtc.org/1065006/ which passes the parameters via methods instead of via constructors.

BUG=1289

Review URL: https://webrtc-codereview.appspot.com/1065007

git-svn-id: http://webrtc.googlecode.com/svn/trunk@3456 4adac7df-926f-26a2-2b94-8c16560cd09d
This commit is contained in:
stefan@webrtc.org
2013-02-01 15:09:57 +00:00
parent b586507986
commit becf9c897c
36 changed files with 316 additions and 130 deletions

View File

@ -733,10 +733,14 @@ class RtpRtcp : public Module {
/*
* Turn negative acknowledgement requests on/off
* |max_reordering_threshold| should be set to how much a retransmitted
* packet can be expected to be reordered (in sequence numbers) compared to
* a packet which has not been retransmitted.
*
* return -1 on failure else 0
*/
virtual WebRtc_Word32 SetNACKStatus(const NACKMethod method) = 0;
virtual WebRtc_Word32 SetNACKStatus(const NACKMethod method,
int max_reordering_threshold) = 0;
/*
* TODO(holmer): Propagate this API to VideoEngine.
@ -774,7 +778,7 @@ class RtpRtcp : public Module {
*/
virtual WebRtc_Word32 SetStorePacketsStatus(
const bool enable,
const WebRtc_UWord16 numberToStore = 200) = 0;
const WebRtc_UWord16 numberToStore) = 0;
/**************************************************************************
*

View File

@ -233,8 +233,8 @@ class MockRtpRtcp : public RtpRtcp {
void(WebRtc_UWord16 bandWidthKbit));
MOCK_CONST_METHOD0(NACK,
NACKMethod());
MOCK_METHOD1(SetNACKStatus,
WebRtc_Word32(const NACKMethod method));
MOCK_METHOD2(SetNACKStatus,
WebRtc_Word32(const NACKMethod method, int oldestSequenceNumberToNack));
MOCK_CONST_METHOD0(SelectiveRetransmissions,
int());
MOCK_METHOD1(SetSelectiveRetransmissions,

View File

@ -800,7 +800,6 @@ RTCPReceiver::HandleNACK(RTCPUtility::RTCPParserV2& rtcpParser,
rtcpParser.Iterate();
return;
}
rtcpPacketInformation.ResetNACKPacketIdArray();
RTCPUtility::RTCPPacketTypes pktType = rtcpParser.Iterate();
@ -1271,13 +1270,11 @@ void RTCPReceiver::TriggerCallbacksFromRTCPPacket(
_rtpRtcp.OnRequestSendReport();
}
if (rtcpPacketInformation.rtcpPacketTypeFlags & kRtcpNack) {
if (rtcpPacketInformation.nackSequenceNumbersLength > 0) {
if (rtcpPacketInformation.nackSequenceNumbers.size() > 0) {
WEBRTC_TRACE(kTraceStateInfo, kTraceRtpRtcp, _id,
"SIG [RTCP] Incoming NACK length:%d",
rtcpPacketInformation.nackSequenceNumbersLength);
_rtpRtcp.OnReceivedNACK(
rtcpPacketInformation.nackSequenceNumbersLength,
rtcpPacketInformation.nackSequenceNumbers);
rtcpPacketInformation.nackSequenceNumbers.size());
_rtpRtcp.OnReceivedNACK(rtcpPacketInformation.nackSequenceNumbers);
}
}
{
@ -1451,4 +1448,5 @@ void RTCPReceiver::PacketTimeout()
_cbRtcpFeedback->OnRTCPPacketTimeout(_id);
}
}
} // namespace webrtc

View File

@ -8,12 +8,12 @@
* be found in the AUTHORS file in the root of the source tree.
*/
#include "rtcp_receiver_help.h"
#include "webrtc/modules/rtp_rtcp/source/rtcp_receiver_help.h"
#include <string.h> // memset
#include <cassert> // assert
#include "modules/rtp_rtcp/source/rtp_utility.h"
#include "webrtc/modules/rtp_rtcp/source/rtp_utility.h"
namespace webrtc {
using namespace RTCPHelp;
@ -21,8 +21,7 @@ using namespace RTCPHelp;
RTCPPacketInformation::RTCPPacketInformation()
: rtcpPacketTypeFlags(0),
remoteSSRC(0),
nackSequenceNumbers(0),
nackSequenceNumbersLength(0),
nackSequenceNumbers(),
applicationSubType(0),
applicationName(0),
applicationData(),
@ -44,7 +43,6 @@ RTCPPacketInformation::RTCPPacketInformation()
RTCPPacketInformation::~RTCPPacketInformation()
{
delete [] nackSequenceNumbers;
delete [] applicationData;
delete VoIPMetric;
}
@ -84,23 +82,16 @@ void RTCPPacketInformation::AddApplicationData(const WebRtc_UWord8* data,
void
RTCPPacketInformation::ResetNACKPacketIdArray()
{
if (NULL == nackSequenceNumbers)
{
nackSequenceNumbers = new WebRtc_UWord16[NACK_PACKETS_MAX_SIZE];
}
nackSequenceNumbersLength = 0;
nackSequenceNumbers.clear();
}
void
RTCPPacketInformation::AddNACKPacket(const WebRtc_UWord16 packetID)
{
assert(nackSequenceNumbers);
WebRtc_UWord16& idx = nackSequenceNumbersLength;
if (idx < NACK_PACKETS_MAX_SIZE)
{
nackSequenceNumbers[idx++] = packetID;
}
if (nackSequenceNumbers.size() >= kSendSideNackListSizeSanity) {
return;
}
nackSequenceNumbers.push_back(packetID);
}
void

View File

@ -11,12 +11,14 @@
#ifndef WEBRTC_MODULES_RTP_RTCP_SOURCE_RTCP_RECEIVER_HELP_H_
#define WEBRTC_MODULES_RTP_RTCP_SOURCE_RTCP_RECEIVER_HELP_H_
#include <vector>
#include <list>
#include "modules/rtp_rtcp/interface/rtp_rtcp_defines.h" // RTCPReportBlock
#include "modules/rtp_rtcp/source/rtcp_utility.h"
#include "modules/rtp_rtcp/source/tmmbr_help.h"
#include "typedefs.h"
#include "webrtc/modules/rtp_rtcp/interface/rtp_rtcp_defines.h" // RTCPReportBlock
#include "webrtc/modules/rtp_rtcp/source/rtcp_utility.h"
#include "webrtc/modules/rtp_rtcp/source/tmmbr_help.h"
#include "webrtc/system_wrappers/interface/constructor_magic.h"
#include "webrtc/system_wrappers/interface/scoped_ptr.h"
#include "webrtc/typedefs.h"
namespace webrtc {
namespace RTCPHelp
@ -44,8 +46,7 @@ public:
WebRtc_UWord32 rtcpPacketTypeFlags; // RTCPPacketTypeFlags bit field
WebRtc_UWord32 remoteSSRC;
WebRtc_UWord16* nackSequenceNumbers;
WebRtc_UWord16 nackSequenceNumbersLength;
std::list<uint16_t> nackSequenceNumbers;
WebRtc_UWord8 applicationSubType;
WebRtc_UWord32 applicationName;
@ -69,6 +70,9 @@ public:
uint32_t rtp_timestamp;
RTCPVoIPMetric* VoIPMetric;
private:
DISALLOW_COPY_AND_ASSIGN(RTCPPacketInformation);
};

View File

@ -205,7 +205,29 @@ class RtcpReceiverTest : public ::testing::Test {
RTCPHelp::RTCPPacketInformation rtcpPacketInformation;
int result = rtcp_receiver_->IncomingRTCPPacket(rtcpPacketInformation,
&rtcpParser);
rtcp_packet_info_ = rtcpPacketInformation;
// The NACK list is on purpose not copied below as it isn't needed by the
// test.
rtcp_packet_info_.rtcpPacketTypeFlags =
rtcpPacketInformation.rtcpPacketTypeFlags;
rtcp_packet_info_.remoteSSRC = rtcpPacketInformation.remoteSSRC;
rtcp_packet_info_.applicationSubType =
rtcpPacketInformation.applicationSubType;
rtcp_packet_info_.applicationName = rtcpPacketInformation.applicationName;
rtcp_packet_info_.reportBlock = rtcpPacketInformation.reportBlock;
rtcp_packet_info_.fractionLost = rtcpPacketInformation.fractionLost;
rtcp_packet_info_.roundTripTime = rtcpPacketInformation.roundTripTime;
rtcp_packet_info_.lastReceivedExtendedHighSeqNum =
rtcpPacketInformation.lastReceivedExtendedHighSeqNum;
rtcp_packet_info_.jitter = rtcpPacketInformation.jitter;
rtcp_packet_info_.interArrivalJitter =
rtcpPacketInformation.interArrivalJitter;
rtcp_packet_info_.sliPictureId = rtcpPacketInformation.sliPictureId;
rtcp_packet_info_.rpsiPictureId = rtcpPacketInformation.rpsiPictureId;
rtcp_packet_info_.receiverEstimatedMaxBitrate =
rtcpPacketInformation.receiverEstimatedMaxBitrate;
rtcp_packet_info_.ntp_secs = rtcpPacketInformation.ntp_secs;
rtcp_packet_info_.ntp_frac = rtcpPacketInformation.ntp_frac;
rtcp_packet_info_.rtp_timestamp = rtcpPacketInformation.rtp_timestamp;
return result;
}

View File

@ -80,8 +80,28 @@ class TestTransport : public Transport,
EXPECT_TRUE(rtcpParser.IsValid());
RTCPHelp::RTCPPacketInformation rtcpPacketInformation;
EXPECT_EQ(0, rtcp_receiver_->IncomingRTCPPacket(rtcpPacketInformation,
&rtcpParser));
rtcp_packet_info_ = rtcpPacketInformation;
&rtcpParser));
rtcp_packet_info_.rtcpPacketTypeFlags =
rtcpPacketInformation.rtcpPacketTypeFlags;
rtcp_packet_info_.remoteSSRC = rtcpPacketInformation.remoteSSRC;
rtcp_packet_info_.applicationSubType =
rtcpPacketInformation.applicationSubType;
rtcp_packet_info_.applicationName = rtcpPacketInformation.applicationName;
rtcp_packet_info_.reportBlock = rtcpPacketInformation.reportBlock;
rtcp_packet_info_.fractionLost = rtcpPacketInformation.fractionLost;
rtcp_packet_info_.roundTripTime = rtcpPacketInformation.roundTripTime;
rtcp_packet_info_.lastReceivedExtendedHighSeqNum =
rtcpPacketInformation.lastReceivedExtendedHighSeqNum;
rtcp_packet_info_.jitter = rtcpPacketInformation.jitter;
rtcp_packet_info_.interArrivalJitter =
rtcpPacketInformation.interArrivalJitter;
rtcp_packet_info_.sliPictureId = rtcpPacketInformation.sliPictureId;
rtcp_packet_info_.rpsiPictureId = rtcpPacketInformation.rpsiPictureId;
rtcp_packet_info_.receiverEstimatedMaxBitrate =
rtcpPacketInformation.receiverEstimatedMaxBitrate;
rtcp_packet_info_.ntp_secs = rtcpPacketInformation.ntp_secs;
rtcp_packet_info_.ntp_frac = rtcpPacketInformation.ntp_frac;
rtcp_packet_info_.rtp_timestamp = rtcpPacketInformation.rtp_timestamp;
return packet_len;
}

View File

@ -91,6 +91,7 @@ RTPReceiver::RTPReceiver(const WebRtc_Word32 id,
last_report_jitter_transmission_time_offset_(0),
nack_method_(kNackOff),
max_reordering_threshold_(kDefaultMaxReorderingThreshold),
rtx_(false),
ssrc_rtx_(0) {
assert(incoming_audio_messages_callback &&
@ -267,8 +268,16 @@ NACKMethod RTPReceiver::NACK() const {
}
// Turn negative acknowledgment requests on/off.
WebRtc_Word32 RTPReceiver::SetNACKStatus(const NACKMethod method) {
WebRtc_Word32 RTPReceiver::SetNACKStatus(const NACKMethod method,
int max_reordering_threshold) {
CriticalSectionScoped lock(critical_section_rtp_receiver_);
if (max_reordering_threshold < 0) {
return -1;
} else if (method == kNackRtcp) {
max_reordering_threshold_ = max_reordering_threshold;
} else {
max_reordering_threshold_ = kDefaultMaxReorderingThreshold;
}
nack_method_ = method;
return 0;
}
@ -572,7 +581,7 @@ bool RTPReceiver::InOrderPacket(const WebRtc_UWord16 sequence_number) const {
if (received_seq_max_ >= sequence_number) {
// Detect wrap-around.
if (!(received_seq_max_ > 0xff00 && sequence_number < 0x0ff)) {
if (received_seq_max_ - NACK_PACKETS_MAX_SIZE > sequence_number) {
if (received_seq_max_ - max_reordering_threshold_ > sequence_number) {
// We have a restart of the remote side.
} else {
// we received a retransmit of a packet we already have.
@ -582,7 +591,7 @@ bool RTPReceiver::InOrderPacket(const WebRtc_UWord16 sequence_number) const {
} else {
// Detect wrap-around.
if (sequence_number > 0xff00 && received_seq_max_ < 0x0ff) {
if (received_seq_max_ - NACK_PACKETS_MAX_SIZE > sequence_number) {
if (received_seq_max_ - max_reordering_threshold_ > sequence_number) {
// We have a restart of the remote side
} else {
// We received a retransmit of a packet we already have

View File

@ -88,7 +88,8 @@ class RTPReceiver : public Bitrate {
NACKMethod NACK() const ;
// Turn negative acknowledgement requests on/off.
WebRtc_Word32 SetNACKStatus(const NACKMethod method);
WebRtc_Word32 SetNACKStatus(const NACKMethod method,
int max_reordering_threshold);
// Returns the last received timestamp.
virtual WebRtc_UWord32 TimeStamp() const;
@ -159,7 +160,6 @@ class RTPReceiver : public Bitrate {
virtual WebRtc_Word8 REDPayloadType() const;
bool HaveNotReceivedPackets() const;
protected:
virtual bool RetransmitOfOldPacket(const WebRtc_UWord16 sequence_number,
const WebRtc_UWord32 rtp_time_stamp) const;
@ -184,7 +184,6 @@ class RTPReceiver : public Bitrate {
void UpdateNACKBitRate(WebRtc_Word32 bytes, WebRtc_UWord32 now);
bool ProcessNACKBitRate(WebRtc_UWord32 now);
private:
RTPPayloadRegistry* rtp_payload_registry_;
scoped_ptr<RTPReceiverStrategy> rtp_media_receiver_;
@ -243,6 +242,7 @@ class RTPReceiver : public Bitrate {
mutable WebRtc_UWord32 last_report_jitter_transmission_time_offset_;
NACKMethod nack_method_;
int max_reordering_threshold_;
bool rtx_;
WebRtc_UWord32 ssrc_rtx_;

View File

@ -18,8 +18,10 @@ enum { kRtpRtcpMaxIdleTimeProcess = 5,
kRtpRtcpPacketTimeoutProcessTimeMs = 100,
kRtpRtcpRttProcessTimeMs = 1000 };
enum { NACK_PACKETS_MAX_SIZE = 256 }; // in packets
enum { NACK_BYTECOUNT_SIZE = 60}; // size of our NACK history
// A sanity for the NACK list parsing at the send-side.
enum { kSendSideNackListSizeSanity = 20000 };
enum { kDefaultMaxReorderingThreshold = 50 }; // In sequence numbers.
enum { RTCP_INTERVAL_VIDEO_MS = 1000 };
enum { RTCP_INTERVAL_AUDIO_MS = 5000 };

View File

@ -1477,14 +1477,15 @@ NACKMethod ModuleRtpRtcpImpl::NACK() const {
}
// Turn negative acknowledgment requests on/off.
WebRtc_Word32 ModuleRtpRtcpImpl::SetNACKStatus(NACKMethod method) {
WebRtc_Word32 ModuleRtpRtcpImpl::SetNACKStatus(
NACKMethod method, int max_reordering_threshold) {
WEBRTC_TRACE(kTraceModuleCall,
kTraceRtpRtcp,
id_,
"SetNACKStatus(%u)", method);
nack_method_ = method;
rtp_receiver_->SetNACKStatus(method);
rtp_receiver_->SetNACKStatus(method, max_reordering_threshold);
return 0;
}
@ -1516,10 +1517,6 @@ WebRtc_Word32 ModuleRtpRtcpImpl::SendNACK(const WebRtc_UWord16* nack_list,
id_,
"SendNACK(size:%u)", size);
if (size > NACK_PACKETS_MAX_SIZE) {
RequestKeyFrame();
return -1;
}
WebRtc_UWord16 avg_rtt = 0;
rtcp_receiver_.RTT(rtp_receiver_->SSRC(), NULL, &avg_rtt, NULL, NULL);
@ -2006,18 +2003,14 @@ WebRtc_UWord32 ModuleRtpRtcpImpl::SendTimeOfSendReport(
}
void ModuleRtpRtcpImpl::OnReceivedNACK(
const WebRtc_UWord16 nack_sequence_numbers_length,
const WebRtc_UWord16* nack_sequence_numbers) {
const std::list<uint16_t>& nack_sequence_numbers) {
if (!rtp_sender_.StorePackets() ||
nack_sequence_numbers == NULL ||
nack_sequence_numbers_length == 0) {
nack_sequence_numbers.size() == 0) {
return;
}
WebRtc_UWord16 avg_rtt = 0;
rtcp_receiver_.RTT(rtp_receiver_->SSRC(), NULL, &avg_rtt, NULL, NULL);
rtp_sender_.OnReceivedNACK(nack_sequence_numbers_length,
nack_sequence_numbers,
avg_rtt);
rtp_sender_.OnReceivedNACK(nack_sequence_numbers, avg_rtt);
}
WebRtc_Word32 ModuleRtpRtcpImpl::LastReceivedNTP(

View File

@ -312,7 +312,8 @@ class ModuleRtpRtcpImpl : public RtpRtcp {
virtual NACKMethod NACK() const;
// Turn negative acknowledgment requests on/off.
virtual WebRtc_Word32 SetNACKStatus(const NACKMethod method);
virtual WebRtc_Word32 SetNACKStatus(const NACKMethod method,
int max_reordering_threshold);
virtual int SelectiveRetransmissions() const;
@ -325,7 +326,7 @@ class ModuleRtpRtcpImpl : public RtpRtcp {
// Store the sent packets, needed to answer to a negative acknowledgment
// requests.
virtual WebRtc_Word32 SetStorePacketsStatus(
const bool enable, const WebRtc_UWord16 number_to_store = 200);
const bool enable, const WebRtc_UWord16 number_to_store);
// (APP) Application specific data.
virtual WebRtc_Word32 SetRTCPApplicationSpecificData(
@ -447,8 +448,7 @@ class ModuleRtpRtcpImpl : public RtpRtcp {
void OnReceivedReferencePictureSelectionIndication(
const WebRtc_UWord64 picture_id);
void OnReceivedNACK(const WebRtc_UWord16 nack_sequence_numbers_length,
const WebRtc_UWord16* nack_sequence_numbers);
void OnReceivedNACK(const std::list<uint16_t>& nack_sequence_numbers);
void OnRequestSendReport();

View File

@ -514,8 +514,7 @@ int RTPSender::SetSelectiveRetransmissions(uint8_t settings) {
}
void RTPSender::OnReceivedNACK(
const WebRtc_UWord16 nack_sequence_numbers_length,
const WebRtc_UWord16 *nack_sequence_numbers,
const std::list<uint16_t>& nack_sequence_numbers,
const WebRtc_UWord16 avg_rtt) {
const WebRtc_Word64 now = clock_->TimeInMilliseconds();
WebRtc_UWord32 bytes_re_sent = 0;
@ -528,9 +527,9 @@ void RTPSender::OnReceivedNACK(
return;
}
for (WebRtc_UWord16 i = 0; i < nack_sequence_numbers_length; ++i) {
const WebRtc_Word32 bytes_sent = ReSendPacket(nack_sequence_numbers[i],
5 + avg_rtt);
for (std::list<uint16_t>::const_iterator it = nack_sequence_numbers.begin();
it != nack_sequence_numbers.end(); ++it) {
const WebRtc_Word32 bytes_sent = ReSendPacket(*it, 5 + avg_rtt);
if (bytes_sent > 0) {
bytes_re_sent += bytes_sent;
} else if (bytes_sent == 0) {
@ -541,7 +540,7 @@ void RTPSender::OnReceivedNACK(
// Failed to send one Sequence number. Give up the rest in this nack.
WEBRTC_TRACE(kTraceWarning, kTraceRtpRtcp, id_,
"Failed resending RTP packet %d, Discard rest of packets",
nack_sequence_numbers[i]);
*it);
break;
}
// Delay bandwidth estimate (RTT * BW).

View File

@ -159,8 +159,7 @@ class RTPSender : public Bitrate, public RTPSenderInterface {
// NACK.
int SelectiveRetransmissions() const;
int SetSelectiveRetransmissions(uint8_t settings);
void OnReceivedNACK(const WebRtc_UWord16 nack_sequence_numbers_length,
const WebRtc_UWord16 *nack_sequence_numbers,
void OnReceivedNACK(const std::list<uint16_t>& nack_sequence_numbers,
const WebRtc_UWord16 avg_rtt);
void SetStorePacketsStatus(const bool enable,

View File

@ -109,6 +109,6 @@ TEST_F(RtpRtcpAPITest, RTCP) {
EXPECT_FALSE(module->TMMBR());
EXPECT_EQ(kNackOff, module->NACK());
EXPECT_EQ(0, module->SetNACKStatus(kNackRtcp));
EXPECT_EQ(0, module->SetNACKStatus(kNackRtcp, 450));
EXPECT_EQ(kNackRtcp, module->NACK());
}

View File

@ -116,8 +116,8 @@ class RtpRtcpNackTest : public ::testing::Test {
EXPECT_EQ(0, video_module_->SetRTCPStatus(kRtcpCompound));
EXPECT_EQ(0, video_module_->SetSSRC(kTestSsrc));
EXPECT_EQ(0, video_module_->SetNACKStatus(kNackRtcp));
EXPECT_EQ(0, video_module_->SetStorePacketsStatus(true));
EXPECT_EQ(0, video_module_->SetNACKStatus(kNackRtcp, 450));
EXPECT_EQ(0, video_module_->SetStorePacketsStatus(true, 600));
EXPECT_EQ(0, video_module_->SetSendingStatus(true));
EXPECT_EQ(0, video_module_->SetSequenceNumber(kTestSequenceNumber));
EXPECT_EQ(0, video_module_->SetStartTimestamp(111111));

View File

@ -47,8 +47,8 @@ class RtpRtcpVideoTest : public ::testing::Test {
EXPECT_EQ(0, video_module_->SetRTCPStatus(kRtcpCompound));
EXPECT_EQ(0, video_module_->SetSSRC(test_ssrc_));
EXPECT_EQ(0, video_module_->SetNACKStatus(kNackRtcp));
EXPECT_EQ(0, video_module_->SetStorePacketsStatus(true));
EXPECT_EQ(0, video_module_->SetNACKStatus(kNackRtcp, 450));
EXPECT_EQ(0, video_module_->SetStorePacketsStatus(true, 600));
EXPECT_EQ(0, video_module_->SetSendingStatus(true));
transport_->SetSendModule(video_module_);