Change type of nack_last_time_sent_full_ from uint32_t to int64_t.
Could cause nack requests to be sent too frequently.
R=stefan@webrtc.org
Review URL: https://webrtc-codereview.appspot.com/27339004
git-svn-id: http://webrtc.googlecode.com/svn/trunk/webrtc@7825 4adac7df-926f-26a2-2b94-8c16560cd09d
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
index 7483616..44f7231 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -87,6 +87,7 @@
padding_index_(static_cast<size_t>(-1)), // Start padding at first child.
nack_method_(kNackOff),
nack_last_time_sent_full_(0),
+ nack_last_time_sent_full_prev_(0),
nack_last_seq_number_sent_(0),
simulcast_(false),
key_frame_req_method_(kKeyFrameReqFirRtp),
@@ -911,51 +912,57 @@
// Send a Negative acknowledgment packet.
int32_t ModuleRtpRtcpImpl::SendNACK(const uint16_t* nack_list,
const uint16_t size) {
+ uint16_t nack_length = size;
+ uint16_t start_id = 0;
+ int64_t now = clock_->TimeInMilliseconds();
+ if (TimeToSendFullNackList(now)) {
+ nack_last_time_sent_full_ = now;
+ nack_last_time_sent_full_prev_ = now;
+ } else {
+ // Only send extended list.
+ if (nack_last_seq_number_sent_ == nack_list[size - 1]) {
+ // Last sequence number is the same, do not send list.
+ return 0;
+ }
+ // Send new sequence numbers.
+ for (int i = 0; i < size; ++i) {
+ if (nack_last_seq_number_sent_ == nack_list[i]) {
+ start_id = i + 1;
+ break;
+ }
+ }
+ nack_length = size - start_id;
+ }
+
+ // Our RTCP NACK implementation is limited to kRtcpMaxNackFields sequence
+ // numbers per RTCP packet.
+ if (nack_length > kRtcpMaxNackFields) {
+ nack_length = kRtcpMaxNackFields;
+ }
+ nack_last_seq_number_sent_ = nack_list[start_id + nack_length - 1];
+
+ return rtcp_sender_.SendRTCP(
+ GetFeedbackState(), kRtcpNack, nack_length, &nack_list[start_id]);
+}
+
+bool ModuleRtpRtcpImpl::TimeToSendFullNackList(int64_t now) const {
// Use RTT from RtcpRttStats class if provided.
uint16_t rtt = rtt_ms();
if (rtt == 0) {
rtcp_receiver_.RTT(rtcp_receiver_.RemoteSSRC(), NULL, &rtt, NULL, NULL);
}
+ const int64_t kStartUpRttMs = 100;
int64_t wait_time = 5 + ((rtt * 3) >> 1); // 5 + RTT * 1.5.
- if (wait_time == 5) {
- wait_time = 100; // During startup we don't have an RTT.
+ if (rtt == 0) {
+ wait_time = kStartUpRttMs;
}
- const int64_t now = clock_->TimeInMilliseconds();
- const int64_t time_limit = now - wait_time;
- uint16_t nackLength = size;
- uint16_t start_id = 0;
- if (nack_last_time_sent_full_ < time_limit) {
- // Send list. Set the timer to make sure we only send a full NACK list once
- // within every time_limit.
- nack_last_time_sent_full_ = now;
- } else {
- // Only send if extended list.
- if (nack_last_seq_number_sent_ == nack_list[size - 1]) {
- // Last seq num is the same don't send list.
- return 0;
- } else {
- // Send NACKs only for new sequence numbers to avoid re-sending
- // NACKs for sequences we have already sent.
- for (int i = 0; i < size; ++i) {
- if (nack_last_seq_number_sent_ == nack_list[i]) {
- start_id = i + 1;
- break;
- }
- }
- nackLength = size - start_id;
- }
+ // Send a full NACK list once within every |wait_time|.
+ if (rtt_stats_) {
+ return now - nack_last_time_sent_full_ > wait_time;
}
- // Our RTCP NACK implementation is limited to kRtcpMaxNackFields sequence
- // numbers per RTCP packet.
- if (nackLength > kRtcpMaxNackFields) {
- nackLength = kRtcpMaxNackFields;
- }
- nack_last_seq_number_sent_ = nack_list[start_id + nackLength - 1];
-
- return rtcp_sender_.SendRTCP(
- GetFeedbackState(), kRtcpNack, nackLength, &nack_list[start_id]);
+ return now - nack_last_time_sent_full_prev_ > wait_time;
}
// Store the sent packets, needed to answer to a Negative acknowledgment
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.h b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
index 8af32f3..dd99a9d 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.h
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.h
@@ -391,6 +391,8 @@
void set_rtt_ms(uint32_t rtt_ms);
uint32_t rtt_ms() const;
+ bool TimeToSendFullNackList(int64_t now) const;
+
bool IsDefaultModule() const;
int32_t id_;
@@ -409,8 +411,9 @@
// Send side
NACKMethod nack_method_;
- uint32_t nack_last_time_sent_full_;
- uint16_t nack_last_seq_number_sent_;
+ int64_t nack_last_time_sent_full_;
+ uint32_t nack_last_time_sent_full_prev_;
+ uint16_t nack_last_seq_number_sent_;
bool simulcast_;
VideoCodec send_video_codec_;
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc
index bf6ab39..d6e0252 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl_unittest.cc
@@ -145,7 +145,7 @@
class RtpRtcpImplTest : public ::testing::Test {
protected:
RtpRtcpImplTest()
- : clock_(1335900000),
+ : clock_(133590000000000),
sender_(&clock_),
receiver_(&clock_) {
// Send module.
@@ -195,12 +195,13 @@
}
void IncomingRtcpNack(const RtpRtcpModule* module, uint16_t sequence_number) {
+ bool sender = module->impl_->SSRC() == kSenderSsrc;
rtcp::Nack nack;
uint16_t list[1];
list[0] = sequence_number;
const uint16_t kListLength = sizeof(list) / sizeof(list[0]);
- nack.From(kReceiverSsrc);
- nack.To(kSenderSsrc);
+ nack.From(sender ? kReceiverSsrc : kSenderSsrc);
+ nack.To(sender ? kSenderSsrc : kReceiverSsrc);
nack.WithList(list, kListLength);
rtcp::RawPacket packet = nack.Build();
EXPECT_EQ(0, module->impl_->IncomingRtcpPacket(packet.buffer(),
@@ -354,6 +355,61 @@
EXPECT_EQ(1U, sender_.RtcpReceived().pli_packets);
}
+TEST_F(RtpRtcpImplTest, SendsInitialNackList) {
+ // Send module sends a NACK.
+ const uint16_t kNackLength = 1;
+ uint16_t nack_list[kNackLength] = {123};
+ EXPECT_EQ(0U, sender_.RtcpSent().nack_packets);
+ EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+ EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
+ EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123));
+}
+
+TEST_F(RtpRtcpImplTest, SendsExtendedNackList) {
+ // Send module sends a NACK.
+ const uint16_t kNackLength = 1;
+ uint16_t nack_list[kNackLength] = {123};
+ EXPECT_EQ(0U, sender_.RtcpSent().nack_packets);
+ EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+ EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
+ EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123));
+
+ // Same list not re-send.
+ EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+ EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
+ EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123));
+
+ // Only extended list sent.
+ const uint16_t kNackExtLength = 2;
+ uint16_t nack_list_ext[kNackExtLength] = {123, 124};
+ EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list_ext, kNackExtLength));
+ EXPECT_EQ(2U, sender_.RtcpSent().nack_packets);
+ EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(124));
+}
+
+TEST_F(RtpRtcpImplTest, ReSendsNackListAfterRttMs) {
+ sender_.transport_.SimulateNetworkDelay(0, &clock_);
+ // Send module sends a NACK.
+ const uint16_t kNackLength = 2;
+ uint16_t nack_list[kNackLength] = {123, 125};
+ EXPECT_EQ(0U, sender_.RtcpSent().nack_packets);
+ EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+ EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
+ EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123, 125));
+
+ // Same list not re-send, rtt interval has not passed.
+ const int kStartupRttMs = 100;
+ clock_.AdvanceTimeMilliseconds(kStartupRttMs);
+ EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+ EXPECT_EQ(1U, sender_.RtcpSent().nack_packets);
+
+ // Rtt interval passed, full list sent.
+ clock_.AdvanceTimeMilliseconds(1);
+ EXPECT_EQ(0, sender_.impl_->SendNACK(nack_list, kNackLength));
+ EXPECT_EQ(2U, sender_.RtcpSent().nack_packets);
+ EXPECT_THAT(sender_.LastNackListSent(), ElementsAre(123, 125));
+}
+
TEST_F(RtpRtcpImplTest, UniqueNackRequests) {
receiver_.transport_.SimulateNetworkDelay(0, &clock_);
EXPECT_EQ(0U, receiver_.RtcpSent().nack_packets);
