[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 <mbonadei@webrtc.org>
Commit-Queue: Artem Titov <titovartem@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#37181}

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") {

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

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<size_t, ReceiverFrameStats> receiver_stats_;
   };
 
-  class NamesCollection {
-   public:
-    NamesCollection() = default;
-    explicit NamesCollection(rtc::ArrayView<const std::string> names) {
-      names_ = std::vector<std::string>(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<std::string> names_;
-    std::map<absl::string_view, size_t> 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_);

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<const std::string> names) {
+  names_ = std::vector<std::string>(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<size_t> 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

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 <map>
+#include <string>
+#include <vector>
+
+#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<const std::string> 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<size_t> RemoveIfPresent(absl::string_view name);
+
+ private:
+  std::vector<std::string> names_;
+  std::vector<bool> removed_;
+  std::map<absl::string_view, size_t> index_;
+  size_t size_ = 0;
+};
+
+}  // namespace webrtc
+
+#endif  // TEST_PC_E2E_ANALYZER_VIDEO_NAMES_COLLECTION_H_

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 <string>
+#include <vector>
+
+#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<std::string>{"alice", "bob"});
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(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<std::string>{});
+  collection.AddIfAbsent("alice");
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(1)));
+  EXPECT_TRUE(collection.HasName("alice"));
+  EXPECT_THAT(collection.name(collection.index("alice")), Eq("alice"));
+}
+
+TEST(NamesCollectionTest, AddBobDoesNotChangeAliceIndex) {
+  NamesCollection collection(std::vector<std::string>{"alice"});
+
+  size_t alice_index = collection.index("alice");
+
+  collection.AddIfAbsent("bob");
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(2)));
+  EXPECT_THAT(collection.index("alice"), Eq(alice_index));
+  EXPECT_THAT(collection.index("bob"), Ne(alice_index));
+}
+
+TEST(NamesCollectionTest, AddAliceSecondTimeDoesNotChangeIndex) {
+  NamesCollection collection(std::vector<std::string>{"alice"});
+
+  size_t alice_index = collection.index("alice");
+
+  EXPECT_THAT(collection.AddIfAbsent("alice"), Eq(alice_index));
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(1)));
+  EXPECT_THAT(collection.index("alice"), Eq(alice_index));
+}
+
+TEST(NamesCollectionTest, RemoveRemovesFromCollectionButNotIndex) {
+  NamesCollection collection(std::vector<std::string>{"alice", "bob"});
+
+  size_t bob_index = collection.index("bob");
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(2)));
+
+  EXPECT_THAT(collection.RemoveIfPresent("bob"),
+              Eq(absl::optional<size_t>(bob_index)));
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(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<std::string>{"alice", "bob"});
+
+  size_t alice_index = collection.index("alice");
+  size_t bob_index = collection.index("bob");
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(2)));
+
+  EXPECT_THAT(collection.RemoveIfPresent("alice"),
+              Eq(absl::optional<size_t>(alice_index)));
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(1)));
+  EXPECT_THAT(collection.index("bob"), Eq(bob_index));
+  EXPECT_THAT(collection.name(bob_index), Eq("bob"));
+}
+
+TEST(NamesCollectionTest, RemoveSecondTimeHasNoEffect) {
+  NamesCollection collection(std::vector<std::string>{"bob"});
+
+  size_t bob_index = collection.index("bob");
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(1)));
+  EXPECT_THAT(collection.RemoveIfPresent("bob"),
+              Eq(absl::optional<size_t>(bob_index)));
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(0)));
+  EXPECT_THAT(collection.RemoveIfPresent("bob"), Eq(absl::nullopt));
+}
+
+TEST(NamesCollectionTest, RemoveOfNotExistingHasNoEffect) {
+  NamesCollection collection(std::vector<std::string>{"bob"});
+
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(1)));
+  EXPECT_THAT(collection.RemoveIfPresent("alice"), Eq(absl::nullopt));
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(1)));
+}
+
+TEST(NamesCollectionTest, AddRemoveAddPreserveTheIndex) {
+  NamesCollection collection(std::vector<std::string>{});
+
+  size_t alice_index = collection.AddIfAbsent("alice");
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(1)));
+
+  EXPECT_THAT(collection.RemoveIfPresent("alice"),
+              Eq(absl::optional<size_t>(alice_index)));
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(0)));
+
+  EXPECT_THAT(collection.AddIfAbsent("alice"), Eq(alice_index));
+  EXPECT_THAT(collection.index("alice"), Eq(alice_index));
+  EXPECT_THAT(collection.size(), Eq(static_cast<size_t>(1)));
+}
+
+}  // namespace
+}  // namespace webrtc