From 0c43f779f8ed38e02fd8bf7bef6f8bf15e8f1aad Mon Sep 17 00:00:00 2001 From: asapersson Date: Wed, 30 Nov 2016 01:42:26 -0800 Subject: [PATCH] Update video histograms that do not have a minimum lifetime limit before being recorded. Updated histograms: "WebRTC.Video.ReceivedPacketsLostInPercent" (two RTCP RR previously needed) "WebRTC.Video.ReceivedFecPacketsInPercent" (one received packet previously needed) "WebRTC.Video.RecoveredMediaPacketsInPercentOfFec" (one received FEC packet previously needed) Prevents logging stats if call was shortly in use. BUG=b/32659204 Review-Url: https://codereview.webrtc.org/2536653002 Cr-Commit-Position: refs/heads/master@{#15315} --- .../rtp_rtcp/include/ulpfec_receiver.h | 6 +- .../rtp_rtcp/source/ulpfec_receiver_impl.cc | 5 ++ .../source/ulpfec_receiver_unittest.cc | 14 ++++- webrtc/video/receive_statistics_proxy.cc | 19 ++++-- webrtc/video/receive_statistics_proxy.h | 1 + .../receive_statistics_proxy_unittest.cc | 59 +++++++++++++++++++ webrtc/video/rtp_stream_receiver.cc | 8 +++ 7 files changed, 105 insertions(+), 7 deletions(-) diff --git a/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h b/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h index fbb4b4c99c..af416f9469 100644 --- a/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h +++ b/webrtc/modules/rtp_rtcp/include/ulpfec_receiver.h @@ -18,11 +18,15 @@ namespace webrtc { struct FecPacketCounter { FecPacketCounter() - : num_packets(0), num_fec_packets(0), num_recovered_packets(0) {} + : num_packets(0), + num_fec_packets(0), + num_recovered_packets(0), + first_packet_time_ms(-1) {} size_t num_packets; // Number of received packets. size_t num_fec_packets; // Number of received FEC packets. size_t num_recovered_packets; // Number of recovered media packets using FEC. + int64_t first_packet_time_ms; // Time when first packet is received. }; class UlpfecReceiver { diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc index 8ba53b5a3c..2752a378c9 100644 --- a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc +++ b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_impl.cc @@ -17,6 +17,7 @@ #include "webrtc/base/logging.h" #include "webrtc/modules/rtp_rtcp/source/byte_io.h" #include "webrtc/modules/rtp_rtcp/source/rtp_receiver_video.h" +#include "webrtc/system_wrappers/include/clock.h" namespace webrtc { @@ -125,6 +126,10 @@ int32_t UlpfecReceiverImpl::AddReceivedRedPacket( } } ++packet_counter_.num_packets; + if (packet_counter_.first_packet_time_ms == -1) { + packet_counter_.first_packet_time_ms = + Clock::GetRealTimeClock()->TimeInMilliseconds(); + } std::unique_ptr second_received_packet; diff --git a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc index 89e91c4af7..ddca65530f 100644 --- a/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc +++ b/webrtc/modules/rtp_rtcp/source/ulpfec_receiver_unittest.cc @@ -187,11 +187,22 @@ TEST_F(UlpfecReceiverTest, TwoMediaOneFec) { std::list fec_packets; EncodeFec(media_packets, kNumFecPackets, &fec_packets); + FecPacketCounter counter = receiver_fec_->GetPacketCounter(); + EXPECT_EQ(0u, counter.num_packets); + EXPECT_EQ(-1, counter.first_packet_time_ms); + // Recovery auto it = augmented_media_packets.begin(); BuildAndAddRedMediaPacket(*it); VerifyReconstructedMediaPacket(**it, 1); EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); + counter = receiver_fec_->GetPacketCounter(); + EXPECT_EQ(1u, counter.num_packets); + EXPECT_EQ(0u, counter.num_fec_packets); + EXPECT_EQ(0u, counter.num_recovered_packets); + const int64_t first_packet_time_ms = counter.first_packet_time_ms; + EXPECT_NE(-1, first_packet_time_ms); + // Drop one media packet. auto fec_it = fec_packets.begin(); BuildAndAddRedFecPacket(*fec_it); @@ -199,10 +210,11 @@ TEST_F(UlpfecReceiverTest, TwoMediaOneFec) { VerifyReconstructedMediaPacket(**it, 1); EXPECT_EQ(0, receiver_fec_->ProcessReceivedFec()); - FecPacketCounter counter = receiver_fec_->GetPacketCounter(); + counter = receiver_fec_->GetPacketCounter(); EXPECT_EQ(2u, counter.num_packets); EXPECT_EQ(1u, counter.num_fec_packets); EXPECT_EQ(1u, counter.num_recovered_packets); + EXPECT_EQ(first_packet_time_ms, counter.first_packet_time_ms); } TEST_F(UlpfecReceiverTest, InjectGarbageFecHeaderLengthRecovery) { diff --git a/webrtc/video/receive_statistics_proxy.cc b/webrtc/video/receive_statistics_proxy.cc index 7d5fbc0a7c..15a2206589 100644 --- a/webrtc/video/receive_statistics_proxy.cc +++ b/webrtc/video/receive_statistics_proxy.cc @@ -35,7 +35,8 @@ ReceiveStatisticsProxy::ReceiveStatisticsProxy( renders_fps_estimator_(1000, 1000), render_fps_tracker_(100, 10u), render_pixel_tracker_(100, 10u), - freq_offset_counter_(clock, nullptr, kFreqOffsetProcessIntervalMs) { + freq_offset_counter_(clock, nullptr, kFreqOffsetProcessIntervalMs), + first_report_block_time_ms_(-1) { stats_.ssrc = config_.rtp.remote_ssrc; for (auto it : config_.rtp.rtx) rtx_stats_[it.second.ssrc] = StreamDataCounters(); @@ -50,11 +51,16 @@ void ReceiveStatisticsProxy::UpdateHistograms() { "WebRTC.Video.ReceiveStreamLifetimeInSeconds", (clock_->TimeInMilliseconds() - start_ms_) / 1000); - int fraction_lost = report_block_stats_.FractionLostInPercent(); - if (fraction_lost != -1) { - RTC_HISTOGRAM_PERCENTAGE("WebRTC.Video.ReceivedPacketsLostInPercent", - fraction_lost); + 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); + } } + const int kMinRequiredSamples = 200; int samples = static_cast(render_fps_tracker_.TotalSampleCount()); if (samples > kMinRequiredSamples) { @@ -233,6 +239,9 @@ void ReceiveStatisticsProxy::StatisticsUpdated( return; stats_.rtcp_stats = statistics; report_block_stats_.Store(statistics, ssrc, 0); + + if (first_report_block_time_ms_ == -1) + first_report_block_time_ms_ = clock_->TimeInMilliseconds(); } void ReceiveStatisticsProxy::CNameChanged(const char* cname, uint32_t ssrc) { diff --git a/webrtc/video/receive_statistics_proxy.h b/webrtc/video/receive_statistics_proxy.h index 445a731f24..b75ad8c093 100644 --- a/webrtc/video/receive_statistics_proxy.h +++ b/webrtc/video/receive_statistics_proxy.h @@ -123,6 +123,7 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, SampleCounter delay_counter_ GUARDED_BY(crit_); SampleCounter e2e_delay_counter_ GUARDED_BY(crit_); MaxCounter freq_offset_counter_ GUARDED_BY(crit_); + int64_t first_report_block_time_ms_ GUARDED_BY(crit_); ReportBlockStats report_block_stats_ GUARDED_BY(crit_); QpCounters qp_counters_; // Only accessed on the decoding thread. std::map rtx_stats_ GUARDED_BY(crit_); diff --git a/webrtc/video/receive_statistics_proxy_unittest.cc b/webrtc/video/receive_statistics_proxy_unittest.cc index 66856a70ad..74186f040c 100644 --- a/webrtc/video/receive_statistics_proxy_unittest.cc +++ b/webrtc/video/receive_statistics_proxy_unittest.cc @@ -12,6 +12,7 @@ #include +#include "webrtc/system_wrappers/include/metrics.h" #include "webrtc/system_wrappers/include/metrics_default.h" #include "webrtc/test/gtest.h" @@ -112,6 +113,64 @@ TEST_F(ReceiveStatisticsProxyTest, LifetimeHistogramIsUpdated) { kTimeSec)); } +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.cumulative_lost = kCumLost1; + rtcp_stats1.extended_max_sequence_number = kExtSeqNum1; + statistics_proxy_->StatisticsUpdated(rtcp_stats1, kRemoteSsrc); + + // Two report blocks received. + RtcpStatistics rtcp_stats2; + rtcp_stats2.cumulative_lost = kCumLost2; + rtcp_stats2.extended_max_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. + 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); + SetUp(); + 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.cumulative_lost = 1; + rtcp_stats1.extended_max_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")); +} + TEST_F(ReceiveStatisticsProxyTest, AvSyncOffsetHistogramIsUpdated) { const int64_t kSyncOffsetMs = 22; const double kFreqKhz = 90.0; diff --git a/webrtc/video/rtp_stream_receiver.cc b/webrtc/video/rtp_stream_receiver.cc index 2f27d8dc9c..3bbf69e9b2 100644 --- a/webrtc/video/rtp_stream_receiver.cc +++ b/webrtc/video/rtp_stream_receiver.cc @@ -620,6 +620,14 @@ bool RtpStreamReceiver::IsPacketRetransmitted(const RTPHeader& header, void RtpStreamReceiver::UpdateHistograms() { FecPacketCounter counter = ulpfec_receiver_->GetPacketCounter(); + if (counter.first_packet_time_ms == -1) + return; + + int64_t elapsed_sec = + (clock_->TimeInMilliseconds() - counter.first_packet_time_ms) / 1000; + if (elapsed_sec < metrics::kMinRunTimeInSeconds) + return; + if (counter.num_packets > 0) { RTC_HISTOGRAM_PERCENTAGE( "WebRTC.Video.ReceivedFecPacketsInPercent",