Add RFC 2198 RED (redundant audio) support for Opus#982
Conversation
d17d18e to
e222d66
Compare
|
I started a review this morning, and I'm still working on it. I have some concerns of the placement of the changes in session.rs, and I want to explore myself if there is some better solution I would feel happier with. |
|
In general one would not enable RED unless there is packetloss that shows there is a need for it. So it should be enabled on demand. Also RED can support all codecs, so it should not be bound to Opus. |
A default-off `RtcConfig::enable_red` negotiates RFC 2198 RED for Opus, modeled on RTX: a `Codec::Red` marker and a `PayloadParams.red` linkage folded onto the primary codec in SDP (`red/48000/2` and `fmtp <opus>/<opus>`), kept only when both peers offer it. In frame mode RED is transparent. Outgoing Opus is wrapped with one level of redundancy (the previous frame); incoming RED is unwrapped back to Opus and a single lost packet is recovered from the next packet's redundancy. In rtp mode the RED packets pass through unchanged, and `RedEncoder`/`RedDecoder` are exposed for callers that want to build or parse RED themselves. A self-contained `packet::red` codec does the byte-level RFC 2198 work and is fuzzed; recovered packets reuse the existing depayload path and the jitter buffer's de-duplication.
e222d66 to
5b0799a
Compare
You mean all audio codecs, right? |
Yes, let me add some more information here. RED should be available for all Audio Codecs. You should be able to switch it on demand, so you start sending RED when you need and not all the time unless you want to. RED is not limited to 1 redundant payload or just take main-1. You could do a RED packet with main, main-1, main-3, main-5 etc. If we anyway do the work here I think this should be doable. |
I was debating this with Claude. It seems a browser doesn't have an "easy" way to turn RED on/off. You need to do an SDP negotiation to set the codec preference "red, opus" then the browser starts sending RED. This is not true for SFU -> Browser, because any PT that is declared for the m-line is valid, so we can "just switch" at any point. str0m currently doesn't support renegotiating codecs. Do you think runtime enabling of RED is so important that we should work on renegotiating codecs? I know you guys don't use SDP at all, so the question is maybe meaningless :P |
|
Right, so Chrome you need to re-negotiate to be able to start sending RED or not. I think we can keep that as a separate issue. We just need to make sure str0m can depacketize RED and use the main packets depacketizer For sending, (str0m is also used for clients) I think there should be a way to indicate that we now want to to send this codec in RED with this set of distances. We could limit the amount of RED packets to maybe 4 then just have a fixed array that indicate the distance from the main packet. 0 means not used. [1, 3, 5] would mean that we keep a buffer around to save the last 5 sent packets and then resend them in that pattern. But I am not sure where this sort of dynamic configure would fit in the sample api. |
It sort of affects the shape of the API. Whether it involved SDP API or not. I will ponder.
Are you saying you want to dynamically be able to change the sets of distances, or can that be configured once and for all and then you chose RED on/off? |
|
Thanks, this has been really useful. I went deep on the receive side and it convinced me we should treat this as a proper complete RED rather than the minimal Opus version the PR is now, so let me lay out where I landed, flag one small existing bug I found along the way, and then a question about how you want to take it. The thing that reframed it for me: RFC 2198 tags each redundant block with a timestamp offset, not a sequence number, but our DepacketizingBuffer is keyed and deduped by seq_no. So to actually recover a lost packet we have to hand it a correct seq_no. The current code works it out positionally, rec_seq = seq - (n - i), which is only right when the redundancy is contiguous, main-1 then main-2. The moment you allow main-1, main-3, main-5 like you mentioned @xnorpx, it produces wrong seq_nos and the buffer silently drops both the recovered frame and the real one. It doesn't bite today only because we send a single main-1 block. The fix is to derive the distance from the timestamp offset instead, using the negotiated ptime for the frame size, offset / frame_size. That reproduces the contiguous case exactly and also handles the non-contiguous one, and I'd make it correct-or-skip: if an offset doesn't land on a clean packet boundary, or the seq isn't actually missing, just skip that block rather than guess. So it never corrupts, and the plain unwrap underneath is always correct regardless. Frame size isn't truly fixed (Opus can be 10/20/40/60ms), so recovering arbitrary distances is best effort by nature, which I think is fine as long as we're honest about it. That pushes me to split the receive path into the two things it currently tangles together, which might be close to the session.rs restructuring you had in mind @algesten. First, just unwrapping RED and handing the primary block to the normal depacketizer, no seq mapping, works for any audio codec and any block count. That's your must-have @xnorpx and it's unconditionally correct. Second, the best effort recovery sitting on top, isolated. It extracts cleanly too, a method on StreamRx that returns the recovered packets with the delivery staying in the session loop, same shape as un_rtx. Audio agnostic turned out to be a bit more than swapping == Opus for is_audio(). The RED rtpmap carries a clock rate per pt, so a single RED pt can't cover two audio codecs with different clock rates, and today we'd actually hand the same pt 63 to both and collide in SDP. So the right thing is a RED pt per red-enabled codec, each advertised with that codec's rtp clock rate. While I was in there I noticed the RED rtpmap emits the codec's user-facing clock rate rather than the rtp clock rate, which is invisible for Opus but would be wrong for G722, rtx already does it the right way. Small fix regardless. On static vs dynamic distances, I'd keep the distance pattern static and add a plain on/off at runtime. We can't renegotiate codecs, so the pt and the pattern are fixed up front anyway, but flipping wrapping on and off needs no renegotiation. It is new api though, nothing like it exists yet, so it would follow the set_direction shape. Browser side on/off and auto enabling on loss both still look like separate issues to me. Where I'd like your steer: this is a fair bit more than the minimal PR. The byte codec and the negotiation model carry over as is, but the recovery, the per-codec RED pt, the send side and the on/off api all need real rework to become the complete thing. Would you rather I evolve this PR toward that, or settle the shape first, especially where you want the recovery code to live, and I redo the changed parts against that. Happy either way, I just don't want to build the recovery out in a spot you're about to restructure. |
The first junction is to decide if we are letting go of the "str0m does not further negotiate m-lines" or stick with it. In practice, I never seen the need to renegotiate m-lines after the initial OFFER/ANSWER, but then my experience might be too limited. I haven't used RED, and I don't know if toggling RED on/off is common. The fastest/simplest way forward is to say that:
|
|
That works for me and it keeps str0m's model intact, which I like. The initial offer/answer decides whether RED is available on the m-line, and we don't renegotiate to add or remove it after. Agreed. One nuance on the toggle though, coming back to your own point that an SFU sending to a browser can just switch at any time. Once the RED pt is negotiated, whether a given outgoing packet is actually wrapped in RED is purely a send side choice with no renegotiation needed, since both the RED pt and the plain pt are valid on the m-line and the receiver already handles both. So I'd hang the on/off on the send side and let it work for an SDP negotiated sender too, not just DirectAPI. SDP decides RED is available, a runtime flag decides whether we wrap right now. That's the case where toggling actually earns its keep, an SFU turning RED on for a leg that's losing packets and off again when it's clean, without touching SDP. A pure client without SDP gets the same flag through DirectAPI. So the shape I'm converging on is this: RED availability on the m-line is fixed at negotiation or set up via DirectAPI, the distance pattern is static config, and the only runtime lever is wrap on/off on the send side, available to both paths, with no renegotiation anywhere. And picking up @xnorpx's earlier point, this is for all audio codecs rather than just Opus, which in practice means a RED pt per codec the same way RTX already gets a resend pt per codec, each with that codec's rtp clock rate. I can't say how common toggling is either, but as a plain send side flag it's cheap enough to just have, and it covers the loss driven case @xnorpx had in mind. |
|
To me this sounds great. |
What
Adds optional RFC 2198 RED (redundant audio) for Opus via
RtcConfig::enable_red(true)(off by default). RED carries a redundant copy of the previous Opus frame in each packet, letting the receiver recover isolated packet losses without retransmission, at the cost of roughly doubling audio payload size.Design
Modeled on the existing RTX support:
Codec::Redmarker +PayloadParams.red: Option<Pt>linkage, folded onto the primary Opus codec in SDP (a=rtpmap:63 red/48000/2,a=fmtp:63 111/111), mirroring howaptlinks RTX. Kept only when both peers offer it.RedEncoder/RedDecoderare public for callers that want to build or parse RED themselves.packet::redmodule does the byte-level RFC 2198 work and is fuzzed.Robustness
RED payloads are untrusted remote input, so the receive path is bounded:
Tests
Unit (byte-level encode/decode round-trips, no-panic fuzz-style decode, block-count cap, recovery depth/PT selection) plus integration in
tests/red.rs: transparent round-trip, single-loss recovery vs. plain Opus under seeded loss, interop fallback when only one side enables RED (both directions), rtp-mode pass-through, and that the offer lists the RED PT in the m-line fmt list.Notes
[bool; 128]indexing), which affects the main/RTX paths too. Happy to send a separate fix.