From 781c12e7fbb4d68a407eda9671f5c9233d8ce0d7 Mon Sep 17 00:00:00 2001 From: Sameer Vijaykar Date: Thu, 2 Jun 2022 16:01:12 +0200 Subject: [PATCH] Reland "Refactor IceControllerEvent" 2/n This is a partial reland of commit 794e68cf3dbc3b16ee8b12b5615d8a1622154dbd which uses the new enum in P2PTransportChannel and BasicIceController. Original change's description: > Refactor IceControllerEvent > > This change is the first step in decoupling IceControllerEvent from > the ICE switch reason. Further cleanup is earmarked, and will be > landed after some internal cleanup. > > This change > - adds a new enum - IceSwitchReason > - adds a member for the new enum in IceControllerEvent > - uses the new enum within P2PTransportChannel > - adds methods to IceControllerInterface accepting the new enum > - deprecates usages of the old enum in IceControllerInterface > - adds conversion between the old and new enums for compatibility > > Bug: webrtc:14125, webrtc:14131 > Change-Id: I5b7201c3f631eb40db334dfeec842841a7e58174 > Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264140 > Reviewed-by: Jonas Oreland > Commit-Queue: Sameer Vijaykar > Cr-Commit-Position: refs/heads/main@{#37051} Bug: webrtc:14125, webrtc:14131 Change-Id: I81b0338ae2f0560cd3df7ad9bd9af37e7c3499df Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/264554 Reviewed-by: Jonas Oreland Commit-Queue: Sameer Vijaykar Cr-Commit-Position: refs/heads/main@{#37105} --- p2p/base/basic_ice_controller.cc | 17 +++++---- p2p/base/basic_ice_controller.h | 6 ++-- p2p/base/ice_controller_interface.h | 25 ++++++------- p2p/base/p2p_transport_channel.cc | 42 +++++++++++----------- p2p/base/p2p_transport_channel.h | 11 +++--- p2p/base/p2p_transport_channel_unittest.cc | 6 ++-- 6 files changed, 52 insertions(+), 55 deletions(-) diff --git a/p2p/base/basic_ice_controller.cc b/p2p/base/basic_ice_controller.cc index 9025fbe2a7..edd5681f5f 100644 --- a/p2p/base/basic_ice_controller.cc +++ b/p2p/base/basic_ice_controller.cc @@ -415,7 +415,7 @@ BasicIceController::GetBestWritableConnectionPerNetwork() const { IceControllerInterface::SwitchResult BasicIceController::HandleInitialSelectDampening( - IceControllerEvent reason, + IceSwitchReason reason, const Connection* new_connection) { if (!field_trials_->initial_select_dampening.has_value() && !field_trials_->initial_select_dampening_ping_received.has_value()) { @@ -464,13 +464,13 @@ BasicIceController::HandleInitialSelectDampening( } RTC_LOG(LS_INFO) << "delay initial selection up to " << min_delay << "ms"; - reason.type = IceControllerEvent::ICE_CONTROLLER_RECHECK; - reason.recheck_delay_ms = min_delay; - return {absl::nullopt, reason}; + return {.connection = absl::nullopt, + .recheck_event = IceControllerEvent( + IceSwitchReason::ICE_CONTROLLER_RECHECK, min_delay)}; } IceControllerInterface::SwitchResult BasicIceController::ShouldSwitchConnection( - IceControllerEvent reason, + IceSwitchReason reason, const Connection* new_connection) { if (!ReadyToSend(new_connection) || selected_connection_ == new_connection) { return {absl::nullopt, absl::nullopt}; @@ -503,9 +503,8 @@ IceControllerInterface::SwitchResult BasicIceController::ShouldSwitchConnection( // threshold, the new connection is in a better receiving state than the // currently selected connection. So we need to re-check whether it needs // to be switched at a later time. - recheck_event = reason; - recheck_event->recheck_delay_ms = - config_.receiving_switching_delay_or_default(); + recheck_event.emplace(reason, + config_.receiving_switching_delay_or_default()); } if (cmp < 0) { @@ -524,7 +523,7 @@ IceControllerInterface::SwitchResult BasicIceController::ShouldSwitchConnection( } IceControllerInterface::SwitchResult -BasicIceController::SortAndSwitchConnection(IceControllerEvent reason) { +BasicIceController::SortAndSwitchConnection(IceSwitchReason reason) { // Find the best alternative connection by sorting. It is important to note // that amongst equal preference, writable connections, this will choose the // one whose estimated latency is lowest. So it is the only one that we diff --git a/p2p/base/basic_ice_controller.h b/p2p/base/basic_ice_controller.h index e8861952c3..b941a0dd7e 100644 --- a/p2p/base/basic_ice_controller.h +++ b/p2p/base/basic_ice_controller.h @@ -46,9 +46,9 @@ class BasicIceController : public IceControllerInterface { NominationMode mode, IceMode remote_ice_mode) const override; - SwitchResult ShouldSwitchConnection(IceControllerEvent reason, + SwitchResult ShouldSwitchConnection(IceSwitchReason reason, const Connection* connection) override; - SwitchResult SortAndSwitchConnection(IceControllerEvent reason) override; + SwitchResult SortAndSwitchConnection(IceSwitchReason reason) override; std::vector PruneConnections() override; @@ -136,7 +136,7 @@ class BasicIceController : public IceControllerInterface { absl::optional receiving_unchanged_threshold, bool* missed_receiving_unchanged_threshold) const; - SwitchResult HandleInitialSelectDampening(IceControllerEvent reason, + SwitchResult HandleInitialSelectDampening(IceSwitchReason reason, const Connection* new_connection); std::function ice_transport_state_func_; diff --git a/p2p/base/ice_controller_interface.h b/p2p/base/ice_controller_interface.h index 52bbd5b873..dad2b70c6e 100644 --- a/p2p/base/ice_controller_interface.h +++ b/p2p/base/ice_controller_interface.h @@ -42,8 +42,8 @@ struct IceControllerEvent { ICE_CONTROLLER_RECHECK, }; - // TODO(bugs.webrtc.org/14125) remove. - IceControllerEvent(const Type& _type) // NOLINT: runtime/explicit + [[deprecated("bugs.webrtc.org/14125")]] IceControllerEvent( + const Type& _type) // NOLINT: runtime/explicit : type(_type), reason(FromType(_type)) {} IceControllerEvent(IceSwitchReason _reason, int _recheck_delay_ms) @@ -151,20 +151,21 @@ class IceControllerInterface { // Check if we should switch to `connection`. // This method is called for IceSwitchReasons that can switch directly // i.e without resorting. - virtual SwitchResult ShouldSwitchConnection(IceControllerEvent reason, - const Connection* connection) = 0; - virtual SwitchResult ShouldSwitchConnection(IceSwitchReason reason, - const Connection* connection) { - return ShouldSwitchConnection( - IceControllerEvent::FromIceSwitchReason(reason), connection); + [[deprecated("bugs.webrtc.org/14125")]] virtual SwitchResult + ShouldSwitchConnection(IceControllerEvent reason, + const Connection* connection) { + return ShouldSwitchConnection(IceControllerEvent::FromType(reason.type), + connection); } + virtual SwitchResult ShouldSwitchConnection(IceSwitchReason reason, + const Connection* connection) = 0; // Sort connections and check if we should switch. - virtual SwitchResult SortAndSwitchConnection(IceControllerEvent reason) = 0; - virtual SwitchResult SortAndSwitchConnection(IceSwitchReason reason) { - return SortAndSwitchConnection( - IceControllerEvent::FromIceSwitchReason(reason)); + [[deprecated("bugs.webrtc.org/14125")]] virtual SwitchResult + SortAndSwitchConnection(IceControllerEvent reason) { + return SortAndSwitchConnection(IceControllerEvent::FromType(reason.type)); } + virtual SwitchResult SortAndSwitchConnection(IceSwitchReason reason) = 0; // Prune connections. virtual std::vector PruneConnections() = 0; diff --git a/p2p/base/p2p_transport_channel.cc b/p2p/base/p2p_transport_channel.cc index c1e4cf7b7c..692d4dcdb5 100644 --- a/p2p/base/p2p_transport_channel.cc +++ b/p2p/base/p2p_transport_channel.cc @@ -284,7 +284,7 @@ void P2PTransportChannel::AddConnection(Connection* connection) { bool P2PTransportChannel::MaybeSwitchSelectedConnection( Connection* new_connection, - IceControllerEvent reason) { + IceSwitchReason reason) { RTC_DCHECK_RUN_ON(network_thread_); return MaybeSwitchSelectedConnection( @@ -292,12 +292,12 @@ bool P2PTransportChannel::MaybeSwitchSelectedConnection( } bool P2PTransportChannel::MaybeSwitchSelectedConnection( - IceControllerEvent reason, + IceSwitchReason reason, IceControllerInterface::SwitchResult result) { RTC_DCHECK_RUN_ON(network_thread_); if (result.connection.has_value()) { RTC_LOG(LS_INFO) << "Switching selected connection due to: " - << reason.ToString(); + << IceSwitchReasonToString(reason); SwitchSelectedConnection(FromIceController(*result.connection), reason); } @@ -308,8 +308,8 @@ bool P2PTransportChannel::MaybeSwitchSelectedConnection( // to be switched at a later time. network_thread_->PostDelayedTask( ToQueuedTask(task_safety_, - [this, recheck = *result.recheck_event]() { - SortConnectionsAndUpdateState(recheck); + [this, reason = result.recheck_event->reason]() { + SortConnectionsAndUpdateState(reason); }), result.recheck_event->recheck_delay_ms); } @@ -521,7 +521,7 @@ void P2PTransportChannel::SetRemoteIceParameters( } // Updating the remote ICE candidate generation could change the sort order. RequestSortAndStateUpdate( - IceControllerEvent::REMOTE_CANDIDATE_GENERATION_CHANGE); + IceSwitchReason::REMOTE_CANDIDATE_GENERATION_CHANGE); } void P2PTransportChannel::SetRemoteIceMode(IceMode mode) { @@ -675,7 +675,7 @@ void P2PTransportChannel::SetIceConfig(const IceConfig& config) { if (config_.network_preference != config.network_preference) { config_.network_preference = config.network_preference; - RequestSortAndStateUpdate(IceControllerEvent::NETWORK_PREFERENCE_CHANGE); + RequestSortAndStateUpdate(IceSwitchReason::NETWORK_PREFERENCE_CHANGE); RTC_LOG(LS_INFO) << "Set network preference to " << (config_.network_preference.has_value() ? config_.network_preference.value() @@ -979,7 +979,7 @@ void P2PTransportChannel::OnPortReady(PortAllocatorSession* session, } SortConnectionsAndUpdateState( - IceControllerEvent::NEW_CONNECTION_FROM_LOCAL_CANDIDATE); + IceSwitchReason::NEW_CONNECTION_FROM_LOCAL_CANDIDATE); } // A new candidate is available, let listeners know @@ -1159,7 +1159,7 @@ void P2PTransportChannel::OnUnknownAddress(PortInterface* port, // after sending the response since it could (in principle) delete the // connection in question. SortConnectionsAndUpdateState( - IceControllerEvent::NEW_CONNECTION_FROM_UNKNOWN_REMOTE_ADDRESS); + IceSwitchReason::NEW_CONNECTION_FROM_UNKNOWN_REMOTE_ADDRESS); } void P2PTransportChannel::OnCandidateFilterChanged(uint32_t prev_filter, @@ -1211,11 +1211,10 @@ void P2PTransportChannel::OnNominated(Connection* conn) { // TODO(qingsi): RequestSortAndStateUpdate will eventually call // MaybeSwitchSelectedConnection again. Rewrite this logic. if (MaybeSwitchSelectedConnection( - conn, IceControllerEvent::NOMINATION_ON_CONTROLLED_SIDE)) { + conn, IceSwitchReason::NOMINATION_ON_CONTROLLED_SIDE)) { // Now that we have selected a connection, it is time to prune other // connections and update the read/write state of the channel. - RequestSortAndStateUpdate( - IceControllerEvent::NOMINATION_ON_CONTROLLED_SIDE); + RequestSortAndStateUpdate(IceSwitchReason::NOMINATION_ON_CONTROLLED_SIDE); } else { RTC_LOG(LS_INFO) << "Not switching the selected connection on controlled side yet: " @@ -1365,7 +1364,7 @@ void P2PTransportChannel::FinishAddingRemoteCandidate( // Resort the connections list, which may have new elements. SortConnectionsAndUpdateState( - IceControllerEvent::NEW_CONNECTION_FROM_REMOTE_CANDIDATE); + IceSwitchReason::NEW_CONNECTION_FROM_REMOTE_CANDIDATE); } void P2PTransportChannel::RemoveRemoteCandidate( @@ -1718,7 +1717,7 @@ void P2PTransportChannel::UpdateConnectionStates() { // Prepare for best candidate sorting. void P2PTransportChannel::RequestSortAndStateUpdate( - IceControllerEvent reason_to_sort) { + IceSwitchReason reason_to_sort) { RTC_DCHECK_RUN_ON(network_thread_); if (!sort_dirty_) { network_thread_->PostTask( @@ -1768,7 +1767,7 @@ bool P2PTransportChannel::PresumedWritable(const Connection* conn) const { // Sort the available connections to find the best one. We also monitor // the number of available connections and the current state. void P2PTransportChannel::SortConnectionsAndUpdateState( - IceControllerEvent reason_to_sort) { + IceSwitchReason reason_to_sort) { RTC_DCHECK_RUN_ON(network_thread_); // Make sure the connection states are up-to-date since this affects how they @@ -1850,7 +1849,7 @@ rtc::NetworkRoute P2PTransportChannel::ConfigureNetworkRoute( // Change the selected connection, and let listeners know. void P2PTransportChannel::SwitchSelectedConnection(Connection* conn, - IceControllerEvent reason) { + IceSwitchReason reason) { RTC_DCHECK_RUN_ON(network_thread_); // Note: if conn is NULL, the previous `selected_connection_` has been // destroyed, so don't use it. @@ -1899,7 +1898,7 @@ void P2PTransportChannel::SwitchSelectedConnection(Connection* conn, // Create event for candidate pair change. if (selected_connection_) { CandidatePairChangeEvent pair_change; - pair_change.reason = reason.ToString(); + pair_change.reason = IceSwitchReasonToString(reason); pair_change.selected_candidate_pair = *GetSelectedCandidatePair(); pair_change.last_data_received_ms = selected_connection_->last_data_received(); @@ -2029,7 +2028,7 @@ void P2PTransportChannel::MaybeStopPortAllocatorSessions() { // RTC_RUN_ON(network_thread_) void P2PTransportChannel::OnSelectedConnectionDestroyed() { RTC_LOG(LS_INFO) << "Selected connection destroyed. Will choose a new one."; - IceControllerEvent reason = IceControllerEvent::SELECTED_CONNECTION_DESTROYED; + IceSwitchReason reason = IceSwitchReason::SELECTED_CONNECTION_DESTROYED; SwitchSelectedConnection(nullptr, reason); RequestSortAndStateUpdate(reason); } @@ -2159,8 +2158,8 @@ void P2PTransportChannel::OnConnectionStateChange(Connection* connection) { // We have to unroll the stack before doing this because we may be changing // the state of connections while sorting. RequestSortAndStateUpdate( - IceControllerEvent::CONNECT_STATE_CHANGE); // "candidate pair state - // changed"); + IceSwitchReason::CONNECT_STATE_CHANGE); // "candidate pair state + // changed"); } // When a connection is removed, edit it out, and then update our best @@ -2289,8 +2288,7 @@ void P2PTransportChannel::OnReadPacket(Connection* connection, // May need to switch the sending connection based on the receiving media path // if this is the controlled side. if (ice_role_ == ICEROLE_CONTROLLED) { - MaybeSwitchSelectedConnection(connection, - IceControllerEvent::DATA_RECEIVED); + MaybeSwitchSelectedConnection(connection, IceSwitchReason::DATA_RECEIVED); } } diff --git a/p2p/base/p2p_transport_channel.h b/p2p/base/p2p_transport_channel.h index a9a1886a1d..cf7de90d9e 100644 --- a/p2p/base/p2p_transport_channel.h +++ b/p2p/base/p2p_transport_channel.h @@ -49,6 +49,7 @@ #include "p2p/base/connection.h" #include "p2p/base/ice_controller_factory_interface.h" #include "p2p/base/ice_controller_interface.h" +#include "p2p/base/ice_switch_reason.h" #include "p2p/base/ice_transport_internal.h" #include "p2p/base/p2p_constants.h" #include "p2p/base/p2p_transport_channel_ice_field_trials.h" @@ -257,16 +258,16 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { bool ReadyToSend(const Connection* connection) const; bool PresumedWritable(const Connection* conn) const; void UpdateConnectionStates(); - void RequestSortAndStateUpdate(IceControllerEvent reason_to_sort); + void RequestSortAndStateUpdate(IceSwitchReason reason_to_sort); // Start pinging if we haven't already started, and we now have a connection // that's pingable. void MaybeStartPinging(); - void SortConnectionsAndUpdateState(IceControllerEvent reason_to_sort); + void SortConnectionsAndUpdateState(IceSwitchReason reason_to_sort); void SortConnections(); void SortConnectionsIfNeeded(); rtc::NetworkRoute ConfigureNetworkRoute(const Connection* conn); - void SwitchSelectedConnection(Connection* conn, IceControllerEvent reason); + void SwitchSelectedConnection(Connection* conn, IceSwitchReason reason); void UpdateState(); void HandleAllTimedOut(); void MaybeStopPortAllocatorSessions(); @@ -341,9 +342,9 @@ class RTC_EXPORT P2PTransportChannel : public IceTransportInternal { // Returns true if the new_connection is selected for transmission. bool MaybeSwitchSelectedConnection(Connection* new_connection, - IceControllerEvent reason); + IceSwitchReason reason); bool MaybeSwitchSelectedConnection( - IceControllerEvent reason, + IceSwitchReason reason, IceControllerInterface::SwitchResult result); void PruneConnections(); diff --git a/p2p/base/p2p_transport_channel_unittest.cc b/p2p/base/p2p_transport_channel_unittest.cc index 082f4853b2..ccf8e4e1ca 100644 --- a/p2p/base/p2p_transport_channel_unittest.cc +++ b/p2p/base/p2p_transport_channel_unittest.cc @@ -6097,15 +6097,13 @@ class ForgetLearnedStateController : public cricket::BasicIceController { const cricket::IceControllerFactoryArgs& args) : cricket::BasicIceController(args) {} - SwitchResult SortAndSwitchConnection(IceControllerEvent reason) override { + SwitchResult SortAndSwitchConnection(IceSwitchReason reason) override { auto result = cricket::BasicIceController::SortAndSwitchConnection(reason); if (forget_connnection_) { result.connections_to_forget_state_on.push_back(forget_connnection_); forget_connnection_ = nullptr; } - result.recheck_event = - IceControllerEvent(IceControllerEvent::ICE_CONTROLLER_RECHECK); - result.recheck_event->recheck_delay_ms = 100; + result.recheck_event.emplace(IceSwitchReason::ICE_CONTROLLER_RECHECK, 100); return result; }