From da843fee1a420542d579f6aa2910ccb7c2f2ccb6 Mon Sep 17 00:00:00 2001 From: Johannes Kron Date: Thu, 23 Jun 2022 13:18:26 +0200 Subject: [PATCH] Remove WebRTC.Video.DecodeTimePerFrameInMs. histograms The decode time per frame and codec profile histograms were added temporarily to make it possible to get an overview of the decode time distributions. This fine grained information is not needed longer and the histograms can be deleted. Bug: chromium:1007526 Change-Id: Ie59627a88813e0710700cf0e13eedd6627010266 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/266496 Reviewed-by: Ilya Nikolaevskiy Commit-Queue: Johannes Kron Cr-Commit-Position: refs/heads/main@{#37316} --- video/receive_statistics_proxy2.cc | 71 +--------- video/receive_statistics_proxy2.h | 9 +- video/receive_statistics_proxy2_unittest.cc | 135 +------------------- video/video_receive_stream2.cc | 5 +- 4 files changed, 7 insertions(+), 213 deletions(-) diff --git a/video/receive_statistics_proxy2.cc b/video/receive_statistics_proxy2.cc index 286ef62fe9..66dfe9cf80 100644 --- a/video/receive_statistics_proxy2.cc +++ b/video/receive_statistics_proxy2.cc @@ -97,15 +97,11 @@ bool IsCurrentTaskQueueOrThread(TaskQueueBase* task_queue) { } // namespace -ReceiveStatisticsProxy::ReceiveStatisticsProxy( - uint32_t remote_ssrc, - Clock* clock, - TaskQueueBase* worker_thread, - const FieldTrialsView& field_trials) +ReceiveStatisticsProxy::ReceiveStatisticsProxy(uint32_t remote_ssrc, + Clock* clock, + TaskQueueBase* worker_thread) : clock_(clock), start_ms_(clock->TimeInMilliseconds()), - enable_decode_time_histograms_( - !field_trials.IsEnabled("WebRTC-DecodeTimeHistogramsKillSwitch")), last_sample_time_(clock->TimeInMilliseconds()), fps_threshold_(kLowFpsThreshold, kHighFpsThreshold, @@ -583,63 +579,6 @@ void ReceiveStatisticsProxy::UpdateFramerate(int64_t now_ms) const { stats_.network_frame_rate = static_cast(framerate); } -void ReceiveStatisticsProxy::UpdateDecodeTimeHistograms( - int width, - int height, - int decode_time_ms) const { - RTC_DCHECK_RUN_ON(&main_thread_); - - bool is_4k = (width == 3840 || width == 4096) && height == 2160; - bool is_hd = width == 1920 && height == 1080; - // Only update histograms for 4k/HD and VP9/H264. - if ((is_4k || is_hd) && (last_codec_type_ == kVideoCodecVP9 || - last_codec_type_ == kVideoCodecH264)) { - const std::string kDecodeTimeUmaPrefix = - "WebRTC.Video.DecodeTimePerFrameInMs."; - - // Each histogram needs its own line for it to not be reused in the wrong - // way when the format changes. - if (last_codec_type_ == kVideoCodecVP9) { - bool is_sw_decoder = - stats_.decoder_implementation_name.compare(0, 6, "libvpx") == 0; - if (is_4k) { - if (is_sw_decoder) - RTC_HISTOGRAM_COUNTS_1000(kDecodeTimeUmaPrefix + "Vp9.4k.Sw", - decode_time_ms); - else - RTC_HISTOGRAM_COUNTS_1000(kDecodeTimeUmaPrefix + "Vp9.4k.Hw", - decode_time_ms); - } else { - if (is_sw_decoder) - RTC_HISTOGRAM_COUNTS_1000(kDecodeTimeUmaPrefix + "Vp9.Hd.Sw", - decode_time_ms); - else - RTC_HISTOGRAM_COUNTS_1000(kDecodeTimeUmaPrefix + "Vp9.Hd.Hw", - decode_time_ms); - } - } else { - bool is_sw_decoder = - stats_.decoder_implementation_name.compare(0, 6, "FFmpeg") == 0; - if (is_4k) { - if (is_sw_decoder) - RTC_HISTOGRAM_COUNTS_1000(kDecodeTimeUmaPrefix + "H264.4k.Sw", - decode_time_ms); - else - RTC_HISTOGRAM_COUNTS_1000(kDecodeTimeUmaPrefix + "H264.4k.Hw", - decode_time_ms); - - } else { - if (is_sw_decoder) - RTC_HISTOGRAM_COUNTS_1000(kDecodeTimeUmaPrefix + "H264.Hd.Sw", - decode_time_ms); - else - RTC_HISTOGRAM_COUNTS_1000(kDecodeTimeUmaPrefix + "H264.Hd.Hw", - decode_time_ms); - } - } - } -} - absl::optional ReceiveStatisticsProxy::GetCurrentEstimatedPlayoutNtpTimestampMs( int64_t now_ms) const { @@ -911,10 +850,6 @@ void ReceiveStatisticsProxy::OnDecodedFrame( if (!assembly_time.IsZero()) { ++stats_.frames_assembled_from_multiple_packets; } - if (enable_decode_time_histograms_) { - UpdateDecodeTimeHistograms(frame_meta.width, frame_meta.height, - decode_time.ms()); - } last_content_type_ = content_type; decode_fps_estimator_.Update(1, frame_meta.decode_timestamp.ms()); diff --git a/video/receive_statistics_proxy2.h b/video/receive_statistics_proxy2.h index 657920f539..14f3421600 100644 --- a/video/receive_statistics_proxy2.h +++ b/video/receive_statistics_proxy2.h @@ -17,7 +17,6 @@ #include #include "absl/types/optional.h" -#include "api/field_trials_view.h" #include "api/sequence_checker.h" #include "api/task_queue/pending_task_safety_flag.h" #include "api/task_queue/task_queue_base.h" @@ -51,8 +50,7 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, public: ReceiveStatisticsProxy(uint32_t remote_ssrc, Clock* clock, - TaskQueueBase* worker_thread, - const FieldTrialsView& field_trials); + TaskQueueBase* worker_thread); ~ReceiveStatisticsProxy() override; VideoReceiveStreamInterface::Stats GetStats() const; @@ -148,16 +146,11 @@ class ReceiveStatisticsProxy : public VCMReceiveStatisticsCallback, // Removes info about old frames and then updates the framerate. void UpdateFramerate(int64_t now_ms) const; - void UpdateDecodeTimeHistograms(int width, - int height, - int decode_time_ms) const; - absl::optional GetCurrentEstimatedPlayoutNtpTimestampMs( int64_t now_ms) const; Clock* const clock_; const int64_t start_ms_; - const bool enable_decode_time_histograms_; int64_t last_sample_time_ RTC_GUARDED_BY(main_thread_); diff --git a/video/receive_statistics_proxy2_unittest.cc b/video/receive_statistics_proxy2_unittest.cc index afcba15dbe..fe468f612f 100644 --- a/video/receive_statistics_proxy2_unittest.cc +++ b/video/receive_statistics_proxy2_unittest.cc @@ -43,11 +43,10 @@ const int kHeight = 720; // TODO(sakal): ReceiveStatisticsProxy is lacking unittesting. class ReceiveStatisticsProxy2Test : public ::testing::Test { public: - explicit ReceiveStatisticsProxy2Test(std::string field_trials = "") - : field_trials_(field_trials), fake_clock_(1234) { + ReceiveStatisticsProxy2Test() : fake_clock_(1234) { metrics::Reset(); statistics_proxy_.reset(new ReceiveStatisticsProxy( - kRemoteSsrc, &fake_clock_, loop_.task_queue(), field_trials_)); + kRemoteSsrc, &fake_clock_, loop_.task_queue())); } ~ReceiveStatisticsProxy2Test() override { statistics_proxy_.reset(); } @@ -1864,135 +1863,5 @@ TEST_P(ReceiveStatisticsProxy2TestWithContent, } } -class ReceiveStatisticsProxy2TestWithDecodeTimeHistograms - : public ::testing::WithParamInterface< - std::tuple>, - public ReceiveStatisticsProxy2Test { - public: - ReceiveStatisticsProxy2TestWithDecodeTimeHistograms() - : ReceiveStatisticsProxy2Test( - std::get<0>(GetParam()) - ? "WebRTC-DecodeTimeHistogramsKillSwitch/Enabled/" - : "") {} - - protected: - const std::string kUmaPrefix = "WebRTC.Video.DecodeTimePerFrameInMs."; - const int expected_number_of_samples_ = {std::get<1>(GetParam())}; - const int width_ = {std::get<2>(GetParam())}; - const int height_ = {std::get<3>(GetParam())}; - const VideoCodecType codec_type_ = {std::get<4>(GetParam())}; - const std::string implementation_name_ = {std::get<5>(GetParam())}; - const std::string uma_histogram_name_ = - kUmaPrefix + (codec_type_ == kVideoCodecVP9 ? "Vp9." : "H264.") + - (height_ == 2160 ? "4k." : "Hd.") + - (implementation_name_.compare("ExternalDecoder") == 0 ? "Hw" : "Sw"); -}; - -TEST_P(ReceiveStatisticsProxy2TestWithDecodeTimeHistograms, - DecodeTimeHistogramsUpdated) { - constexpr int kNumberOfFrames = 10; - constexpr int kDecodeTimeMs = 7; - constexpr int kFrameDurationMs = 1000 / 60; - - webrtc::VideoFrame frame = CreateFrame(width_, height_); - - statistics_proxy_->OnDecoderImplementationName(implementation_name_.c_str()); - statistics_proxy_->OnPreDecode(codec_type_, /*qp=*/0); - - for (int i = 0; i < kNumberOfFrames; ++i) { - statistics_proxy_->OnDecodedFrame(frame, /*qp=*/absl::nullopt, - TimeDelta::Millis(kDecodeTimeMs), - VideoContentType::UNSPECIFIED); - fake_clock_.AdvanceTimeMilliseconds(kFrameDurationMs); - } - - loop_.Flush(); - - EXPECT_METRIC_EQ(expected_number_of_samples_, - metrics::NumSamples(uma_histogram_name_)); - EXPECT_METRIC_EQ(expected_number_of_samples_, - metrics::NumEvents(uma_histogram_name_, kDecodeTimeMs)); -} - -const auto kVp94kHw = std::make_tuple(/*killswitch=*/false, - /*expected_number_of_samples=*/10, - /*width=*/3840, - /*height=*/2160, - kVideoCodecVP9, - /*implementation=*/"ExternalDecoder"); -const auto kVp94kSw = std::make_tuple(/*killswitch=*/false, - /*expected_number_of_samples=*/10, - /*width=*/3840, - /*height=*/2160, - kVideoCodecVP9, - /*implementation=*/"libvpx"); -const auto kVp9HdHw = std::make_tuple(/*killswitch=*/false, - /*expected_number_of_samples=*/10, - /*width=*/1920, - /*height=*/1080, - kVideoCodecVP9, - /*implementation=*/"ExternalDecoder"); -const auto kVp9HdSw = std::make_tuple(/*killswitch=*/false, - /*expected_number_of_samples=*/10, - /*width=*/1920, - /*height=*/1080, - kVideoCodecVP9, - /*implementation=*/"libvpx"); -const auto kH2644kHw = std::make_tuple(/*killswitch=*/false, - /*expected_number_of_samples=*/10, - /*width=*/3840, - /*height=*/2160, - kVideoCodecH264, - /*implementation=*/"ExternalDecoder"); -const auto kH2644kSw = std::make_tuple(/*killswitch=*/false, - /*expected_number_of_samples=*/10, - /*width=*/3840, - /*height=*/2160, - kVideoCodecH264, - /*implementation=*/"FFmpeg"); -const auto kH264HdHw = std::make_tuple(/*killswitch=*/false, - /*expected_number_of_samples=*/10, - /*width=*/1920, - /*height=*/1080, - kVideoCodecH264, - /*implementation=*/"ExternalDecoder"); -const auto kH264HdSw = std::make_tuple(/*killswitch=*/false, - /*expected_number_of_samples=*/10, - /*width=*/1920, - /*height=*/1080, - kVideoCodecH264, - /*implementation=*/"FFmpeg"); - -INSTANTIATE_TEST_SUITE_P(AllHistogramsPopulated, - ReceiveStatisticsProxy2TestWithDecodeTimeHistograms, - ::testing::Values(kVp94kHw, - kVp94kSw, - kVp9HdHw, - kVp9HdSw, - kH2644kHw, - kH2644kSw, - kH264HdHw, - kH264HdSw)); - -const auto kKillswitchDisabled = - std::make_tuple(/*killswitch=*/false, - /*expected_number_of_samples=*/10, - /*width=*/1920, - /*height=*/1080, - kVideoCodecVP9, - /*implementation=*/"libvpx"); -const auto kKillswitchEnabled = - std::make_tuple(/*killswitch=*/true, - /*expected_number_of_samples=*/0, - /*width=*/1920, - /*height=*/1080, - kVideoCodecVP9, - /*implementation=*/"libvpx"); - -INSTANTIATE_TEST_SUITE_P(KillswitchEffective, - ReceiveStatisticsProxy2TestWithDecodeTimeHistograms, - ::testing::Values(kKillswitchDisabled, - kKillswitchEnabled)); - } // namespace internal } // namespace webrtc diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc index 80c58c36fd..f333c446a7 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -221,10 +221,7 @@ VideoReceiveStream2::VideoReceiveStream2( clock_(clock), call_stats_(call_stats), source_tracker_(clock_), - stats_proxy_(remote_ssrc(), - clock_, - call->worker_thread(), - call->trials()), + stats_proxy_(remote_ssrc(), clock_, call->worker_thread()), rtp_receive_statistics_(ReceiveStatistics::Create(clock_)), timing_(std::move(timing)), video_receiver_(clock_, timing_.get(), call->trials()),