Include sdpMid and sdpMLineIndex for ICECandidates returned by OneICE…

…Candidate (#2990)

#### Description
Currently, Pion returns an empty `sdpMid` and a 0 `sdpMLineIndex`. This
PR ensures Pion returns the corresponding `sdpMid` and `sdpMLineIndex`
for ICE candidates for clients that expects it. Fixes trickle issues.

#### Changes

1. `ICECandidates`: New fields `SDPMid` and `SDPMLineIndex`.
2. `ICEGatherer`: `SetMediaStreamIdentification` and return the correct
`SDPMid` and `SDPMLineIndex`.
3. `extractICEDetails`: Return a struct instead of multiple values.
4. `extractICEDetails` refactored the media description selection to a
different function.
5. Added new tests.

#### Reference issue
Fixes #2690
Fixes #1833

icecandidate.go

@@ -22,15 +22,17 @@ type ICECandidate struct {
 	RelatedAddress string           `json:"relatedAddress"`
 	RelatedPort    uint16           `json:"relatedPort"`
 	TCPType        string           `json:"tcpType"`
+	SDPMid         string           `json:"sdpMid"`
+	SDPMLineIndex  uint16           `json:"sdpMLineIndex"`
 }
 
 // Conversion for package ice
 
-func newICECandidatesFromICE(iceCandidates []ice.Candidate) ([]ICECandidate, error) {
+func newICECandidatesFromICE(iceCandidates []ice.Candidate, sdpMid string, sdpMLineIndex uint16) ([]ICECandidate, error) {
 	candidates := []ICECandidate{}
 
 	for _, i := range iceCandidates {
-		c, err := newICECandidateFromICE(i)
+		c, err := newICECandidateFromICE(i, sdpMid, sdpMLineIndex)
 		if err != nil {
 			return nil, err
 		}
@@ -40,7 +42,7 @@ func newICECandidatesFromICE(iceCandidates []ice.Candidate) ([]ICECandidate, err
 	return candidates, nil
 }
 
-func newICECandidateFromICE(i ice.Candidate) (ICECandidate, error) {
+func newICECandidateFromICE(i ice.Candidate, sdpMid string, sdpMLineIndex uint16) (ICECandidate, error) {
 	typ, err := convertTypeFromICE(i.Type())
 	if err != nil {
 		return ICECandidate{}, err
@@ -51,15 +53,17 @@ func newICECandidateFromICE(i ice.Candidate) (ICECandidate, error) {
 	}
 
 	c := ICECandidate{
-		statsID:    i.ID(),
-		Foundation: i.Foundation(),
-		Priority:   i.Priority(),
-		Address:    i.Address(),
-		Protocol:   protocol,
-		Port:       uint16(i.Port()),
-		Component:  i.Component(),
-		Typ:        typ,
-		TCPType:    i.TCPType().String(),
+		statsID:       i.ID(),
+		Foundation:    i.Foundation(),
+		Priority:      i.Priority(),
+		Address:       i.Address(),
+		Protocol:      protocol,
+		Port:          uint16(i.Port()),
+		Component:     i.Component(),
+		Typ:           typ,
+		TCPType:       i.TCPType().String(),
+		SDPMid:        sdpMid,
+		SDPMLineIndex: sdpMLineIndex,
 	}
 
 	if i.RelatedAddress() != nil {
@@ -155,8 +159,6 @@ func (c ICECandidate) String() string {
 // ToJSON returns an ICECandidateInit
 // as indicated by the spec https://w3c.github.io/webrtc-pc/#dom-rtcicecandidate-tojson
 func (c ICECandidate) ToJSON() ICECandidateInit {
-	zeroVal := uint16(0)
-	emptyStr := ""
 	candidateStr := ""
 
 	candidate, err := c.toICE()
@@ -166,7 +168,7 @@ func (c ICECandidate) ToJSON() ICECandidateInit {
 
 	return ICECandidateInit{
 		Candidate:     fmt.Sprintf("candidate:%s", candidateStr),
-		SDPMid:        &emptyStr,
-		SDPMLineIndex: &zeroVal,
+		SDPMid:        &c.SDPMid,
+		SDPMLineIndex: &c.SDPMLineIndex,
 	}
 }

icecandidate_test.go

@@ -167,6 +167,54 @@ func TestConvertTypeFromICE(t *testing.T) {
 	})
 }
 
+func TestNewIdentifiedICECandidateFromICE(t *testing.T) {
+	config := ice.CandidateHostConfig{
+		Network:    "udp",
+		Address:    "::1",
+		Port:       1234,
+		Component:  1,
+		Foundation: "foundation",
+		Priority:   128,
+	}
+	ice, err := ice.NewCandidateHost(&config)
+	assert.NoError(t, err)
+
+	ct, err := newICECandidateFromICE(ice, "1", 2)
+	assert.NoError(t, err)
+
+	assert.Equal(t, "1", ct.SDPMid)
+	assert.Equal(t, uint16(2), ct.SDPMLineIndex)
+}
+
+func TestNewIdentifiedICECandidatesFromICE(t *testing.T) {
+	ic, err := ice.NewCandidateHost(&ice.CandidateHostConfig{
+		Network:    "udp",
+		Address:    "::1",
+		Port:       1234,
+		Component:  1,
+		Foundation: "foundation",
+		Priority:   128,
+	})
+
+	assert.NoError(t, err)
+
+	candidates := []ice.Candidate{ic, ic, ic}
+
+	sdpMid := "1"
+	sdpMLineIndex := uint16(2)
+
+	results, err := newICECandidatesFromICE(candidates, sdpMid, sdpMLineIndex)
+
+	assert.NoError(t, err)
+
+	assert.Equal(t, 3, len(results))
+
+	for _, result := range results {
+		assert.Equal(t, sdpMid, result.SDPMid)
+		assert.Equal(t, sdpMLineIndex, result.SDPMLineIndex)
+	}
+}
+
 func TestICECandidate_ToJSON(t *testing.T) {
 	candidate := ICECandidate{
 		Foundation: "foundation",
@@ -183,3 +231,20 @@ func TestICECandidate_ToJSON(t *testing.T) {
 	assert.Equal(t, uint16(0), *candidateInit.SDPMLineIndex)
 	assert.Equal(t, "candidate:foundation 1 udp 128 1.0.0.1 1234 typ host", candidateInit.Candidate)
 }
+
+func TestICECandidateZeroSDPid(t *testing.T) {
+	candidate := ICECandidate{}
+
+	assert.Equal(t, candidate.SDPMid, "")
+	assert.Equal(t, candidate.SDPMLineIndex, uint16(0))
+}
+
+func TestICECandidateSDPMid_ToJSON(t *testing.T) {
+	candidate := ICECandidate{}
+
+	candidate.SDPMid = "0"
+	candidate.SDPMLineIndex = 1
+
+	assert.Equal(t, candidate.SDPMid, "0")
+	assert.Equal(t, candidate.SDPMLineIndex, uint16(1))
+}

icegatherer.go

@@ -37,6 +37,11 @@ type ICEGatherer struct {
 	onGatheringCompleteHandler atomic.Value // func()
 
 	api *API
+
+	// Used to set the corresponding media stream identification tag and media description index
+	// for ICE candidates generated by this gatherer.
+	sdpMid        atomic.Value  // string
+	sdpMLineIndex atomic.Uint32 // uint16
 }
 
 // NewICEGatherer creates a new NewICEGatherer.
@@ -60,6 +65,8 @@ func (api *API) NewICEGatherer(opts ICEGatherOptions) (*ICEGatherer, error) {
 		validatedServers: validatedServers,
 		api:              api,
 		log:              api.settingEngine.LoggerFactory.NewLogger("ice"),
+		sdpMid:           atomic.Value{},
+		sdpMLineIndex:    atomic.Uint32{},
 	}, nil
 }
 
@@ -169,8 +176,16 @@ func (g *ICEGatherer) Gather() error {
 			onGatheringCompleteHandler = handler
 		}
 
+		sdpMid := ""
+
+		if mid, ok := g.sdpMid.Load().(string); ok {
+			sdpMid = mid
+		}
+
+		sdpMLineIndex := uint16(g.sdpMLineIndex.Load())
+
 		if candidate != nil {
-			c, err := newICECandidateFromICE(candidate)
+			c, err := newICECandidateFromICE(candidate, sdpMid, sdpMLineIndex)
 			if err != nil {
 				g.log.Warnf("Failed to convert ice.Candidate: %s", err)
 				return
@@ -188,6 +203,12 @@ func (g *ICEGatherer) Gather() error {
 	return agent.GatherCandidates()
 }
 
+// set media stream identification tag and media description index for this gatherer
+func (g *ICEGatherer) setMediaStreamIdentification(mid string, mLineIndex uint16) {
+	g.sdpMid.Store(mid)
+	g.sdpMLineIndex.Store(uint32(mLineIndex))
+}
+
 // Close prunes all local candidates, and closes the ports.
 func (g *ICEGatherer) Close() error {
 	return g.close(false /* shouldGracefullyClose */)
@@ -264,7 +285,14 @@ func (g *ICEGatherer) GetLocalCandidates() ([]ICECandidate, error) {
 		return nil, err
 	}
 
-	return newICECandidatesFromICE(iceCandidates)
+	sdpMid := ""
+	if mid, ok := g.sdpMid.Load().(string); ok {
+		sdpMid = mid
+	}
+
+	sdpMLineIndex := uint16(g.sdpMLineIndex.Load())
+
+	return newICECandidatesFromICE(iceCandidates, sdpMid, sdpMLineIndex)
 }
 
 // OnLocalCandidate sets an event handler which fires when a new local ICE candidate is available

icegatherer_test.go

@@ -156,3 +156,72 @@ func TestICEGatherer_AlreadyClosed(t *testing.T) {
 		assert.ErrorIs(t, err, errICEAgentNotExist)
 	})
 }
+
+func TestNewICEGathererSetMediaStreamIdentification(t *testing.T) {
+	// Limit runtime in case of deadlocks
+	lim := test.TimeOut(time.Second * 20)
+	defer lim.Stop()
+
+	report := test.CheckRoutines(t)
+	defer report()
+
+	opts := ICEGatherOptions{
+		ICEServers: []ICEServer{{URLs: []string{"stun:stun.l.google.com:19302"}}},
+	}
+
+	gatherer, err := NewAPI().NewICEGatherer(opts)
+	if err != nil {
+		t.Error(err)
+	}
+
+	expectedMid := "5"
+	expectedMLineIndex := uint16(1)
+
+	gatherer.setMediaStreamIdentification(expectedMid, expectedMLineIndex)
+
+	if gatherer.State() != ICEGathererStateNew {
+		t.Fatalf("Expected gathering state new")
+	}
+
+	gatherFinished := make(chan struct{})
+	gatherer.OnLocalCandidate(func(i *ICECandidate) {
+		if i == nil {
+			close(gatherFinished)
+		} else {
+			assert.Equal(t, expectedMid, i.SDPMid)
+			assert.Equal(t, expectedMLineIndex, i.SDPMLineIndex)
+		}
+	})
+
+	if err = gatherer.Gather(); err != nil {
+		t.Error(err)
+	}
+
+	<-gatherFinished
+
+	params, err := gatherer.GetLocalParameters()
+	if err != nil {
+		t.Error(err)
+	}
+
+	if params.UsernameFragment == "" ||
+		params.Password == "" {
+		t.Fatalf("Empty local username or password frag")
+	}
+
+	candidates, err := gatherer.GetLocalCandidates()
+	if err != nil {
+		t.Error(err)
+	}
+
+	if len(candidates) == 0 {
+		t.Fatalf("No candidates gathered")
+	}
+
+	for _, c := range candidates {
+		assert.Equal(t, expectedMid, c.SDPMid)
+		assert.Equal(t, expectedMLineIndex, c.SDPMLineIndex)
+	}
+
+	assert.NoError(t, gatherer.Close())
+}

icetransport.go

@@ -57,12 +57,12 @@ func (t *ICETransport) GetSelectedCandidatePair() (*ICECandidatePair, error) {
 		return nil, err
 	}
 
-	local, err := newICECandidateFromICE(icePair.Local)
+	local, err := newICECandidateFromICE(icePair.Local, "", 0)
 	if err != nil {
 		return nil, err
 	}
 
-	remote, err := newICECandidateFromICE(icePair.Remote)
+	remote, err := newICECandidateFromICE(icePair.Remote, "", 0)
 	if err != nil {
 		return nil, err
 	}
@@ -118,7 +118,7 @@ func (t *ICETransport) Start(gatherer *ICEGatherer, params ICEParameters, role *
 		return err
 	}
 	if err := agent.OnSelectedCandidatePairChange(func(local, remote ice.Candidate) {
-		candidates, err := newICECandidatesFromICE([]ice.Candidate{local, remote})
+		candidates, err := newICECandidatesFromICE([]ice.Candidate{local, remote}, "", 0)
 		if err != nil {
 			t.log.Warnf("%w: %s", errICECandiatesCoversionFailed, err)
 			return

peerconnection.go

@@ -1017,6 +1017,11 @@ func (pc *PeerConnection) SetLocalDescription(desc SessionDescription) error {
 		})
 	}
 
+	mediaSection, ok := selectCandidateMediaSection(desc.parsed)
+	if ok {
+		pc.iceGatherer.setMediaStreamIdentification(mediaSection.SDPMid, mediaSection.SDPMLineIndex)
+	}
+
 	if pc.iceGatherer.State() == ICEGathererStateNew {
 		return pc.iceGatherer.Gather()
 	}
@@ -1151,26 +1156,26 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
 		}
 	}
 
-	remoteUfrag, remotePwd, candidates, err := extractICEDetails(desc.parsed, pc.log)
+	iceDetails, err := extractICEDetails(desc.parsed, pc.log)
 	if err != nil {
 		return err
 	}
 
-	if isRenegotiation && pc.iceTransport.haveRemoteCredentialsChange(remoteUfrag, remotePwd) {
+	if isRenegotiation && pc.iceTransport.haveRemoteCredentialsChange(iceDetails.Ufrag, iceDetails.Password) {
 		// An ICE Restart only happens implicitly for a SetRemoteDescription of type offer
 		if !weOffer {
 			if err = pc.iceTransport.restart(); err != nil {
 				return err
 			}
 		}
 
-		if err = pc.iceTransport.setRemoteCredentials(remoteUfrag, remotePwd); err != nil {
+		if err = pc.iceTransport.setRemoteCredentials(iceDetails.Ufrag, iceDetails.Password); err != nil {
 			return err
 		}
 	}
 
-	for i := range candidates {
-		if err = pc.iceTransport.AddRemoteCandidate(&candidates[i]); err != nil {
+	for i := range iceDetails.Candidates {
+		if err = pc.iceTransport.AddRemoteCandidate(&iceDetails.Candidates[i]); err != nil {
 			return err
 		}
 	}
@@ -1218,7 +1223,7 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
 	}
 
 	pc.ops.Enqueue(func() {
-		pc.startTransports(iceRole, dtlsRoleFromRemoteSDP(desc.parsed), remoteUfrag, remotePwd, fingerprint, fingerprintHash)
+		pc.startTransports(iceRole, dtlsRoleFromRemoteSDP(desc.parsed), iceDetails.Ufrag, iceDetails.Password, fingerprint, fingerprintHash)
 		if weOffer {
 			pc.startRTP(false, &desc, currentTransceivers)
 		}
@@ -1806,7 +1811,7 @@ func (pc *PeerConnection) AddICECandidate(candidate ICECandidateInit) error {
 			return err
 		}
 
-		c, err := newICECandidateFromICE(candidate)
+		c, err := newICECandidateFromICE(candidate, "", 0)
 		if err != nil {
 			return err
 		}