From 2e867e33465b723aa11e8c231941f1e1050c9551 Mon Sep 17 00:00:00 2001 From: Mathis Engelbart Date: Fri, 4 Sep 2026 09:22:40 +0200 Subject: [PATCH] Fix cleanBefore underflow --- pkg/rtpfb/history.go | 2 +- pkg/rtpfb/history_test.go | 287 ++++++++++++++++++++------------------ 2 files changed, 152 insertions(+), 137 deletions(-) diff --git a/pkg/rtpfb/history.go b/pkg/rtpfb/history.go index 75549c34..787dffdb 100644 --- a/pkg/rtpfb/history.go +++ b/pkg/rtpfb/history.go @@ -190,5 +190,5 @@ func (h *history) cleanBefore(counter uint64) { delete(h.packets, i) } } - h.cleanUntil = counter - 1 + h.cleanUntil = counter } diff --git a/pkg/rtpfb/history_test.go b/pkg/rtpfb/history_test.go index de6849dc..1c6cc99a 100644 --- a/pkg/rtpfb/history_test.go +++ b/pkg/rtpfb/history_test.go @@ -12,150 +12,165 @@ import ( "github.com/stretchr/testify/assert" ) -func TestHistory(t *testing.T) { - t.Run("test_ccfb", func(t *testing.T) { - cases := []struct { - packets []uint16 // RTP sequence number - feedbackFirst []acknowledgement - feedbackSecond []acknowledgement - expectedFirst []PacketReport - expectedSecond []PacketReport - }{ - { - packets: []uint16{0, 1, 2, 3}, - feedbackFirst: []acknowledgement{ - {sequenceNumber: 0, arrived: true}, - {sequenceNumber: 1, arrived: true}, - {sequenceNumber: 2, arrived: true}, - }, - expectedFirst: []PacketReport{ - {SSRC: 1, SequenceNumber: 0, RTPSequenceNumber: 0, Arrived: true}, - {SSRC: 1, SequenceNumber: 1, RTPSequenceNumber: 1, Arrived: true}, - {SSRC: 1, SequenceNumber: 2, RTPSequenceNumber: 2, Arrived: true}, - }, +func TestHistoryCCFB(t *testing.T) { + cases := []struct { + packets []uint16 // RTP sequence number + feedbackFirst []acknowledgement + feedbackSecond []acknowledgement + expectedFirst []PacketReport + expectedSecond []PacketReport + }{ + { + packets: []uint16{0, 1, 2, 3}, + feedbackFirst: []acknowledgement{ + {sequenceNumber: 0, arrived: true}, + {sequenceNumber: 1, arrived: true}, + {sequenceNumber: 2, arrived: true}, }, - { - packets: []uint16{5, 6, 7, 8, 9}, - feedbackFirst: []acknowledgement{ - {sequenceNumber: 5, arrived: true}, - {sequenceNumber: 6, arrived: false}, - {sequenceNumber: 7, arrived: true}, - }, - expectedFirst: []PacketReport{ - {SSRC: 1, SequenceNumber: 0, RTPSequenceNumber: 5, Arrived: true}, - {SSRC: 1, SequenceNumber: 1, RTPSequenceNumber: 6, Arrived: false}, - {SSRC: 1, SequenceNumber: 2, RTPSequenceNumber: 7, Arrived: true}, - }, + expectedFirst: []PacketReport{ + {SSRC: 1, SequenceNumber: 0, RTPSequenceNumber: 0, Arrived: true}, + {SSRC: 1, SequenceNumber: 1, RTPSequenceNumber: 1, Arrived: true}, + {SSRC: 1, SequenceNumber: 2, RTPSequenceNumber: 2, Arrived: true}, }, - { - packets: []uint16{1, 2, 3, 4, 5}, - feedbackFirst: []acknowledgement{ - {sequenceNumber: 1, arrived: true}, - {sequenceNumber: 2, arrived: true}, - }, - feedbackSecond: []acknowledgement{ - {sequenceNumber: 3, arrived: true}, - {sequenceNumber: 4, arrived: true}, - {sequenceNumber: 5, arrived: true}, - }, - expectedFirst: []PacketReport{ - {SSRC: 1, SequenceNumber: 0, RTPSequenceNumber: 1, Arrived: true}, - {SSRC: 1, SequenceNumber: 1, RTPSequenceNumber: 2, Arrived: true}, - }, - expectedSecond: []PacketReport{ - {SSRC: 1, SequenceNumber: 2, RTPSequenceNumber: 3, Arrived: true}, - {SSRC: 1, SequenceNumber: 3, RTPSequenceNumber: 4, Arrived: true}, - {SSRC: 1, SequenceNumber: 4, RTPSequenceNumber: 5, Arrived: true}, - }, + }, + { + packets: []uint16{5, 6, 7, 8, 9}, + feedbackFirst: []acknowledgement{ + {sequenceNumber: 5, arrived: true}, + {sequenceNumber: 6, arrived: false}, + {sequenceNumber: 7, arrived: true}, }, - { - packets: []uint16{1, 2, 3, 4, 5}, - feedbackFirst: []acknowledgement{ - {sequenceNumber: 1, arrived: true}, - {sequenceNumber: 2, arrived: true}, - {sequenceNumber: 3, arrived: true}, - }, - feedbackSecond: []acknowledgement{ - {sequenceNumber: 1, arrived: true}, - {sequenceNumber: 2, arrived: true}, - {sequenceNumber: 3, arrived: true}, - {sequenceNumber: 4, arrived: true}, - }, - expectedFirst: []PacketReport{ - {SSRC: 1, SequenceNumber: 0, RTPSequenceNumber: 1, Arrived: true}, - {SSRC: 1, SequenceNumber: 1, RTPSequenceNumber: 2, Arrived: true}, - {SSRC: 1, SequenceNumber: 2, RTPSequenceNumber: 3, Arrived: true}, - }, - expectedSecond: []PacketReport{ - {SSRC: 1, SequenceNumber: 3, RTPSequenceNumber: 4, Arrived: true}, - }, + expectedFirst: []PacketReport{ + {SSRC: 1, SequenceNumber: 0, RTPSequenceNumber: 5, Arrived: true}, + {SSRC: 1, SequenceNumber: 1, RTPSequenceNumber: 6, Arrived: false}, + {SSRC: 1, SequenceNumber: 2, RTPSequenceNumber: 7, Arrived: true}, }, - { - packets: []uint16{65534, 65535, 0, 1}, - feedbackFirst: []acknowledgement{ - {sequenceNumber: 65534, arrived: true}, - {sequenceNumber: 65535, arrived: true}, - {sequenceNumber: 0, arrived: true}, - {sequenceNumber: 1, arrived: true}, - }, - feedbackSecond: []acknowledgement{}, - expectedFirst: []PacketReport{ - {SSRC: 1, SequenceNumber: 0, RTPSequenceNumber: 65534, Arrived: true}, - {SSRC: 1, SequenceNumber: 1, RTPSequenceNumber: 65535, Arrived: true}, - {SSRC: 1, SequenceNumber: 2, RTPSequenceNumber: 0, Arrived: true}, - {SSRC: 1, SequenceNumber: 3, RTPSequenceNumber: 1, Arrived: true}, - }, - expectedSecond: nil, + }, + { + packets: []uint16{1, 2, 3, 4, 5}, + feedbackFirst: []acknowledgement{ + {sequenceNumber: 1, arrived: true}, + {sequenceNumber: 2, arrived: true}, }, - } - for i, tc := range cases { - t.Run(fmt.Sprintf("%v", i), func(t *testing.T) { - history := newHistory() - for _, p := range tc.packets { - history.addOutgoing(1, p, false, 0, 0, time.Time{}) - } - for _, f := range tc.feedbackFirst { - history.onCCFBFeedback(time.Time{}, 1, f) - } - reports := history.buildReport() - assert.Equal(t, tc.expectedFirst, reports) + feedbackSecond: []acknowledgement{ + {sequenceNumber: 3, arrived: true}, + {sequenceNumber: 4, arrived: true}, + {sequenceNumber: 5, arrived: true}, + }, + expectedFirst: []PacketReport{ + {SSRC: 1, SequenceNumber: 0, RTPSequenceNumber: 1, Arrived: true}, + {SSRC: 1, SequenceNumber: 1, RTPSequenceNumber: 2, Arrived: true}, + }, + expectedSecond: []PacketReport{ + {SSRC: 1, SequenceNumber: 2, RTPSequenceNumber: 3, Arrived: true}, + {SSRC: 1, SequenceNumber: 3, RTPSequenceNumber: 4, Arrived: true}, + {SSRC: 1, SequenceNumber: 4, RTPSequenceNumber: 5, Arrived: true}, + }, + }, + { + packets: []uint16{1, 2, 3, 4, 5}, + feedbackFirst: []acknowledgement{ + {sequenceNumber: 1, arrived: true}, + {sequenceNumber: 2, arrived: true}, + {sequenceNumber: 3, arrived: true}, + }, + feedbackSecond: []acknowledgement{ + {sequenceNumber: 1, arrived: true}, + {sequenceNumber: 2, arrived: true}, + {sequenceNumber: 3, arrived: true}, + {sequenceNumber: 4, arrived: true}, + }, + expectedFirst: []PacketReport{ + {SSRC: 1, SequenceNumber: 0, RTPSequenceNumber: 1, Arrived: true}, + {SSRC: 1, SequenceNumber: 1, RTPSequenceNumber: 2, Arrived: true}, + {SSRC: 1, SequenceNumber: 2, RTPSequenceNumber: 3, Arrived: true}, + }, + expectedSecond: []PacketReport{ + {SSRC: 1, SequenceNumber: 3, RTPSequenceNumber: 4, Arrived: true}, + }, + }, + { + packets: []uint16{65534, 65535, 0, 1}, + feedbackFirst: []acknowledgement{ + {sequenceNumber: 65534, arrived: true}, + {sequenceNumber: 65535, arrived: true}, + {sequenceNumber: 0, arrived: true}, + {sequenceNumber: 1, arrived: true}, + }, + feedbackSecond: []acknowledgement{}, + expectedFirst: []PacketReport{ + {SSRC: 1, SequenceNumber: 0, RTPSequenceNumber: 65534, Arrived: true}, + {SSRC: 1, SequenceNumber: 1, RTPSequenceNumber: 65535, Arrived: true}, + {SSRC: 1, SequenceNumber: 2, RTPSequenceNumber: 0, Arrived: true}, + {SSRC: 1, SequenceNumber: 3, RTPSequenceNumber: 1, Arrived: true}, + }, + expectedSecond: nil, + }, + } + for i, tc := range cases { + t.Run(fmt.Sprintf("%v", i), func(t *testing.T) { + history := newHistory() + for _, p := range tc.packets { + history.addOutgoing(1, p, false, 0, 0, time.Time{}) + } + for _, f := range tc.feedbackFirst { + history.onCCFBFeedback(time.Time{}, 1, f) + } + reports := history.buildReport() + assert.Equal(t, tc.expectedFirst, reports) - for _, f := range tc.feedbackSecond { - history.onCCFBFeedback(time.Time{}, 1, f) - } - reports = history.buildReport() - assert.Equal(t, tc.expectedSecond, reports) - }) - } - }) + for _, f := range tc.feedbackSecond { + history.onCCFBFeedback(time.Time{}, 1, f) + } + reports = history.buildReport() + assert.Equal(t, tc.expectedSecond, reports) + }) + } +} - t.Run("prunes_packets", func(t *testing.T) { - history := newHistory() - for i := range uint16(10) { - history.addOutgoing(1, i, false, 0, 0, time.Time{}) - } - for i := range uint16(10) { - history.onCCFBFeedback(time.Time{}, 1, acknowledgement{sequenceNumber: i, arrived: true}) - } - reports := history.buildReport() - assert.Len(t, reports, 10) - assert.Empty(t, history.packets) - }) +func TestHistoryPrunesPackets(t *testing.T) { + history := newHistory() + for i := range uint16(10) { + history.addOutgoing(1, i, false, 0, 0, time.Time{}) + } + for i := range uint16(10) { + history.onCCFBFeedback(time.Time{}, 1, acknowledgement{sequenceNumber: i, arrived: true}) + } + reports := history.buildReport() + assert.Len(t, reports, 10) + assert.Empty(t, history.packets) +} + +func TestHistoryCleanUntilDoesNotUnderflow(t *testing.T) { + history := newHistory() + reports := history.buildReport() + assert.Empty(t, reports) + assert.Equal(t, uint64(0), history.cleanUntil) - t.Run("sets_is_twcc", func(t *testing.T) { - history := newHistory() - for i := range uint16(10) { - history.addOutgoing(1, i, true, i, 0, time.Time{}) - } - for i := range uint16(10) { - history.onTWCCFeedback(time.Time{}, acknowledgement{sequenceNumber: i, arrived: true}) - } - reports := history.buildReport() - for _, r := range reports { - assert.True(t, r.IsTWCC) - } - assert.Empty(t, history.twccToCounter) - }) + for i := range uint16(5) { + history.addOutgoing(1, i, false, 0, 0, time.Time{}) + } + for i := range uint16(5) { + history.onCCFBFeedback(time.Time{}, 1, acknowledgement{sequenceNumber: i, arrived: true}) + } + reports = history.buildReport() + assert.Len(t, reports, 5) + assert.Equal(t, uint64(5), history.cleanUntil) +} + +func TestHistorySetsIsTWCC(t *testing.T) { + history := newHistory() + for i := range uint16(10) { + history.addOutgoing(1, i, true, i, 0, time.Time{}) + } + for i := range uint16(10) { + history.onTWCCFeedback(time.Time{}, acknowledgement{sequenceNumber: i, arrived: true}) + } + reports := history.buildReport() + for _, r := range reports { + assert.True(t, r.IsTWCC) + } + assert.Empty(t, history.twccToCounter) } // TestHistoryConcurrentFeedback verifies that concurrent calls to onTWCCFeedback