Repository: pions/webrtc
Pull request #546 (2019-03-22 22:07:53)

Add ICE candidate event handlers
#### Description

Add OnICECandidate and OnICEGatheringStateChange methods to PeerConnection. The main goal of this change is to improve API compatibility with the JavaScript/Wasm bindings. It does not actually add trickle ICE support or change the ICE candidate gathering process, which is still synchronous in the Go implementation. Rather, it fires the appropriate events similar to they way they would be fired in a true trickle ICE process.


albrow: This PR doesn't add all methods. Before we merge this PR, I plan to make another pass to see if there are any other easy changes to make the Go and JavaScript/Wasm APIs more similar.
coveralls: ## Pull Request Test Coverage Report for [Build 2634](https://coveralls.io/builds/22355926)

* **29** of **45**   **(64.44%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage decreased (**-0.08%**) to **81.31%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [peerconnection.go](https://coveralls.io/builds/22355926/source?filename=peerconnection.go#L257) | 29 | 45 | 64.44%
<!-- | **Total:** | **29** | **45** | **64.44%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/22355926/badge)](https://coveralls.io/builds/22355926) |
| :-- | --: |
| Change from base [Build 2628](https://coveralls.io/builds/22353606): |  -0.08% |
| Covered Lines: | 4120 |
| Relevant Lines: | 5067 |

---
##### 💛  - [Coveralls](https://coveralls.io)

albrow: There are still potentially more event handlers or methods to add in the future, but I've done all I want to do for this PR. The Go implementation and the JavaScript/Wasm bindings now have matching function signatures for `OnICECandidate` and `OnICEGatheringStateChange`.
albrow: @Sean-Der I applied changes based on your feedback. Ready for another look.
albrow: > Do you feel good about it testing wise? It might be nice to assert we get candidates or we got a nil, also another useful test is the blocking in the callbacks doesn't break anything.

Testing could definitely be improved, but I think it's good enough for now. The updated `signalPair` function uses the `OnICECandidate` event handler, and `signalPair` is used in a lot of places. `OnICEGatheringStateChange` doesn't have any test coverage but it's also really simple.
albrow: And yes, I'll squash before merging.

Patch:
diff --git a/examples/data-channels/jsfiddle/main.go b/examples/data-channels/jsfiddle/main.go
index dfb601cd..d75d0bad 100644
--- a/examples/data-channels/jsfiddle/main.go
+++ b/examples/data-channels/jsfiddle/main.go
@@ -39,24 +39,26 @@ func main() {
 		log(fmt.Sprintf("Message from DataChannel %s payload %s", sendChannel.Label(), string(msg.Data)))
 	})
 
+	// Create offer
+	offer, err := pc.CreateOffer(nil)
+	if err != nil {
+		handleError(err)
+	}
+	if err := pc.SetLocalDescription(offer); err != nil {
+		handleError(err)
+	}
+
 	// Add handlers for setting up the connection.
 	pc.OnICEConnectionStateChange(func(state webrtc.ICEConnectionState) {
 		log(fmt.Sprint(state))
 	})
-	pc.OnICECandidate(func(candidate *string) {
+	pc.OnICECandidate(func(candidate *webrtc.ICECandidate) {
 		if candidate != nil {
 			encodedDescr := signal.Encode(pc.LocalDescription())
 			el := getElementByID("localSessionDescription")
 			el.Set("value", encodedDescr)
 		}
 	})
-	pc.OnNegotiationNeeded(func() {
-		offer, err := pc.CreateOffer(nil)
-		if err != nil {
-			handleError(err)
-		}
-		pc.SetLocalDescription(offer)
-	})
 
 	// Set up global callbacks which will be triggered on button clicks.
 	js.Global().Set("sendMessage", js.FuncOf(func(_ js.Value, _ []js.Value) interface{} {
diff --git a/examples/data-channels/main.go b/examples/data-channels/main.go
index b9a33cd2..a688796a 100644
--- a/examples/data-channels/main.go
+++ b/examples/data-channels/main.go
@@ -5,7 +5,6 @@ import (
 	"time"
 
 	"github.com/pions/webrtc"
-
 	"github.com/pions/webrtc/examples/internal/signal"
 )
 
diff --git a/js_utils.go b/js_utils.go
index b234d1d7..f5636f02 100644
--- a/js_utils.go
+++ b/js_utils.go
@@ -92,6 +92,20 @@ func valueToUint8OrZero(val js.Value) uint8 {
 	return uint8(val.Int())
 }
 
+func valueToUint16OrZero(val js.Value) uint16 {
+	if val == js.Null() || val == js.Undefined() {
+		return 0
+	}
+	return uint16(val.Int())
+}
+
+func valueToUint32OrZero(val js.Value) uint32 {
+	if val == js.Null() || val == js.Undefined() {
+		return 0
+	}
+	return uint32(val.Int())
+}
+
 func valueToStrings(val js.Value) []string {
 	result := make([]string, val.Length())
 	for i := 0; i < val.Length(); i++ {
diff --git a/peerconnection.go b/peerconnection.go
index ddcbbbe6..81897acd 100644
--- a/peerconnection.go
+++ b/peerconnection.go
@@ -35,7 +35,7 @@ type PeerConnection struct {
 	currentRemoteDescription *SessionDescription
 	pendingRemoteDescription *SessionDescription
 	signalingState           SignalingState
-	iceGatheringState        ICEGatheringState // FIXME NOT-USED
+	iceGatheringState        ICEGatheringState
 	iceConnectionState       ICEConnectionState
 	connectionState          PeerConnectionState
 
@@ -53,16 +53,16 @@ type PeerConnection struct {
 	dataChannels map[uint16]*DataChannel
 
 	// OnNegotiationNeeded        func() // FIXME NOT-USED
-	// OnICECandidate             func() // FIXME NOT-USED
 	// OnICECandidateError        func() // FIXME NOT-USED
 
-	// OnICEGatheringStateChange  func() // FIXME NOT-USED
 	// OnConnectionStateChange    func() // FIXME NOT-USED
 
 	onSignalingStateChangeHandler     func(SignalingState)
 	onICEConnectionStateChangeHandler func(ICEConnectionState)
 	onTrackHandler                    func(*Track, *RTPReceiver)
 	onDataChannelHandler              func(*DataChannel)
+	onICECandidateHandler             func(*ICECandidate)
+	onICEGatheringStateChangeHandler  func()
 
 	iceGatherer   *ICEGatherer
 	iceTransport  *ICETransport
@@ -238,6 +238,61 @@ func (pc *PeerConnection) OnDataChannel(f func(*DataChannel)) {
 	pc.onDataChannelHandler = f
 }
 
+// OnICECandidate sets an event handler which is invoked when a new ICE
+// candidate is found.
+// BUG: trickle ICE is not supported so this event is triggered immediately when
+// SetLocalDescription is called. Typically, you only need to use this method
+// if you want API compatibility with the JavaScript/Wasm bindings.
+func (pc *PeerConnection) OnICECandidate(f func(*ICECandidate)) {
+	pc.mu.Lock()
+	defer pc.mu.Unlock()
+	pc.onICECandidateHandler = f
+}
+
+// OnICEGatheringStateChange sets an event handler which is invoked when the
+// ICE candidate gathering state has changed.
+// BUG: trickle ICE is not supported so this event is triggered immediately when
+// SetLocalDescription is called. Typically, you only need to use this method
+// if you want API compatibility with the JavaScript/Wasm bindings.
+func (pc *PeerConnection) OnICEGatheringStateChange(f func()) {
+	pc.mu.Lock()
+	defer pc.mu.Unlock()
+	pc.onICEGatheringStateChangeHandler = f
+}
+
+// signalICECandidateGatheringComplete should be called after ICE candidate
+// gathering is complete. It triggers the appropriate event handlers in order to
+// emulate a trickle ICE process.
+func (pc *PeerConnection) signalICECandidateGatheringComplete() error {
+	pc.mu.Lock()
+	defer pc.mu.Unlock()
+
+	// Call onICECandidateHandler for all candidates.
+	if pc.onICECandidateHandler != nil {
+		candidates, err := pc.iceGatherer.GetLocalCandidates()
+		if err != nil {
+			return err
+		}
+		for i := range candidates {
+			go pc.onICECandidateHandler(&candidates[i])
+		}
+		// Call the handler one last time with nil. This is a signal that candidate
+		// gathering is complete.
+		go pc.onICECandidateHandler(nil)
+	}
+
+	pc.iceGatheringState = ICEGatheringStateComplete
+
+	// Also trigger the onICEGatheringStateChangeHandler
+	if pc.onICEGatheringStateChangeHandler != nil {
+		// Note: Gathering is already done at this point, but some clients might
+		// still expect the state change handler to be triggered.
+		go pc.onICEGatheringStateChangeHandler()
+	}
+
+	return nil
+}
+
 // OnTrack sets an event handler which is called when remote track
 // arrives from a remote peer.
 func (pc *PeerConnection) OnTrack(f func(*Track, *RTPReceiver)) {
@@ -687,7 +742,18 @@ func (pc *PeerConnection) SetLocalDescription(desc SessionDescription) error {
 	if err := desc.parsed.Unmarshal([]byte(desc.SDP)); err != nil {
 		return err
 	}
-	return pc.setDescription(&desc, stateChangeOpSetLocal)
+	if err := pc.setDescription(&desc, stateChangeOpSetLocal); err != nil {
+		return err
+	}
+
+	// Call the appropriate event handlers to signal that ICE candidate gathering
+	// is complete. In reality it completed a while ago, but triggering these
+	// events helps maintain API compatibility with the JavaScript/Wasm bindings.
+	if err := pc.signalICECandidateGatheringComplete(); err != nil {
+		return err
+	}
+
+	return nil
 }
 
 // LocalDescription returns pendingLocalDescription if it is not null and
@@ -1510,7 +1576,6 @@ func (pc *PeerConnection) SignalingState() SignalingState {
 
 // ICEGatheringState attribute returns the ICE gathering state of the
 // PeerConnection instance.
-// FIXME NOT-USED
 func (pc *PeerConnection) ICEGatheringState() ICEGatheringState {
 	return pc.iceGatheringState
 }
diff --git a/peerconnection_js.go b/peerconnection_js.go
index e6d651e7..bf12678d 100644
--- a/peerconnection_js.go
+++ b/peerconnection_js.go
@@ -22,7 +22,7 @@ type PeerConnection struct {
 	onDataChannelHandler             *js.Func
 	onICEConectionStateChangeHandler *js.Func
 	onICECandidateHandler            *js.Func
-	onNegotiationNeededHandler       *js.Func
+	onICEGatheringStateChangeHandler *js.Func
 }
 
 // NewPeerConnection creates a peerconnection with the default
@@ -276,17 +276,15 @@ func (pc *PeerConnection) ICEConnectionState() ICEConnectionState {
 	return newICEConnectionState(pc.underlying.Get("iceConnectionState").String())
 }
 
-// TODO(albrow): This function doesn't exist in the Go implementation.
-// TODO(albrow): Follow the spec more closely. Handler should accept
-// RTCPeerConnectionIceEvent instead of *string.
-// https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/onicecandidate
-func (pc *PeerConnection) OnICECandidate(f func(candidate *string)) {
+// OnICECandidate sets an event handler which is invoked when a new ICE
+// candidate is found.
+func (pc *PeerConnection) OnICECandidate(f func(candidate *ICECandidate)) {
 	if pc.onICECandidateHandler != nil {
 		oldHandler := pc.onICECandidateHandler
 		defer oldHandler.Release()
 	}
 	onICECandidateHandler := js.FuncOf(func(this js.Value, args []js.Value) interface{} {
-		candidate := valueToStringPointer(args[0].Get("candidate"))
+		candidate := valueToICECandidate(args[0].Get("candidate"))
 		go f(candidate)
 		return js.Undefined()
 	})
@@ -294,18 +292,19 @@ func (pc *PeerConnection) OnICECandidate(f func(candidate *string)) {
 	pc.underlying.Set("onicecandidate", onICECandidateHandler)
 }
 
-// TODO(albrow): This function doesn't exist in the Go implementation.
-func (pc *PeerConnection) OnNegotiationNeeded(f func()) {
-	if pc.onNegotiationNeededHandler != nil {
-		oldHandler := pc.onNegotiationNeededHandler
+// OnICEGatheringStateChange sets an event handler which is invoked when the
+// ICE candidate gathering state has changed.
+func (pc *PeerConnection) OnICEGatheringStateChange(f func()) {
+	if pc.onICEGatheringStateChangeHandler != nil {
+		oldHandler := pc.onICEGatheringStateChangeHandler
 		defer oldHandler.Release()
 	}
-	onNegotiationNeededHandler := js.FuncOf(func(this js.Value, args []js.Value) interface{} {
+	onICEGatheringStateChangeHandler := js.FuncOf(func(this js.Value, args []js.Value) interface{} {
 		go f()
 		return js.Undefined()
 	})
-	pc.onNegotiationNeededHandler = &onNegotiationNeededHandler
-	pc.underlying.Set("onnegotiationneeded", onNegotiationNeededHandler)
+	pc.onICEGatheringStateChangeHandler = &onICEGatheringStateChangeHandler
+	pc.underlying.Set("onicegatheringstatechange", onICEGatheringStateChangeHandler)
 }
 
 // // GetSenders returns the RTPSender that are currently attached to this PeerConnection
@@ -384,8 +383,8 @@ func (pc *PeerConnection) Close() (err error) {
 	if pc.onICECandidateHandler != nil {
 		pc.onICECandidateHandler.Release()
 	}
-	if pc.onNegotiationNeededHandler != nil {
-		pc.onNegotiationNeededHandler.Release()
+	if pc.onICEGatheringStateChangeHandler != nil {
+		pc.onICEGatheringStateChangeHandler.Release()
 	}
 
 	return nil
@@ -529,6 +528,36 @@ func valueToICEServer(iceServerValue js.Value) ICEServer {
 	}
 }
 
+func valueToICECandidate(val js.Value) *ICECandidate {
+	if val == js.Null() || val == js.Undefined() {
+		return nil
+	}
+	protocol, _ := newICEProtocol(val.Get("protocol").String())
+	candidateType, _ := newICECandidateType(val.Get("type").String())
+	return &ICECandidate{
+		Foundation:     val.Get("foundation").String(),
+		Priority:       valueToUint32OrZero(val.Get("priority")),
+		IP:             val.Get("ip").String(),
+		Protocol:       protocol,
+		Port:           valueToUint16OrZero(val.Get("port")),
+		Typ:            candidateType,
+		Component:      stringToComponentIDOrZero(val.Get("component").String()),
+		RelatedAddress: val.Get("relatedAddress").String(),
+		RelatedPort:    valueToUint16OrZero(val.Get("relatedPort")),
+	}
+}
+
+func stringToComponentIDOrZero(val string) uint16 {
+	// See: https://developer.mozilla.org/en-US/docs/Web/API/RTCIceComponent
+	switch val {
+	case "rtp":
+		return 1
+	case "rtcp":
+		return 2
+	}
+	return 0
+}
+
 func sessionDescriptionToValue(desc *SessionDescription) js.Value {
 	if desc == nil {
 		return js.Undefined()


Test patch:
diff --git a/datachannel_go_test.go b/datachannel_go_test.go
index 6ecf2036..fd46b272 100644
--- a/datachannel_go_test.go
+++ b/datachannel_go_test.go
@@ -158,6 +158,12 @@ func TestDataChannelParamters_Go(t *testing.T) {
 		}
 
 		answerPC.OnDataChannel(func(d *DataChannel) {
+			// Make sure this is the data channel we were looking for. (Not the one
+			// created in signalPair).
+			if d.Label() != expectedLabel {
+				return
+			}
+
 			// Check if parameters are correctly set
 			assert.True(t, d.ordered, "Ordered should be set to true")
 			if assert.NotNil(t, d.maxPacketLifeTime, "should not be nil") {
diff --git a/peerconnection_go_test.go b/peerconnection_go_test.go
index 3ef75b84..e0546a23 100644
--- a/peerconnection_go_test.go
+++ b/peerconnection_go_test.go
@@ -34,39 +34,6 @@ func (api *API) newPair() (pcOffer *PeerConnection, pcAnswer *PeerConnection, er
 
 	return pca, pcb, nil
 }
-
-func signalPair(pcOffer *PeerConnection, pcAnswer *PeerConnection) error {
-	offer, err := pcOffer.CreateOffer(nil)
-	if err != nil {
-		return err
-	}
-
-	if err = pcOffer.SetLocalDescription(offer); err != nil {
-		return err
-	}
-
-	err = pcAnswer.SetRemoteDescription(offer)
-	if err != nil {
-		return err
-	}
-
-	answer, err := pcAnswer.CreateAnswer(nil)
-	if err != nil {
-		return err
-	}
-
-	if err = pcAnswer.SetLocalDescription(answer); err != nil {
-		return err
-	}
-
-	err = pcOffer.SetRemoteDescription(answer)
-	if err != nil {
-		return err
-	}
-
-	return nil
-}
-
 func TestNew_Go(t *testing.T) {
 	api := NewAPI()
 	t.Run("Success", func(t *testing.T) {
diff --git a/peerconnection_js_test.go b/peerconnection_js_test.go
deleted file mode 100644
index da171a5c..00000000
--- a/peerconnection_js_test.go
+++ /dev/null
@@ -1,57 +0,0 @@
-// +build js,wasm
-
-package webrtc
-
-import (
-	"fmt"
-	"time"
-)
-
-func signalPair(pcOffer *PeerConnection, pcAnswer *PeerConnection) (err error) {
-	offerChan := make(chan SessionDescription)
-	pcOffer.OnICECandidate(func(candidate *string) {
-		if candidate == nil {
-			offerChan <- *pcOffer.PendingLocalDescription()
-		}
-	})
-
-	// Note(albrow): We need to create a data channel in order to trigger ICE
-	// candidate gathering in the background.
-	if _, err := pcOffer.CreateDataChannel("initial_data_channel", nil); err != nil {
-		return err
-	}
-
-	offer, err := pcOffer.CreateOffer(nil)
-	if err != nil {
-		return err
-	}
-	if err := pcOffer.SetLocalDescription(offer); err != nil {
-		return err
-	}
-
-	timeout := time.After(3 * time.Second)
-	select {
-	case <-timeout:
-		return fmt.Errorf("timed out waiting to receive offer")
-	case offer := <-offerChan:
-		if err := pcAnswer.SetRemoteDescription(offer); err != nil {
-			return err
-		}
-
-		answer, err := pcAnswer.CreateAnswer(nil)
-		if err != nil {
-			return err
-		}
-
-		if err = pcAnswer.SetLocalDescription(answer); err != nil {
-			return err
-		}
-
-		err = pcOffer.SetRemoteDescription(answer)
-		if err != nil {
-			return err
-		}
-		return nil
-	}
-	return nil
-}
diff --git a/peerconnection_test.go b/peerconnection_test.go
index 37c67e34..309d922b 100644
--- a/peerconnection_test.go
+++ b/peerconnection_test.go
@@ -1,6 +1,7 @@
 package webrtc
 
 import (
+	"fmt"
 	"reflect"
 	"sync"
 	"testing"
@@ -26,6 +27,56 @@ func newPair() (pcOffer *PeerConnection, pcAnswer *PeerConnection, err error) {
 	return pca, pcb, nil
 }
 
+func signalPair(pcOffer *PeerConnection, pcAnswer *PeerConnection) error {
+	offerChan := make(chan SessionDescription)
+	pcOffer.OnICECandidate(func(candidate *ICECandidate) {
+		if candidate == nil {
+			offerChan <- *pcOffer.PendingLocalDescription()
+		}
+	})
+
+	// Note(albrow): We need to create a data channel in order to trigger ICE
+	// candidate gathering in the background for the JavaScript/Wasm bindings. If
+	// we don't do this, the complete offer including ICE candidates will never be
+	// generated.
+	if _, err := pcOffer.CreateDataChannel("initial_data_channel", nil); err != nil {
+		return err
+	}
+
+	offer, err := pcOffer.CreateOffer(nil)
+	if err != nil {
+		return err
+	}
+	if err := pcOffer.SetLocalDescription(offer); err != nil {
+		return err
+	}
+
+	timeout := time.After(3 * time.Second)
+	select {
+	case <-timeout:
+		return fmt.Errorf("timed out waiting to receive offer")
+	case offer := <-offerChan:
+		if err := pcAnswer.SetRemoteDescription(offer); err != nil {
+			return err
+		}
+
+		answer, err := pcAnswer.CreateAnswer(nil)
+		if err != nil {
+			return err
+		}
+
+		if err = pcAnswer.SetLocalDescription(answer); err != nil {
+			return err
+		}
+
+		err = pcOffer.SetRemoteDescription(answer)
+		if err != nil {
+			return err
+		}
+		return nil
+	}
+}
+
 func TestNew(t *testing.T) {
 	pc, err := NewPeerConnection(Configuration{
 		ICEServers: []ICEServer{