From d0fd37eaa0a5a64e844d4e0e62a509262efee7d7 Mon Sep 17 00:00:00 2001 From: Raja Subramanian Date: Wed, 14 Oct 2026 11:07:35 +0530 Subject: [PATCH] Send NACKs when processing RTX packets. (#5000) * Send NACKs when processing RTX packets. Applying the fix proposed in https://github.com/livekit/livekit/issues/4999 * Add test for NACKs sent while processing RTX packets. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- pkg/sfu/buffer/buffer.go | 19 +++++++++---- pkg/sfu/buffer/buffer_test.go | 53 +++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/pkg/sfu/buffer/buffer.go b/pkg/sfu/buffer/buffer.go index 551a60b5e..b55275508 100644 --- a/pkg/sfu/buffer/buffer.go +++ b/pkg/sfu/buffer/buffer.go @@ -327,15 +327,24 @@ func (b *Buffer) NotifyRTX(ssrc uint32, repairSSRC uint32, rsid string) { } func (b *Buffer) writeRTX(rtxPkt *rtp.Packet, arrivalTime int64) { + rtcpPackets := b.writeRTXLocked(rtxPkt, arrivalTime) + if len(rtcpPackets) != 0 { + if cb := b.getOnRtcpFeedback(); cb != nil { + cb(rtcpPackets) + } + } +} + +func (b *Buffer) writeRTXLocked(rtxPkt *rtp.Packet, arrivalTime int64) []rtcp.Packet { b.Lock() defer b.Unlock() if !b.isBound { - return + return nil } if rtxPkt.PayloadType != b.rtxPayloadType { b.logger.Debugw("unexpected rtx payload type", "expected", b.rtxPayloadType, "actual", rtxPkt.PayloadType) - return + return nil } if b.rtxPktBuf == nil { @@ -344,7 +353,7 @@ func (b *Buffer) writeRTX(rtxPkt *rtp.Packet, arrivalTime int64) { if len(rtxPkt.Payload) < 2 { b.logger.Warnw("rtx payload too short", nil, "size", len(rtxPkt.Payload)) - return + return nil } repairedPkt := *rtxPkt @@ -355,10 +364,10 @@ func (b *Buffer) writeRTX(rtxPkt *rtp.Packet, arrivalTime int64) { n, err := repairedPkt.MarshalTo(b.rtxPktBuf) if err != nil { b.logger.Errorw("could not marshal repaired packet", err, "ssrc", b.BufferBase.SSRC(), "sn", repairedPkt.SequenceNumber) - return + return nil } - b.calc(b.rtxPktBuf[:n], &repairedPkt, arrivalTime, false, true) + return b.calc(b.rtxPktBuf[:n], &repairedPkt, arrivalTime, false, true) } func (b *Buffer) Read(buff []byte) (n int, err error) { diff --git a/pkg/sfu/buffer/buffer_test.go b/pkg/sfu/buffer/buffer_test.go index 39267dade..032596dfd 100644 --- a/pkg/sfu/buffer/buffer_test.go +++ b/pkg/sfu/buffer/buffer_test.go @@ -182,6 +182,59 @@ func TestNack(t *testing.T) { }) } +func TestNackOnRTX(t *testing.T) { + buff := NewBuffer(123, 1, 1) + buff.codecType = webrtc.RTPCodecTypeVideo + var mu sync.Mutex + var nacked []uint16 + buff.OnRtcpFeedback(func(fb []rtcp.Packet) { + mu.Lock() + defer mu.Unlock() + for _, pkt := range fb { + if p, ok := pkt.(*rtcp.TransportLayerNack); ok && p.MediaSSRC == 123 { + for _, pair := range p.Nacks { + nacked = append(nacked, pair.PacketList()...) + } + } + } + }) + require.NoError(t, buff.Bind(webrtc.RTPParameters{ + Codecs: []webrtc.RTPCodecParameters{vp8Codec}, + }, vp8Codec.RTPCodecCapability, 0)) + buff.rtxPayloadType = 97 + rtt := uint32(20) + buff.nacker.SetRTT(rtt) + + payload := []byte{0xff, 0xff, 0xff, 0xfd, 0xb4, 0x9f, 0x94, 0x1} + // sequence numbers 1 and 2 are lost + for _, sn := range []uint16{0, 3} { + pkt := rtp.Packet{ + Header: rtp.Header{Version: 2, PayloadType: 96, SequenceNumber: sn, Timestamp: uint32(sn), SSRC: 123}, + Payload: payload, + } + b, err := pkt.Marshal() + require.NoError(t, err) + _, err = buff.Write(b) + require.NoError(t, err) + } + + time.Sleep(time.Duration(2*rtt) * time.Millisecond) + mu.Lock() + nacked = nil + mu.Unlock() + + // RTX repairs sequence number 2, the NACK for 1 is due and must be sent + rtxPkt := rtp.Packet{ + Header: rtp.Header{Version: 2, PayloadType: 97, SequenceNumber: 500, Timestamp: 2, SSRC: 456}, + Payload: append([]byte{0x00, 0x02}, payload...), + } + buff.writeRTX(&rtxPkt, time.Now().UnixNano()) + + mu.Lock() + defer mu.Unlock() + require.Equal(t, []uint16{1}, nacked) +} + func TestNewBuffer(t *testing.T) { tests := []struct { name string