pion / webrtc

Pure Go implementation of the WebRTC API
https://pion.ly
MIT License
13.9k stars 1.66k forks source link

Add Retransmission and FEC to TrackLocal #2914

Closed Sean-Der closed 1 month ago

Sean-Der commented 1 month ago

If the MediaEngine contains support for them a SSRC will be generated appropriately

Resolves #1989 Resolves #1675

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 97.72727% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.20%. Comparing base (bd2309f) to head (e71e50a). Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
sdp.go 50.00% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ 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 ``` | [Flag](https://app.codecov.io/gh/pion/webrtc/pull/2914/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | | |---|---|---| | [go](https://app.codecov.io/gh/pion/webrtc/pull/2914/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `80.77% <97.72%> (+0.21%)` | :arrow_up: | | [wasm](https://app.codecov.io/gh/pion/webrtc/pull/2914/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `64.92% <9.09%> (-0.20%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Sean-Der commented 1 month ago

@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.

Sean-Der commented 1 month ago

These means that going forward Offers/Answers (if enabled) will have a=ssrc-group:FID lines

jech commented 1 month ago

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)?

nils-ohlmeier commented 1 month ago

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)?

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.

adriancable commented 1 month ago

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.

pc = new RTCPeerConnection();
pc.addTransceiver("video", { direction: 'recvonly' });
pc.addTransceiver("audio", { direction: 'sendrecv' });
await pc.createOffer();

Produces this: (NB, note no a=ssrc-group:FID lines of course because the video is recvonly)

