Skip to content

Commit

Permalink
Add ToJSON ICECandidate method
Browse files Browse the repository at this point in the history 
With this change we can always exchange ICECandidateInit when signaling
  • Loading branch information
@hugoArregui
Hugo Arregui authored and hugoArregui committed Jul 31, 2019
1 parent 1464ad4 commit 7c18bbc
Show file tree
Hide file tree
Showing 5 changed files with 38 additions and 23 deletions.
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ require (
github.com/pion/rtcp v1.2.1
github.com/pion/rtp v1.1.3
github.com/pion/sctp v1.6.4
github.com/pion/sdp/v2 v2.2.0
github.com/pion/sdp/v2 v2.3.0
github.com/pion/srtp v1.2.6
github.com/pion/transport v0.8.6
github.com/stretchr/testify v1.3.0
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ github.com/pion/rtp v1.1.3/go.mod h1:/l4cvcKd0D3u9JLs2xSVI95YkfXW87a3br3nqmVtSlE
github.com/pion/sctp v1.6.3/go.mod h1:cCqpLdYvgEUdl715+qbWtgT439CuQrAgy8BZTp0aEfA=
github.com/pion/sctp v1.6.4 h1:edUNxTabSErLWOdeUUAxds8gwx5kGnFam4zL5DWpILk=
github.com/pion/sctp v1.6.4/go.mod h1:cCqpLdYvgEUdl715+qbWtgT439CuQrAgy8BZTp0aEfA=
github.com/pion/sdp/v2 v2.2.0 h1:JiixCEU8g6LbSsh1Bg5SOk0TPnJrn2HBOA1yJ+mRYhI=
github.com/pion/sdp/v2 v2.2.0/go.mod h1:idSlWxhfWQDtTy9J05cgxpHBu/POwXN2VDRGYxT/EjU=
github.com/pion/sdp/v2 v2.3.0 h1:5EhwPh1xKWYYjjvMuubHoMLy6M0B9U26Hh7q3f7vEGk=
github.com/pion/sdp/v2 v2.3.0/go.mod h1:idSlWxhfWQDtTy9J05cgxpHBu/POwXN2VDRGYxT/EjU=
github.com/pion/srtp v1.2.6 h1:mHQuAMh0P67R7/j1F260u3O+fbRWLyjKLRPZYYvODFM=
github.com/pion/srtp v1.2.6/go.mod h1:rd8imc5htjfs99XiEoOjLMEOcVjME63UHx9Ek9IGst0=
github.com/pion/stun v0.3.1 h1:d09JJzOmOS8ZzIp8NppCMgrxGZpJ4Ix8qirfNYyI3BA=
Expand Down
14 changes: 0 additions & 14 deletions ice_go.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,17 +53,3 @@ func newICECandidateFromSDP(c sdp.ICECandidate) (ICECandidate, error) {
RelatedPort: c.RelatedPort,
}, nil
}

func iceCandidateToSDP(c ICECandidate) sdp.ICECandidate {
return sdp.ICECandidate{
Foundation: c.Foundation,
Priority: c.Priority,
Address: c.Address,
Protocol: c.Protocol.String(),
Port: c.Port,
Component: c.Component,
Typ: c.Typ.String(),
RelatedAddress: c.RelatedAddress,
RelatedPort: c.RelatedPort,
}
}
22 changes: 22 additions & 0 deletions icecandidate.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"fmt"

"github.com/pion/ice"
"github.com/pion/sdp/v2"
)

// ICECandidate represents a ice candidate
Expand Down Expand Up @@ -137,3 +138,24 @@ func (c ICECandidate) String() string {
}
return ic.String()
}

func iceCandidateToSDP(c ICECandidate) sdp.ICECandidate {
return sdp.ICECandidate{
Foundation: c.Foundation,
Priority: c.Priority,
Address: c.Address,
Protocol: c.Protocol.String(),
Port: c.Port,
Component: c.Component,
Typ: c.Typ.String(),
RelatedAddress: c.RelatedAddress,
RelatedPort: c.RelatedPort,
}
}

