From 0751e671a481998a0daf71b6828b37537c45b7e4 Mon Sep 17 00:00:00 2001 From: Steffen Vogel Date: Mon, 15 May 2023 23:18:00 +0200 Subject: [PATCH] Minor style fixes for active TCP candidate support --- agent.go | 2 +- agent_active_tcp_test.go | 14 ++++----- candidate_base.go | 61 +++++++++++++++++++++++++--------------- candidate_test.go | 52 ++++++++++++++++++++++++++++++++++ candidatetype.go | 13 +++------ candidatetype_test.go | 42 --------------------------- gather.go | 28 +++++++++--------- 7 files changed, 116 insertions(+), 96 deletions(-) delete mode 100644 candidatetype_test.go diff --git a/agent.go b/agent.go index a6439092..0f8e8c95 100644 --- a/agent.go +++ b/agent.go @@ -623,7 +623,7 @@ func (a *Agent) addPair(local, remote Candidate) *CandidatePair { a.log.Errorf("Failed to cast local candidate to CandidateHost") return nil } - localCandidateHost.port = localAddress.Port // this causes a data race with candidateBase.Port() + localCandidateHost.port = localAddress.Port // This causes a data race with candidateBase.Port() local.start(a, packetConn, a.startedCh) } p := newCandidatePair(local, remote, a.isControlling) diff --git a/agent_active_tcp_test.go b/agent_active_tcp_test.go index 0b11cf2e..43697d1d 100644 --- a/agent_active_tcp_test.go +++ b/agent_active_tcp_test.go @@ -48,6 +48,7 @@ func TestAgentActiveTCP(t *testing.T) { listenIPAddress net.IP selectedPairNetworkType string } + testCases := []testCase{ { name: "TCP4 connection", @@ -56,7 +57,7 @@ func TestAgentActiveTCP(t *testing.T) { selectedPairNetworkType: tcp, }, { - name: "UDP is preferred over TCP4", // fails some time + name: "UDP is preferred over TCP4", // This fails some time networkTypes: supportedNetworkTypes(), listenIPAddress: getLocalIPAddress(t, NetworkTypeTCP4), selectedPairNetworkType: udp, @@ -64,21 +65,20 @@ func TestAgentActiveTCP(t *testing.T) { } if ipv6Available(t) { - tcpv6Cases := []testCase{ - { + testCases = append(testCases, + testCase{ name: "TCP6 connection", networkTypes: []NetworkType{NetworkTypeTCP6}, listenIPAddress: getLocalIPAddress(t, NetworkTypeTCP6), selectedPairNetworkType: tcp, }, - { - name: "UDP is preferred over TCP6", // fails some time + testCase{ + name: "UDP is preferred over TCP6", // This fails some time networkTypes: supportedNetworkTypes(), listenIPAddress: getLocalIPAddress(t, NetworkTypeTCP6), selectedPairNetworkType: udp, }, - } - testCases = append(testCases, tcpv6Cases...) + ) } for _, testCase := range testCases { diff --git a/candidate_base.go b/candidate_base.go index 4c9e75b8..d9547f0e 100644 --- a/candidate_base.go +++ b/candidate_base.go @@ -188,6 +188,44 @@ func (c *candidateBase) LocalPreference() uint16 { return defaultLocalPreference } +// TypePreference returns the type preference for this candidate +func (c *candidateBase) TypePreference() uint16 { + pref := c.Type().Preference() + if pref == 0 { + return 0 + } + + if c.NetworkType().IsTCP() { + var tcpPriorityOffset uint16 = defaultTCPPriorityOffset + if c.agent() != nil { + tcpPriorityOffset = c.agent().tcpPriorityOffset + } + + pref -= tcpPriorityOffset + } + + return pref +} + +// Priority computes the priority for this ICE Candidate +// See: https://www.rfc-editor.org/rfc/rfc8445#section-5.1.2.1 +func (c *candidateBase) Priority() uint32 { + if c.priorityOverride != 0 { + return c.priorityOverride + } + + // The local preference MUST be an integer from 0 (lowest preference) to + // 65535 (highest preference) inclusive. When there is only a single IP + // address, this value SHOULD be set to 65535. If there are multiple + // candidates for a particular component for a particular data stream + // that have the same type, the local preference MUST be unique for each + // one. + + return (1<<24)*uint32(c.TypePreference()) + + (1<<8)*uint32(c.LocalPreference()) + + (1<<0)*uint32(256-c.Component()) +} + // RelatedAddress returns *CandidateRelatedAddress func (c *candidateBase) RelatedAddress() *CandidateRelatedAddress { return c.relatedAddress @@ -343,29 +381,6 @@ func (c *candidateBase) writeTo(raw []byte, dst Candidate) (int, error) { return n, nil } -// Priority computes the priority for this ICE Candidate -func (c *candidateBase) Priority() uint32 { - if c.priorityOverride != 0 { - return c.priorityOverride - } - - // The local preference MUST be an integer from 0 (lowest preference) to - // 65535 (highest preference) inclusive. When there is only a single IP - // address, this value SHOULD be set to 65535. If there are multiple - // candidates for a particular component for a particular data stream - // that have the same type, the local preference MUST be unique for each - // one. - - var tcpPriorityOffset uint16 = defaultTCPPriorityOffset - if c.agent() != nil { - tcpPriorityOffset = c.agent().tcpPriorityOffset - } - - return (1<<24)*uint32(c.Type().Preference(c.networkType, tcpPriorityOffset)) + - (1<<8)*uint32(c.LocalPreference()) + - uint32(256-c.Component()) -} - // Equal is used to compare two candidateBases func (c *candidateBase) Equal(other Candidate) bool { return c.NetworkType() == other.NetworkType() && diff --git a/candidate_test.go b/candidate_test.go index 3d2b696b..753633b0 100644 --- a/candidate_test.go +++ b/candidate_test.go @@ -13,6 +13,58 @@ import ( "github.com/stretchr/testify/require" ) +func TestCandidateTypePreference(t *testing.T) { + r := require.New(t) + + hostDefaultPreference := uint16(126) + prflxDefaultPreference := uint16(110) + srflxDefaultPreference := uint16(100) + relayDefaultPreference := uint16(0) + + tcpOffsets := []uint16{0, 10} + + for _, tcpOffset := range tcpOffsets { + agent := &Agent{ + tcpPriorityOffset: tcpOffset, + } + + for _, networkType := range supportedNetworkTypes() { + hostCandidate := candidateBase{ + candidateType: CandidateTypeHost, + networkType: networkType, + currAgent: agent, + } + prflxCandidate := candidateBase{ + candidateType: CandidateTypePeerReflexive, + networkType: networkType, + currAgent: agent, + } + srflxCandidate := candidateBase{ + candidateType: CandidateTypeServerReflexive, + networkType: networkType, + currAgent: agent, + } + relayCandidate := candidateBase{ + candidateType: CandidateTypeRelay, + networkType: networkType, + currAgent: agent, + } + + if networkType.IsTCP() { + r.Equal(hostDefaultPreference-tcpOffset, hostCandidate.TypePreference()) + r.Equal(prflxDefaultPreference-tcpOffset, prflxCandidate.TypePreference()) + r.Equal(srflxDefaultPreference-tcpOffset, srflxCandidate.TypePreference()) + } else { + r.Equal(hostDefaultPreference, hostCandidate.TypePreference()) + r.Equal(prflxDefaultPreference, prflxCandidate.TypePreference()) + r.Equal(srflxDefaultPreference, srflxCandidate.TypePreference()) + } + + r.Equal(relayDefaultPreference, relayCandidate.TypePreference()) + } + } +} + func TestCandidatePriority(t *testing.T) { for _, test := range []struct { Candidate Candidate diff --git a/candidatetype.go b/candidatetype.go index 2a231bad..a1351fa8 100644 --- a/candidatetype.go +++ b/candidatetype.go @@ -38,24 +38,19 @@ func (c CandidateType) String() string { // The RECOMMENDED values are 126 for host candidates, 100 // for server reflexive candidates, 110 for peer reflexive candidates, // and 0 for relayed candidates. -func (c CandidateType) Preference(networkType NetworkType, tcpOffset uint16) uint16 { - var result uint16 +func (c CandidateType) Preference() uint16 { switch c { case CandidateTypeHost: - result = 126 + return 126 case CandidateTypePeerReflexive: - result = 110 + return 110 case CandidateTypeServerReflexive: - result = 100 + return 100 case CandidateTypeRelay, CandidateTypeUnspecified: return 0 default: return 0 } - if networkType.IsTCP() { - return result - tcpOffset - } - return result } func containsCandidateType(candidateType CandidateType, candidateTypeList []CandidateType) bool { diff --git a/candidatetype_test.go b/candidatetype_test.go deleted file mode 100644 index 1bc89df6..00000000 --- a/candidatetype_test.go +++ /dev/null @@ -1,42 +0,0 @@ -// SPDX-FileCopyrightText: 2023 The Pion community -// SPDX-License-Identifier: MIT - -package ice - -import ( - "testing" - - "github.com/stretchr/testify/require" -) - -func TestCandidateTypePreference(t *testing.T) { - r := require.New(t) - - hostDefaultPreference := uint16(126) - prflxDefaultPreference := uint16(110) - srflxDefaultPreference := uint16(100) - relayDefaultPreference := uint16(0) - unspecifiedDefaultPreference := uint16(0) - - tcpOffsets := []uint16{0, 10} - - for _, tcpOffset := range tcpOffsets { - for _, networkType := range supportedNetworkTypes() { - if networkType.IsTCP() { - r.Equal(hostDefaultPreference-tcpOffset, CandidateTypeHost.Preference(networkType, tcpOffset)) - r.Equal(prflxDefaultPreference-tcpOffset, CandidateTypePeerReflexive.Preference(networkType, tcpOffset)) - r.Equal(srflxDefaultPreference-tcpOffset, CandidateTypeServerReflexive.Preference(networkType, tcpOffset)) - } else { - r.Equal(hostDefaultPreference, CandidateTypeHost.Preference(networkType, tcpOffset)) - r.Equal(prflxDefaultPreference, CandidateTypePeerReflexive.Preference(networkType, tcpOffset)) - r.Equal(srflxDefaultPreference, CandidateTypeServerReflexive.Preference(networkType, tcpOffset)) - } - } - } - for _, tcpOffset := range tcpOffsets { - for _, networkType := range supportedNetworkTypes() { - r.Equal(relayDefaultPreference, CandidateTypeRelay.Preference(networkType, tcpOffset)) - r.Equal(unspecifiedDefaultPreference, CandidateTypeUnspecified.Preference(networkType, tcpOffset)) - } - } -} diff --git a/gather.go b/gather.go index 05f1874e..2a2802a9 100644 --- a/gather.go +++ b/gather.go @@ -25,7 +25,7 @@ const ( stunGatherTimeout = time.Second * 5 ) -type connAndPort struct { +type connConfig struct { conn net.PacketConn port int tcpType TCPType @@ -161,18 +161,18 @@ func (a *Agent) gatherCandidatesLocal(ctx context.Context, networkTypes []Networ } for network := range networks { - var connectionConfigurations []connAndPort + var connConfigs []connConfig switch network { case tcp: // Handle ICE TCP active mode - connectionConfigurations = append(connectionConfigurations, connAndPort{nil, 0, TCPTypeActive}) + connConfigs = append(connConfigs, connConfig{nil, 0, TCPTypeActive}) // Handle ICE TCP passive mode if a.tcpMux != nil { - connectionConfigurations = a.getTCPMuxConnections(mappedIP, ip, network, connectionConfigurations) + connConfigs = a.getTCPMuxConns(mappedIP, ip, network, connConfigs) } - if len(connectionConfigurations) == 0 { + if len(connConfigs) == 0 { // Didn't succeed with any, try the next network. continue } @@ -186,36 +186,36 @@ func (a *Agent) gatherCandidatesLocal(ctx context.Context, networkTypes []Networ } if udpConn, ok := conn.LocalAddr().(*net.UDPAddr); ok { - connectionConfigurations = append(connectionConfigurations, connAndPort{conn, udpConn.Port, TCPTypeUnspecified}) + connConfigs = append(connConfigs, connConfig{conn, udpConn.Port, TCPTypeUnspecified}) } else { a.log.Warnf("Failed to get port of UDPAddr from ListenUDPInPortRange: %s %s %s", network, ip, a.localUfrag) continue } } - for _, connectionConfiguration := range connectionConfigurations { + for _, connConfig := range connConfigs { hostConfig := CandidateHostConfig{ Network: network, Address: address, - Port: connectionConfiguration.port, + Port: connConfig.port, Component: ComponentRTP, - TCPType: connectionConfiguration.tcpType, + TCPType: connConfig.tcpType, } c, err := NewCandidateHost(&hostConfig) if err != nil { - closeConnAndLog(connectionConfiguration.conn, a.log, "failed to create host candidate: %s %s %d: %v", network, mappedIP, connectionConfiguration.port, err) + closeConnAndLog(connConfig.conn, a.log, "failed to create host candidate: %s %s %d: %v", network, mappedIP, connConfig.port, err) continue } if a.mDNSMode == MulticastDNSModeQueryAndGather { if err = c.setIP(ip); err != nil { - closeConnAndLog(connectionConfiguration.conn, a.log, "failed to create host candidate: %s %s %d: %v", network, mappedIP, connectionConfiguration.port, err) + closeConnAndLog(connConfig.conn, a.log, "failed to create host candidate: %s %s %d: %v", network, mappedIP, connConfig.port, err) continue } } - if err := a.addCandidate(ctx, c, connectionConfiguration.conn); err != nil { + if err := a.addCandidate(ctx, c, connConfig.conn); err != nil { if closeErr := c.close(); closeErr != nil { a.log.Warnf("Failed to close candidate: %v", closeErr) } @@ -226,7 +226,7 @@ func (a *Agent) gatherCandidatesLocal(ctx context.Context, networkTypes []Networ } } -func (a *Agent) getTCPMuxConnections(mappedIP net.IP, ip net.IP, network string, conns []connAndPort) []connAndPort { +func (a *Agent) getTCPMuxConns(mappedIP net.IP, ip net.IP, network string, conns []connConfig) []connConfig { var muxConns []net.PacketConn if multi, ok := a.tcpMux.(AllConnsGetter); ok { a.log.Debugf("GetAllConns by ufrag: %s", a.localUfrag) @@ -249,7 +249,7 @@ func (a *Agent) getTCPMuxConnections(mappedIP net.IP, ip net.IP, network string, // Extract the port for each PacketConn we got. for _, conn := range muxConns { if tcpConn, ok := conn.LocalAddr().(*net.TCPAddr); ok { - conns = append(conns, connAndPort{conn, tcpConn.Port, TCPTypePassive}) + conns = append(conns, connConfig{conn, tcpConn.Port, TCPTypePassive}) } else { a.log.Warnf("Failed to get port of connection from TCPMux: %s %s %s", network, ip, a.localUfrag) }