Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ice trickle support #691

Merged
merged 1 commit into from
May 30, 2019
Merged

Add ice trickle support #691

merged 1 commit into from
May 30, 2019

Conversation

trivigy
Copy link
Member

@trivigy trivigy commented May 26, 2019

resolves pion/ice#51

@trivigy trivigy requested review from Sean-Der and masterada May 26, 2019 06:40
@codecov-io
Copy link

codecov-io commented May 26, 2019

Codecov Report

Merging #691 into master will decrease coverage by 0.23%.
The diff coverage is 64.4%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #691      +/-   ##
==========================================
- Coverage   77.33%   77.09%   -0.24%     
==========================================
  Files          57       57              
  Lines        3644     3694      +50     
==========================================
+ Hits         2818     2848      +30     
- Misses        585      599      +14     
- Partials      241      247       +6
Impacted Files Coverage Δ
icetransport.go 77.16% <0%> (-0.18%) ⬇️
peerconnection.go 70.88% <60%> (-0.98%) ⬇️
icegatherer.go 78.57% <69.35%> (-3.48%) ⬇️
dtlstransport.go 70.74% <0%> (+1.36%) ⬆️
sctptransport.go 72.72% <0%> (+1.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e6149d...0ae5654. Read the comment docs.

@Sean-Der Sean-Der force-pushed the ice-issue-51 branch 2 times, most recently from 2862e79 to a04e625 Compare May 29, 2019 08:07
@Sean-Der
Copy link
Member

Sean-Der commented May 29, 2019

@trivigy Ok ready for a first pass!

It toggles between trickle/non-trickle if you have the callback set. When we do /v3 eventually we can break that behavior (and only do trickle)

I moved most of the behavior into ICEGatherer and had things follow ORTC stuff.

Can you try listening to OnICECandidate and then first time you get a srflx candidate try pulling pc.PendingLocalDescription

@Sean-Der Sean-Der requested review from hugoArregui and removed request for Sean-Der May 29, 2019 08:25
@Sean-Der Sean-Der requested a review from ernado May 29, 2019 18:50
Copy link
Member Author

@trivigy trivigy left a comment

Choose a reason for hiding this comment

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

One other thing if you don't mind addressing. There is inconsistency regarding what SessionDescription. Previously session description was extracted from CreateOffer() where it was returned as a struct instance. Now with trickle changes we need to extract it from LocalDescription() where it is returned as a pointer. This makes things inconsistent.

peerconnection.go Show resolved Hide resolved
peerconnection.go Show resolved Hide resolved
@Sean-Der
Copy link
Member

Sean-Der commented May 29, 2019

One other thing if you don't mind addressing. There is inconsistency regarding what SessionDescription. Previously session description was extracted from CreateOffer() where it was returned as a struct instance. Now with trickle changes we need to extract it from LocalDescription() where it is returned as a pointer. This makes things inconsistent.

we need a way to express null, we can update this to return an error (but having no currentLocalDescription I don't believe is an error) so not sure what the answer is.

Resolves pion/ice#51

Co-authored-by: Konstantin Itskov <[email protected]>
@Sean-Der Sean-Der merged commit 1d72119 into master May 30, 2019
@Sean-Der Sean-Der deleted the ice-issue-51 branch May 30, 2019 04:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Slowdown because of server reflexive address allocation
3 participants