Refactoring: Move groups-by-mid into Bundle manager

Bug: webrtc:12837
Change-Id: I2431dbbf8cc291b5f3848d81cf290fd3e97ec15d
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/222614
Commit-Queue: Harald Alvestrand <hta@webrtc.org>
Reviewed-by: Henrik Boström <hbos@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34324}

pc/jsep_transport_collection.cc

@@ -27,12 +27,40 @@ void BundleManager::Update(const cricket::SessionDescription* description) {
        description->GetGroupsByName(cricket::GROUP_TYPE_BUNDLE)) {
     bundle_groups_.push_back(
         std::make_unique<cricket::ContentGroup>(*new_bundle_group));
+    RTC_DLOG(LS_VERBOSE) << "Establishing bundle group "
+                         << new_bundle_group->ToString();
   }
+  established_bundle_groups_by_mid_.clear();
+  for (const auto& bundle_group : bundle_groups_) {
+    for (const std::string& content_name : bundle_group->content_names()) {
+      established_bundle_groups_by_mid_[content_name] = bundle_group.get();
+    }
+  }
+}
+
+const cricket::ContentGroup* BundleManager::LookupGroupByMid(
+    const std::string& mid) const {
+  auto it = established_bundle_groups_by_mid_.find(mid);
+  return it != established_bundle_groups_by_mid_.end() ? it->second : nullptr;
+}
+bool BundleManager::IsFirstMidInGroup(const std::string& mid) const {
+  auto group = LookupGroupByMid(mid);
+  if (!group) {
+    return true;  // Unbundled MIDs are considered group leaders
+  }
+  return mid == *(group->FirstContentName());
+}
+
+cricket::ContentGroup* BundleManager::LookupGroupByMid(const std::string& mid) {
+  auto it = established_bundle_groups_by_mid_.find(mid);
+  return it != established_bundle_groups_by_mid_.end() ? it->second : nullptr;
 }
 
 void BundleManager::DeleteMid(const cricket::ContentGroup* bundle_group,
                               const std::string& mid) {
   RTC_DCHECK_RUN_ON(&sequence_checker_);
+  RTC_LOG(LS_VERBOSE) << "Deleting mid " << mid << " from bundle group "
+                      << bundle_group->ToString();
   // Remove the rejected content from the |bundle_group|.
   // The const pointer arg is used to identify the group, we verify
   // it before we use it to make a modification.
@@ -43,17 +71,24 @@ void BundleManager::DeleteMid(const cricket::ContentGroup* bundle_group,
       });
   RTC_DCHECK(bundle_group_it != bundle_groups_.end());
   (*bundle_group_it)->RemoveContentName(mid);
+  established_bundle_groups_by_mid_.erase(
+      established_bundle_groups_by_mid_.find(mid));
 }
 
 void BundleManager::DeleteGroup(const cricket::ContentGroup* bundle_group) {
   RTC_DCHECK_RUN_ON(&sequence_checker_);
-  // Delete the BUNDLE group.
+  RTC_DLOG(LS_VERBOSE) << "Deleting bundle group " << bundle_group->ToString();
+
   auto bundle_group_it = std::find_if(
       bundle_groups_.begin(), bundle_groups_.end(),
       [bundle_group](std::unique_ptr<cricket::ContentGroup>& group) {
         return bundle_group == group.get();
       });
   RTC_DCHECK(bundle_group_it != bundle_groups_.end());
+  auto mid_list = (*bundle_group_it)->content_names();
+  for (const auto& content_name : mid_list) {
+    DeleteMid(bundle_group, content_name);
+  }
   bundle_groups_.erase(bundle_group_it);
 }
 
@@ -208,7 +243,8 @@ bool JsepTransportCollection::IsConsistent() {
     }
     const auto& lookup = mid_to_transport_.find(it.first);
     if (lookup->second != it.second.get()) {
-      RTC_LOG(LS_ERROR) << "Note: Mid " << it.first << " was registered to "
+      // Not an error, but unusual.
+      RTC_DLOG(LS_INFO) << "Note: Mid " << it.first << " was registered to "
                         << it.second.get() << " but currently maps to "
                         << lookup->second;
     }

pc/jsep_transport_collection.h

