From 892dab52b6436ce00f508726729eded75a25503d Mon Sep 17 00:00:00 2001 From: sprang Date: Tue, 15 Aug 2017 05:00:33 -0700 Subject: [PATCH] Fix incorrect InterframeDelayMaxInMs histogram metrics Two bugs: 1) The max value should only be reported if the average is also reported. Otherwise the max might become lower than average. (On average). 2) When reporting that max value, actually use the max value. BUG=webrtc:7420 Review-Url: https://codereview.webrtc.org/3002593002 Cr-Commit-Position: refs/heads/master@{#19352} --- webrtc/video/receive_statistics_proxy.cc | 24 +++---- .../receive_statistics_proxy_unittest.cc | 63 ++++++++++++++++++- 2 files changed, 70 insertions(+), 17 deletions(-) diff --git a/webrtc/video/receive_statistics_proxy.cc b/webrtc/video/receive_statistics_proxy.cc index 4673707cd9..7852a05913 100644 --- a/webrtc/video/receive_statistics_proxy.cc +++ b/webrtc/video/receive_statistics_proxy.cc @@ -216,6 +216,11 @@ void ReceiveStatisticsProxy::UpdateHistograms() { if (interframe_delay_ms_screenshare != -1) { RTC_HISTOGRAM_COUNTS_10000("WebRTC.Video.Screenshare.InterframeDelayInMs", interframe_delay_ms_screenshare); + RTC_DCHECK_GE(interframe_delay_max_ms_screenshare_, + interframe_delay_ms_screenshare); + RTC_HISTOGRAM_COUNTS_10000( + "WebRTC.Video.Screenshare.InterframeDelayMaxInMs", + interframe_delay_max_ms_screenshare_); } int interframe_delay_ms_video = @@ -223,24 +228,11 @@ void ReceiveStatisticsProxy::UpdateHistograms() { if (interframe_delay_ms_video != -1) { RTC_HISTOGRAM_COUNTS_10000("WebRTC.Video.InterframeDelayInMs", interframe_delay_ms_video); + RTC_DCHECK_GE(interframe_delay_max_ms_video_, interframe_delay_ms_video); + RTC_HISTOGRAM_COUNTS_10000("WebRTC.Video.InterframeDelayMaxInMs", + interframe_delay_max_ms_video_); } - int interframe_delay_max_ms_screenshare = - interframe_delay_max_ms_screenshare_; - if (interframe_delay_max_ms_screenshare != -1) { - RTC_HISTOGRAM_COUNTS_10000( - "WebRTC.Video.Screenshare.InterframeDelayMaxInMs", - interframe_delay_ms_screenshare); - } - - int interframe_delay_max_ms_video = interframe_delay_max_ms_video_; - if (interframe_delay_max_ms_video != -1) { - RTC_HISTOGRAM_COUNTS_10000( - "WebRTC.Video.InterframeDelayMaxInMs", - interframe_delay_ms_video); - } - - StreamDataCounters rtp = stats_.rtp_stats; StreamDataCounters rtx; for (auto it : rtx_stats_) diff --git a/webrtc/video/receive_statistics_proxy_unittest.cc b/webrtc/video/receive_statistics_proxy_unittest.cc index 6112dd7fd4..d37d203548 100644 --- a/webrtc/video/receive_statistics_proxy_unittest.cc +++ b/webrtc/video/receive_statistics_proxy_unittest.cc @@ -30,7 +30,8 @@ const int kMinRequiredSamples = 200; } // namespace // TODO(sakal): ReceiveStatisticsProxy is lacking unittesting. -class ReceiveStatisticsProxyTest : public ::testing::Test { +class ReceiveStatisticsProxyTest + : public ::testing::TestWithParam { public: ReceiveStatisticsProxyTest() : fake_clock_(1234), config_(GetTestConfig()) {} virtual ~ReceiveStatisticsProxyTest() {} @@ -718,4 +719,64 @@ TEST_F(ReceiveStatisticsProxyTest, RtcpHistogramsAreUpdated) { kNackPackets * 60 / metrics::kMinRunTimeInSeconds)); } +INSTANTIATE_TEST_CASE_P(ContentTypes, + ReceiveStatisticsProxyTest, + ::testing::Values(VideoContentType::UNSPECIFIED, + VideoContentType::SCREENSHARE)); + +TEST_P(ReceiveStatisticsProxyTest, InterFrameDelaysAreReported) { + const VideoContentType content_type = GetParam(); + const int kInterFrameDelayMs = 33; + for (int i = 0; i < kMinRequiredSamples; ++i) { + statistics_proxy_->OnDecodedFrame(rtc::Optional(), content_type); + fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); + } + // One extra with with double the interval. + fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); + statistics_proxy_->OnDecodedFrame(rtc::Optional(), content_type); + + statistics_proxy_.reset(); + const int kExpectedInterFrame = + (kInterFrameDelayMs * (kMinRequiredSamples - 1) + + kInterFrameDelayMs * 2) / + kMinRequiredSamples; + switch (content_type) { + case VideoContentType::UNSPECIFIED: + EXPECT_EQ(kExpectedInterFrame, + metrics::MinSample("WebRTC.Video.InterframeDelayInMs")); + EXPECT_EQ(kInterFrameDelayMs * 2, + metrics::MinSample("WebRTC.Video.InterframeDelayMaxInMs")); + break; + case VideoContentType::SCREENSHARE: + EXPECT_EQ( + kExpectedInterFrame, + metrics::MinSample("WebRTC.Video.Screenshare.InterframeDelayInMs")); + EXPECT_EQ(kInterFrameDelayMs * 2, + metrics::MinSample( + "WebRTC.Video.Screenshare.InterframeDelayMaxInMs")); + break; + default: + RTC_NOTREACHED(); + } +} + +TEST_P(ReceiveStatisticsProxyTest, MaxInterFrameDelayOnlyWithValidAverage) { + const VideoContentType content_type = GetParam(); + const int kInterFrameDelayMs = 33; + for (int i = 0; i < kMinRequiredSamples; ++i) { + statistics_proxy_->OnDecodedFrame(rtc::Optional(), content_type); + fake_clock_.AdvanceTimeMilliseconds(kInterFrameDelayMs); + } + + // |kMinRequiredSamples| samples, and thereby intervals, is required. That + // means we're one frame short of having a valid data set. + statistics_proxy_.reset(); + EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.InterframeDelayInMs")); + EXPECT_EQ(0, metrics::NumSamples("WebRTC.Video.InterframeDelayMaxInMs")); + EXPECT_EQ( + 0, metrics::NumSamples("WebRTC.Video.Screenshare.InterframeDelayInMs")); + EXPECT_EQ(0, metrics::NumSamples( + "WebRTC.Video.Screenshare.InterframeDelayMaxInMs")); +} + } // namespace webrtc