From 9a9f18a736685cafbf56da794393d6da3b0eba52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Niels=20M=C3=B6ller?= Date: Fri, 2 Aug 2019 13:52:37 +0200 Subject: [PATCH] Get WebRTC.Video.ReceivedPacketsLostInPercent from ReceiveStatistics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Old way to produce this histogram was based on RtcpStatisticsCallback reporting sent RTCP messages, with some additional processing by the ReportBlockStats class. After this cl, to grand average fraction loss is computed by StreamStatistician, queried by VideoReceiveStream when the stream is closed down, and passed to ReceiveStatisticsProxy which produces histograms. This is a preparation for deleting the RtcpStatisticsCallback from ReceiveStatistics. Bug: webrtc:10679 Change-Id: Ie37062c1ae590fd92d3bd0f94c510e135ab93e8d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/147722 Reviewed-by: Åsa Persson Reviewed-by: Danil Chapovalov Commit-Queue: Niels Moller Cr-Commit-Position: refs/heads/master@{#28747} --- modules/rtp_rtcp/include/receive_statistics.h | 4 + .../source/receive_statistics_impl.cc | 14 ++ .../rtp_rtcp/source/receive_statistics_impl.h | 1 + .../source/receive_statistics_unittest.cc | 47 +++--- video/receive_statistics_proxy.cc | 48 +++---- video/receive_statistics_proxy.h | 11 +- video/receive_statistics_proxy_unittest.cc | 135 ++++++------------ video/video_quality_observer.cc | 4 - video/video_quality_observer.h | 6 +- video/video_receive_stream.cc | 12 ++ video/video_receive_stream.h | 2 + 11 files changed, 135 insertions(+), 149 deletions(-) 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_;