Simplify book-keeping of lost packets

Update the |cumulative_lost_| counter per received packet. The rules
follow from RFC 3550 and are fairly simple: Decrement the counter by
one for every received packet. For every in-order packet, i.e., increasing
|received_seq_max_|, add that change to |cumulative_lost_|.

Net change is zero as long as packets are received in proper sequence.

This way, GetStats() always returns an up-to-date value, independent
of the timing of RTCP report blocks.

For RTCP reports, keep a workaround to never report negative cumulative loss.

Bug: webrtc:10679
Change-Id: I47ff3bf266ff2382f405ec9828d34f7fad7068b4
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/150641
Commit-Queue: Niels Moller <nisse@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#29058}

modules/rtp_rtcp/source/receive_statistics_impl.cc

@@ -41,37 +41,37 @@ StreamStatisticianImpl::StreamStatisticianImpl(uint32_t ssrc,
       enable_retransmit_detection_(false),
       jitter_q4_(0),
       cumulative_loss_(0),
+      cumulative_loss_rtcp_offset_(0),
       last_receive_time_ms_(0),
       last_received_timestamp_(0),
-      received_seq_first_(0),
+      received_seq_first_(-1),
       received_seq_max_(-1),
-      last_report_inorder_packets_(0),
+      last_report_cumulative_loss_(0),
-      last_report_old_packets_(0),
       last_report_seq_max_(-1) {}
 
 StreamStatisticianImpl::~StreamStatisticianImpl() = default;
 
-void StreamStatisticianImpl::OnRtpPacket(const RtpPacketReceived& packet) {
-  UpdateCounters(packet);
-}
-
 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_) {
+    // Count the previous packet as a received; it was postponed below.
+    --cumulative_loss_;
+
     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
+      // Ignore sequence number gap caused by stream restart for packet loss
-      // loss calculation.
+      // calculation, by setting received_seq_max_ to the sequence number just
-      last_report_seq_max_ = sequence_number;
+      // before the out-of-order seqno. This gives a net zero change of
-      last_report_inorder_packets_ = receive_counters_.transmitted.packets -
+      // |cumulative_loss_|, for the two packets interpreted as a stream reset.
-                                     receive_counters_.retransmitted.packets;
+      //
-      // As final part of stream restart consider |packet| is not out of order.
+      // Fraction loss for the next report may get a bit off, since we don't
+      // update last_report_seq_max_ and last_report_cumulative_loss_ in a
+      // consistent way.
+      last_report_seq_max_ = sequence_number - 2;
+      received_seq_max_ = sequence_number - 2;
       return false;
     }
   }
@@ -81,6 +81,13 @@ bool StreamStatisticianImpl::UpdateOutOfOrder(const RtpPacketReceived& packet,
     // Sequence number gap looks too large, wait until next packet to check
     // for a stream restart.
     received_seq_out_of_order_ = packet.SequenceNumber();
+    // Postpone counting this as a received packet until we know how to update
+    // |received_seq_max_|, otherwise we temporarily decrement
+    // |cumulative_loss_|. The
+    // ReceiveStatisticsTest.StreamRestartDoesntCountAsLoss test expects
+    // |cumulative_loss_| to be unchanged by the reception of the first packet
+    // after stream reset.
+    ++cumulative_loss_;
     return true;
   }
 
@@ -93,8 +100,7 @@ bool StreamStatisticianImpl::UpdateOutOfOrder(const RtpPacketReceived& packet,
   return true;
 }
 
