feat: support NACK retransmission without RTX via in-band resend on main SSRC#980
feat: support NACK retransmission without RTX via in-band resend on main SSRC#980mstyura wants to merge 1 commit into
Conversation
mstyura
force-pushed
the
nack-is-nack-rtx-is-rtx
branch
from
a0aaa87 to
dcfa60e
Compare
|
@mstyura not necessarily arguing against this, but why wouldn't you use RTX also for the Opus case? RTX is a cryptographic safety to avoid an evil MITM forcing us to repeat the same data over the main SRTP channel. Seems like you want to enable it, no? |
|
I guess the answer is: libWebRTC doesn't do that. |
algesten
left a comment
algesten
left a comment
There was a problem hiding this comment.
I'm on the fence about this one.
I do think supporting Opus resends without RTX is a nice goal, but I'm not sure this PR goes all the way. In my mind, I've used the "rtx" meaning "can resend" interchangeably, which means to make the code base consistent in language and logic, there's a deeper take that is needed.
I would want to do that manually myself.
| pub fn set_resend(&mut self, resend: Option<Pt>) { | ||
| self.resend = resend; | ||
| } | ||
|
|
There was a problem hiding this comment.
pt and resend are set on construction and immutable. There deliberately is no setter for resend.
| } else if !param.fb_nack() { | ||
| // No RTX PT and no `fb_nack`: the packet cannot be resent (neither | ||
| // via RTX nor in-band), so it is de-facto not nackable. This ensures | ||
| // there are no entries in the rtx cache we can't resend. |
There was a problem hiding this comment.
This causes a confusion in the code base. "rtx" is in many places used to mean "can resend" interchangeably. I'm a bit frustrated with myself for doing that back in the day, because at this point this PR is working against language and patterns that are consistent across the code base.
I don't know what the solution is.
| // payload decrypts at the receiver. | ||
| header_ref.ext_vals.rid_repair = None; | ||
| header_ref.clone() | ||
| } |
There was a problem hiding this comment.
I'm uncertain about having a new enum value here. Whether we want just NextPacketKind::Resend, but discover the "mode" of whether we are operating with or without RTX.
However this has consequences for the BWE and Pacer, so again. I'm not certain.
|
I have the following use-case of |
|
@mstyura yes. It surely is an omission. |
|
Note that RTX usage for audio is not so common due to re-order of packets and causing extra delay (if that matters) In bacnd fec or DRED in Opus is better in terms of latency. |
3 participants
I'd like to propose to decouple
NACKfromRTX. This PR gatesNACKon negotiatednackfeedback rather than the presence ofRTX. When noRTXSSRCis configured, retransmissions are resent in-band on the mainSSRC(original PT + sequence number, aslibwebrtcdoes).The main motivation:
str0mcan now NACK-retransmitOpuspackets, which previously had no loss recovery since audio never getsRTX. In practice this is audible - a long sustained sound (e.g. "AAAAAAA" for 5 seconds) recorded over a lossy network (5%) now plays back without the audible glitches that the dropped packets used to cause.Also adds
PayloadParams::set_resendto configure/disable RTX.