diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc index ca74908271..60349c446d 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/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_old_packets_(0), + last_report_cumulative_loss_(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 - // 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. + // Ignore sequence number gap caused by stream restart for packet loss + // calculation, by setting received_seq_max_ to the sequence number just + // before the out-of-order seqno. This gives a net zero change of + // |cumulative_loss_|, for the two packets interpreted as a stream reset. + // + // 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( - const RtpPacketReceived& packet) { +void StreamStatisticianImpl::UpdateCounters(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 - // not re-transmissions. - 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) { + int32_t lost_since_last = cumulative_loss_ - last_report_cumulative_loss_; + if (exp_since_last > 0 && lost_since_last > 0) { // Scale 0 to 255, where 255 is 100% loss. - local_fraction_lost = static_cast(255 * missing / exp_since_last); + stats.fraction_lost = + static_cast(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 cumulative loss is below maximum value of 2^24. - cumulative_loss_ += missing; - stats.packets_lost = cumulative_loss_; + // TODO(danilchap): Ensure |stats.packets_lost| is clamped to fit in a signed + // 24-bit value. + stats.packets_lost = cumulative_loss_ + cumulative_loss_rtcp_offset_; + 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(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 - - receive_counters_.retransmitted.packets; - last_report_old_packets_ = receive_counters_.retransmitted.packets; + last_report_cumulative_loss_ = cumulative_loss_; 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 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 - // this expression is always non-negative. + if (cumulative_loss_ <= 0) { + return 0; + } return 100 * static_cast(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( diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.h b/modules/rtp_rtcp/source/receive_statistics_impl.h index b7b9be3fdf..8b8dde0150 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.h +++ b/modules/rtp_rtcp/source/receive_statistics_impl.h @@ -24,8 +24,7 @@ namespace webrtc { -class StreamStatisticianImpl : public StreamStatistician, - public RtpPacketSinkInterface { +class StreamStatisticianImpl : public StreamStatistician { 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_); - uint32_t last_report_old_packets_ RTC_GUARDED_BY(&stream_lock_); + int32_t last_report_cumulative_loss_ 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_); };