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}diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc
index 066dc4b..bc742d1 100644
--- a/modules/rtp_rtcp/source/receive_statistics_impl.cc
+++ b/modules/rtp_rtcp/source/receive_statistics_impl.cc
@@ -48,10 +48,11 @@
last_receive_time_ms_(0),
last_received_timestamp_(0),
received_seq_first_(0),
- received_seq_max_(-1),
+ received_seq_max_(0),
+ received_seq_wraps_(0),
last_report_inorder_packets_(0),
last_report_old_packets_(0),
- last_report_seq_max_(-1),
+ last_report_seq_max_(0),
rtcp_callback_(rtcp_callback),
rtp_callback_(rtp_callback) {}
@@ -63,75 +64,54 @@
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);
+ }
- int64_t sequence_number =
- seq_unwrapper_.UnwrapWithoutUpdate(packet.SequenceNumber());
- if (!ReceivedRtpPacket()) {
- received_seq_first_ = sequence_number;
- last_report_seq_max_ = sequence_number - 1;
+ if (receive_counters_.transmitted.packets == 1) {
+ received_seq_first_ = packet.SequenceNumber();
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);
- // 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);
+ // 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;
}
- last_received_timestamp_ = packet.Timestamp();
- last_receive_time_ms_ = now_ms;
return receive_counters_;
}
@@ -183,7 +163,9 @@
bool reset) {
{
rtc::CritScope cs(&stream_lock_);
- if (!ReceivedRtpPacket()) {
+ if (received_seq_first_ == 0 &&
+ receive_counters_.transmitted.payload_bytes == 0) {
+ // We have not received anything.
return false;
}
@@ -214,7 +196,9 @@
// Not active.
return false;
}
- if (!ReceivedRtpPacket()) {
+ if (received_seq_first_ == 0 &&
+ receive_counters_.transmitted.payload_bytes == 0) {
+ // We have not received anything.
return false;
}
@@ -228,9 +212,19 @@
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.
- int64_t exp_since_last = received_seq_max_ - last_report_seq_max_;
- RTC_DCHECK_GE(exp_since_last, 0);
+ 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;
+ }
// Number of received RTP packets since last report, counts all packets but
// not re-transmissions.
@@ -267,7 +261,7 @@
cumulative_loss_ += missing;
stats.packets_lost = cumulative_loss_;
stats.extended_highest_sequence_number =
- static_cast<uint32_t>(received_seq_max_);
+ (received_seq_wraps_ << 16) + 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 0221d85..8153c44 100644
--- a/modules/rtp_rtcp/source/receive_statistics_impl.h
+++ b/modules/rtp_rtcp/source/receive_statistics_impl.h
@@ -17,8 +17,6 @@
#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"
@@ -60,18 +58,7 @@
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_;
@@ -87,13 +74,9 @@
int64_t last_receive_time_ms_ RTC_GUARDED_BY(&stream_lock_);
uint32_t last_received_timestamp_ 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_);
+ 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_);
// Current counter values.
StreamDataCounters receive_counters_ RTC_GUARDED_BY(&stream_lock_);
@@ -101,7 +84,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_);
- int64_t last_report_seq_max_ RTC_GUARDED_BY(&stream_lock_);
+ uint16_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 1703cee..2539363 100644
--- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc
+++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc
@@ -295,217 +295,6 @@
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()