@@ -50,6 +50,12 @@ class BundleManager {
     RTC_DCHECK_RUN_ON(&sequence_checker_);
     return bundle_groups_;
   }
+  // Lookup a bundle group by a member mid name.
+  const cricket::ContentGroup* LookupGroupByMid(const std::string& mid) const;
+  cricket::ContentGroup* LookupGroupByMid(const std::string& mid);
+  // Returns true if the MID is the first item of a group, or if
+  // the MID is not a member of a group.
+  bool IsFirstMidInGroup(const std::string& mid) const;
   // Update the groups description. This completely replaces the group
   // description with the one from the SessionDescription.
   void Update(const cricket::SessionDescription* description);
@@ -63,6 +69,8 @@ class BundleManager {
   RTC_NO_UNIQUE_ADDRESS SequenceChecker sequence_checker_;
   std::vector<std::unique_ptr<cricket::ContentGroup>> bundle_groups_
       RTC_GUARDED_BY(sequence_checker_);
+  std::map<std::string, cricket::ContentGroup*>
+      established_bundle_groups_by_mid_;
 };
 
 // This class keeps the mapping of MIDs to transports.

pc/jsep_transport_controller.cc

@@ -38,19 +38,6 @@ using webrtc::SdpType;
 
 namespace webrtc {
 
-namespace {
-
-bool IsBundledButNotFirstMid(
-    const std::map<std::string, cricket::ContentGroup*>& bundle_groups_by_mid,
-    const std::string& mid) {
-  auto it = bundle_groups_by_mid.find(mid);
-  if (it == bundle_groups_by_mid.end())
-    return false;
-  return mid != *it->second->FirstContentName();
-}
-
-}  // namespace
-
 JsepTransportController::JsepTransportController(
     rtc::Thread* network_thread,
     cricket::PortAllocator* port_allocator,
@@ -556,21 +543,12 @@ RTCError JsepTransportController::ApplyDescription_n(
   if (!error.ok()) {
     return error;
   }
-  // Established BUNDLE groups by MID.
-  std::map<std::string, cricket::ContentGroup*>
-      established_bundle_groups_by_mid;
-  for (const auto& bundle_group : bundles_.bundle_groups()) {
-    for (const std::string& content_name : bundle_group->content_names()) {
-      established_bundle_groups_by_mid[content_name] = bundle_group.get();
-    }
-  }
 
   std::map<const cricket::ContentGroup*, std::vector<int>>
       merged_encrypted_extension_ids_by_bundle;
   if (!bundles_.bundle_groups().empty()) {
     merged_encrypted_extension_ids_by_bundle =
-        MergeEncryptedHeaderExtensionIdsForBundles(
+        MergeEncryptedHeaderExtensionIdsForBundles(description);
-            established_bundle_groups_by_mid, description);
   }
 
   // Because the creation of transports depends on whether
@@ -579,17 +557,15 @@ RTCError JsepTransportController::ApplyDescription_n(
   for (size_t i = 0; i < description->contents().size(); ++i) {
     const cricket::ContentInfo& content_info = description->contents()[i];
     if (content_info.rejected) {
-      // This may cause groups to be removed from |bundles_.bundle_groups()| and
+      // This may cause groups to be removed from |bundles_.bundle_groups()|.
-      // |established_bundle_groups_by_mid|.
+      HandleRejectedContent(content_info);
-      HandleRejectedContent(content_info, established_bundle_groups_by_mid);
     }
   }
 
   for (const cricket::ContentInfo& content_info : description->contents()) {
     // Don't create transports for rejected m-lines and bundled m-lines.
     if (content_info.rejected ||
-        IsBundledButNotFirstMid(established_bundle_groups_by_mid,
+        !bundles_.IsFirstMidInGroup(content_info.name)) {
-                                content_info.name)) {
       continue;
     }
     error = MaybeCreateJsepTransport(local, content_info, *description);
@@ -609,9 +585,8 @@ RTCError JsepTransportController::ApplyDescription_n(
       continue;
     }
 
-    auto it = established_bundle_groups_by_mid.find(content_info.name);
     const cricket::ContentGroup* established_bundle_group =
-        it != established_bundle_groups_by_mid.end() ? it->second : nullptr;
+        bundles_.LookupGroupByMid(content_info.name);
 
     // For bundle members that are not BUNDLE-tagged (not first in the group),
     // configure their transport to be the same as the BUNDLE-tagged transport.
@@ -835,25 +810,18 @@ RTCError JsepTransportController::ValidateContent(
 }
 
 void JsepTransportController::HandleRejectedContent(
-    const cricket::ContentInfo& content_info,
+    const cricket::ContentInfo& content_info) {
-    std::map<std::string, cricket::ContentGroup*>&
-        established_bundle_groups_by_mid) {
   // If the content is rejected, let the
   // BaseChannel/SctpTransport change the RtpTransport/DtlsTransport first,
   // then destroy the cricket::JsepTransport.
-  auto it = established_bundle_groups_by_mid.find(content_info.name);
   cricket::ContentGroup* bundle_group =
-      it != established_bundle_groups_by_mid.end() ? it->second : nullptr;
+      bundles_.LookupGroupByMid(content_info.name);
   if (bundle_group && !bundle_group->content_names().empty() &&
       content_info.name == *bundle_group->FirstContentName()) {
     // Rejecting a BUNDLE group's first mid means we are rejecting the entire
     // group.
     for (const auto& content_name : bundle_group->content_names()) {
       transports_.RemoveTransportForMid(content_name);
-      // We are about to delete this BUNDLE group, erase all mappings to it.
-      it = established_bundle_groups_by_mid.find(content_name);
-      RTC_DCHECK(it != established_bundle_groups_by_mid.end());
-      established_bundle_groups_by_mid.erase(it);
     }
     // Delete the BUNDLE group.
     bundles_.DeleteGroup(bundle_group);
@@ -947,7 +915,6 @@ std::vector<int> JsepTransportController::GetEncryptedHeaderExtensionIds(
 
 std::map<const cricket::ContentGroup*, std::vector<int>>
 JsepTransportController::MergeEncryptedHeaderExtensionIdsForBundles(
-    const std::map<std::string, cricket::ContentGroup*>& bundle_groups_by_mid,
     const cricket::SessionDescription* description) {
   RTC_DCHECK(description);
   RTC_DCHECK(!bundles_.bundle_groups().empty());
@@ -955,12 +922,12 @@ JsepTransportController::MergeEncryptedHeaderExtensionIdsForBundles(
       merged_encrypted_extension_ids_by_bundle;
   // Union the encrypted header IDs in the group when bundle is enabled.
   for (const cricket::ContentInfo& content_info : description->contents()) {
-    auto it = bundle_groups_by_mid.find(content_info.name);
+    auto group = bundles_.LookupGroupByMid(content_info.name);
-    if (it == bundle_groups_by_mid.end())
+    if (!group)
       continue;
     // Get or create list of IDs for the BUNDLE group.
     std::vector<int>& merged_ids =
-        merged_encrypted_extension_ids_by_bundle[it->second];
+        merged_encrypted_extension_ids_by_bundle[group];
     // Add IDs not already in the list.
     std::vector<int> extension_ids =
         GetEncryptedHeaderExtensionIds(content_info);

pc/jsep_transport_controller.h

@@ -330,9 +330,7 @@ class JsepTransportController : public sigslot::has_slots<> {
       const cricket::SessionDescription* description);
   RTCError ValidateContent(const cricket::ContentInfo& content_info);
 
-  void HandleRejectedContent(const cricket::ContentInfo& content_info,
+  void HandleRejectedContent(const cricket::ContentInfo& content_info)
-                             std::map<std::string, cricket::ContentGroup*>&
-                                 established_bundle_groups_by_mid)
       RTC_RUN_ON(network_thread_);
   bool HandleBundledContent(const cricket::ContentInfo& content_info,
                             const cricket::ContentGroup& bundle_group)
@@ -349,7 +347,6 @@ class JsepTransportController : public sigslot::has_slots<> {
 
   std::map<const cricket::ContentGroup*, std::vector<int>>
   MergeEncryptedHeaderExtensionIdsForBundles(
-      const std::map<std::string, cricket::ContentGroup*>& bundle_groups_by_mid,
       const cricket::SessionDescription* description);
   std::vector<int> GetEncryptedHeaderExtensionIds(
       const cricket::ContentInfo& content_info);