// ToJSON returns an ICECandidateInit
// as indicated by the spec https://w3c.github.io/webrtc-pc/#dom-rtcicecandidate-tojson
func (c ICECandidate) ToJSON() ICECandidateInit {
var sdpmLineIndex uint16
return ICECandidateInit{Candidate: iceCandidateToSDP(c).Marshal(), SDPMLineIndex: &sdpmLineIndex}
}
19 changes: 13 additions & 6 deletions peerconnection.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -825,17 +825,21 @@ func (pc *PeerConnection) SetLocalDescription(desc SessionDescription) error {
return err
}

// To support all unittests which are following the future trickle=true
// setup while also support the old trickle=false synchronous gathering
// process this is necessary to avoid calling Garther() in multiple
// pleces; which causes race conditions. (issue-707)
if !pc.iceGatherer.agentIsTrickle {
// To support all unittests which are following the future trickle=true
// setup while also support the old trickle=false synchronous gathering
// process this is necessary to avoid calling Garther() in multiple
// pleces; which causes race conditions. (issue-707)
if err := pc.iceGatherer.SignalCandidates(); err != nil {
return err
}
return nil
}
return pc.iceGatherer.Gather()

if desc.Type == SDPTypeAnswer {
return pc.iceGatherer.Gather()
}
return nil
}

// LocalDescription returns pendingLocalDescription if it is not null and
Expand All @@ -850,7 +854,7 @@ func (pc *PeerConnection) LocalDescription() *SessionDescription {
}

// SetRemoteDescription sets the SessionDescription of the remote peer
func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error { //nolint pion/webrtc#614
if pc.currentRemoteDescription != nil { // pion/webrtc#207
return fmt.Errorf("remoteDescription is already defined, SetRemoteDescription can only be called once")
}
Expand Down Expand Up @@ -1027,6 +1031,9 @@ func (pc *PeerConnection) SetRemoteDescription(desc SessionDescription) error {
pc.mu.Unlock()
}()

if (desc.Type == SDPTypeAnswer || desc.Type == SDPTypePranswer) && pc.iceGatherer.agentIsTrickle {
return pc.iceGatherer.Gather()
}
return nil
}

Expand Down

5 comments on commit 7c18bbc

@cohosh
Copy link
Contributor

@cohosh cohosh commented on 7c18bbc Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about the decisions made in this commit, and they are causing breaking changes for us in Snowflake. This change prevents clients using the trickle method for gathering ICE candidates to block indefinitely when generating offers and makes it so that the trickle method can only be used to generate answers.

I'm also confused about the practice of gathering candidates in SetRemoteDescription. This can cause an ErrMultipleGatherAttempted to be thrown. It's also not immediately obvious to me how these changes relate to the summary or description of this commit.

@hugoArregui
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cohosh, @Sean-Der mentioned in the PR (#763) I had to update the gathering code to only start when signaling is in a stable enough state. If someone emits candidates before the other side is ready it causes Chrome/Pion to fail, I think that's the rationale behind this particular change.

Sorry about the poor commit description, I think we squashed the commits too much.

@cohosh
Copy link
Contributor

@cohosh cohosh commented on 7c18bbc Sep 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries, and thanks for the response!

I will take my comments to the PR :)

@Sean-Der
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cohosh really sorry about that :(

If we generate candidates after SetLocalDescription of an Offer it will send candidates before the remote has had a chance to generate an Answer. This causes Chrome to thrown an error (addicecandidate before SetLocalDescription)

re: ErrMultipleGatherAttempted, we don't have great test coverage on this :/ for the current release I want to implement offer/answer multiple times. But yea 100% total oversight there, we should have a bool in pion/webrtc that we flip to make sure we don't gather multiple times. Nice catch!

@Sean-Der
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh awesome, yes send the PR my way will get that in quick!

Please sign in to comment.