From c4dd03dfcbe5577f47e1fa9d735be1ecb170f98e Mon Sep 17 00:00:00 2001 From: Tommi Date: Wed, 31 Jan 2024 10:44:27 +0100 Subject: [PATCH] Remove kUnknown as a possible value for IceCandidateType. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subsequently also tighten IceCandidateType error checking. The Candidate type in `cricket` should be using something similar (currently using a string for the type), so I'm making sure that types that we have already, align with where we'd like to be overall. Possibly we can move IceCandidateType to where Candidate is defined. Bug: none Change-Id: Iffeba7268f2a393e18a5f33249efae46e6e08252 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/335980 Reviewed-by: Björn Terelius Commit-Queue: Tomas Gunnarsson Cr-Commit-Position: refs/heads/main@{#41640} --- .../encoder/rtc_event_log_encoder_legacy.cc | 8 +- .../rtc_event_log_encoder_new_format.cc | 2 - .../rtc_event_ice_candidate_pair_config.cc | 8 +- .../rtc_event_ice_candidate_pair_config.h | 4 +- logging/rtc_event_log/ice_logger.cc | 3 +- logging/rtc_event_log/rtc_event_log_parser.cc | 74 +++++++++++++------ .../rtc_event_log_unittest_helper.cc | 4 +- p2p/base/connection.cc | 10 +-- rtc_tools/rtc_event_log_to_text/converter.cc | 3 +- .../rtc_event_log_visualizer/analyzer.cc | 30 ++++---- 10 files changed, 84 insertions(+), 62 deletions(-) diff --git a/logging/rtc_event_log/encoder/rtc_event_log_encoder_legacy.cc b/logging/rtc_event_log/encoder/rtc_event_log_encoder_legacy.cc index 2c1444af07..ba1e597ad8 100644 --- a/logging/rtc_event_log/encoder/rtc_event_log_encoder_legacy.cc +++ b/logging/rtc_event_log/encoder/rtc_event_log_encoder_legacy.cc @@ -131,8 +131,6 @@ ConvertIceCandidatePairConfigType(IceCandidatePairConfigType type) { rtclog::IceCandidatePairConfig::IceCandidateType ConvertIceCandidateType( IceCandidateType type) { switch (type) { - case IceCandidateType::kUnknown: - return rtclog::IceCandidatePairConfig::UNKNOWN_CANDIDATE_TYPE; case IceCandidateType::kLocal: return rtclog::IceCandidatePairConfig::LOCAL; case IceCandidateType::kStun: @@ -141,11 +139,11 @@ rtclog::IceCandidatePairConfig::IceCandidateType ConvertIceCandidateType( return rtclog::IceCandidatePairConfig::PRFLX; case IceCandidateType::kRelay: return rtclog::IceCandidatePairConfig::RELAY; - case IceCandidateType::kNumValues: + default: + // TODO(tommi): Remove the default handler when kNumValues is gone. RTC_DCHECK_NOTREACHED(); + return rtclog::IceCandidatePairConfig::UNKNOWN_CANDIDATE_TYPE; } - RTC_DCHECK_NOTREACHED(); - return rtclog::IceCandidatePairConfig::UNKNOWN_CANDIDATE_TYPE; } rtclog::IceCandidatePairConfig::Protocol ConvertIceCandidatePairProtocol( diff --git a/logging/rtc_event_log/encoder/rtc_event_log_encoder_new_format.cc b/logging/rtc_event_log/encoder/rtc_event_log_encoder_new_format.cc index 01bd89718d..a07afbe48a 100644 --- a/logging/rtc_event_log/encoder/rtc_event_log_encoder_new_format.cc +++ b/logging/rtc_event_log/encoder/rtc_event_log_encoder_new_format.cc @@ -198,8 +198,6 @@ ConvertToProtoFormat(IceCandidatePairConfigType type) { rtclog2::IceCandidatePairConfig::IceCandidateType ConvertToProtoFormat( IceCandidateType type) { switch (type) { - case IceCandidateType::kUnknown: - return rtclog2::IceCandidatePairConfig::UNKNOWN_CANDIDATE_TYPE; case IceCandidateType::kLocal: return rtclog2::IceCandidatePairConfig::LOCAL; case IceCandidateType::kStun: diff --git a/logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.cc b/logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.cc index eb458c4640..59d76e7a03 100644 --- a/logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.cc +++ b/logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.cc @@ -14,12 +14,14 @@ namespace webrtc { -IceCandidatePairDescription::IceCandidatePairDescription() { - local_candidate_type = IceCandidateType::kUnknown; +IceCandidatePairDescription::IceCandidatePairDescription( + IceCandidateType local_candidate_type, + IceCandidateType remote_candidate_type) + : local_candidate_type(local_candidate_type), + remote_candidate_type(remote_candidate_type) { local_relay_protocol = IceCandidatePairProtocol::kUnknown; local_network_type = IceCandidateNetworkType::kUnknown; local_address_family = IceCandidatePairAddressFamily::kUnknown; - remote_candidate_type = IceCandidateType::kUnknown; remote_address_family = IceCandidatePairAddressFamily::kUnknown; candidate_pair_protocol = IceCandidatePairProtocol::kUnknown; } diff --git a/logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h b/logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h index e72d999cff..a44f530e77 100644 --- a/logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h +++ b/logging/rtc_event_log/events/rtc_event_ice_candidate_pair_config.h @@ -35,7 +35,6 @@ enum class IceCandidatePairConfigType { // TODO(qingsi): Change the names of candidate types to "host", "srflx", "prflx" // and "relay" after the naming is spec-compliant in the signaling part enum class IceCandidateType { - kUnknown, kLocal, kStun, kPrflx, @@ -88,7 +87,8 @@ struct LoggedIceCandidatePairConfig { class IceCandidatePairDescription { public: - IceCandidatePairDescription(); + IceCandidatePairDescription(IceCandidateType local_candidate_type, + IceCandidateType remote_candidate_type); explicit IceCandidatePairDescription( const IceCandidatePairDescription& other); diff --git a/logging/rtc_event_log/ice_logger.cc b/logging/rtc_event_log/ice_logger.cc index 390deda953..daa7057439 100644 --- a/logging/rtc_event_log/ice_logger.cc +++ b/logging/rtc_event_log/ice_logger.cc @@ -26,7 +26,8 @@ void IceEventLog::LogCandidatePairConfig( if (event_log_ == nullptr) { return; } - candidate_pair_desc_by_id_[candidate_pair_id] = candidate_pair_desc; + + candidate_pair_desc_by_id_.emplace(candidate_pair_id, candidate_pair_desc); event_log_->Log(std::make_unique( type, candidate_pair_id, candidate_pair_desc)); } diff --git a/logging/rtc_event_log/rtc_event_log_parser.cc b/logging/rtc_event_log/rtc_event_log_parser.cc index 37bb70a69b..c53b1dd59a 100644 --- a/logging/rtc_event_log/rtc_event_log_parser.cc +++ b/logging/rtc_event_log/rtc_event_log_parser.cc @@ -163,22 +163,29 @@ IceCandidatePairConfigType GetRuntimeIceCandidatePairConfigType( return IceCandidatePairConfigType::kAdded; } -IceCandidateType GetRuntimeIceCandidateType( - rtclog::IceCandidatePairConfig::IceCandidateType type) { - switch (type) { +// Converts a log type (proto based) to a matching `IceCandidateType` value +// and checks for validity of the log type (since the enums aren't a perfect +// match). +bool GetRuntimeIceCandidateType( + rtclog::IceCandidatePairConfig::IceCandidateType log_type, + IceCandidateType& parsed_type) { + switch (log_type) { case rtclog::IceCandidatePairConfig::LOCAL: - return IceCandidateType::kLocal; + parsed_type = IceCandidateType::kLocal; + break; case rtclog::IceCandidatePairConfig::STUN: - return IceCandidateType::kStun; + parsed_type = IceCandidateType::kStun; + break; case rtclog::IceCandidatePairConfig::PRFLX: - return IceCandidateType::kPrflx; + parsed_type = IceCandidateType::kPrflx; + break; case rtclog::IceCandidatePairConfig::RELAY: - return IceCandidateType::kRelay; - case rtclog::IceCandidatePairConfig::UNKNOWN_CANDIDATE_TYPE: - return IceCandidateType::kUnknown; + parsed_type = IceCandidateType::kRelay; + break; + default: + return false; } - RTC_DCHECK_NOTREACHED(); - return IceCandidateType::kUnknown; + return true; } IceCandidatePairProtocol GetRuntimeIceCandidatePairProtocol( @@ -813,11 +820,32 @@ IceCandidateType GetRuntimeIceCandidateType( return IceCandidateType::kPrflx; case rtclog2::IceCandidatePairConfig::RELAY: return IceCandidateType::kRelay; - case rtclog2::IceCandidatePairConfig::UNKNOWN_CANDIDATE_TYPE: - return IceCandidateType::kUnknown; + default: + RTC_DCHECK_NOTREACHED(); + return IceCandidateType::kLocal; } - RTC_DCHECK_NOTREACHED(); - return IceCandidateType::kUnknown; +} + +bool GetRuntimeIceCandidateType( + rtclog2::IceCandidatePairConfig::IceCandidateType log_type, + IceCandidateType& parsed_type) { + switch (log_type) { + case rtclog2::IceCandidatePairConfig::LOCAL: + parsed_type = IceCandidateType::kLocal; + break; + case rtclog2::IceCandidatePairConfig::STUN: + parsed_type = IceCandidateType::kStun; + break; + case rtclog2::IceCandidatePairConfig::PRFLX: + parsed_type = IceCandidateType::kPrflx; + break; + case rtclog2::IceCandidatePairConfig::RELAY: + parsed_type = IceCandidateType::kRelay; + break; + default: + return false; + } + return true; } IceCandidatePairProtocol GetRuntimeIceCandidatePairProtocol( @@ -2142,8 +2170,8 @@ ParsedRtcEventLog::GetIceCandidatePairConfig( RTC_PARSE_CHECK_OR_RETURN(config.has_candidate_pair_id()); res.candidate_pair_id = config.candidate_pair_id(); RTC_PARSE_CHECK_OR_RETURN(config.has_local_candidate_type()); - res.local_candidate_type = - GetRuntimeIceCandidateType(config.local_candidate_type()); + RTC_PARSE_CHECK_OR_RETURN(GetRuntimeIceCandidateType( + config.local_candidate_type(), res.local_candidate_type)); RTC_PARSE_CHECK_OR_RETURN(config.has_local_relay_protocol()); res.local_relay_protocol = GetRuntimeIceCandidatePairProtocol(config.local_relay_protocol()); @@ -2154,8 +2182,8 @@ ParsedRtcEventLog::GetIceCandidatePairConfig( res.local_address_family = GetRuntimeIceCandidatePairAddressFamily(config.local_address_family()); RTC_PARSE_CHECK_OR_RETURN(config.has_remote_candidate_type()); - res.remote_candidate_type = - GetRuntimeIceCandidateType(config.remote_candidate_type()); + RTC_PARSE_CHECK_OR_RETURN(GetRuntimeIceCandidateType( + config.remote_candidate_type(), res.remote_candidate_type)); RTC_PARSE_CHECK_OR_RETURN(config.has_remote_address_family()); res.remote_address_family = GetRuntimeIceCandidatePairAddressFamily(config.remote_address_family()); @@ -3498,8 +3526,8 @@ ParsedRtcEventLog::ParseStatus ParsedRtcEventLog::StoreIceCandidatePairConfig( RTC_PARSE_CHECK_OR_RETURN(proto.has_candidate_pair_id()); ice_config.candidate_pair_id = proto.candidate_pair_id(); RTC_PARSE_CHECK_OR_RETURN(proto.has_local_candidate_type()); - ice_config.local_candidate_type = - GetRuntimeIceCandidateType(proto.local_candidate_type()); + RTC_PARSE_CHECK_OR_RETURN(GetRuntimeIceCandidateType( + proto.local_candidate_type(), ice_config.local_candidate_type)); RTC_PARSE_CHECK_OR_RETURN(proto.has_local_relay_protocol()); ice_config.local_relay_protocol = GetRuntimeIceCandidatePairProtocol(proto.local_relay_protocol()); @@ -3510,8 +3538,8 @@ ParsedRtcEventLog::ParseStatus ParsedRtcEventLog::StoreIceCandidatePairConfig( ice_config.local_address_family = GetRuntimeIceCandidatePairAddressFamily(proto.local_address_family()); RTC_PARSE_CHECK_OR_RETURN(proto.has_remote_candidate_type()); - ice_config.remote_candidate_type = - GetRuntimeIceCandidateType(proto.remote_candidate_type()); + RTC_PARSE_CHECK_OR_RETURN(GetRuntimeIceCandidateType( + proto.remote_candidate_type(), ice_config.remote_candidate_type)); RTC_PARSE_CHECK_OR_RETURN(proto.has_remote_address_family()); ice_config.remote_address_family = GetRuntimeIceCandidatePairAddressFamily(proto.remote_address_family()); diff --git a/logging/rtc_event_log/rtc_event_log_unittest_helper.cc b/logging/rtc_event_log/rtc_event_log_unittest_helper.cc index aece4b5753..1aff19fc34 100644 --- a/logging/rtc_event_log/rtc_event_log_unittest_helper.cc +++ b/logging/rtc_event_log/rtc_event_log_unittest_helper.cc @@ -238,12 +238,10 @@ EventGenerator::NewIceCandidatePairConfig() { static_cast(prng_.Rand( static_cast(IceCandidatePairProtocol::kNumValues) - 1)); - IceCandidatePairDescription desc; - desc.local_candidate_type = local_candidate_type; + IceCandidatePairDescription desc(local_candidate_type, remote_candidate_type); desc.local_relay_protocol = protocol_type; desc.local_network_type = local_network_type; desc.local_address_family = local_address_family; - desc.remote_candidate_type = remote_candidate_type; desc.remote_address_family = remote_address_family; desc.candidate_pair_protocol = protocol_type; diff --git a/p2p/base/connection.cc b/p2p/base/connection.cc index a2e3dfce41..cd5166af06 100644 --- a/p2p/base/connection.cc +++ b/p2p/base/connection.cc @@ -82,10 +82,9 @@ webrtc::IceCandidateType GetRtcEventLogCandidateType(const Candidate& c) { return webrtc::IceCandidateType::kStun; } else if (c.is_prflx()) { return webrtc::IceCandidateType::kPrflx; - } else if (c.is_relay()) { - return webrtc::IceCandidateType::kRelay; } - return webrtc::IceCandidateType::kUnknown; + RTC_DCHECK(c.is_relay()); + return webrtc::IceCandidateType::kRelay; } webrtc::IceCandidatePairProtocol GetProtocolByString( @@ -1366,14 +1365,13 @@ const webrtc::IceCandidatePairDescription& Connection::ToLogDescription() { const Candidate& local = local_candidate(); const Candidate& remote = remote_candidate(); const rtc::Network* network = port()->Network(); - log_description_ = webrtc::IceCandidatePairDescription(); - log_description_->local_candidate_type = GetRtcEventLogCandidateType(local); + log_description_ = webrtc::IceCandidatePairDescription( + GetRtcEventLogCandidateType(local), GetRtcEventLogCandidateType(remote)); log_description_->local_relay_protocol = GetProtocolByString(local.relay_protocol()); log_description_->local_network_type = ConvertNetworkType(network->type()); log_description_->local_address_family = GetAddressFamilyByInt(local.address().family()); - log_description_->remote_candidate_type = GetRtcEventLogCandidateType(remote); log_description_->remote_address_family = GetAddressFamilyByInt(remote.address().family()); log_description_->candidate_pair_protocol = diff --git a/rtc_tools/rtc_event_log_to_text/converter.cc b/rtc_tools/rtc_event_log_to_text/converter.cc index 90d568f30f..35a780f043 100644 --- a/rtc_tools/rtc_event_log_to_text/converter.cc +++ b/rtc_tools/rtc_event_log_to_text/converter.cc @@ -231,8 +231,7 @@ bool Convert(std::string inputfile, {IceCandidatePairConfigType::kNumValues, "NUM_VALUES"}}; static const std::map - candidate_type_name{{IceCandidateType::kUnknown, "UNKNOWN"}, - {IceCandidateType::kLocal, "LOCAL"}, + candidate_type_name{{IceCandidateType::kLocal, "LOCAL"}, {IceCandidateType::kStun, "STUN"}, {IceCandidateType::kPrflx, "PRFLX"}, {IceCandidateType::kRelay, "RELAY"}, diff --git a/rtc_tools/rtc_event_log_visualizer/analyzer.cc b/rtc_tools/rtc_event_log_visualizer/analyzer.cc index 1d8d5f12c0..33a7ce3933 100644 --- a/rtc_tools/rtc_event_log_visualizer/analyzer.cc +++ b/rtc_tools/rtc_event_log_visualizer/analyzer.cc @@ -238,7 +238,9 @@ TimeSeries CreateRtcpTypeTimeSeries(const std::vector& rtcp_list, const char kUnknownEnumValue[] = "unknown"; +// TODO(tommi): This should be "host". const char kIceCandidateTypeLocal[] = "local"; +// TODO(tommi): This should be "srflx". const char kIceCandidateTypeStun[] = "stun"; const char kIceCandidateTypePrflx[] = "prflx"; const char kIceCandidateTypeRelay[] = "relay"; @@ -257,17 +259,18 @@ const char kNetworkTypeWifi[] = "wifi"; const char kNetworkTypeVpn[] = "vpn"; const char kNetworkTypeCellular[] = "cellular"; -std::string GetIceCandidateTypeAsString(webrtc::IceCandidateType type) { +absl::string_view GetIceCandidateTypeAsString(IceCandidateType type) { switch (type) { - case webrtc::IceCandidateType::kLocal: + case IceCandidateType::kLocal: return kIceCandidateTypeLocal; - case webrtc::IceCandidateType::kStun: + case IceCandidateType::kStun: return kIceCandidateTypeStun; - case webrtc::IceCandidateType::kPrflx: + case IceCandidateType::kPrflx: return kIceCandidateTypePrflx; - case webrtc::IceCandidateType::kRelay: + case IceCandidateType::kRelay: return kIceCandidateTypeRelay; default: + RTC_DCHECK_NOTREACHED(); return kUnknownEnumValue; } } @@ -323,18 +326,15 @@ std::string GetCandidatePairLogDescriptionAsString( // and a remote relay candidate using TCP as the relay protocol on a cell // network, when the candidate pair communicates over UDP using IPv4. rtc::StringBuilder ss; - std::string local_candidate_type = - GetIceCandidateTypeAsString(config.local_candidate_type); - std::string remote_candidate_type = - GetIceCandidateTypeAsString(config.remote_candidate_type); - if (config.local_candidate_type == webrtc::IceCandidateType::kRelay) { - local_candidate_type += - "(" + GetProtocolAsString(config.local_relay_protocol) + ")"; + ss << GetIceCandidateTypeAsString(config.local_candidate_type); + + if (config.local_candidate_type == IceCandidateType::kRelay) { + ss << "(" << GetProtocolAsString(config.local_relay_protocol) << ")"; } - ss << local_candidate_type << ":" - << GetNetworkTypeAsString(config.local_network_type) << ":" + + ss << ":" << GetNetworkTypeAsString(config.local_network_type) << ":" << GetAddressFamilyAsString(config.local_address_family) << "->" - << remote_candidate_type << ":" + << GetIceCandidateTypeAsString(config.remote_candidate_type) << ":" << GetAddressFamilyAsString(config.remote_address_family) << "@" << GetProtocolAsString(config.candidate_pair_protocol); return ss.Release();