-StreamDataCounters StreamStatisticianImpl::UpdateCounters(
+void StreamStatisticianImpl::UpdateCounters(const RtpPacketReceived& packet) {
-    const RtpPacketReceived& packet) {
   rtc::CritScope cs(&stream_lock_);
   RTC_DCHECK_EQ(ssrc_, packet.Ssrc());
   int64_t now_ms = clock_->TimeInMilliseconds();
@@ -102,17 +108,21 @@ StreamDataCounters StreamStatisticianImpl::UpdateCounters(
   incoming_bitrate_.Update(packet.size(), now_ms);
   receive_counters_.last_packet_received_timestamp_ms = now_ms;
   receive_counters_.transmitted.AddPacket(packet);
+  --cumulative_loss_;
 
   int64_t sequence_number =
       seq_unwrapper_.UnwrapWithoutUpdate(packet.SequenceNumber());
+
   if (!ReceivedRtpPacket()) {
     received_seq_first_ = sequence_number;
     last_report_seq_max_ = sequence_number - 1;
+    received_seq_max_ = sequence_number - 1;
     receive_counters_.first_packet_time_ms = now_ms;
   } else if (UpdateOutOfOrder(packet, sequence_number, now_ms)) {
-    return receive_counters_;
+    return;
   }
   // In order packet.
+  cumulative_loss_ += sequence_number - received_seq_max_;
   received_seq_max_ = sequence_number;
   seq_unwrapper_.UpdateLast(sequence_number);
 
@@ -125,7 +135,6 @@ StreamDataCounters StreamStatisticianImpl::UpdateCounters(
   }
   last_received_timestamp_ = packet.Timestamp();
   last_receive_time_ms_ = now_ms;
-  return receive_counters_;
 }
 
 void StreamStatisticianImpl::UpdateJitter(const RtpPacketReceived& packet,
@@ -181,7 +190,7 @@ bool StreamStatisticianImpl::GetStatistics(RtcpStatistics* statistics,
   }
 
   if (!reset) {
-    if (last_report_inorder_packets_ == 0) {
+    if (!ReceivedRtpPacket()) {
       // No report.
       return false;
     }
@@ -218,40 +227,24 @@ RtcpStatistics StreamStatisticianImpl::CalculateRtcpStatistics() {
   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
+  int32_t lost_since_last = cumulative_loss_ - last_report_cumulative_loss_;
-  // not re-transmissions.
+  if (exp_since_last > 0 && lost_since_last > 0) {
-  uint32_t rec_since_last = (receive_counters_.transmitted.packets -
-                             receive_counters_.retransmitted.packets) -
-                            last_report_inorder_packets_;
-
-  // With NACK we don't know the expected retransmissions during the last
-  // second. We know how many "old" packets we have received. We just count
-  // the number of old received to estimate the loss, but it still does not
-  // guarantee an exact number since we run this based on time triggered by
-  // sending of an RTP packet. This should have a minimum effect.
-
-  // With NACK we don't count old packets as received since they are
-  // re-transmitted. We use RTT to decide if a packet is re-ordered or
-  // re-transmitted.
-  uint32_t retransmitted_packets =
-      receive_counters_.retransmitted.packets - last_report_old_packets_;
-  rec_since_last += retransmitted_packets;
-
-  int32_t missing = 0;
-  if (exp_since_last > rec_since_last) {
-    missing = (exp_since_last - rec_since_last);
-  }
-  uint8_t local_fraction_lost = 0;
-  if (exp_since_last) {
     // Scale 0 to 255, where 255 is 100% loss.
-    local_fraction_lost = static_cast<uint8_t>(255 * missing / exp_since_last);
+    stats.fraction_lost =
+        static_cast<uint8_t>(255 * lost_since_last / exp_since_last);
+  } else {
+    stats.fraction_lost = 0;
   }
-  stats.fraction_lost = local_fraction_lost;
 
-  // We need a counter for cumulative loss too.
+  // TODO(danilchap): Ensure |stats.packets_lost| is clamped to fit in a signed
-  // TODO(danilchap): Ensure cumulative loss is below maximum value of 2^24.
+  // 24-bit value.
-  cumulative_loss_ += missing;
+  stats.packets_lost = cumulative_loss_ + cumulative_loss_rtcp_offset_;
-  stats.packets_lost = cumulative_loss_;
+  if (stats.packets_lost < 0) {
+    // Clamp to zero. Work around to accomodate for senders that misbehave with
+    // negative cumulative loss.
+    stats.packets_lost = 0;
+    cumulative_loss_rtcp_offset_ = -cumulative_loss_;
+  }
   stats.extended_highest_sequence_number =
       static_cast<uint32_t>(received_seq_max_);
   // Note: internal jitter value is in Q4 and needs to be scaled by 1/16.
@@ -261,9 +254,7 @@ RtcpStatistics StreamStatisticianImpl::CalculateRtcpStatistics() {
   last_reported_statistics_ = stats;
 
   // Only for report blocks in RTCP SR and RR.
-  last_report_inorder_packets_ = receive_counters_.transmitted.packets -
+  last_report_cumulative_loss_ = cumulative_loss_;
-                                 receive_counters_.retransmitted.packets;
-  last_report_old_packets_ = receive_counters_.retransmitted.packets;
   last_report_seq_max_ = received_seq_max_;
   BWE_TEST_LOGGING_PLOT_WITH_SSRC(1, "cumulative_loss_pkts",
                                   clock_->TimeInMilliseconds(),
@@ -277,15 +268,16 @@ RtcpStatistics StreamStatisticianImpl::CalculateRtcpStatistics() {
 
 absl::optional<int> StreamStatisticianImpl::GetFractionLostInPercent() const {
   rtc::CritScope cs(&stream_lock_);
-  if (received_seq_max_ < 0) {
+  if (!ReceivedRtpPacket()) {
     return absl::nullopt;
   }
   int64_t expected_packets = 1 + received_seq_max_ - received_seq_first_;
   if (expected_packets <= 0) {
     return absl::nullopt;
   }
-  // Spec allows negative cumulative loss, but implementation uses uint32_t, so
+  if (cumulative_loss_ <= 0) {
-  // this expression is always non-negative.
+    return 0;
+  }
   return 100 * static_cast<int64_t>(cumulative_loss_) / expected_packets;
 }
 
@@ -349,7 +341,7 @@ void ReceiveStatisticsImpl::OnRtpPacket(const RtpPacketReceived& packet) {
   // this whole ReceiveStatisticsImpl is destroyed. StreamStatisticianImpl has
   // it's own locking so don't hold receive_statistics_lock_ (potential
   // deadlock).
-  GetOrCreateStatistician(packet.Ssrc())->OnRtpPacket(packet);
+  GetOrCreateStatistician(packet.Ssrc())->UpdateCounters(packet);
 }
 
 StreamStatisticianImpl* ReceiveStatisticsImpl::GetStatistician(

modules/rtp_rtcp/source/receive_statistics_impl.h

@@ -24,8 +24,7 @@
 
 namespace webrtc {
 
-class StreamStatisticianImpl : public StreamStatistician,
+class StreamStatisticianImpl : public StreamStatistician {
-                               public RtpPacketSinkInterface {
  public:
   StreamStatisticianImpl(uint32_t ssrc,
                          Clock* clock,
@@ -41,12 +40,12 @@ class StreamStatisticianImpl : public StreamStatistician,
   StreamDataCounters GetReceiveStreamDataCounters() const override;
   uint32_t BitrateReceived() const override;
 
-  // Implements RtpPacketSinkInterface
-  void OnRtpPacket(const RtpPacketReceived& packet) override;
-
   void SetMaxReorderingThreshold(int max_reordering_threshold);
   void EnableRetransmitDetection(bool enable);
 
+  // Updates StreamStatistician for incoming packets.
+  void UpdateCounters(const RtpPacketReceived& packet);
+
  private:
   bool IsRetransmitOfOldPacket(const RtpPacketReceived& packet,
                                int64_t now_ms) const
@@ -61,11 +60,9 @@ class StreamStatisticianImpl : public StreamStatistician,
                         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;
+    return received_seq_first_ >= 0;
   }
 
   const uint32_t ssrc_;
@@ -78,7 +75,13 @@ class StreamStatisticianImpl : public StreamStatistician,
 
   // Stats on received RTP packets.
   uint32_t jitter_q4_ RTC_GUARDED_BY(&stream_lock_);
-  uint32_t cumulative_loss_ RTC_GUARDED_BY(&stream_lock_);
+  // Cumulative loss according to RFC 3550, which may be negative (and often is,
+  // if packets are reordered and there are non-RTX retransmissions).
+  int32_t cumulative_loss_ RTC_GUARDED_BY(&stream_lock_);
+  // Offset added to outgoing rtcp reports, to make ensure that the reported
+  // cumulative loss is non-negative. Reports with negative values confuse some
+  // senders, in particular, our own loss-based bandwidth estimator.
+  int32_t cumulative_loss_rtcp_offset_ RTC_GUARDED_BY(&stream_lock_);
 
   int64_t last_receive_time_ms_ RTC_GUARDED_BY(&stream_lock_);
   uint32_t last_received_timestamp_ RTC_GUARDED_BY(&stream_lock_);
@@ -94,8 +97,7 @@ class StreamStatisticianImpl : public StreamStatistician,
   StreamDataCounters receive_counters_ RTC_GUARDED_BY(&stream_lock_);
 
   // Counter values when we sent the last report.
-  uint32_t last_report_inorder_packets_ RTC_GUARDED_BY(&stream_lock_);
+  int32_t last_report_cumulative_loss_ 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_);
   RtcpStatistics last_reported_statistics_ RTC_GUARDED_BY(&stream_lock_);
 };