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

Fix AddRemoteCandidate to accept nil candidate #315

Closed
wants to merge 1 commit into from

Conversation

scorpionknifes
Copy link
Member

@scorpionknifes scorpionknifes commented Dec 7, 2020

Description

Accept nil candidate to AddRemoteCandidate in ICE

pass nil candidate from pion/webrtc to pion/ice -> pion/webrtc#1584

Read comment #315 (comment) for more issue reference

@codecov
Copy link

codecov bot commented Dec 7, 2020

Codecov Report

Merging #315 (78cd19f) into master (5e5f171) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
+ Coverage   79.47%   79.49%   +0.01%     
==========================================
  Files          31       31              
  Lines        2295     2297       +2     
==========================================
+ Hits         1824     1826       +2     
  Misses        333      333              
  Partials      138      138              
Flag Coverage Δ
go 79.49% <100.00%> (+0.01%) ⬆️
wasm 28.82% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
agent.go 82.47% <100.00%> (+0.06%) ⬆️

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 5e5f171...78cd19f. Read the comment docs.

@scorpionknifes scorpionknifes force-pushed the nil-candidate branch 3 times, most recently from be75944 to 667ad8f Compare December 7, 2020 11:06
empty candidate.Candidate won't make an error when pass into pion/ice
@scorpionknifes scorpionknifes marked this pull request as ready for review December 7, 2020 11:21
@scorpionknifes
Copy link
Member Author

Further improvement required to fix #271

1st step to fix pion/webrtc#1212

referencing here to remove issue link (so the issue doesn't close cause this PR don't fix them)

@jech
Copy link
Member

jech commented Dec 7, 2020

What about the case candidate->Candidate == ""?

@scorpionknifes
Copy link
Member Author

@jesh this PR is the first part of 2 PRs. The candidate=="" check is done in pion/webrtc and is implemented here https://github.com/pion/webrtc/pull/1584/files.

Since EOC is passed to pion/ice we can determine the correct implementation in pion/ice in the future.

@Sean-Der
Copy link
Member

Sean-Der commented Dec 9, 2020

Merged with 7c89762

@Sean-Der Sean-Der closed this Dec 9, 2020
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.

3 participants