Repository: pion/webrtc (go)

Issue:
OnSelectedCandidatePairChange doesn't work
### Your environment.
  - Version: v2.0.22
  - Browser: n/a

### What did you do?
Attempted to set callback handler via OnSelectedCandidatePairChange() in order to learn the resulting selected-pair for diagnostic purpose.

My app is DC-only.  iceTranport is obtained via dc => sctpTransport => dtlsTransprot => iceTransport...

### What did you expect?
The callback would be made to report the current selected-pair.

### What happened?
#### Offerer:
* Not possible to set the callback because dc.sctpTransport is nil
* When OnOpen is called, sctpTransport was non-nil, but callback was already made (too late)
#### Answerer:
* dc.sctpTransport is never set


Pull request:
Add SCTPTransport getter to PeerConnection
This is needed so users can use OnSelectedCandidatePairChange
for DataChannel use cases.

Resolves #713

Patch:
diff --git a/peerconnection.go b/peerconnection.go
index 8ed94155..4ef3ec47 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -330,10 +330,6 @@ func (pc *PeerConnection) checkNegotiationNeeded() bool { //nolint:gocognit
 		return true
 	}
 
-	if pc.sctpTransport == nil {
-		return true
-	}
-
 	pc.sctpTransport.lock.Lock()
 	lenDataChannel := len(pc.sctpTransport.dataChannels)
 	pc.sctpTransport.lock.Unlock()
@@ -1773,13 +1769,11 @@ func (pc *PeerConnection) Close() error {
 	}
 
 	// https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #5)
