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 issue #1212 - Candidate with empty string #1428

Closed
wants to merge 3 commits into from

Conversation

DevRockstarZ
Copy link
Contributor

Description

I made the first test code by myself, it can be not perfect 😢
I tested on my local terminal, it passes all the cases.
I created test PeerConnection to use AddICECandidate, and I tested 3 test cases.
Thanks for reviewing!

Reference issue

Fixes issue #1212

@Sean-Der
Copy link
Member

Hey @DevRockstarZ this PR looks really good to me!

If you want to reopen I would love to merge this :) I will just squash your work into one commit. Really great work!

@DevRockstarZ
Copy link
Contributor Author

DevRockstarZ commented Sep 18, 2020

@Sean-Der Thanks for your reply! I closed this PR because I thought you're working on this work in Pion-V3.
I thought this PR will unnecessary... 😢
If re-open this PR is fine, I will re-open it! Thanks!

@DevRockstarZ DevRockstarZ reopened this Sep 18, 2020
@jsmouret
Copy link

Isn't it hiding the real issue?
pion/ice#271

@DevRockstarZ
Copy link
Contributor Author

@jsmouret
I also think real issue is not cleared.
So I did temporary fix to do not print errors from empty candidate.
I think real issue will be cleared when pion releases version 3.
Thanks!

@Sean-Der
Copy link
Member

Sean-Der commented Sep 21, 2020

You are right @jsmouret thanks for keeping me honest :)

So I think this is a two step process

  • Update pion/ice to accept a nil candidate. AddRemoteCandidate when called with nil should then set a field on the agent.

in the contact loop it shouldn't go to failed unless this field has been set.

restart should update this field as well.

  • then pion/webrtc should be updated to pass a nil candidate in.

This is a good amount of work, but I think there is a clear path forward! If anyone is interested in grabbing I can help :) the work done on the tests here is really great, I think we can use as is

@scorpionknifes
Copy link
Member

Hey, @Sean-Der I'm reading into this and trying to clarify some stuff. (1st step ICE)

I'm reading into 5.6.3 RTCIceGathererState and

complete The RTCIceTransport has completed gathering and the end-of-candidates indication for this transport has been sent. It will not gather candidates again until an ICE restart causes it to restart.

I'm assuming the field on the agent that should be set is GatheringState by agent.setGatheringState

Since GatheringState is already reset in agent.Restart i think we don't need to update this field.

I'm also not too sure where to check this in contact loop.

@Sean-Der
Copy link
Member

Sean-Der commented Nov 23, 2020

ICEGatherState is good, I don't believe we need to update that! The implementation of Trickling out of Pion I think is complete.

What we need to do is

  • Update the ICE Agent to not go to failed until it sees a end-of-candidates that check would go here
  • Update pion/webrtc to accept nil candidates, and send a end-of-candidates notification into pion/ice
  • Restart just needs to clear has-remote-sent-end-of-candidates flag

@scorpionknifes
Copy link
Member

@Sean-Der, need some advice

how do I store end-of-candidate attribute in ICE Agent?

since we sending nil to AddICECandidate do we want to pass nil to ICE or should we create a new func?

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.

pc.AddIceCandidate complains when candidate.Candidate is the empty string
4 participants