From b438b5a33d136115b72407a017e8d4c93edc13e6 Mon Sep 17 00:00:00 2001 From: Danil Chapovalov Date: Wed, 5 Dec 2018 14:55:46 +0000 Subject: [PATCH] Reland "Change ReceiveStatistics reaction to large sequence numbers jumps" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 7e0299e2452b021fcd14a8fdb86257459eeacf90. Reason for revert: audio receive stream fix not to use 0 reordering threshold Original change's description: > Revert "Change ReceiveStatistics reaction to large sequence numbers jumps" > > This reverts commit c4f120130f495e9726bf221356642de69125f4a2. > > Reason for revert: breaks downstream tests due to zero max reordering for audio receive channels > > Original change's description: > > Change ReceiveStatistics reaction to large sequence numbers jumps > > > > 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} > > TBR=danilchap@webrtc.org,asapersson@webrtc.org > > Change-Id: Icc9f4d86d9f0b07f0fa2f3d443f9a90aa91f5e21 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: webrtc:9445, b/38179459 > Reviewed-on: https://webrtc-review.googlesource.com/c/113067 > Reviewed-by: Danil Chapovalov > Commit-Queue: Danil Chapovalov > Cr-Commit-Position: refs/heads/master@{#25897} TBR=danilchap@webrtc.org,asapersson@webrtc.org Change-Id: I8747aa5cb6209b92fafefed077bc19d305d11db6 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:9445, b/38179459 Reviewed-on: https://webrtc-review.googlesource.com/c/113263 Reviewed-by: Danil Chapovalov Commit-Queue: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#25907} --- .../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()