Add T3-rtx timer and congestion control about sctp HOT 17 CLOSED

The reason this part is having a tons of issues is because of compliance. It's not according to the abstractions described in the spec. I am slowing changing this to be compliant with the spec but this means that networkManager is going to disappear. In order to continue being backwards compatible, I am actually creating a parallel process that does all the association start and the whole nine yards correctly. Next PR is going to show that. In the process, I am just adding a bunch of deprecation messages to tell users to stop using the old functions. When SCTP is going to be working properly we can then talk about doing the official cut over.

from sctp.

Most implementation is done. Now I am writing tests, which may take ~10 more hours. Once the tests are done, I will push my code for review.
Here's my implementation note

from sctp.

Guys my goal is to make this happen with a compliant sctp. I actually need the congestion control functionality so will get it done. This is literally what I am working on right now. You can keep checking out the pions/transports branch for updates as it slowly develops. Periodically I am making PRs that are adding more and more functionality.

from sctp.

In /examples there is pion-to-pion which is what I was using to test.

It sends the SDP over a unix socket, and makes testing really easy. Last time I checked DTLS+ICE worked fine, just that SCTP didn't try to start the association. So if you can get that part working it would really unblock a lot of other stuff from happening.

Anything I can do to help happy to jump in, I was in the SCTP stuff also.

from sctp.

You don't have to do any deprecation notices for networkManager or anything in internal are there any other external APIs you see changing?

Feel free to make changes you need, but it would be really easy to kick off an association now. When ICE+DTLS is done just put the association start behind a timer (in case SCTP peer doesn't recv at first)

You could just copy everything in the networkManager and put it inside RTCPeerConnection but that isn't the right thing to do it will bloat that package even more.

from sctp.

Okay cool. You actually answer my question when the association needs to be kicked off. I am going to make it actually sequential because I am layering the structs of the different protocols on top of each other.

Something that looks like this roughly

type RTCSctpTransport struct {
	Transport: RTCDtlsTransport{
		Transport: RTCIceTransport {
			Agent: ice.Agent
		}
	}
}

Each one of these objects actually abstracts stuff into their respective modules privately. So RTCSctpTransport just initializes sctp.Association and later invokes it when things need to be kicked off during internal steps of say dtls resolution. That's how it is described in the specs and it sorta makes total sense because it is layering different protocols.

I still want to keep deprecating stuff even internal because I know @backkem works with Jetbrains Goland and I am sure if he sees internal stuff being deprecated he will be less likely to develop more on top of those. It's not a lot of work for me and I like it, and someone else might like it too.

from sctp.

Oh btw, the same struct exists for the video/audio streams. But I am not touching that stuff for now. #83 Where I first started talking about it.

type RTCRtpTransceiver struct {
	Transport: RTCDtlsTransport{
		Transport: RTCIceTransport {
			Agent: ice.Agent
		}
	}
}

from sctp.

Okay this story will have to wait a bit after I am done with the rewiring because I need to understand the internals of Ice Agent a little better. Not just the sink/source section.

from sctp.

Yea 100%!

I wouldn't touch anything relating to SCTP except converting the API to be generic transports. Then when that lands I would finish implementing the SCTP RFC

from sctp.

One consideration:
getPayloadDataToSend() method (will land on master soon) has time.Now() inside it. We should put it outside to make testing easier.

from sctp.

One consideration:
getPayloadDataToSend() method (will land on master soon) has time.Now() inside it. We should put it outside to make testing easier.

As well as time.Since

from sctp.

I am going to start working on this issue form this weekend. The implementation would somewhat relate to congestion control, #14 and likely #21 would be addressed all together.

During the work of pion/webrtc#334, I ran a test with lossy connections using Network Link Conditioner. I noticed nearly 40% of the time, data channel establishment stalled (at 10% packet loss ratio on both directions) although ICE state becomes connected, which is due to the lack of INIT and COOKIE ECHO retransmissions. This is very crucial for the real world use cases. So, I am thinking about implementing those in this work as well.

Retransmission timers missing:

• T1-init timer
• T1-cookie timer
• T2-shutdown timer (note1)
• T3-rtx timer
• T5-shutdown-guard timer (note1)
• Heartbeat timer (note2)

Note 1: this may be done after the shutdown is fully implemented in a separate issue

Note 2: this may be done after the heartbeat is implemented in a separate issue

from sctp.

I created a separate issue #24 for T1-init/cookie timers as I'd like to make these timers available earlier.

from sctp.

I have just realized that the bufferredAmout and its threshold are per-stream (per-datachannel) properties. I will need to update the code...

from sctp.

We need to determine the size of the receive window size (rwnd), which is currently set to 64KB.
Initial ssthresh value is set to the rwnd as well (as recommended in RFC 4960)

From my quick research, Firefox and Chromium used to have 128KB and both were requested to increase it to 1MB (not sure if that were accepted or not).

I guess 64KB is too small, and 128KB does not seem to be large enough for the modern Internet. I guess we should bring it up to somewhere between 256KB and 1MB.

from sctp.

Let's start with 128KB. We will do more benchmark later and increase it as we become more confident about it.

from sctp.

I found a bug in handleForwardTSN() sending SACK to every single FowardTSN. The SACK again generates ForwardTSN beyond advancedPeerTSNAckPoint.

According to RFC 3758 Sec 3.6, SACK should only be sent when FowardTSN's newCumulativeTSN is the current peerLastTSN or behind - which was not correctly implemented!

I have locally confirmed that this fixed the "snowballing effect", and my stress test passed.

I will add the change to PR #26 soon. (@Sean-Der)

from sctp.