diff --git a/modules/rtp_rtcp/include/receive_statistics.h b/modules/rtp_rtcp/include/receive_statistics.h index 36e7b2496c..a0dcd85efe 100644 --- a/modules/rtp_rtcp/include/receive_statistics.h +++ b/modules/rtp_rtcp/include/receive_statistics.h @@ -15,6 +15,7 @@ #include #include +#include "absl/types/optional.h" #include "call/rtp_packet_sink_interface.h" #include "modules/include/module.h" #include "modules/include/module_common_types.h" @@ -44,6 +45,9 @@ class StreamStatistician { virtual void GetDataCounters(size_t* bytes_received, uint32_t* packets_received) const = 0; + // Returns average over the stream life time. + virtual absl::optional GetFractionLostInPercent() const = 0; + // Gets received stream data counters (includes reset counter values). virtual void GetReceiveStreamDataCounters( StreamDataCounters* data_counters) const = 0; diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.cc b/modules/rtp_rtcp/source/receive_statistics_impl.cc index 0a9bc9667e..45be4d1d39 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.cc +++ b/modules/rtp_rtcp/source/receive_statistics_impl.cc @@ -302,6 +302,20 @@ void StreamStatisticianImpl::GetDataCounters(size_t* bytes_received, } } +absl::optional StreamStatisticianImpl::GetFractionLostInPercent() const { + rtc::CritScope cs(&stream_lock_); + if (received_seq_max_ < 0) { + 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. + return 100 * static_cast(cumulative_loss_) / expected_packets; +} + void StreamStatisticianImpl::GetReceiveStreamDataCounters( StreamDataCounters* data_counters) const { rtc::CritScope cs(&stream_lock_); diff --git a/modules/rtp_rtcp/source/receive_statistics_impl.h b/modules/rtp_rtcp/source/receive_statistics_impl.h index d4bcc45bc2..74150a9601 100644 --- a/modules/rtp_rtcp/source/receive_statistics_impl.h +++ b/modules/rtp_rtcp/source/receive_statistics_impl.h @@ -39,6 +39,7 @@ class StreamStatisticianImpl : public StreamStatistician, bool GetActiveStatisticsAndReset(RtcpStatistics* statistics); void GetDataCounters(size_t* bytes_received, uint32_t* packets_received) const override; + absl::optional GetFractionLostInPercent() const override; void GetReceiveStreamDataCounters( StreamDataCounters* data_counters) const override; uint32_t BitrateReceived() const override; diff --git a/modules/rtp_rtcp/source/receive_statistics_unittest.cc b/modules/rtp_rtcp/source/receive_statistics_unittest.cc index f899503f55..6a8ba6620a 100644 --- a/modules/rtp_rtcp/source/receive_statistics_unittest.cc +++ b/modules/rtp_rtcp/source/receive_statistics_unittest.cc @@ -306,12 +306,14 @@ TEST_F(ReceiveStatisticsTest, SimpleLossComputation) { packet1_.SetSequenceNumber(5); receive_statistics_->OnRtpPacket(packet1_); + StreamStatistician* statistician = + receive_statistics_->GetStatistician(kSsrc1); RtcpStatistics statistics; - receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, - true); + statistician->GetStatistics(&statistics, true); // 20% = 51/255. EXPECT_EQ(51u, statistics.fraction_lost); EXPECT_EQ(1, statistics.packets_lost); + EXPECT_EQ(20, statistician->GetFractionLostInPercent()); } TEST_F(ReceiveStatisticsTest, LossComputationWithReordering) { @@ -324,12 +326,14 @@ TEST_F(ReceiveStatisticsTest, LossComputationWithReordering) { packet1_.SetSequenceNumber(5); receive_statistics_->OnRtpPacket(packet1_); + StreamStatistician* statistician = + receive_statistics_->GetStatistician(kSsrc1); RtcpStatistics statistics; - receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, - true); + statistician->GetStatistics(&statistics, true); // 20% = 51/255. EXPECT_EQ(51u, statistics.fraction_lost); EXPECT_EQ(1, statistics.packets_lost); + EXPECT_EQ(20, statistician->GetFractionLostInPercent()); } TEST_F(ReceiveStatisticsTest, LossComputationWithDuplicates) { @@ -344,12 +348,14 @@ TEST_F(ReceiveStatisticsTest, LossComputationWithDuplicates) { packet1_.SetSequenceNumber(5); receive_statistics_->OnRtpPacket(packet1_); + StreamStatistician* statistician = + receive_statistics_->GetStatistician(kSsrc1); RtcpStatistics statistics; - receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, - true); + statistician->GetStatistics(&statistics, true); // 20% = 51/255. EXPECT_EQ(51u, statistics.fraction_lost); EXPECT_EQ(1, statistics.packets_lost); + EXPECT_EQ(20, statistician->GetFractionLostInPercent()); } TEST_F(ReceiveStatisticsTest, LossComputationWithSequenceNumberWrapping) { @@ -365,21 +371,24 @@ TEST_F(ReceiveStatisticsTest, LossComputationWithSequenceNumberWrapping) { receive_statistics_->OnRtpPacket(packet1_); // Only one packet was actually lost, 0xffff. + StreamStatistician* statistician = + receive_statistics_->GetStatistician(kSsrc1); RtcpStatistics statistics; - receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, - true); + statistician->GetStatistics(&statistics, true); // 20% = 51/255. EXPECT_EQ(51u, statistics.fraction_lost); EXPECT_EQ(1, statistics.packets_lost); + EXPECT_EQ(20, statistician->GetFractionLostInPercent()); // Now test losing one packet *after* the rollover. packet1_.SetSequenceNumber(3); receive_statistics_->OnRtpPacket(packet1_); - receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, - true); + statistician->GetStatistics(&statistics, true); // 50% = 127/255. EXPECT_EQ(127u, statistics.fraction_lost); EXPECT_EQ(2, statistics.packets_lost); + // 2 packets lost, 7 expected + EXPECT_EQ(28, statistician->GetFractionLostInPercent()); } TEST_F(ReceiveStatisticsTest, StreamRestartDoesntCountAsLoss) { @@ -393,17 +402,19 @@ TEST_F(ReceiveStatisticsTest, StreamRestartDoesntCountAsLoss) { packet1_.SetSequenceNumber(400); receive_statistics_->OnRtpPacket(packet1_); - receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, - true); + StreamStatistician* statistician = + receive_statistics_->GetStatistician(kSsrc1); + statistician->GetStatistics(&statistics, true); EXPECT_EQ(0, statistics.fraction_lost); EXPECT_EQ(0, statistics.packets_lost); + EXPECT_EQ(0, statistician->GetFractionLostInPercent()); packet1_.SetSequenceNumber(401); receive_statistics_->OnRtpPacket(packet1_); - receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, - true); + statistician->GetStatistics(&statistics, true); EXPECT_EQ(0, statistics.fraction_lost); EXPECT_EQ(0, statistics.packets_lost); + EXPECT_EQ(0, statistician->GetFractionLostInPercent()); } TEST_F(ReceiveStatisticsTest, CountsLossAfterStreamRestart) { @@ -422,9 +433,13 @@ TEST_F(ReceiveStatisticsTest, CountsLossAfterStreamRestart) { packet1_.SetSequenceNumber(403); receive_statistics_->OnRtpPacket(packet1_); - receive_statistics_->GetStatistician(kSsrc1)->GetStatistics(&statistics, - true); + StreamStatistician* statistician = + receive_statistics_->GetStatistician(kSsrc1); + + statistician->GetStatistics(&statistics, true); EXPECT_EQ(1, statistics.packets_lost); + // Is this reasonable? */ + EXPECT_EQ(0, statistician->GetFractionLostInPercent()); } TEST_F(ReceiveStatisticsTest, StreamCanRestartAtSequenceNumberWrapAround) { diff --git a/video/receive_statistics_proxy.cc b/video/receive_statistics_proxy.cc index 937a374974..158146a9ad 100644 --- a/video/receive_statistics_proxy.cc +++ b/video/receive_statistics_proxy.cc @@ -110,7 +110,6 @@ ReceiveStatisticsProxy::ReceiveStatisticsProxy( new VideoQualityObserver(VideoContentType::UNSPECIFIED)), interframe_delay_max_moving_(kMovingMaxWindowMs), freq_offset_counter_(clock, nullptr, kFreqOffsetProcessIntervalMs), - first_report_block_time_ms_(-1), avg_rtt_ms_(0), last_content_type_(VideoContentType::UNSPECIFIED), last_codec_type_(kVideoCodecVP8), @@ -127,20 +126,16 @@ ReceiveStatisticsProxy::ReceiveStatisticsProxy( } } -ReceiveStatisticsProxy::~ReceiveStatisticsProxy() { - RTC_DCHECK_RUN_ON(&main_thread_); - // In case you're reading this wondering "hmm... we're on the main thread but - // calling a method that needs to be called on the decoder thread...", then - // here's what's going on: - // - The decoder thread has been stopped and DecoderThreadStopped() has been - // called. - // - The decode_thread_ thread checker has been detached, and will now become - // attached to the current thread, which is OK since we're in the dtor. - UpdateHistograms(); -} - -void ReceiveStatisticsProxy::UpdateHistograms() { +void ReceiveStatisticsProxy::UpdateHistograms( + absl::optional fraction_lost) { + // Not actually running on the decoder thread, but must be called after + // DecoderThreadStopped, which detaches the thread checker. It is therefore + // safe to access |qp_counters_|, which were updated on the decode thread + // earlier. RTC_DCHECK_RUN_ON(&decode_thread_); + + rtc::CritScope lock(&crit_); + char log_stream_buf[8 * 1024]; rtc::SimpleStringBuilder log_stream(log_stream_buf); int stream_duration_sec = (clock_->TimeInMilliseconds() - start_ms_) / 1000; @@ -162,16 +157,11 @@ void ReceiveStatisticsProxy::UpdateHistograms() { << '\n'; } - if (first_report_block_time_ms_ != -1 && - ((clock_->TimeInMilliseconds() - first_report_block_time_ms_) / 1000) >= - metrics::kMinRunTimeInSeconds) { - int fraction_lost = report_block_stats_.FractionLostInPercent(); - if (fraction_lost != -1) { - RTC_HISTOGRAM_PERCENTAGE("WebRTC.Video.ReceivedPacketsLostInPercent", - fraction_lost); - log_stream << "WebRTC.Video.ReceivedPacketsLostInPercent " - << fraction_lost << '\n'; - } + if (fraction_lost && stream_duration_sec >= metrics::kMinRunTimeInSeconds) { + RTC_HISTOGRAM_PERCENTAGE("WebRTC.Video.ReceivedPacketsLostInPercent", + *fraction_lost); + log_stream << "WebRTC.Video.ReceivedPacketsLostInPercent " << *fraction_lost + << '\n'; } if (first_decoded_frame_time_ms_) { @@ -489,6 +479,7 @@ void ReceiveStatisticsProxy::UpdateHistograms() { } RTC_LOG(LS_INFO) << log_stream.str(); + video_quality_observer_->UpdateHistograms(); } void ReceiveStatisticsProxy::QualitySample() { @@ -677,10 +668,6 @@ void ReceiveStatisticsProxy::StatisticsUpdated( if (stats_.ssrc != ssrc) return; stats_.rtcp_stats = statistics; - report_block_stats_.Store(0, statistics); - - if (first_report_block_time_ms_ == -1) - first_report_block_time_ms_ = clock_->TimeInMilliseconds(); } void ReceiveStatisticsProxy::CNameChanged(const char* cname, uint32_t ssrc) { @@ -726,8 +713,9 @@ void ReceiveStatisticsProxy::OnDecodedFrame(const VideoFrame& frame, if (videocontenttypehelpers::IsScreenshare(content_type) != videocontenttypehelpers::IsScreenshare(last_content_type_)) { - // Reset the quality observer if content type is switched. This will - // report stats for the previous part of the call. + // Reset the quality observer if content type is switched. But first report + // stats for the previous part of the call. + video_quality_observer_->UpdateHistograms(); video_quality_observer_.reset(new VideoQualityObserver(content_type)); } diff --git a/video/receive_statistics_proxy.h b/video/receive_statistics_proxy.h index 8aa8f5ea00..289efb9287 100644 --- a/video/receive_statistics_proxy.h +++ b/video/receive_statistics_proxy.h @@ -28,7 +28,6 @@ #include "rtc_base/thread_annotations.h" #include "rtc_base/thread_checker.h" #include "video/quality_threshold.h" -#include "video/report_block_stats.h" #include "video/stats_counter.h" #include "video/video_quality_observer.h" @@ -45,7 +44,7 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, public: ReceiveStatisticsProxy(const VideoReceiveStream::Config* config, Clock* clock); - ~ReceiveStatisticsProxy() override; + ~ReceiveStatisticsProxy() = default; VideoReceiveStream::Stats GetStats() const; @@ -99,6 +98,10 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, void DecoderThreadStarting(); void DecoderThreadStopped(); + // Produce histograms. Must be called after DecoderThreadStopped(), typically + // at the end of the call. + void UpdateHistograms(absl::optional fraction_lost); + private: struct QpCounters { rtc::SampleCounter vp8; @@ -121,8 +124,6 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, rtc::HistogramPercentileCounter interframe_delay_percentiles; }; - void UpdateHistograms() RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_); - void QualitySample() RTC_EXCLUSIVE_LOCKS_REQUIRED(crit_); // Removes info about old frames and then updates the framerate. @@ -167,8 +168,6 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, std::map content_specific_stats_ RTC_GUARDED_BY(crit_); MaxCounter freq_offset_counter_ RTC_GUARDED_BY(crit_); - int64_t first_report_block_time_ms_ RTC_GUARDED_BY(crit_); - ReportBlockStats report_block_stats_ RTC_GUARDED_BY(crit_); QpCounters qp_counters_ RTC_GUARDED_BY(decode_thread_); std::map rtx_stats_ RTC_GUARDED_BY(crit_); int64_t avg_rtt_ms_ RTC_GUARDED_BY(crit_); diff --git a/video/receive_statistics_proxy_unittest.cc b/video/receive_statistics_proxy_unittest.cc index 276a059e46..c6290674d8 100644 --- a/video/receive_statistics_proxy_unittest.cc +++ b/video/receive_statistics_proxy_unittest.cc @@ -103,7 +103,7 @@ TEST_F(ReceiveStatisticsProxyTest, DecodedFpsIsReported) { VideoContentType::UNSPECIFIED); fake_clock_.AdvanceTimeMilliseconds(1000 / kFps); } - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.DecodedFramesPerSecond")); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.DecodedFramesPerSecond", kFps)); } @@ -117,7 +117,7 @@ TEST_F(ReceiveStatisticsProxyTest, DecodedFpsIsNotReportedForTooFewSamples) { VideoContentType::UNSPECIFIED); fake_clock_.AdvanceTimeMilliseconds(1000 / kFps); } - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.DecodedFramesPerSecond")); } @@ -545,8 +545,7 @@ TEST_F(ReceiveStatisticsProxyTest, LifetimeHistogramIsUpdated) { fake_clock_.AdvanceTimeMilliseconds(kTimeSec * 1000); // Need at least one frame to report stream lifetime. statistics_proxy_->OnCompleteFrame(true, 1000, VideoContentType::UNSPECIFIED); - // Histograms are updated when the statistics_proxy_ is deleted. - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.ReceiveStreamLifetimeInSeconds")); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.ReceiveStreamLifetimeInSeconds", @@ -558,8 +557,7 @@ TEST_F(ReceiveStatisticsProxyTest, const int64_t kTimeSec = 3; fake_clock_.AdvanceTimeMilliseconds(kTimeSec * 1000); // No frames received. - // Histograms are updated when the statistics_proxy_ is deleted. - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.ReceiveStreamLifetimeInSeconds")); } @@ -582,8 +580,7 @@ TEST_F(ReceiveStatisticsProxyTest, BadCallHistogramsAreUpdated) { fake_clock_.AdvanceTimeMilliseconds(kBadFameIntervalMs); statistics_proxy_->OnRenderedFrame(frame); } - // Histograms are updated when the statistics_proxy_ is deleted. - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.BadCall.Any")); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.BadCall.Any", 100)); @@ -596,61 +593,20 @@ TEST_F(ReceiveStatisticsProxyTest, BadCallHistogramsAreUpdated) { } TEST_F(ReceiveStatisticsProxyTest, PacketLossHistogramIsUpdated) { - const uint32_t kCumLost1 = 1; - const uint32_t kExtSeqNum1 = 10; - const uint32_t kCumLost2 = 2; - const uint32_t kExtSeqNum2 = 20; - - // One report block received. - RtcpStatistics rtcp_stats1; - rtcp_stats1.packets_lost = kCumLost1; - rtcp_stats1.extended_highest_sequence_number = kExtSeqNum1; - statistics_proxy_->StatisticsUpdated(rtcp_stats1, kRemoteSsrc); - - // Two report blocks received. - RtcpStatistics rtcp_stats2; - rtcp_stats2.packets_lost = kCumLost2; - rtcp_stats2.extended_highest_sequence_number = kExtSeqNum2; - statistics_proxy_->StatisticsUpdated(rtcp_stats2, kRemoteSsrc); - - // Two received report blocks but min run time has not passed. - fake_clock_.AdvanceTimeMilliseconds(metrics::kMinRunTimeInSeconds * 1000 - 1); - SetUp(); // Reset stat proxy causes histograms to be updated. + statistics_proxy_->UpdateHistograms(10); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.ReceivedPacketsLostInPercent")); - // Two report blocks received. - statistics_proxy_->StatisticsUpdated(rtcp_stats1, kRemoteSsrc); - statistics_proxy_->StatisticsUpdated(rtcp_stats2, kRemoteSsrc); - - // Two received report blocks and min run time has passed. - fake_clock_.AdvanceTimeMilliseconds(metrics::kMinRunTimeInSeconds * 1000); + // Restart SetUp(); + + // Min run time has passed. + fake_clock_.AdvanceTimeMilliseconds(metrics::kMinRunTimeInSeconds * 1000); + statistics_proxy_->UpdateHistograms(10); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.ReceivedPacketsLostInPercent")); - EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.ReceivedPacketsLostInPercent", - (kCumLost2 - kCumLost1) * 100 / - (kExtSeqNum2 - kExtSeqNum1))); -} - -TEST_F(ReceiveStatisticsProxyTest, - PacketLossHistogramIsNotUpdatedIfLessThanTwoReportBlocksAreReceived) { - RtcpStatistics rtcp_stats1; - rtcp_stats1.packets_lost = 1; - rtcp_stats1.extended_highest_sequence_number = 10; - - // Min run time has passed but no received report block. - fake_clock_.AdvanceTimeMilliseconds(metrics::kMinRunTimeInSeconds * 1000); - SetUp(); // Reset stat proxy causes histograms to be updated. - EXPECT_EQ(0, - metrics::NumSamples("WebRTC.Video.ReceivedPacketsLostInPercent")); - - // Min run time has passed but only one received report block. - statistics_proxy_->StatisticsUpdated(rtcp_stats1, kRemoteSsrc); - fake_clock_.AdvanceTimeMilliseconds(metrics::kMinRunTimeInSeconds * 1000); - SetUp(); - EXPECT_EQ(0, - metrics::NumSamples("WebRTC.Video.ReceivedPacketsLostInPercent")); + EXPECT_EQ( + 1, metrics::NumEvents("WebRTC.Video.ReceivedPacketsLostInPercent", 10)); } TEST_F(ReceiveStatisticsProxyTest, GetStatsReportsAvSyncOffset) { @@ -667,8 +623,7 @@ TEST_F(ReceiveStatisticsProxyTest, AvSyncOffsetHistogramIsUpdated) { const double kFreqKhz = 90.0; for (int i = 0; i < kMinRequiredSamples; ++i) statistics_proxy_->OnSyncOffsetUpdated(kSyncOffsetMs, kFreqKhz); - // Histograms are updated when the statistics_proxy_ is deleted. - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.AVSyncOffsetInMs")); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.AVSyncOffsetInMs", kSyncOffsetMs)); @@ -687,7 +642,7 @@ TEST_F(ReceiveStatisticsProxyTest, RtpToNtpFrequencyOffsetHistogramIsUpdated) { fake_clock_.AdvanceTimeMilliseconds(kFreqOffsetProcessIntervalInMs); // Process interval passed, max diff: 4. statistics_proxy_->OnSyncOffsetUpdated(kSyncOffsetMs, kFreqKhz); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); // Average reported: (2 + 4) / 2 = 3. EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.RtpToNtpFreqOffsetInKhz")); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.RtpToNtpFreqOffsetInKhz", 3)); @@ -699,7 +654,7 @@ TEST_F(ReceiveStatisticsProxyTest, Vp8QpHistogramIsUpdated) { for (int i = 0; i < kMinRequiredSamples; ++i) statistics_proxy_->OnPreDecode(kVideoCodecVP8, kQp); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.Decoded.Vp8.Qp")); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.Decoded.Vp8.Qp", kQp)); } @@ -710,7 +665,7 @@ TEST_F(ReceiveStatisticsProxyTest, Vp8QpHistogramIsNotUpdatedForTooFewSamples) { for (int i = 0; i < kMinRequiredSamples - 1; ++i) statistics_proxy_->OnPreDecode(kVideoCodecVP8, kQp); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.Decoded.Vp8.Qp")); } @@ -718,7 +673,7 @@ TEST_F(ReceiveStatisticsProxyTest, Vp8QpHistogramIsNotUpdatedIfNoQpValue) { for (int i = 0; i < kMinRequiredSamples; ++i) statistics_proxy_->OnPreDecode(kVideoCodecVP8, -1); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.Decoded.Vp8.Qp")); } @@ -735,7 +690,7 @@ TEST_F(ReceiveStatisticsProxyTest, EXPECT_EQ(kMinRequiredSamples - 1, statistics_proxy_->GetStats().frame_counts.delta_frames); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.KeyFramesReceivedInPermille")); } @@ -752,7 +707,7 @@ TEST_F(ReceiveStatisticsProxyTest, EXPECT_EQ(kMinRequiredSamples, statistics_proxy_->GetStats().frame_counts.delta_frames); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.KeyFramesReceivedInPermille")); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.KeyFramesReceivedInPermille", 0)); @@ -774,7 +729,7 @@ TEST_F(ReceiveStatisticsProxyTest, KeyFrameHistogramIsUpdated) { EXPECT_EQ(kMinRequiredSamples, statistics_proxy_->GetStats().frame_counts.delta_frames); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.KeyFramesReceivedInPermille")); EXPECT_EQ( 1, metrics::NumEvents("WebRTC.Video.KeyFramesReceivedInPermille", 500)); @@ -794,7 +749,7 @@ TEST_F(ReceiveStatisticsProxyTest, TimingHistogramsNotUpdatedForTooFewSamples) { kMinPlayoutDelayMs, kRenderDelayMs); } - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.DecodeTimeInMs")); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.JitterBufferDelayInMs")); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.TargetDelayInMs")); @@ -816,7 +771,7 @@ TEST_F(ReceiveStatisticsProxyTest, TimingHistogramsAreUpdated) { kMinPlayoutDelayMs, kRenderDelayMs); } - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.JitterBufferDelayInMs")); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.TargetDelayInMs")); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.CurrentDelayInMs")); @@ -872,7 +827,7 @@ TEST_F(ReceiveStatisticsProxyTest, for (int i = 0; i < kMinRequiredSamples - 1; ++i) statistics_proxy_->OnRenderedFrame(CreateFrame(kWidth, kHeight)); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.ReceivedWidthInPixels")); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.ReceivedHeightInPixels")); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.RenderFramesPerSecond")); @@ -883,7 +838,7 @@ TEST_F(ReceiveStatisticsProxyTest, ReceivedFrameHistogramsAreUpdated) { for (int i = 0; i < kMinRequiredSamples; ++i) statistics_proxy_->OnRenderedFrame(CreateFrame(kWidth, kHeight)); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.ReceivedWidthInPixels")); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.ReceivedHeightInPixels")); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.RenderFramesPerSecond")); @@ -905,7 +860,7 @@ TEST_F(ReceiveStatisticsProxyTest, ZeroDelayReportedIfFrameNotDelayed) { // Min run time has passed. fake_clock_.AdvanceTimeMilliseconds((metrics::kMinRunTimeInSeconds * 1000)); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.DelayedFramesToRenderer")); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.DelayedFramesToRenderer", 0)); EXPECT_EQ(0, metrics::NumSamples( @@ -925,7 +880,7 @@ TEST_F(ReceiveStatisticsProxyTest, // Min run time has not passed. fake_clock_.AdvanceTimeMilliseconds((metrics::kMinRunTimeInSeconds * 1000) - 1); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.DelayedFramesToRenderer")); EXPECT_EQ(0, metrics::NumSamples( "WebRTC.Video.DelayedFramesToRenderer_AvgDelayInMs")); @@ -939,7 +894,7 @@ TEST_F(ReceiveStatisticsProxyTest, // Min run time has passed. No rendered frames. fake_clock_.AdvanceTimeMilliseconds((metrics::kMinRunTimeInSeconds * 1000)); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.DelayedFramesToRenderer")); EXPECT_EQ(0, metrics::NumSamples( "WebRTC.Video.DelayedFramesToRenderer_AvgDelayInMs")); @@ -956,7 +911,7 @@ TEST_F(ReceiveStatisticsProxyTest, DelayReportedIfFrameIsDelayed) { // Min run time has passed. fake_clock_.AdvanceTimeMilliseconds((metrics::kMinRunTimeInSeconds * 1000)); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.DelayedFramesToRenderer")); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.DelayedFramesToRenderer", 100)); EXPECT_EQ(1, metrics::NumSamples( @@ -979,7 +934,7 @@ TEST_F(ReceiveStatisticsProxyTest, AverageDelayOfDelayedFramesIsReported) { // Min run time has passed. fake_clock_.AdvanceTimeMilliseconds((metrics::kMinRunTimeInSeconds * 1000)); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.DelayedFramesToRenderer")); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.DelayedFramesToRenderer", 50)); EXPECT_EQ(1, metrics::NumSamples( @@ -997,7 +952,7 @@ TEST_F(ReceiveStatisticsProxyTest, RtcpPacketTypeCounter counter; statistics_proxy_->RtcpPacketTypesCounterUpdated(kRemoteSsrc, counter); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.FirPacketsSentPerMinute")); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.PliPacketsSentPerMinute")); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.NackPacketsSentPerMinute")); @@ -1017,7 +972,7 @@ TEST_F(ReceiveStatisticsProxyTest, RtcpHistogramsAreUpdated) { counter.nack_packets = kNackPackets; statistics_proxy_->RtcpPacketTypesCounterUpdated(kRemoteSsrc, counter); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.FirPacketsSentPerMinute")); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.PliPacketsSentPerMinute")); EXPECT_EQ(1, metrics::NumSamples("WebRTC.Video.NackPacketsSentPerMinute")); @@ -1109,7 +1064,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, InterFrameDelaysAreReported) { fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); const int kExpectedInterFrame = (kInterFrameDelayMs * (kMinRequiredSamples - 1) + kInterFrameDelayMs * 2) / @@ -1148,7 +1103,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, fake_clock_.AdvanceTimeMilliseconds(10 * kInterFrameDelayMs); statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); const int kExpectedInterFrame = kInterFrameDelayMs * 2; if (videocontenttypehelpers::IsScreenshare(content_type_)) { EXPECT_EQ(kExpectedInterFrame, @@ -1173,7 +1128,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, // |kMinRequiredSamples| samples, and thereby intervals, is required. That // means we're one frame short of having a valid data set. - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.InterframeDelayInMs")); EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.InterframeDelayMaxInMs")); EXPECT_EQ( @@ -1202,7 +1157,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, MaxInterFrameDelayOnlyWithPause) { fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); if (videocontenttypehelpers::IsScreenshare(content_type_)) { EXPECT_EQ( 1, metrics::NumSamples("WebRTC.Video.Screenshare.InterframeDelayInMs")); @@ -1241,7 +1196,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, FreezesAreReported) { statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); statistics_proxy_->OnRenderedFrame(frame); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); const int kExpectedTimeBetweenFreezes = kInterFrameDelayMs * (kMinRequiredSamples - 1); const int kExpectedNumberFreezesPerMinute = 60 * 1000 / kCallDurationMs; @@ -1291,7 +1246,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, HarmonicFrameRateIsReported) { statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type_); statistics_proxy_->OnRenderedFrame(frame); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); double kSumSquaredFrameDurationSecs = (kMinRequiredSamples - 1) * (kFrameDurationMs / 1000.0 * kFrameDurationMs / 1000.0); @@ -1331,7 +1286,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, PausesAreIgnored) { fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); } - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); // Average of two playback intervals. const int kExpectedTimeBetweenFreezes = kInterFrameDelayMs * kMinRequiredSamples * 2; @@ -1364,7 +1319,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, ManyPausesAtTheBeginning) { fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); } - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); // No freezes should be detected, as all long inter-frame delays were pauses. if (videocontenttypehelpers::IsScreenshare(content_type_)) { EXPECT_EQ(-1, metrics::MinSample( @@ -1396,7 +1351,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, TimeInHdReported) { // Extra last frame. statistics_proxy_->OnRenderedFrame(frame_sd); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); const int kExpectedTimeInHdPercents = 33; if (videocontenttypehelpers::IsScreenshare(content_type_)) { EXPECT_EQ( @@ -1430,7 +1385,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, TimeInBlockyVideoReported) { statistics_proxy_->OnDecodedFrame(frame, kHighQp, 0, content_type_); statistics_proxy_->OnRenderedFrame(frame); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); const int kExpectedTimeInHdPercents = 66; if (videocontenttypehelpers::IsScreenshare(content_type_)) { EXPECT_EQ(kExpectedTimeInHdPercents, @@ -1463,7 +1418,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, DownscalesReported) { statistics_proxy_->OnRenderedFrame(frame_ld); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); const int kExpectedDownscales = 30; // 2 per 4 seconds = 30 per minute. if (videocontenttypehelpers::IsScreenshare(content_type_)) { EXPECT_EQ( @@ -1488,7 +1443,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, DecodeTimeReported) { statistics_proxy_->OnDecodedFrame(frame, kLowQp, kDecodeMs, content_type_); fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); } - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); EXPECT_EQ(1, metrics::NumEvents("WebRTC.Video.DecodeTimeInMs", kDecodeMs)); } @@ -1512,7 +1467,7 @@ TEST_P(ReceiveStatisticsProxyTestWithContent, fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs2); statistics_proxy_->OnDecodedFrame(frame, absl::nullopt, 0, content_type); } - statistics_proxy_.reset(); + statistics_proxy_->UpdateHistograms(absl::nullopt); if (videocontenttypehelpers::IsScreenshare(content_type)) { EXPECT_EQ( diff --git a/video/video_quality_observer.cc b/video/video_quality_observer.cc index 86f8c3ada9..9f069235b1 100644 --- a/video/video_quality_observer.cc +++ b/video/video_quality_observer.cc @@ -52,10 +52,6 @@ VideoQualityObserver::VideoQualityObserver(VideoContentType content_type) content_type_(content_type), is_paused_(false) {} -VideoQualityObserver::~VideoQualityObserver() { - UpdateHistograms(); -} - void VideoQualityObserver::UpdateHistograms() { // Don't report anything on an empty video stream. if (num_frames_rendered_ == 0) { diff --git a/video/video_quality_observer.h b/video/video_quality_observer.h index 83ef0c852e..6494a6f43c 100644 --- a/video/video_quality_observer.h +++ b/video/video_quality_observer.h @@ -32,7 +32,7 @@ class VideoQualityObserver { // Use either VideoQualityObserver::kBlockyQpThresholdVp8 or // VideoQualityObserver::kBlockyQpThresholdVp9. explicit VideoQualityObserver(VideoContentType content_type); - ~VideoQualityObserver(); + ~VideoQualityObserver() = default; void OnDecodedFrame(const VideoFrame& frame, absl::optional qp, @@ -49,13 +49,13 @@ class VideoQualityObserver { uint32_t TotalFramesDurationMs() const; double SumSquaredFrameDurationsSec() const; + void UpdateHistograms(); + static const uint32_t kMinFrameSamplesToDetectFreeze; static const uint32_t kMinIncreaseForFreezeMs; static const uint32_t kAvgInterframeDelaysWindowSizeFrames; private: - void UpdateHistograms(); - enum Resolution { Low = 0, Medium = 1, diff --git a/video/video_receive_stream.cc b/video/video_receive_stream.cc index a530bcf245..ec9de913ac 100644 --- a/video/video_receive_stream.cc +++ b/video/video_receive_stream.cc @@ -449,6 +449,8 @@ void VideoReceiveStream::Stop() { // running. for (const Decoder& decoder : config_.decoders) video_receiver_.RegisterExternalDecoder(nullptr, decoder.payload_type); + + UpdateHistograms(); } video_stream_decoder_.reset(); @@ -460,6 +462,16 @@ VideoReceiveStream::Stats VideoReceiveStream::GetStats() const { return stats_proxy_.GetStats(); } +void VideoReceiveStream::UpdateHistograms() { + absl::optional fraction_lost; + StreamStatistician* statistician = + rtp_receive_statistics_->GetStatistician(config_.rtp.remote_ssrc); + if (statistician) { + fraction_lost = statistician->GetFractionLostInPercent(); + } + stats_proxy_.UpdateHistograms(fraction_lost); +} + void VideoReceiveStream::AddSecondarySink(RtpPacketSinkInterface* sink) { rtp_video_stream_receiver_.AddSecondarySink(sink); } diff --git a/video/video_receive_stream.h b/video/video_receive_stream.h index 869b111b5d..06713ddad3 100644 --- a/video/video_receive_stream.h +++ b/video/video_receive_stream.h @@ -143,6 +143,8 @@ class VideoReceiveStream : public webrtc::VideoReceiveStream, RTC_EXCLUSIVE_LOCKS_REQUIRED(playout_delay_lock_); void RequestKeyFrame(); + void UpdateHistograms(); + SequenceChecker worker_sequence_checker_; SequenceChecker module_process_sequence_checker_; SequenceChecker network_sequence_checker_;