v=0
o=- 1387888339142592194 2 IN IP4 127.0.0.1
s=-
t=0 0
a=group:BUNDLE 0 1
a=extmap-allow-mixed
a=msid-semantic: WMS
m=video 9 UDP/TLS/RTP/SAVPF 96 97 98 99 100 101 35 36 37 38 102 103 104 105 106 107 108 109 127 125 39 40 41 42 43 44 45 46 47 48 112 113 114 115 116 117 118 49
c=IN IP4 0.0.0.0
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:RYEw
a=ice-pwd:1+1pzvIr0YyIhOVR+o5s9i4u
a=ice-options:trickle
a=fingerprint:sha-256 C4:1A:6E:1A:96:24:25:2D:41:AE:8D:77:02:A5:AA:02:29:BB:7D:0A:00:DB:A9:E4:F4:79:55:D1:9C:C5:4F:CA
a=setup:actpass
a=mid:0
a=extmap:1 urn:ietf:params:rtp-hdrext:toffset
a=extmap:2 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=extmap:3 urn:3gpp:video-orientation
a=extmap:4 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a=extmap:5 http://www.webrtc.org/experiments/rtp-hdrext/playout-delay
a=extmap:6 http://www.webrtc.org/experiments/rtp-hdrext/video-content-type
a=extmap:7 http://www.webrtc.org/experiments/rtp-hdrext/video-timing
a=extmap:8 http://www.webrtc.org/experiments/rtp-hdrext/color-space
a=extmap:9 urn:ietf:params:rtp-hdrext:sdes:mid
a=extmap:10 urn:ietf:params:rtp-hdrext:sdes:rtp-stream-id
a=extmap:11 urn:ietf:params:rtp-hdrext:sdes:repaired-rtp-stream-id
a=recvonly
a=rtcp-mux
a=rtcp-rsize
a=rtpmap:96 VP8/90000
a=rtcp-fb:96 goog-remb
a=rtcp-fb:96 transport-cc
a=rtcp-fb:96 ccm fir
a=rtcp-fb:96 nack
a=rtcp-fb:96 nack pli
a=rtpmap:97 rtx/90000
a=fmtp:97 apt=96
a=rtpmap:98 VP9/90000
a=rtcp-fb:98 goog-remb
a=rtcp-fb:98 transport-cc
a=rtcp-fb:98 ccm fir
a=rtcp-fb:98 nack
a=rtcp-fb:98 nack pli
a=fmtp:98 profile-id=0
a=rtpmap:99 rtx/90000
a=fmtp:99 apt=98
a=rtpmap:100 VP9/90000
a=rtcp-fb:100 goog-remb
a=rtcp-fb:100 transport-cc
a=rtcp-fb:100 ccm fir
a=rtcp-fb:100 nack
a=rtcp-fb:100 nack pli
a=fmtp:100 profile-id=2
a=rtpmap:101 rtx/90000
a=fmtp:101 apt=100
a=rtpmap:35 VP9/90000
a=rtcp-fb:35 goog-remb
a=rtcp-fb:35 transport-cc
a=rtcp-fb:35 ccm fir
a=rtcp-fb:35 nack
a=rtcp-fb:35 nack pli
a=fmtp:35 profile-id=1
a=rtpmap:36 rtx/90000
a=fmtp:36 apt=35
a=rtpmap:37 VP9/90000
a=rtcp-fb:37 goog-remb
a=rtcp-fb:37 transport-cc
a=rtcp-fb:37 ccm fir
a=rtcp-fb:37 nack
a=rtcp-fb:37 nack pli
a=fmtp:37 profile-id=3
a=rtpmap:38 rtx/90000
a=fmtp:38 apt=37
a=rtpmap:102 H264/90000
a=rtcp-fb:102 goog-remb
a=rtcp-fb:102 transport-cc
a=rtcp-fb:102 ccm fir
a=rtcp-fb:102 nack
a=rtcp-fb:102 nack pli
a=fmtp:102 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42001f
a=rtpmap:103 rtx/90000
a=fmtp:103 apt=102
a=rtpmap:104 H264/90000
a=rtcp-fb:104 goog-remb
a=rtcp-fb:104 transport-cc
a=rtcp-fb:104 ccm fir
a=rtcp-fb:104 nack
a=rtcp-fb:104 nack pli
a=fmtp:104 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=42001f
a=rtpmap:105 rtx/90000
a=fmtp:105 apt=104
a=rtpmap:106 H264/90000
a=rtcp-fb:106 goog-remb
a=rtcp-fb:106 transport-cc
a=rtcp-fb:106 ccm fir
a=rtcp-fb:106 nack
a=rtcp-fb:106 nack pli
a=fmtp:106 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=42e01f
a=rtpmap:107 rtx/90000
a=fmtp:107 apt=106
a=rtpmap:108 H264/90000
a=rtcp-fb:108 goog-remb
a=rtcp-fb:108 transport-cc
a=rtcp-fb:108 ccm fir
a=rtcp-fb:108 nack
a=rtcp-fb:108 nack pli
a=fmtp:108 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=42e01f
a=rtpmap:109 rtx/90000
a=fmtp:109 apt=108
a=rtpmap:127 H264/90000
a=rtcp-fb:127 goog-remb
a=rtcp-fb:127 transport-cc
a=rtcp-fb:127 ccm fir
a=rtcp-fb:127 nack
a=rtcp-fb:127 nack pli
a=fmtp:127 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=4d001f
a=rtpmap:125 rtx/90000
a=fmtp:125 apt=127
a=rtpmap:39 H264/90000
a=rtcp-fb:39 goog-remb
a=rtcp-fb:39 transport-cc
a=rtcp-fb:39 ccm fir
a=rtcp-fb:39 nack
a=rtcp-fb:39 nack pli
a=fmtp:39 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=4d001f
a=rtpmap:40 rtx/90000
a=fmtp:40 apt=39
a=rtpmap:41 H264/90000
a=rtcp-fb:41 goog-remb
a=rtcp-fb:41 transport-cc
a=rtcp-fb:41 ccm fir
a=rtcp-fb:41 nack
a=rtcp-fb:41 nack pli
a=fmtp:41 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=f4001f
a=rtpmap:42 rtx/90000
a=fmtp:42 apt=41
a=rtpmap:43 H264/90000
a=rtcp-fb:43 goog-remb
a=rtcp-fb:43 transport-cc
a=rtcp-fb:43 ccm fir
a=rtcp-fb:43 nack
a=rtcp-fb:43 nack pli
a=fmtp:43 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=f4001f
a=rtpmap:44 rtx/90000
a=fmtp:44 apt=43
a=rtpmap:45 AV1/90000
a=rtcp-fb:45 goog-remb
a=rtcp-fb:45 transport-cc
a=rtcp-fb:45 ccm fir
a=rtcp-fb:45 nack
a=rtcp-fb:45 nack pli
a=fmtp:45 level-idx=5;profile=0;tier=0
a=rtpmap:46 rtx/90000
a=fmtp:46 apt=45
a=rtpmap:47 AV1/90000
a=rtcp-fb:47 goog-remb
a=rtcp-fb:47 transport-cc
a=rtcp-fb:47 ccm fir
a=rtcp-fb:47 nack
a=rtcp-fb:47 nack pli
a=fmtp:47 level-idx=5;profile=1;tier=0
a=rtpmap:48 rtx/90000
a=fmtp:48 apt=47
a=rtpmap:112 H264/90000
a=rtcp-fb:112 goog-remb
a=rtcp-fb:112 transport-cc
a=rtcp-fb:112 ccm fir
a=rtcp-fb:112 nack
a=rtcp-fb:112 nack pli
a=fmtp:112 level-asymmetry-allowed=1;packetization-mode=1;profile-level-id=64001f
a=rtpmap:113 rtx/90000
a=fmtp:113 apt=112
a=rtpmap:114 H264/90000
a=rtcp-fb:114 goog-remb
a=rtcp-fb:114 transport-cc
a=rtcp-fb:114 ccm fir
a=rtcp-fb:114 nack
a=rtcp-fb:114 nack pli
a=fmtp:114 level-asymmetry-allowed=1;packetization-mode=0;profile-level-id=64001f
a=rtpmap:115 rtx/90000
a=fmtp:115 apt=114
a=rtpmap:116 red/90000
a=rtpmap:117 rtx/90000
a=fmtp:117 apt=116
a=rtpmap:118 ulpfec/90000
a=rtpmap:49 flexfec-03/90000
a=rtcp-fb:49 goog-remb
a=rtcp-fb:49 transport-cc
a=fmtp:49 repair-window=10000000
m=audio 9 UDP/TLS/RTP/SAVPF 111 63 9 0 8 13 110 126
c=IN IP4 0.0.0.0
a=rtcp:9 IN IP4 0.0.0.0
a=ice-ufrag:RYEw
a=ice-pwd:1+1pzvIr0YyIhOVR+o5s9i4u
a=ice-options:trickle
a=fingerprint:sha-256 C4:1A:6E:1A:96:24:25:2D:41:AE:8D:77:02:A5:AA:02:29:BB:7D:0A:00:DB:A9:E4:F4:79:55:D1:9C:C5:4F:CA
a=setup:actpass
a=mid:1
a=extmap:14 urn:ietf:params:rtp-hdrext:ssrc-audio-level
a=extmap:2 http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
a=extmap:4 http://www.ietf.org/id/draft-holmer-rmcat-transport-wide-cc-extensions-01
a=extmap:9 urn:ietf:params:rtp-hdrext:sdes:mid
a=sendrecv
a=msid:- 88de6a97-4843-49e9-a092-59e24af1b934
a=rtcp-mux
a=rtcp-rsize
a=rtpmap:111 opus/48000/2
a=rtcp-fb:111 transport-cc
a=fmtp:111 minptime=10;useinbandfec=1
a=rtpmap:63 red/48000/2
a=fmtp:63 111/111
a=rtpmap:9 G722/8000
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:13 CN/8000
a=rtpmap:110 telephone-event/48000
a=rtpmap:126 telephone-event/8000
a=ssrc:547643828 cname:V/w5XC9p2K1YZkj0
a=ssrc:547643828 msid:- 88de6a97-4843-49e9-a092-59e24af1b934
adriancable commented 1 month ago

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 profile-level-id=f4001f by default, which I think is Predictive High 444. I didn't actually know that was a thing for WebRTC, but we might want to do the same.

Sean-Der commented 1 month ago

I have added a test that confirms by default NACK on video only!

thank you so much for the reviews everyone :)