From 0f0978d36e25fd85572d44c6f1a0f1543b67d870 Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Fri, 10 Jun 2022 17:33:47 +0200 Subject: [PATCH] [DVQA] Extract NamesCollection into separate file Extract NamesCollection into separate file and add ability to remove peer after it was added. In such case we need to preserve old indexes, because DVQA still may request removed name from collection due to async processing. Bug: b/231397778 Change-Id: I87bdfb4653e7eca50d311482553d2353b1d9974e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/265394 Reviewed-by: Mirko Bonadei Commit-Queue: Artem Titov Cr-Commit-Position: refs/heads/main@{#37181} --- test/pc/e2e/BUILD.gn | 22 ++- .../video/default_video_quality_analyzer.cc | 23 --- .../video/default_video_quality_analyzer.h | 30 +--- .../pc/e2e/analyzer/video/names_collection.cc | 81 ++++++++++ test/pc/e2e/analyzer/video/names_collection.h | 82 ++++++++++ .../analyzer/video/names_collection_test.cc | 141 ++++++++++++++++++ 6 files changed, 326 insertions(+), 53 deletions(-) create mode 100644 test/pc/e2e/analyzer/video/names_collection.cc create mode 100644 test/pc/e2e/analyzer/video/names_collection.h create mode 100644 test/pc/e2e/analyzer/video/names_collection_test.cc diff --git a/test/pc/e2e/BUILD.gn b/test/pc/e2e/BUILD.gn index 3832bba1dc..1778c6a3af 100644 --- a/test/pc/e2e/BUILD.gn +++ b/test/pc/e2e/BUILD.gn @@ -37,6 +37,7 @@ if (!build_with_chromium) { ":default_video_quality_analyzer_frames_comparator_test", ":default_video_quality_analyzer_test", ":multi_head_queue_test", + ":names_collection_test", ":peer_connection_e2e_smoke_test", ":single_process_encoded_image_data_injector_unittest", ":stats_poller_test", @@ -571,6 +572,19 @@ if (!build_with_chromium) { ] } + rtc_library("names_collection_test") { + testonly = true + sources = [ "analyzer/video/names_collection_test.cc" ] + deps = [ + ":default_video_quality_analyzer_internal", + "../..:test_support", + ] + absl_deps = [ + "//third_party/abseil-cpp/absl/strings:strings", + "//third_party/abseil-cpp/absl/types:optional", + ] + } + rtc_library("multi_head_queue_test") { testonly = true sources = [ "analyzer/video/multi_head_queue_test.cc" ] @@ -713,6 +727,7 @@ if (!build_with_chromium) { visibility = [ ":default_video_quality_analyzer", ":default_video_quality_analyzer_frames_comparator_test", + ":names_collection_test", ] testonly = true @@ -723,6 +738,8 @@ if (!build_with_chromium) { "analyzer/video/default_video_quality_analyzer_frames_comparator.h", "analyzer/video/default_video_quality_analyzer_internal_shared_objects.cc", "analyzer/video/default_video_quality_analyzer_internal_shared_objects.h", + "analyzer/video/names_collection.cc", + "analyzer/video/names_collection.h", ] deps = [ @@ -745,7 +762,10 @@ if (!build_with_chromium) { "../../../rtc_tools:video_quality_analysis", "../../../system_wrappers:system_wrappers", ] - absl_deps = [ "//third_party/abseil-cpp/absl/types:optional" ] + absl_deps = [ + "//third_party/abseil-cpp/absl/strings:strings", + "//third_party/abseil-cpp/absl/types:optional", + ] } rtc_library("default_video_quality_analyzer_shared") { 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 e3c8c1272a..88a6f3d462 100644 --- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc +++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer.cc @@ -1229,27 +1229,4 @@ FrameStats DefaultVideoQualityAnalyzer::FrameInFlight::GetStatsForPeer( return stats; } -size_t DefaultVideoQualityAnalyzer::NamesCollection::AddIfAbsent( - absl::string_view name) { - auto it = index_.find(name); - if (it != index_.end()) { - return it->second; - } - size_t out = names_.size(); - size_t old_capacity = names_.capacity(); - names_.emplace_back(name); - size_t new_capacity = names_.capacity(); - - if (old_capacity == new_capacity) { - index_.emplace(names_[out], out); - } else { - // Reallocation happened in the vector, so we need to rebuild `index_` - index_.clear(); - for (size_t i = 0; i < names_.size(); ++i) { - index_.emplace(names_[i], i); - } - } - return out; -} - } // namespace webrtc 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 78ff6db254..0f4c1ddf29 100644 --- a/test/pc/e2e/analyzer/video/default_video_quality_analyzer.h +++ b/test/pc/e2e/analyzer/video/default_video_quality_analyzer.h @@ -38,6 +38,7 @@ #include "test/pc/e2e/analyzer/video/default_video_quality_analyzer_internal_shared_objects.h" #include "test/pc/e2e/analyzer/video/default_video_quality_analyzer_shared_objects.h" #include "test/pc/e2e/analyzer/video/multi_head_queue.h" +#include "test/pc/e2e/analyzer/video/names_collection.h" #include "test/testsupport/perf_test.h" namespace webrtc { @@ -289,35 +290,6 @@ class DefaultVideoQualityAnalyzer : public VideoQualityAnalyzerInterface { std::map receiver_stats_; }; - class NamesCollection { - public: - NamesCollection() = default; - explicit NamesCollection(rtc::ArrayView names) { - names_ = std::vector(names.begin(), names.end()); - for (size_t i = 0; i < names_.size(); ++i) { - index_.emplace(names_[i], i); - } - } - - size_t size() const { return names_.size(); } - - size_t index(absl::string_view name) const { return index_.at(name); } - - const std::string& name(size_t index) const { return names_[index]; } - - bool HasName(absl::string_view name) const { - return index_.find(name) != index_.end(); - } - - // Add specified `name` to the collection if it isn't presented. - // Returns index which corresponds to specified `name`. - size_t AddIfAbsent(absl::string_view name); - - private: - std::vector names_; - std::map index_; - }; - // Returns next frame id to use. Frame ID can't be `VideoFrame::kNotSetId`, // because this value is reserved by `VideoFrame` as "ID not set". uint16_t GetNextFrameId() RTC_EXCLUSIVE_LOCKS_REQUIRED(mutex_); diff --git a/test/pc/e2e/analyzer/video/names_collection.cc b/test/pc/e2e/analyzer/video/names_collection.cc new file mode 100644 index 0000000000..845f73d398 --- /dev/null +++ b/test/pc/e2e/analyzer/video/names_collection.cc @@ -0,0 +1,81 @@ +/* + * Copyright (c) 2022 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "test/pc/e2e/analyzer/video/names_collection.h" + +#include "absl/strings/string_view.h" +#include "absl/types/optional.h" + +namespace webrtc { + +NamesCollection::NamesCollection(rtc::ArrayView names) { + names_ = std::vector(names.begin(), names.end()); + for (size_t i = 0; i < names_.size(); ++i) { + index_.emplace(names_[i], i); + removed_.emplace_back(false); + } + size_ = names_.size(); +} + +bool NamesCollection::HasName(absl::string_view name) const { + auto it = index_.find(name); + if (it == index_.end()) { + return false; + } + return !removed_[it->second]; +} + +size_t NamesCollection::AddIfAbsent(absl::string_view name) { + auto it = index_.find(name); + if (it != index_.end()) { + // Name was registered in the collection before: we need to restore it. + size_t index = it->second; + if (removed_[index]) { + removed_[index] = false; + size_++; + } + return index; + } + size_t out = names_.size(); + size_t old_capacity = names_.capacity(); + names_.emplace_back(name); + removed_.emplace_back(false); + size_++; + size_t new_capacity = names_.capacity(); + + if (old_capacity == new_capacity) { + index_.emplace(names_[out], out); + } else { + // Reallocation happened in the vector, so we need to rebuild `index_` to + // fix absl::string_view internal references. + index_.clear(); + for (size_t i = 0; i < names_.size(); ++i) { + index_.emplace(names_[i], i); + } + } + return out; +} + +absl::optional NamesCollection::RemoveIfPresent( + absl::string_view name) { + auto it = index_.find(name); + if (it == index_.end()) { + return absl::nullopt; + } + size_t index = it->second; + if (removed_[index]) { + return absl::nullopt; + } + removed_[index] = true; + size_--; + return index; +} + +} // namespace webrtc diff --git a/test/pc/e2e/analyzer/video/names_collection.h b/test/pc/e2e/analyzer/video/names_collection.h new file mode 100644 index 0000000000..77c4939f85 --- /dev/null +++ b/test/pc/e2e/analyzer/video/names_collection.h @@ -0,0 +1,82 @@ +/* + * Copyright (c) 2022 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#ifndef TEST_PC_E2E_ANALYZER_VIDEO_NAMES_COLLECTION_H_ +#define TEST_PC_E2E_ANALYZER_VIDEO_NAMES_COLLECTION_H_ + +#include +#include +#include + +#include "absl/strings/string_view.h" +#include "absl/types/optional.h" +#include "api/array_view.h" + +namespace webrtc { + +// Contains mapping between string names and unique size_t values (indexes). +// Once the name is added to the collection it is guaranteed: +// 1. Name will have the same index until collection will be destructed +// 2. Adding, removing and re-adding name won't change its index +// +// The name is considered in the collection if it was added and wasn't removed. +// Adding the name when it is in the collection won't change the collection, the +// same as removing the name when it is removed. +// +// Collection will return name's index and name for the index independently from +// was name removed or not. Once the name was added to the collection the index +// will be allocated for it. To check if name is in collection right now user +// has to explicitly call to `HasName` function. +class NamesCollection { + public: + NamesCollection() = default; + + explicit NamesCollection(rtc::ArrayView names); + + // Returns amount of currently presented names in the collection. + size_t size() const { return size_; } + + // Returns index of the `name` which was known to the collection. Crashes + // if `name` was never registered in the collection. + size_t index(absl::string_view name) const { return index_.at(name); } + + // Returns name which was known to the collection for the specified `index`. + // Crashes if there was no any name registered in the collection for such + // `index`. + const std::string& name(size_t index) const { return names_.at(index); } + + // Returns if `name` is currently presented in this collection. + bool HasName(absl::string_view name) const; + + // Adds specified `name` to the collection if it isn't presented. + // Returns index which corresponds to specified `name`. + size_t AddIfAbsent(absl::string_view name); + + // Removes specified `name` from the collection if it is presented. + // + // After name was removed, collection size will be decreased, but `name` index + // will be preserved. Collection will return false for `HasName(name)`, but + // will continue to return previously known index for `index(name)` and return + // `name` for `name(index(name))`. + // + // Returns the index of the removed value or absl::nullopt if no such `name` + // registered in the collection. + absl::optional RemoveIfPresent(absl::string_view name); + + private: + std::vector names_; + std::vector removed_; + std::map index_; + size_t size_ = 0; +}; + +} // namespace webrtc + +#endif // TEST_PC_E2E_ANALYZER_VIDEO_NAMES_COLLECTION_H_ diff --git a/test/pc/e2e/analyzer/video/names_collection_test.cc b/test/pc/e2e/analyzer/video/names_collection_test.cc new file mode 100644 index 0000000000..65932ea26e --- /dev/null +++ b/test/pc/e2e/analyzer/video/names_collection_test.cc @@ -0,0 +1,141 @@ +/* + * Copyright (c) 2022 The WebRTC project authors. All Rights Reserved. + * + * Use of this source code is governed by a BSD-style license + * that can be found in the LICENSE file in the root of the source + * tree. An additional intellectual property rights grant can be found + * in the file PATENTS. All contributing project authors may + * be found in the AUTHORS file in the root of the source tree. + */ + +#include "test/pc/e2e/analyzer/video/names_collection.h" + +#include +#include + +#include "absl/types/optional.h" +#include "test/gmock.h" +#include "test/gtest.h" + +namespace webrtc { +namespace { + +using ::testing::Eq; +using ::testing::Ne; + +TEST(NamesCollectionTest, NamesFromCtorHasUniqueIndexes) { + NamesCollection collection(std::vector{"alice", "bob"}); + + EXPECT_THAT(collection.size(), Eq(static_cast(2))); + EXPECT_TRUE(collection.HasName("alice")); + EXPECT_THAT(collection.name(collection.index("alice")), Eq("alice")); + + EXPECT_TRUE(collection.HasName("bob")); + EXPECT_THAT(collection.name(collection.index("bob")), Eq("bob")); + + EXPECT_THAT(collection.index("bob"), Ne(collection.index("alice"))); +} + +TEST(NamesCollectionTest, AddedNamesHasIndexes) { + NamesCollection collection(std::vector{}); + collection.AddIfAbsent("alice"); + + EXPECT_THAT(collection.size(), Eq(static_cast(1))); + EXPECT_TRUE(collection.HasName("alice")); + EXPECT_THAT(collection.name(collection.index("alice")), Eq("alice")); +} + +TEST(NamesCollectionTest, AddBobDoesNotChangeAliceIndex) { + NamesCollection collection(std::vector{"alice"}); + + size_t alice_index = collection.index("alice"); + + collection.AddIfAbsent("bob"); + + EXPECT_THAT(collection.size(), Eq(static_cast(2))); + EXPECT_THAT(collection.index("alice"), Eq(alice_index)); + EXPECT_THAT(collection.index("bob"), Ne(alice_index)); +} + +TEST(NamesCollectionTest, AddAliceSecondTimeDoesNotChangeIndex) { + NamesCollection collection(std::vector{"alice"}); + + size_t alice_index = collection.index("alice"); + + EXPECT_THAT(collection.AddIfAbsent("alice"), Eq(alice_index)); + + EXPECT_THAT(collection.size(), Eq(static_cast(1))); + EXPECT_THAT(collection.index("alice"), Eq(alice_index)); +} + +TEST(NamesCollectionTest, RemoveRemovesFromCollectionButNotIndex) { + NamesCollection collection(std::vector{"alice", "bob"}); + + size_t bob_index = collection.index("bob"); + + EXPECT_THAT(collection.size(), Eq(static_cast(2))); + + EXPECT_THAT(collection.RemoveIfPresent("bob"), + Eq(absl::optional(bob_index))); + + EXPECT_THAT(collection.size(), Eq(static_cast(1))); + EXPECT_FALSE(collection.HasName("bob")); + + EXPECT_THAT(collection.index("bob"), Eq(bob_index)); + EXPECT_THAT(collection.name(bob_index), Eq("bob")); +} + +TEST(NamesCollectionTest, RemoveOfAliceDoesNotChangeBobIndex) { + NamesCollection collection(std::vector{"alice", "bob"}); + + size_t alice_index = collection.index("alice"); + size_t bob_index = collection.index("bob"); + + EXPECT_THAT(collection.size(), Eq(static_cast(2))); + + EXPECT_THAT(collection.RemoveIfPresent("alice"), + Eq(absl::optional(alice_index))); + + EXPECT_THAT(collection.size(), Eq(static_cast(1))); + EXPECT_THAT(collection.index("bob"), Eq(bob_index)); + EXPECT_THAT(collection.name(bob_index), Eq("bob")); +} + +TEST(NamesCollectionTest, RemoveSecondTimeHasNoEffect) { + NamesCollection collection(std::vector{"bob"}); + + size_t bob_index = collection.index("bob"); + + EXPECT_THAT(collection.size(), Eq(static_cast(1))); + EXPECT_THAT(collection.RemoveIfPresent("bob"), + Eq(absl::optional(bob_index))); + + EXPECT_THAT(collection.size(), Eq(static_cast(0))); + EXPECT_THAT(collection.RemoveIfPresent("bob"), Eq(absl::nullopt)); +} + +TEST(NamesCollectionTest, RemoveOfNotExistingHasNoEffect) { + NamesCollection collection(std::vector{"bob"}); + + EXPECT_THAT(collection.size(), Eq(static_cast(1))); + EXPECT_THAT(collection.RemoveIfPresent("alice"), Eq(absl::nullopt)); + EXPECT_THAT(collection.size(), Eq(static_cast(1))); +} + +TEST(NamesCollectionTest, AddRemoveAddPreserveTheIndex) { + NamesCollection collection(std::vector{}); + + size_t alice_index = collection.AddIfAbsent("alice"); + EXPECT_THAT(collection.size(), Eq(static_cast(1))); + + EXPECT_THAT(collection.RemoveIfPresent("alice"), + Eq(absl::optional(alice_index))); + EXPECT_THAT(collection.size(), Eq(static_cast(0))); + + EXPECT_THAT(collection.AddIfAbsent("alice"), Eq(alice_index)); + EXPECT_THAT(collection.index("alice"), Eq(alice_index)); + EXPECT_THAT(collection.size(), Eq(static_cast(1))); +} + +} // namespace +} // namespace webrtc