Reland "Change ReceiveStatistics reaction to large sequence numbers jumps"
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 <danilchap@webrtc.org>
> > Reviewed-by: Åsa Persson <asapersson@webrtc.org>
> > 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 <danilchap@webrtc.org>
> Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
> 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 <danilchap@webrtc.org>
Commit-Queue: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#25907}diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc
index bc742d1..066dc4b 100644
--- a/modules/rtp_rtcp/source/receive_statistics_impl.cc
+++ b/modules/rtp_rtcp/source/receive_statistics_impl.cc
@@ -48,11 +48,10 @@
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 @@
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 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 @@
// 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 @@
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 @@
cumulative_loss_ += missing;
stats.packets_lost = cumulative_loss_;
stats.extended_highest_sequence_number =
- (received_seq_wraps_ << 16) + received_seq_max_;
+ static_cast<uint32_t>(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 8153c44..0221d85 100644
--- a/modules/rtp_rtcp/source/receive_statistics_impl.h
+++ b/modules/rtp_rtcp/source/receive_statistics_impl.h
@@ -17,6 +17,8 @@
#include <map>
#include <vector>
+#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 @@
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 @@
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<uint16_t> 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 @@
// 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 2539363..1703cee 100644
--- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc
+++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc
@@ -295,6 +295,217 @@
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()