-	if pc.sctpTransport != nil {
-		pc.sctpTransport.lock.Lock()
-		for _, d := range pc.sctpTransport.dataChannels {
-			d.setReadyState(DataChannelStateClosed)
-		}
-		pc.sctpTransport.lock.Unlock()
+	pc.sctpTransport.lock.Lock()
+	for _, d := range pc.sctpTransport.dataChannels {
+		d.setReadyState(DataChannelStateClosed)
 	}
+	pc.sctpTransport.lock.Unlock()
 
 	// https://www.w3.org/TR/webrtc/#dom-rtcpeerconnection-close (step #6)
 	if pc.sctpTransport != nil {
@@ -1916,24 +1910,22 @@ func (pc *PeerConnection) GetStats() StatsReport {
 		pc.iceTransport.collectStats(statsCollector)
 	}
 
-	if pc.sctpTransport != nil {
-		pc.sctpTransport.lock.Lock()
-		dataChannels := append([]*DataChannel{}, pc.sctpTransport.dataChannels...)
-		dataChannelsAccepted = pc.sctpTransport.dataChannelsAccepted
-		dataChannelsOpened = pc.sctpTransport.dataChannelsOpened
-		dataChannelsRequested = pc.sctpTransport.dataChannelsRequested
-		pc.sctpTransport.lock.Unlock()
-
-		for _, d := range dataChannels {
-			state := d.ReadyState()
-			if state != DataChannelStateConnecting && state != DataChannelStateOpen {
-				dataChannelsClosed++
-			}
+	pc.sctpTransport.lock.Lock()
+	dataChannels := append([]*DataChannel{}, pc.sctpTransport.dataChannels...)
+	dataChannelsAccepted = pc.sctpTransport.dataChannelsAccepted
+	dataChannelsOpened = pc.sctpTransport.dataChannelsOpened
+	dataChannelsRequested = pc.sctpTransport.dataChannelsRequested
+	pc.sctpTransport.lock.Unlock()
 
-			d.collectStats(statsCollector)
+	for _, d := range dataChannels {
+		state := d.ReadyState()
+		if state != DataChannelStateConnecting && state != DataChannelStateOpen {
+			dataChannelsClosed++
 		}
-		pc.sctpTransport.collectStats(statsCollector)
+
+		d.collectStats(statsCollector)
 	}
+	pc.sctpTransport.collectStats(statsCollector)
 
 	stats := PeerConnectionStats{
 		Timestamp:             statsTimestampNow(),
@@ -2232,3 +2224,11 @@ func (pc *PeerConnection) generateMatchedSDP(transceivers []*RTPTransceiver, use
 func (pc *PeerConnection) setGatherCompleteHandler(handler func()) {
 	pc.iceGatherer.onGatheringCompleteHandler.Store(handler)
 }
+
+// SCTP returns the SCTPTransport for this PeerConnection
+//
+// The SCTP transport over which SCTP data is sent and received. If SCTP has not been negotiated, the value is nil.
+// https://www.w3.org/TR/webrtc/#attributes-15
+func (pc *PeerConnection) SCTP() *SCTPTransport {
+	return pc.sctpTransport
+}


Test patch:
diff --git a/icetransport_test.go b/icetransport_test.go
index 3fb5262c..7002157b 100644
--- a/icetransport_test.go
+++ b/icetransport_test.go
@@ -7,7 +7,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/pion/randutil"
 	"github.com/pion/transport/test"
 	"github.com/stretchr/testify/assert"
 )
@@ -19,21 +18,11 @@ func TestICETransport_OnSelectedCandidatePairChange(t *testing.T) {
 	lim := test.TimeOut(time.Second * 30)
 	defer lim.Stop()
 
-	api := NewAPI()
-	api.mediaEngine.RegisterDefaultCodecs()
-	pcOffer, pcAnswer, err := api.newPair(Configuration{})
+	pcOffer, pcAnswer, err := newPair()
 	if err != nil {
 		t.Fatal(err)
 	}
 
-	opusTrack, err := pcOffer.NewTrack(DefaultPayloadTypeOpus, randutil.NewMathRandomGenerator().Uint32(), "audio", "pion1")
-	if err != nil {
-		t.Fatal(err)
-	}
-	if _, err = pcOffer.AddTrack(opusTrack); err != nil {
-		t.Fatal(err)
-	}
-
 	iceComplete := make(chan bool)
 	pcAnswer.OnICEConnectionStateChange(func(iceState ICEConnectionState) {
 		if iceState == ICEConnectionStateConnected {
@@ -43,17 +32,9 @@ func TestICETransport_OnSelectedCandidatePairChange(t *testing.T) {
 	})
 
 	senderCalledCandidateChange := int32(0)
-	for _, sender := range pcOffer.GetSenders() {
-		dtlsTransport := sender.Transport()
-		if dtlsTransport == nil {
-			continue
-		}
-		if iceTransport := dtlsTransport.ICETransport(); iceTransport != nil {
-			iceTransport.OnSelectedCandidatePairChange(func(pair *ICECandidatePair) {
-				atomic.StoreInt32(&senderCalledCandidateChange, 1)
-			})
-		}
-	}
+	pcOffer.SCTP().Transport().ICETransport().OnSelectedCandidatePairChange(func(pair *ICECandidatePair) {
+		atomic.StoreInt32(&senderCalledCandidateChange, 1)
+	})
 
 	assert.NoError(t, signalPair(pcOffer, pcAnswer))
 	<-iceComplete
diff --git a/peerconnection_renegotiation_test.go b/peerconnection_renegotiation_test.go
index 09795bf2..dce217c1 100644
--- a/peerconnection_renegotiation_test.go
+++ b/peerconnection_renegotiation_test.go
@@ -662,9 +662,7 @@ func TestPeerConnection_Renegotiation_NoApplication(t *testing.T) {
 		assert.NoError(t, pcOffer.SetLocalDescription(offer))
 		<-offerGatheringComplete
 
-		offer = *pcOffer.LocalDescription()
-		offer.SDP = strings.Split(offer.SDP, "m=application")[0]
-		assert.NoError(t, pcAnswer.SetRemoteDescription(offer))
+		assert.NoError(t, pcAnswer.SetRemoteDescription(*pcOffer.LocalDescription()))
 
 		answer, err := pcAnswer.CreateAnswer(nil)
 		assert.NoError(t, err)
@@ -689,15 +687,26 @@ func TestPeerConnection_Renegotiation_NoApplication(t *testing.T) {
 		t.Fatal(err)
 	}
 
+	pcOfferConnected, pcOfferConnectedCancel := context.WithCancel(context.Background())
+	pcOffer.OnICEConnectionStateChange(func(i ICEConnectionState) {
+		if i == ICEConnectionStateConnected {
+			pcOfferConnectedCancel()
+		}
+	})
+
+	pcAnswerConnected, pcAnswerConnectedCancel := context.WithCancel(context.Background())
+	pcAnswer.OnICEConnectionStateChange(func(i ICEConnectionState) {
+		if i == ICEConnectionStateConnected {
+			pcAnswerConnectedCancel()
+		}
+	})
+
 	_, err = pcOffer.AddTransceiverFromKind(RTPCodecTypeVideo, RtpTransceiverInit{Direction: RTPTransceiverDirectionSendrecv})
 	assert.NoError(t, err)
 
 	_, err = pcAnswer.AddTransceiverFromKind(RTPCodecTypeVideo, RtpTransceiverInit{Direction: RTPTransceiverDirectionSendrecv})
 	assert.NoError(t, err)
 
-	// Setting SCTPTransport to nil ensures any interaction with it will cause a segafault
-	pcAnswer.sctpTransport = nil
-
 	signalPairExcludeDataChannel(pcOffer, pcAnswer)
 	pcOffer.ops.Done()
 	pcAnswer.ops.Done()
@@ -706,6 +715,12 @@ func TestPeerConnection_Renegotiation_NoApplication(t *testing.T) {
 	pcOffer.ops.Done()
 	pcAnswer.ops.Done()
 
+	<-pcAnswerConnected.Done()
+	<-pcOfferConnected.Done()
+
+	assert.Equal(t, pcOffer.SCTP().State(), SCTPTransportStateConnecting)
+	assert.Equal(t, pcAnswer.SCTP().State(), SCTPTransportStateConnecting)
+
 	assert.NoError(t, pcOffer.Close())
 	assert.NoError(t, pcAnswer.Close())
 }