Repository: pion/interceptor
Pull request #294 (2024-12-16 16:21:13)

Fix nack buffer RTX issue
There was previously an issue with the nack buffer that would occur when an uploader was experiencing packet loss (or otherwise had reordering occur). `lastAdded` was getting set for every packet added to the buffer, but packets could get added out of order, causing `lastAdded` to go backwards. The impact was that packets at the end of the buffer were sometimes considered stale and cleared out. Consider the following sequence of packets being added to the buffer:

1, 2, 4, 3, 5, ...

When 4 is added, `lastAdded` would correctly be 4, but then when 3 arrived, `lastAdded` would go backwards to 3. This would cause two related problems: trying to get 4 from the buffer would fail (`uint16(3 - 4) > Uint16SizeHalf`), and then when we add 5, we would treat 4 as a gap between `lastAdded` (3) and `seq` (5) and thus release 4 from the buffer, such that even after 5 was added, we would still not be able to retrieve 4 from the buffer.

This PR modifies the logic to only move `lastAdded` forward if `diff` is "positive" (less than `Uint16SizeHalf`), meaning the new packet is newer than any packet received previously.


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/interceptor/pull/294?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 71.14%. Comparing base [(`9251977`)](https://app.codecov.io/gh/pion/interceptor/commit/925197744916b2590f6d9c6a76bb2c6a3d7d7318?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) to head [(`4fd7c35`)](https://app.codecov.io/gh/pion/interceptor/commit/4fd7c35cfd52e3d6606ae51be39ecfdab7aee004?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff           @@
##           master     #294   +/-   ##
=======================================
  Coverage   71.14%   71.14%           
=======================================
  Files          80       80           
  Lines        4477     4477           
=======================================
  Hits         3185     3185           
+ Misses       1157     1156    -1     
- Partials      135      136    +1     
```

| [Flag](https://app.codecov.io/gh/pion/interceptor/pull/294/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [go](https://app.codecov.io/gh/pion/interceptor/pull/294/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `71.00% <100.00%> (-0.14%)` | :arrow_down: |
| [wasm](https://app.codecov.io/gh/pion/interceptor/pull/294/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `69.28% <100.00%> (+0.06%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/pion/interceptor/pull/294?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).


Patch:
diff --git a/internal/rtpbuffer/rtpbuffer.go b/internal/rtpbuffer/rtpbuffer.go
index 9253507..1deefee 100644
--- a/internal/rtpbuffer/rtpbuffer.go
+++ b/internal/rtpbuffer/rtpbuffer.go
@@ -17,10 +17,10 @@ const (
 
 // RTPBuffer stores RTP packets and allows custom logic around the lifetime of them via the PacketFactory
 type RTPBuffer struct {
-	packets   []*RetainablePacket
-	size      uint16
-	lastAdded uint16
-	started   bool
+	packets      []*RetainablePacket
+	size         uint16
+	highestAdded uint16
+	started      bool
 }
 
 // NewRTPBuffer constructs a new RTPBuffer
@@ -50,16 +50,16 @@ func (r *RTPBuffer) Add(packet *RetainablePacket) {
 	seq := packet.sequenceNumber
 	if !r.started {
 		r.packets[seq%r.size] = packet
-		r.lastAdded = seq
+		r.highestAdded = seq
 		r.started = true
 		return
 	}
 
-	diff := seq - r.lastAdded
+	diff := seq - r.highestAdded
 	if diff == 0 {
 		return
 	} else if diff < Uint16SizeHalf {
-		for i := r.lastAdded + 1; i != seq; i++ {
+		for i := r.highestAdded + 1; i != seq; i++ {
 			idx := i % r.size
 			prevPacket := r.packets[idx]
 			if prevPacket != nil {
@@ -67,6 +67,7 @@ func (r *RTPBuffer) Add(packet *RetainablePacket) {
 			}
 			r.packets[idx] = nil
 		}
+		r.highestAdded = seq
 	}
 
 	idx := seq % r.size
@@ -75,12 +76,11 @@ func (r *RTPBuffer) Add(packet *RetainablePacket) {
 		prevPacket.Release()
 	}
 	r.packets[idx] = packet
-	r.lastAdded = seq
 }
 
 // Get returns the RetainablePacket for the requested sequence number
 func (r *RTPBuffer) Get(seq uint16) *RetainablePacket {
-	diff := r.lastAdded - seq
+	diff := r.highestAdded - seq
 	if diff >= Uint16SizeHalf {
 		return nil
 	}


Test patch:
diff --git a/internal/rtpbuffer/rtpbuffer_test.go b/internal/rtpbuffer/rtpbuffer_test.go
index f1cac81..aa59aec 100644
--- a/internal/rtpbuffer/rtpbuffer_test.go
+++ b/internal/rtpbuffer/rtpbuffer_test.go
@@ -124,6 +124,10 @@ func TestRTPBuffer_WithRTX(t *testing.T) {
 		assertGet(10)
 		assertNOTGet(1, 2, 9)
 
+		// A late packet coming in (such as due to RTX) shouldn't invalidate other packets.
+		add(9)
+		assertGet(3, 4, 5, 6, 7, 8, 9, 10)
+
 		add(22)
 		assertGet(22)
 		assertNOTGet(3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21)