Add Retransmission and FEC to TrackLocal #2914
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2914 +/- ##
==========================================
+ Coverage 78.99% 79.20% +0.20%
==========================================
Files 89 89
Lines 8487 8539 +52
==========================================
+ Hits 6704 6763 +59
+ Misses 1297 1289 -8
- Partials 486 487 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Sean-Der
force-pushed
the
rtx-and-fec
branch
2 times, most recently
from
c9aab06
to
6a46aac
Compare
Sean-Der
requested review from
cnderrauber,
boks1971,
jech,
adriancable and
aalekseevx
|
@jech and @cnderrauber @boks1971 does this API/change look ok? If RTX or FEC is registered in the MediaEngine we generate a SSRC for it. You can query (and the send the packets). After this gets merged I will follow up with APIs to make sending a RTX and passing the values into interceptors. Then we can enable better FEC and RTX support. My goal/hope is for this to have as a little impact as possible and be toggleable as people need. |
|
These means that going forward Offers/Answers (if enabled) will have |
|
I like the fact that it's optional, and that the cost for applications that opt-out is virtually zero (just some minor increase of the TrackLocal struct). I understand that both RTX and FEC are global toggles: if you enable FEC, then all tracks get FEC, right? What if my application only implements FEC for audio? Do I need to negotiate FEC for both audio and video even though I'll be unable to use the video FEC information? Similarly, what if my application wants to negotiate an FEC track with PCMA tracks but not with Opus tracks (since Opus has codec-level FEC)? |
rtpsender.go
Outdated
| track: track, | ||
| ssrc: SSRC(randutil.NewMathRandomGenerator().Uint32()), | ||
| track: track, | ||
| ssrc: SSRC(randutil.NewMathRandomGenerator().Uint32()), |
There was a problem hiding this comment.
How about creating just one MathRandomGenerator and using it multiple times rather than creating a new one every time?
Sean-Der
force-pushed
the
rtx-and-fec
branch
4 times, most recently
from
abd134e
to
d0afb30
Compare
Same goes for RTX. RTX should by default probably only get enabled for video codecs. Folks might want to experiment with RTX for audio, but since Opus has build in FEC I don't think that RTX should be enabled by default for it. |
Yes, we need to make sure that Pion's defaults match Chrome / libwebrtc's behaviour as closely as possible, which is RTX for video only. Produces this: (NB, note no |
|
Also (and sorry that this is somewhat off-topic, although only partially since we are talking here about what default codecs to register) it looks like Chrome is now offering H264 |
|
I have added a test that confirms by default NACK on video only! thank you so much for the reviews everyone :) |
If the MediaEngine contains support for them a SSRC will be generated appropriately
Resolves #1989
Resolves #1675