From e680c83a412bad3edbdf4abe14da1d39b9c05506 Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Thu, 25 Apr 2019 13:38:49 +0000 Subject: [PATCH] Revert "Add Video Bwe stats collection to DefaultVideoQualityAnalyzer." This reverts commit 8848229234aae01ec19582ece7b748d557119d66. Reason for revert: break chromium compilation on iOS https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8915214519549611184/+/steps/compile/0/stdout Original change's description: > Add Video Bwe stats collection to DefaultVideoQualityAnalyzer. > > This CL adds the possibility to collect the following Video BWE stats: > - available_send_bandwidth > - transmission_bitrate > - retransmission_bitrate > - actual_encode_bitrate > - target_encode_bitrate > > Example of the output: > > RESULT available_send_bandwidth: smoke_test/alice= {487754.33,87583.093} bytesPerSecond > RESULT transmission_bitrate: smoke_test/alice= {465779.17,212075.5} bytesPerSecond > RESULT retransmission_bitrate: smoke_test/alice= {20036,26326.751} bytesPerSecond > RESULT actual_encode_bitrate: smoke_test/alice= {418779.33,200486.03} bytesPerSecond > RESULT target_encode_bitrate: smoke_test/alice= {469491.17,77866.909} bytesPerSecond > RESULT available_send_bandwidth: smoke_test/bob= {642924.83,168842.34} bytesPerSecond > RESULT transmission_bitrate: smoke_test/bob= {626115.5,294783.56} bytesPerSecond > RESULT retransmission_bitrate: smoke_test/bob= {0,0} bytesPerSecond > RESULT actual_encode_bitrate: smoke_test/bob= {594235.33,297289.54} bytesPerSecond > RESULT target_encode_bitrate: smoke_test/bob= {640463.5,167676.66} bytesPerSecond > > Bug: webrtc:10138 > Change-Id: I0414055af0010b8fb4d909297e6da86d398157c2 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/132703 > Commit-Queue: Artem Titov > Reviewed-by: Tommi > Reviewed-by: Artem Titov > Reviewed-by: Mirko Bonadei > Reviewed-by: Mirko Bonadei > Reviewed-by: Ilya Nikolaevskiy > Cr-Commit-Position: refs/heads/master@{#27760} TBR=mbonadei@webrtc.org,mbonadei@google.com,ilnik@webrtc.org,tommi@webrtc.org,titovartem@webrtc.org Change-Id: Ib0ef94331410d9b22b6425e4da412b75360fa2d9 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: webrtc:10138 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/134210 Reviewed-by: Artem Titov Commit-Queue: Artem Titov Cr-Commit-Position: refs/heads/master@{#27771} --- test/DEPS | 4 - test/pc/e2e/BUILD.gn | 24 ------ .../video/default_video_quality_analyzer.cc | 81 +------------------ .../video/default_video_quality_analyzer.h | 33 ++------ 4 files changed, 10 insertions(+), 132 deletions(-) diff --git a/test/DEPS b/test/DEPS index 80714fd563..80c47191d6 100644 --- a/test/DEPS +++ b/test/DEPS @@ -20,10 +20,6 @@ include_rules = [ "+sdk", "+system_wrappers", "+third_party/libyuv", - - # This should probably go into //DEPS but at the moment we don't know - # yet if this is OK to use in production code. - "+absl/container/flat_hash_map.h", ] specific_include_rules = { diff --git a/test/pc/e2e/BUILD.gn b/test/pc/e2e/BUILD.gn index f7c90b98c3..1f398f24e5 100644 --- a/test/pc/e2e/BUILD.gn +++ b/test/pc/e2e/BUILD.gn @@ -161,17 +161,6 @@ rtc_source_set("quality_analyzing_video_encoder") { ] } -# TODO(mbonadei): Remove as soon as -Wundef and -Wextra-semi problems are -# fixed upstream (CLs are under review). -config("peerconnection_quality_test_cflags") { - if (is_clang) { - cflags = [ - "-Wno-undef", - "-Wno-extra-semi", - ] - } -} - if (rtc_include_tests) { rtc_source_set("video_quality_analyzer_injection_helper") { visibility = [ "*" ] @@ -236,10 +225,6 @@ if (rtc_include_tests) { rtc_source_set("peerconnection_quality_test") { visibility = [ "*" ] testonly = true - - # TODO(mbonadei): Remove as soon as -Wundef and -Wextra-semi problems are - # fixed upstream (CLs are under review). - configs += [ ":peerconnection_quality_test_cflags" ] sources = [ "peer_connection_quality_test.cc", "peer_connection_quality_test.h", @@ -321,10 +306,6 @@ if (rtc_include_tests) { rtc_source_set("peer_connection_e2e_smoke_test") { testonly = true - - # TODO(mbonadei): Remove as soon as -Wundef and -Wextra-semi problems are - # fixed upstream (CLs are under review). - configs += [ ":peerconnection_quality_test_cflags" ] sources = [ "peer_connection_e2e_smoke_test.cc", ] @@ -433,10 +414,6 @@ rtc_source_set("example_video_quality_analyzer") { rtc_source_set("default_video_quality_analyzer") { visibility = [ "*" ] - - # TODO(mbonadei): Remove as soon as -Wundef and -Wextra-semi problems are - # fixed upstream (CLs are under review). - configs += [ ":peerconnection_quality_test_cflags" ] testonly = true sources = [ "analyzer/video/default_video_quality_analyzer.cc", @@ -457,7 +434,6 @@ rtc_source_set("default_video_quality_analyzer") { "../../../rtc_base:rtc_event", "../../../rtc_base:rtc_numerics", "../../../system_wrappers", - "//third_party/abseil-cpp/absl/container:flat_hash_map", "//third_party/abseil-cpp/absl/memory", ] } diff --git a/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc b/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc index d37ef5f7d5..dff5155dd6 100644 --- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc +++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc @@ -352,59 +352,6 @@ AnalyzerStats DefaultVideoQualityAnalyzer::GetAnalyzerStats() const { return analyzer_stats_; } -// TODO(bugs.webrtc.org/10430): Migrate to the new GetStats as soon as -// bugs.webrtc.org/10428 is fixed. -void DefaultVideoQualityAnalyzer::OnStatsReports( - absl::string_view pc_label, - const StatsReports& stats_reports) { - for (const StatsReport* stats_report : stats_reports) { - // The only stats collected by this analyzer are present in - // kStatsReportTypeBwe reports, so all other reports are just ignored. - if (stats_report->type() != StatsReport::StatsType::kStatsReportTypeBwe) { - continue; - } - const webrtc::StatsReport::Value* available_send_bandwidth = - stats_report->FindValue( - StatsReport::StatsValueName::kStatsValueNameAvailableSendBandwidth); - const webrtc::StatsReport::Value* retransmission_bitrate = - stats_report->FindValue( - StatsReport::StatsValueName::kStatsValueNameRetransmitBitrate); - const webrtc::StatsReport::Value* transmission_bitrate = - stats_report->FindValue( - StatsReport::StatsValueName::kStatsValueNameTransmitBitrate); - const webrtc::StatsReport::Value* actual_encode_bitrate = - stats_report->FindValue( - StatsReport::StatsValueName::kStatsValueNameActualEncBitrate); - const webrtc::StatsReport::Value* target_encode_bitrate = - stats_report->FindValue( - StatsReport::StatsValueName::kStatsValueNameTargetEncBitrate); - RTC_CHECK(available_send_bandwidth); - RTC_CHECK(retransmission_bitrate); - RTC_CHECK(transmission_bitrate); - RTC_CHECK(actual_encode_bitrate); - RTC_CHECK(target_encode_bitrate); - - rtc::CritScope crit(&video_bwe_stats_lock_); - VideoBweStats& video_bwe_stats = video_bwe_stats_[pc_label]; - video_bwe_stats.available_send_bandwidth.AddSample( - available_send_bandwidth->int_val()); - video_bwe_stats.transmission_bitrate.AddSample( - transmission_bitrate->int_val()); - video_bwe_stats.retransmission_bitrate.AddSample( - retransmission_bitrate->int_val()); - video_bwe_stats.actual_encode_bitrate.AddSample( - actual_encode_bitrate->int_val()); - video_bwe_stats.target_encode_bitrate.AddSample( - target_encode_bitrate->int_val()); - } -} - -absl::flat_hash_map -DefaultVideoQualityAnalyzer::GetVideoBweStats() const { - rtc::CritScope crit(&video_bwe_stats_lock_); - return video_bwe_stats_; -} - void DefaultVideoQualityAnalyzer::AddComparison( absl::optional captured, absl::optional rendered, @@ -540,12 +487,6 @@ void DefaultVideoQualityAnalyzer::ReportResults() { ReportResults(GetTestCaseName(item.first), item.second, stream_frame_counters_.at(item.first)); } - { - rtc::CritScope video_bwe_crit(&video_bwe_stats_lock_); - for (const auto& item : video_bwe_stats_) { - ReportVideoBweResults(GetTestCaseName(item.first), item.second); - } - } LogFrameCounters("Global", frame_counters_); for (auto& item : stream_stats_) { LogFrameCounters(item.first, stream_frame_counters_.at(item.first)); @@ -563,25 +504,9 @@ void DefaultVideoQualityAnalyzer::ReportResults() { << analyzer_stats_.overloaded_comparisons_done; } -void DefaultVideoQualityAnalyzer::ReportVideoBweResults( - const std::string& test_case_name, - const VideoBweStats& video_bwe_stats) { - ReportResult("available_send_bandwidth", test_case_name, - video_bwe_stats.available_send_bandwidth, "bytesPerSecond"); - ReportResult("transmission_bitrate", test_case_name, - video_bwe_stats.transmission_bitrate, "bytesPerSecond"); - ReportResult("retransmission_bitrate", test_case_name, - video_bwe_stats.retransmission_bitrate, "bytesPerSecond"); - ReportResult("actual_encode_bitrate", test_case_name, - video_bwe_stats.actual_encode_bitrate, "bytesPerSecond"); - ReportResult("target_encode_bitrate", test_case_name, - video_bwe_stats.target_encode_bitrate, "bytesPerSecond"); -} - -void DefaultVideoQualityAnalyzer::ReportResults( - const std::string& test_case_name, - const StreamStats& stats, - const FrameCounters& frame_counters) { +void DefaultVideoQualityAnalyzer::ReportResults(std::string test_case_name, + StreamStats stats, + FrameCounters frame_counters) { ReportResult("psnr", test_case_name, stats.psnr, "dB"); ReportResult("ssim", test_case_name, stats.ssim, "unitless"); ReportResult("transport_time", test_case_name, stats.transport_time_ms, "ms"); diff --git a/test/pc/e2e/analyzer/video/default_video_quality_analyzer.h b/test/pc/e2e/analyzer/video/default_video_quality_analyzer.h index 22bf4fdea9..b8d186d79b 100644 --- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer.h +++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer.h @@ -19,7 +19,6 @@ #include #include -#include "absl/container/flat_hash_map.h" #include "api/test/video_quality_analyzer_interface.h" #include "api/units/timestamp.h" #include "api/video/encoded_image.h" @@ -69,6 +68,7 @@ struct FrameCounters { }; struct StreamStats { + public: SamplesStatsCounter psnr; SamplesStatsCounter ssim; // Time from frame encoded (time point on exit from encoder) to the @@ -100,6 +100,7 @@ struct StreamStats { }; struct AnalyzerStats { + public: // Size of analyzer internal comparisons queue, measured when new element // id added to the queue. SamplesStatsCounter comparisons_queue_size; @@ -113,14 +114,6 @@ struct AnalyzerStats { int64_t overloaded_comparisons_done = 0; }; -struct VideoBweStats { - SamplesStatsCounter available_send_bandwidth; - SamplesStatsCounter transmission_bitrate; - SamplesStatsCounter retransmission_bitrate; - SamplesStatsCounter actual_encode_bitrate; - SamplesStatsCounter target_encode_bitrate; -}; - class DefaultVideoQualityAnalyzer : public VideoQualityAnalyzerInterface { public: DefaultVideoQualityAnalyzer(); @@ -155,12 +148,8 @@ class DefaultVideoQualityAnalyzer : public VideoQualityAnalyzerInterface { std::map GetStats() const; AnalyzerStats GetAnalyzerStats() const; - // Will be called everytime new stats reports are available for the - // Peer Connection identified by |pc_label|. - void OnStatsReports(absl::string_view pc_label, - const StatsReports& stats_reports) override; - - absl::flat_hash_map GetVideoBweStats() const; + // TODO(bugs.webrtc.org/10138): Provide a real implementation for + // OnStatsReport. private: struct FrameStats { @@ -243,11 +232,9 @@ class DefaultVideoQualityAnalyzer : public VideoQualityAnalyzerInterface { void ProcessComparison(const FrameComparison& comparison); // Report results for all metrics for all streams. void ReportResults(); - static void ReportVideoBweResults(const std::string& test_case_name, - const VideoBweStats& video_bwe_stats); - static void ReportResults(const std::string& test_case_name, - const StreamStats& stats, - const FrameCounters& frame_counters); + static void ReportResults(std::string test_case_name, + StreamStats stats, + FrameCounters frame_counters); // Report result for single metric for specified stream. static void ReportResult(const std::string& metric_name, const std::string& test_case_name, @@ -284,12 +271,6 @@ class DefaultVideoQualityAnalyzer : public VideoQualityAnalyzerInterface { std::deque comparisons_ RTC_GUARDED_BY(comparison_lock_); AnalyzerStats analyzer_stats_ RTC_GUARDED_BY(comparison_lock_); - rtc::CriticalSection video_bwe_stats_lock_; - // Map between a peer connection label (provided by the framework) and - // its video BWE stats. - absl::flat_hash_map video_bwe_stats_ - RTC_GUARDED_BY(video_bwe_stats_lock_); - std::vector> thread_pool_; rtc::Event comparison_available_event_; };