Implement separate RTX payload type for retransmission on NACK

erdnaxe commented 3 years ago

This issue is here to keep track of what was discussed on the mailing list and maybe discuss of how to solve it.

Galène does not advertise a separate RTX track in the SDP and expect the peer to retransmit on the main track. This seems to work with web browsers but it is problematic with other software such as GStreamer.

On Pion v2, RTX tracks were not supported. Galène is now using Pion v3 that can now support RTX. It seems that the standard should be to retransmit on a separate track (https://tools.ietf.org/html/rfc4588).

erdnaxe commented 3 years ago

We want group.APIFromNames() to return a WebRTC API with RTX tracks for some names such as "vp8", i.e. we need either:

to change group.codecFromName() so that it returns a list of codecs rather than only one codec,
to change group.APIFromNames() to call twice group.codecFromName() when a RTX track is needed,
to change group.APIFromCodecs() to register an extra codec when it encounter a video codec.

What do you consider the best?

I am still reading the code and trying to understand how everything is structured, but I guess to announce RTX track we want something like this:

m.RegisterCodec(
    webrtc.RTPCodecParameters{
        RTPCodecCapability: webrtc.RTPCodecCapability{
            MimeType:     "video/rtx",
            ClockRate:    codec.ClockRate,  // same clockrate as video track
            Channels:     codec.Channels,  // same channels as video track
            SDPFmtpLine:  "apt=96", // 96 is the payload of the video track
            RTCPFeedback: nil,
        },
        PayloadType: 97,  // it seems to always be (payload of video track)+1, but we should maybe put it in `payloadType()`
    },
    tpe,
)

Then when the RTX track negotiation is done, there will be the hard part about figuring out how to send retransmissions on this track.

(I am still learning WebRTC/Pion API, so I may write bad code.)

jech commented 3 years ago

Either have codecFromName return a list, or inline the function within APIFromNames.

Negotiating a separate RTX track, though, is the easy part. We also need:

to parse retransmistted packets that arrive on the RTX track (in encapsulated form);
to resend NACKed packets on the RTX track.

Two things to note. One, you need to handle correctly the case when an RTX track has not been successfully negotiated. Two, you'll need to account for packet loss differently depending on whether an RTX track was negotiated or not.

erdnaxe commented 3 years ago

I changed group.codecFromName to return a list: https://github.com/jech/galene/compare/master...erdnaxe:rtx As the RTX track needs to be defined with the payload of the video track, I also moved the payload definitions. If you are not happy about that and have a better solution, I can change the code.

Now the hard part is to modify the rtpconn package to do what you described. I am reading code and trying to understand the structure, can you confirm that:

rtpUpTrack.bufferedNACKs contains the NACK requests that Galène is going to send to peer with rtpconn.nackWriter() then rtpconn.sendNACKs().
rtpconn.gotNACK() is called when a NACK from peer is received by Galène. If it misses some packets, it is going to ask the peer sending the stream.
rtpUpTrack.Nack() sends a NACK from Galène to peer.

to parse retransmistted packets that arrive on the RTX track (in encapsulated form);

Should I only need to modify/create another rtpconn.readLoop() to handle the RTX track and put the parsed packets on the cache of the main track?

to resend NACKed packets on the RTX track.

Should I only need to patch rtpconn.gotNACK() to call WriteRTP on the RTX track?

jech commented 3 years ago

Should I only need to modify/create another rtpconn.readLoop() to handle the RTX track and put the parsed packets on the cache of the main track?

I think so.

Should I only need to patch rtpconn.gotNACK() to call WriteRTP on the RTX track?

I'm not sure. There are four cases:

packet is received normally;
packet is received because the main loop sent a NACK;
packet is sent out because gotNACK found the packet in cache;
packet is received because gotNACK got a cache miss and sent a NACK.

Currently, we don't distinguish between the four cases. In your code, you'll need to ensure that (1) and (2) get sent on the main track, while (3) and (4) are sent over the RTX track. I think there's no obvious way to distinguish between (2) and (4), so I suggest you simply send the (4) case on the main track for now.

jech commented 3 years ago

Here's the offer sent by galene-stream. Contrary to what I believed, it doesn't seem to negotiate an extra m section for the RTX data, it just negotiates an extra payload type within the same m section. This is consistent with what browsers negotiate. I'm not sure how Pion handles that, I'll ask on the Pion slack channel.

v=0
o=- 2654777742637044684 0 IN IP4 0.0.0.0
s=-
t=0 0
a=ice-options:trickle
a=group:BUNDLE video0 audio1
m=video 9 UDP/TLS/RTP/SAVPF 97 98
c=IN IP4 0.0.0.0
a=setup:actpass
a=ice-ufrag:jSoFks1lf5IwraVVx/LO9Mp1Uh8TTlb1
a=ice-pwd:8lcOVbxxtLx1Z9qKPUgvIdzdkKv4RFOL
a=rtcp-mux
a=rtcp-rsize
a=sendrecv
a=rtpmap:97 VP8/90000
a=rtcp-fb:97 nack
a=rtcp-fb:97 nack pli
a=framerate:30
a=rtpmap:98 rtx/90000
a=fmtp:98 apt=97
a=ssrc-group:FID 3229810801 3635649363
a=ssrc:3229810801 msid:user3560569412@host-bdd7e14f webrtctransceiver0
a=ssrc:3229810801 cname:user3560569412@host-bdd7e14f
a=ssrc:3635649363 msid:user3560569412@host-bdd7e14f webrtctransceiver0
a=ssrc:3635649363 cname:user3560569412@host-bdd7e14f
a=mid:video0
a=fingerprint:sha-256 4F:79:4C:5F:AE:F5:C1:31:21:29:F6:9D:9B:94:EC:ED:C1:6D:C5:A7:88:18:F4:88:08:A6:B2:45:F8:91:2F:5B
m=audio 0 UDP/TLS/RTP/SAVPF 96 99
c=IN IP4 0.0.0.0
a=setup:actpass
a=ice-ufrag:jSoFks1lf5IwraVVx/LO9Mp1Uh8TTlb1
a=ice-pwd:8lcOVbxxtLx1Z9qKPUgvIdzdkKv4RFOL
a=bundle-only
a=rtcp-mux
a=rtcp-rsize
a=sendrecv
a=rtpmap:96 OPUS/48000/2
a=rtcp-fb:96 nack
a=rtcp-fb:96 nack pli
a=fmtp:96 sprop-maxcapturerate=48000;sprop-stereo=1
a=rtpmap:99 rtx/48000
a=fmtp:99 apt=96
a=ssrc-group:FID 2803109902 2533515851
a=ssrc:2803109902 msid:user3560569412@host-bdd7e14f webrtctransceiver1
a=ssrc:2803109902 cname:user3560569412@host-bdd7e14f
a=ssrc:2533515851 msid:user3560569412@host-bdd7e14f webrtctransceiver1
a=ssrc:2533515851 cname:user3560569412@host-bdd7e14f
a=mid:audio1
a=fingerprint:sha-256 4F:79:4C:5F:AE:F5:C1:31:21:29:F6:9D:9B:94:EC:ED:C1:6D:C5:A7:88:18:F4:88:08:A6:B2:45:F8:91:2F:5B

jech commented 3 years ago

Looking at it some more, it looks like Pion doesn't support having multiple payload types in a single track. We're going to need to extend Pion in order to implement the RTX ptype.

https://github.com/pion/webrtc/issues/1675

erdnaxe commented 3 years ago

From galene-stream/GStreamer perspective, another way to temporarily solve this may be to use one rtprtxqueue object. If I understand the description of the object, it does exactly what is needed for retranmissions with same SSRC and should work with Galène. The catch is that this module seems to have only been developed/tested for RTP use before WebRTC was introduced. It does not seem to play nice with the webrtcbin object of GStreamer. I am going to continue to experiment a bit on this side but I'm quite pessimistic about it.

I believe implementing the support for multiple payload per track in Pion will be useful to increase interoperability, but it seems to require a lot of effort, code and knowledge.

jech commented 3 years ago

I think that you should report the limitation with the GStreamer people, the issue should be pretty trivial to fix.

As to your code, you should expect Galène to handle a separate RTX SSID as soon as it is implemented in Pion, so I wouldn't bother with temporary workarounds.

jech commented 1 month ago

In progress: https://github.com/pion/webrtc/pull/2914

jech / galene

Implement separate RTX payload type for retransmission on NACK #79