Automatically configure RTX codecs

[3DRX]

Description

Add a setting engine flag to enable automatically configure RTX codecs in MediaEngine.

1. NACK codec only added to video codecs that have Type: "nack" in it's RTCPFeedback, for example:

RTCPFeedback: []RTCPFeedback{
	{Type: "nack", Parameter: ""},
	{Type: "nack", Parameter: "pli"},
},

2. Automatically added NACK codec's payload type is original video's payload type + 1 (a common behavior).
3. If original video's payload type + 1 is already taken, an error is returned in api.NewPeerConnection.

[codecov]

Codecov Report

Attention: Patch coverage is 89.74359% with 4 lines in your changes missing coverage. Please review.

Project coverage is 78.65%. Comparing base (2de1ac4) to head (842a58b).

Files with missing lines	Patch %	Lines
peerconnection.go	42.85%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3095      +/-   ##
==========================================
+ Coverage   78.59%   78.65%   +0.06%     
==========================================
  Files          91       91              
  Lines       11372    11409      +37     
==========================================
+ Hits         8938     8974      +36     
- Misses       1944     1946       +2     
+ Partials      490      489       -1     

Flag	Coverage Δ
go	80.54% <89.74%> (+0.05%)	⬆️
wasm	63.84% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JoeTurki]

My primary concerns about this pull request are:

We should only introduce new settings if they improve behavior by aligning more closely with the specification, and we can't break existing behavior. or a options that cannot be configured otherwise for example, SetNAT1To1IPs.

I think we should try to minimize the number of settings to keep the API as compatible with the JavaScript API as possible, especially considering that i think most users are unaware of the existence of the settingsEngine :)

I'm not convinced where this change would fit, and it appears to make assumptions about how the other side implements RTX and NACK

What do you think @Sean-Der @jech

Thank you so much @3DRX

[JoeTurki]

This can cause conflicts, no?

[3DRX]

Yes, currently this PR’s behavior when conflict occurs is to return an error in api.NewPeerConnection.

[3DRX]

@JoeTurki I understands your concerns, if it’s not that straightforward and beneficial to pion/webrtc, do you think it’s a good idea to have this behavior to be enabled when user is using pion/mediadevices? Although the spec doesn’t describe anything about setting RTX by default, this can make the default behavior closer to libwebrtc.

[3DRX]

and it appears to make assumptions about how the other side implements RTX and NACK

I think this shouldn’t be a problem, since the rtx is added before sdp negotiations, and even if a rtx added when remote doesn’t support rtx and nack, the rtx track will always be empty(since no nack packets generated and received, no rtx packets will be send).

[Sean-Der]

@3DRX I am going to start an issue. I think we can default this to on! We have multiple issues and I want to make sure we get things right (not break something and regret it)

• A RTX doesn't stop Pion from sending NACKs
• RTX isn't enabled by default (users registering codecs, and not dedicated RTX)
• Congestion Controllers are hard to associate with a a PeerConnection
• More? We have so many Interceptor related issues right now I am losing track :)

My goal

• Fix these issues
• Not break existing users
• Have things on by default, otherwise only power users benefit
• Don't make a decision we regret. I have had a few designs in Pion that I regret because I went too fast.

[3DRX]

Thanks for reviewing this @JoeTurki :)

[3DRX]

@Sean-Der I’d like to help on fixing the current issues! It seems to only affect pion as a receiver, this PR is all about pion as a sender. I think the issue (A RTX doesn't stop Pion from sending NACKs) won’t block this PR’s work.

[3DRX]

There’ve been quite a lot going on here: #3063. @arjunshajitech did a refactor to fix the issue, but it breaks current interceptor interface. Don’t know if it’s possible to change it at this point.

[jech]

when remote doesn’t support rtx and nack,

What happens if the remote supports NACK but not RTX? That's the case of most Pion applications.

[3DRX]

when remote doesn’t support rtx and nack,

What happens if the remote supports NACK but not RTX? That's the case of most Pion applications.

If the remote doesn't support RTX, the sdp response shouldn't contain the RTX track, and the rtx codec won't be treated as negotiated, which is fine I guess (changes in this PR doesn't force sending retransmission through RTX, it just automatically configures them for the user)? I'm planning to, but haven't build any WebRTC receivers myself using pion so please correct me if I'm wrong.

I think pion's receiver will support RTX well after #3063 is fixed, which according to @Sean-Der 's issue that tracks a series of work should be merged together with this PR.

[jech]

More? We have so many Interceptor related issues right now I am losing track :)

To be fair, we're trying to achieve something difficult: a library that is both flexible enough to be used for low-level stuff (such as highly efficient SFUs) and easy to use for typical applications. Since we're exposing both low- and high-level functionality, we need to get the layering just right.

There's also #2994.