From c4f120130f495e9726bf221356642de69125f4a2 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Tue, 4 Dec 2018 12:19:51 +0100 Subject: [PATCH] Change ReceiveStatistics reaction to large sequence numbers jumps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consider stream restart when two sequential packets arrived far from previous packets' sequence numbers. instead of resetting on single one. For packet loss calculation ignore sequence number gap during reset. Bug: webrtc:9445, b/38179459 Change-Id: I0c2717ef8f9ec182b280ae757b5582f56d9afcef Reviewed-on: https://webrtc-review.googlesource.com/c/111962 Commit-Queue: Danil Chapovalov Reviewed-by: Åsa Persson Cr-Commit-Position: refs/heads/master@{#25890} --- .../source/receive_statistics_impl.cc | 120 +++++----- .../rtp_rtcp/source/receive_statistics_impl.h | 25 ++- .../source/receive_statistics_unittest.cc | 211 ++++++++++++++++++ 3 files changed, 295 insertions(+), 61 deletions(-) diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc index bc742d1ea6..066dc4b7b9 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/modules/rtp_rtcp/source/receive_statistics_impl.cc @@ -48,11 +48,10 @@ StreamStatisticianImpl::StreamStatisticianImpl( last_receive_time_ms_(0), last_received_timestamp_(0), received_seq_first_(0), - received_seq_max_(0), - received_seq_wraps_(0), + received_seq_max_(-1), last_report_inorder_packets_(0), last_report_old_packets_(0), - last_report_seq_max_(0), + last_report_seq_max_(-1), rtcp_callback_(rtcp_callback), rtp_callback_(rtp_callback) {} @@ -64,54 +63,75 @@ void StreamStatisticianImpl::OnRtpPacket(const RtpPacketReceived& packet) { rtp_callback_->DataCountersUpdated(counters, ssrc_); } +bool StreamStatisticianImpl::UpdateOutOfOrder(const RtpPacketReceived& packet, + int64_t sequence_number, + int64_t now_ms) { + RTC_DCHECK_EQ(sequence_number, + seq_unwrapper_.UnwrapWithoutUpdate(packet.SequenceNumber())); + + // Check if |packet| is second packet of a stream restart. + if (received_seq_out_of_order_) { + uint16_t expected_sequence_number = *received_seq_out_of_order_ + 1; + received_seq_out_of_order_ = absl::nullopt; + if (packet.SequenceNumber() == expected_sequence_number) { + // Ignore sequence number gap caused by stream restart for next packet + // loss calculation. + last_report_seq_max_ = sequence_number; + last_report_inorder_packets_ = receive_counters_.transmitted.packets - + receive_counters_.retransmitted.packets; + // As final part of stream restart consider |packet| is not out of order. + return false; + } + } + + if (std::abs(sequence_number - received_seq_max_) > + max_reordering_threshold_) { + // Sequence number gap looks too large, wait until next packet to check + // for a stream restart. + received_seq_out_of_order_ = packet.SequenceNumber(); + return true; + } + + if (sequence_number > received_seq_max_) + return false; + + // Old out of order packet, may be retransmit. + if (enable_retransmit_detection_ && IsRetransmitOfOldPacket(packet, now_ms)) + receive_counters_.retransmitted.AddPacket(packet); + return true; +} + StreamDataCounters StreamStatisticianImpl::UpdateCounters( const RtpPacketReceived& packet) { rtc::CritScope cs(&stream_lock_); RTC_DCHECK_EQ(ssrc_, packet.Ssrc()); - uint16_t sequence_number = packet.SequenceNumber(); - bool in_order = - // First packet is always in order. - last_receive_time_ms_ == 0 || - IsNewerSequenceNumber(sequence_number, received_seq_max_) || - // If we have a restart of the remote side this packet is still in order. - !IsNewerSequenceNumber(sequence_number, - received_seq_max_ - max_reordering_threshold_); int64_t now_ms = clock_->TimeInMilliseconds(); incoming_bitrate_.Update(packet.size(), now_ms); receive_counters_.transmitted.AddPacket(packet); - if (!in_order && enable_retransmit_detection_ && - IsRetransmitOfOldPacket(packet, now_ms)) { - receive_counters_.retransmitted.AddPacket(packet); - } - if (receive_counters_.transmitted.packets == 1) { - received_seq_first_ = packet.SequenceNumber(); + int64_t sequence_number = + seq_unwrapper_.UnwrapWithoutUpdate(packet.SequenceNumber()); + if (!ReceivedRtpPacket()) { + received_seq_first_ = sequence_number; + last_report_seq_max_ = sequence_number - 1; receive_counters_.first_packet_time_ms = now_ms; + } else if (UpdateOutOfOrder(packet, sequence_number, now_ms)) { + return receive_counters_; } + // In order packet. + received_seq_max_ = sequence_number; + seq_unwrapper_.UpdateLast(sequence_number); - // Count only the new packets received. That is, if packets 1, 2, 3, 5, 4, 6 - // are received, 4 will be ignored. - if (in_order) { - // Wrong if we use RetransmitOfOldPacket. - if (receive_counters_.transmitted.packets > 1 && - received_seq_max_ > packet.SequenceNumber()) { - // Wrap around detected. - received_seq_wraps_++; - } - // New max. - received_seq_max_ = packet.SequenceNumber(); - - // If new time stamp and more than one in-order packet received, calculate - // new jitter statistics. - if (packet.Timestamp() != last_received_timestamp_ && - (receive_counters_.transmitted.packets - - receive_counters_.retransmitted.packets) > 1) { - UpdateJitter(packet, now_ms); - } - last_received_timestamp_ = packet.Timestamp(); - last_receive_time_ms_ = now_ms; + // If new time stamp and more than one in-order packet received, calculate + // new jitter statistics. + if (packet.Timestamp() != last_received_timestamp_ && + (receive_counters_.transmitted.packets - + receive_counters_.retransmitted.packets) > 1) { + UpdateJitter(packet, now_ms); } + last_received_timestamp_ = packet.Timestamp(); + last_receive_time_ms_ = now_ms; return receive_counters_; } @@ -163,9 +183,7 @@ bool StreamStatisticianImpl::GetStatistics(RtcpStatistics* statistics, bool reset) { { rtc::CritScope cs(&stream_lock_); - if (received_seq_first_ == 0 && - receive_counters_.transmitted.payload_bytes == 0) { - // We have not received anything. + if (!ReceivedRtpPacket()) { return false; } @@ -196,9 +214,7 @@ bool StreamStatisticianImpl::GetActiveStatisticsAndReset( // Not active. return false; } - if (received_seq_first_ == 0 && - receive_counters_.transmitted.payload_bytes == 0) { - // We have not received anything. + if (!ReceivedRtpPacket()) { return false; } @@ -212,19 +228,9 @@ bool StreamStatisticianImpl::GetActiveStatisticsAndReset( RtcpStatistics StreamStatisticianImpl::CalculateRtcpStatistics() { RtcpStatistics stats; - - if (last_report_inorder_packets_ == 0) { - // First time we send a report. - last_report_seq_max_ = received_seq_first_ - 1; - } - // Calculate fraction lost. - uint16_t exp_since_last = (received_seq_max_ - last_report_seq_max_); - - if (last_report_seq_max_ > received_seq_max_) { - // Can we assume that the seq_num can't go decrease over a full RTCP period? - exp_since_last = 0; - } + int64_t exp_since_last = received_seq_max_ - last_report_seq_max_; + RTC_DCHECK_GE(exp_since_last, 0); // Number of received RTP packets since last report, counts all packets but // not re-transmissions. @@ -261,7 +267,7 @@ RtcpStatistics StreamStatisticianImpl::CalculateRtcpStatistics() { cumulative_loss_ += missing; stats.packets_lost = cumulative_loss_; stats.extended_highest_sequence_number = - (received_seq_wraps_ << 16) + received_seq_max_; + static_cast(received_seq_max_); // Note: internal jitter value is in Q4 and needs to be scaled by 1/16. stats.jitter = jitter_q4_ >> 4; diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.h b/modules/rtp_rtcp/source/receive_statistics_impl.h index 8153c4400d..0221d857d2 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.h +++ b/modules/rtp_rtcp/source/receive_statistics_impl.h @@ -17,6 +17,8 @@ #include #include +#include "absl/types/optional.h" +#include "modules/include/module_common_types_public.h" #include "rtc_base/criticalsection.h" #include "rtc_base/rate_statistics.h" #include "rtc_base/thread_annotations.h" @@ -58,7 +60,18 @@ class StreamStatisticianImpl : public StreamStatistician, RTC_EXCLUSIVE_LOCKS_REQUIRED(stream_lock_); void UpdateJitter(const RtpPacketReceived& packet, int64_t receive_time_ms) RTC_EXCLUSIVE_LOCKS_REQUIRED(stream_lock_); + // Updates StreamStatistician for out of order packets. + // Returns true if packet considered to be out of order. + bool UpdateOutOfOrder(const RtpPacketReceived& packet, + int64_t sequence_number, + int64_t now_ms) + RTC_EXCLUSIVE_LOCKS_REQUIRED(stream_lock_); + // Updates StreamStatistician for incoming packets. StreamDataCounters UpdateCounters(const RtpPacketReceived& packet); + // Checks if this StreamStatistician received any rtp packets. + bool ReceivedRtpPacket() const RTC_EXCLUSIVE_LOCKS_REQUIRED(stream_lock_) { + return received_seq_max_ >= 0; + } const uint32_t ssrc_; Clock* const clock_; @@ -74,9 +87,13 @@ class StreamStatisticianImpl : public StreamStatistician, int64_t last_receive_time_ms_ RTC_GUARDED_BY(&stream_lock_); uint32_t last_received_timestamp_ RTC_GUARDED_BY(&stream_lock_); - uint16_t received_seq_first_ RTC_GUARDED_BY(&stream_lock_); - uint16_t received_seq_max_ RTC_GUARDED_BY(&stream_lock_); - uint16_t received_seq_wraps_ RTC_GUARDED_BY(&stream_lock_); + SequenceNumberUnwrapper seq_unwrapper_ RTC_GUARDED_BY(&stream_lock_); + int64_t received_seq_first_ RTC_GUARDED_BY(&stream_lock_); + int64_t received_seq_max_ RTC_GUARDED_BY(&stream_lock_); + // Assume that the other side restarted when there are two sequential packets + // with large jump from received_seq_max_. + absl::optional received_seq_out_of_order_ + RTC_GUARDED_BY(&stream_lock_); // Current counter values. StreamDataCounters receive_counters_ RTC_GUARDED_BY(&stream_lock_); @@ -84,7 +101,7 @@ class StreamStatisticianImpl : public StreamStatistician, // Counter values when we sent the last report. uint32_t last_report_inorder_packets_ RTC_GUARDED_BY(&stream_lock_); uint32_t last_report_old_packets_ RTC_GUARDED_BY(&stream_lock_); - uint16_t last_report_seq_max_ RTC_GUARDED_BY(&stream_lock_); + int64_t last_report_seq_max_ RTC_GUARDED_BY(&stream_lock_); RtcpStatistics last_reported_statistics_ RTC_GUARDED_BY(&stream_lock_); // stream_lock_ shouldn't be held when calling callbacks. diff --git a/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/modules/rtp_rtcp/source/receive_statistics_unittest.cc index 25393631df..1703ceef41 100644 --- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc +++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc @@ -295,6 +295,217 @@ TEST_F(ReceiveStatisticsTest, RtcpCallbacks) { EXPECT_EQ(177u, statistics.jitter); } +TEST_F(ReceiveStatisticsTest, SimpleLossComputation) { + packet1_.SetSequenceNumber(1); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(3); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(4); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(5); + receive_statistics_->OnRtpPacket(packet1_); + + RtcpStatistics statistics; + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + // 20% = 51/255. + EXPECT_EQ(51u, statistics.fraction_lost); + EXPECT_EQ(1, statistics.packets_lost); +} + +TEST_F(ReceiveStatisticsTest, LossComputationWithReordering) { + packet1_.SetSequenceNumber(1); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(3); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(2); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(5); + receive_statistics_->OnRtpPacket(packet1_); + + RtcpStatistics statistics; + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + // 20% = 51/255. + EXPECT_EQ(51u, statistics.fraction_lost); + EXPECT_EQ(1, statistics.packets_lost); +} + +TEST_F(ReceiveStatisticsTest, LossComputationWithDuplicates) { + // Lose 2 packets, but also receive 1 duplicate. Should actually count as + // only 1 packet being lost. + packet1_.SetSequenceNumber(1); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(4); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(4); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(5); + receive_statistics_->OnRtpPacket(packet1_); + + RtcpStatistics statistics; + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + // 20% = 51/255. + EXPECT_EQ(51u, statistics.fraction_lost); + EXPECT_EQ(1, statistics.packets_lost); +} + +TEST_F(ReceiveStatisticsTest, LossComputationWithSequenceNumberWrapping) { + // First, test loss computation over a period that included a sequence number + // rollover. + packet1_.SetSequenceNumber(0xfffd); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(0); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(0xfffe); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(1); + receive_statistics_->OnRtpPacket(packet1_); + + // Only one packet was actually lost, 0xffff. + RtcpStatistics statistics; + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + // 20% = 51/255. + EXPECT_EQ(51u, statistics.fraction_lost); + EXPECT_EQ(1, statistics.packets_lost); + + // Now test losing one packet *after* the rollover. + packet1_.SetSequenceNumber(3); + receive_statistics_->OnRtpPacket(packet1_); + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + // 50% = 127/255. + EXPECT_EQ(127u, statistics.fraction_lost); + EXPECT_EQ(2, statistics.packets_lost); +} + +TEST_F(ReceiveStatisticsTest, StreamRestartDoesntCountAsLoss) { + RtcpStatistics statistics; + receive_statistics_->SetMaxReorderingThreshold(200); + + packet1_.SetSequenceNumber(0); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(1); + receive_statistics_->OnRtpPacket(packet1_); + + packet1_.SetSequenceNumber(400); + receive_statistics_->OnRtpPacket(packet1_); + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + EXPECT_EQ(0, statistics.fraction_lost); + EXPECT_EQ(0, statistics.packets_lost); + + packet1_.SetSequenceNumber(401); + receive_statistics_->OnRtpPacket(packet1_); + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + EXPECT_EQ(0, statistics.fraction_lost); + EXPECT_EQ(0, statistics.packets_lost); +} + +TEST_F(ReceiveStatisticsTest, CountsLossAfterStreamRestart) { + RtcpStatistics statistics; + receive_statistics_->SetMaxReorderingThreshold(200); + + packet1_.SetSequenceNumber(0); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(1); + receive_statistics_->OnRtpPacket(packet1_); + + packet1_.SetSequenceNumber(400); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(401); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(403); + receive_statistics_->OnRtpPacket(packet1_); + + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + EXPECT_EQ(1, statistics.packets_lost); +} + +TEST_F(ReceiveStatisticsTest, StreamCanRestartAtSequenceNumberWrapAround) { + RtcpStatistics statistics; + receive_statistics_->SetMaxReorderingThreshold(200); + + packet1_.SetSequenceNumber(0xffff - 401); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(0xffff - 400); + receive_statistics_->OnRtpPacket(packet1_); + + packet1_.SetSequenceNumber(0xffff); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(0); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(2); + receive_statistics_->OnRtpPacket(packet1_); + + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + EXPECT_EQ(1, statistics.packets_lost); +} + +TEST_F(ReceiveStatisticsTest, StreamRestartNeedsTwoConsecutivePackets) { + RtcpStatistics statistics; + receive_statistics_->SetMaxReorderingThreshold(200); + + packet1_.SetSequenceNumber(400); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(401); + receive_statistics_->OnRtpPacket(packet1_); + + packet1_.SetSequenceNumber(1); + receive_statistics_->OnRtpPacket(packet1_); + packet1_.SetSequenceNumber(3); + receive_statistics_->OnRtpPacket(packet1_); + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + EXPECT_EQ(401u, statistics.extended_highest_sequence_number); + + packet1_.SetSequenceNumber(4); + receive_statistics_->OnRtpPacket(packet1_); + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + EXPECT_EQ(4u, statistics.extended_highest_sequence_number); +} + +TEST_F(ReceiveStatisticsTest, WrapsAroundExtendedHighestSequenceNumber) { + RtcpStatistics statistics; + packet1_.SetSequenceNumber(0xffff); + receive_statistics_->OnRtpPacket(packet1_); + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + EXPECT_EQ(0xffffu, statistics.extended_highest_sequence_number); + + // Wrap around. + packet1_.SetSequenceNumber(1); + receive_statistics_->OnRtpPacket(packet1_); + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + EXPECT_EQ(0x10001u, statistics.extended_highest_sequence_number); + + // Should be treated as out of order; shouldn't increment highest extended + // sequence number. + packet1_.SetSequenceNumber(0x10000 - 6); + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + EXPECT_EQ(0x10001u, statistics.extended_highest_sequence_number); + + // Receive a couple packets then wrap around again. + receive_statistics_->SetMaxReorderingThreshold(200); + for (int i = 10; i < 0xffff; i += 150) { + packet1_.SetSequenceNumber(i); + receive_statistics_->OnRtpPacket(packet1_); + } + packet1_.SetSequenceNumber(1); + receive_statistics_->OnRtpPacket(packet1_); + receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, + true); + EXPECT_EQ(0x20001u, statistics.extended_highest_sequence_number); +} + class RtpTestCallback : public StreamDataCountersCallback { public: RtpTestCallback()