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

Indicate trickle end-of-candidates #767

Closed
jackfreed opened this issue Aug 1, 2019 · 8 comments · Fixed by #990
Closed

Indicate trickle end-of-candidates #767

jackfreed opened this issue Aug 1, 2019 · 8 comments · Fixed by #990

Comments

@jackfreed
Copy link

Is there any way to indicate the end-of-candidates for trickle,
because we experience a noticable delay when connecting between

ICE Connection State has changed: checking and
ICE Connection State has changed: connected

normally its indicated with an empty string:
https://developer.mozilla.org/en-US/docs/Web/API/RTCPeerConnection/addIceCandidate

but i keep getting attribute not long enough to be ICE candidate (1)

pc.onicegatheringstatechange = () => {
        if (pc.iceGatheringState === 'complete') {
                // indicate end-of-candidates to pion here
        }
};
@Sean-Der
Copy link
Member

Sean-Der commented Aug 2, 2019

Hey @jackfreed

I checked out our trickle ICE implementation and found an issue! I updated the code so connectivity checks should be faster.

I will update AddICECandidate to accept an empty string. However It will have no effect, we go to Failed on a timeout and don't distinguish if you are still gathering or not.

We are going to cleanup up pion/ice with Trickle ICE in mind for v2.2.0 though! Then this will start to matter, but this is at least 3 months out.

Thanks for using Pion!

@Sean-Der
Copy link
Member

Sean-Der commented Aug 2, 2019

If anyone is interested in grabbing this issue AddICECandidate just needs to detect and discard a ICECandidate if it has a candidate member that is an empty string.

In the future we will do more with it. Also make sure to add a test!

@jackfreed
Copy link
Author

Thx @Sean-Der,

it indeed feels a bit quicker.
I still think the end-of-candidates should go to Failed before the timeout triggers.

@jackfreed
Copy link
Author

Hi @Sean-Der

i am still trying to find the huge delay we are experiencing in the connection setup compared to janus.
The tests were done in the same browser, two different windows.
When using janus the picture pops up within the same second of connecting.

Could you please take a quick look at those timings:

00:00:36.267076176 Creating logger for ortc

localTrack := <-localTrackChan

00:00:36.405667762 Creating logger for ortc
00:00:36.405819317 Debug: Started agent: isControlling? true, remoteUfrag: "foo", remotePwd: "bar"
00:00:36.405938770 - ICE Connection State has changed in join: checking
00:00:36.407751091 Debug: adding a new peer-reflexive candiate: 1.3.3.7:13337

here i see over a second prolly wasted on the timeout

00:00:37.596428939 - ICE Connection State has changed in join: connected
00:00:37.596464660 Creating logger for mux
00:00:37.596521766 Creating logger for dtls
00:00:37.708722642 Creating logger for srtp
00:00:37.708775701 Creating logger for srtp
00:00:37.708848331 Creating logger for sctp
00:00:37.708871085 Debug: [0xc000206420] state change: 'Closed' => 'CookieWait'
00:00:37.708881686 Debug: [0xc000206420] sending INIT
00:00:37.708917184 Debug: [0xc000206420] readLoop entered
00:00:37.708965057 Debug: [0xc000206420] writeLoop entered
00:00:37.759677392 Debug: [0xc000206420] chunkInit received in state 'CookieWait'
00:00:37.759878746 Debug: [0xc000206420] chunkInitAck received in state 'CookieWait'
00:00:37.759907168 Debug: [0xc000206420] initial rwnd=131072
00:00:37.759926863 Debug: [0xc000206420] sending COOKIE-ECHO
00:00:37.759946881 Debug: [0xc000206420] state change: 'CookieWait' => 'CookieEchoed'
00:00:37.807618240 Debug: [0xc000206420] COOKIE-ECHO received in state 'CookieEchoed'
00:00:37.807702157 Debug: [0xc000206420] state change: 'CookieEchoed' => 'Established'
00:00:37.807877373 Debug: [0xc000206420] COOKIE-ACK received in state 'Established'

picture coming here somewhere

00:00:41.409404931 Debug: [0xc0000ed4a0] sending INIT

@Sean-Der
Copy link
Member

Sean-Der commented Sep 5, 2019

Hey @jackfreed

Sorry this took so long, are you still using Pion? I have a possible patch that kicks the connectivity checks if we get another prflx candidate!

Would you be able to test it?

thanks

@Sean-Der
Copy link
Member

Sean-Der commented Sep 5, 2019

I am making this issue part of our next release, I am sure this is biting other people so would love to get it fixed :)

@jackfreed
Copy link
Author

Hi @Sean-Der

i am still using it. Please patch me up :-)
I will try it right away.

Many thanks!

Sean-Der added a commit that referenced this issue Jan 30, 2020
Sean-Der added a commit that referenced this issue Jan 30, 2020
Sean-Der added a commit that referenced this issue Jan 30, 2020
Sean-Der added a commit to pion/ice that referenced this issue Jan 30, 2020
Match behavior of adding a remote candidate

Relates to pion/webrtc#767
Sean-Der added a commit to pion/ice that referenced this issue Jan 30, 2020
Match behavior of adding a remote candidate

Relates to pion/webrtc#767
Sean-Der added a commit to pion/ice that referenced this issue Jan 30, 2020
Match behavior of adding a remote candidate

Relates to pion/webrtc#767
Sean-Der added a commit that referenced this issue Jan 30, 2020
Sean-Der added a commit that referenced this issue Jan 30, 2020
@Sean-Der
Copy link
Member

Hey @jackfreed

Sorry I forgot to come back to this. If you ever need help with anything/want to ping me I am always on Slack or please email me sean @ pion.ly I am really excited when people use Pion so sorry for falling through :(

I have a PR now that adds tests (and asserts) that OnICEGatheringStateChange behaves properly.

I also updated pion/ice to kick off connectivity checks anytime a new local or remote candidate is added. Before pion/ice would just fire on a 1 second interval, which would cause needless delays.

Hopefully that fixes up everything, and sorry again for the delay :(

Sean-Der added a commit that referenced this issue Jan 30, 2020
Sean-Der added a commit that referenced this issue Jan 30, 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 a pull request may close this issue.

2 participants