Repository: pion/interceptor
Pull request #310 (2025-01-31 12:53:03)

NACK responder: bypass auxiliary SSRCs
#### Description

The current implementation of RTX in the NACK interceptor implies that there may be packets with different SSRCs in a single RTP stream (stream in terms of the interceptor interface), e.g., with `SSRCRetransmission` or (more importantly for this PR) `SSRCForwardErrorCorrection`. These packets should not be stored in the buffer or used for responding to NACKs.

I suggest skipping such packets in the responder interceptor. This might be a breaking change for some exotic use cases, but I can't think of any sensible scenario where the NACK interceptor is used to resend packets with a non-main SSRC. Therefore, I believe this is a reasonable default.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/interceptor/pull/310?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 69.38%. Comparing base [(`e187410`)](https://app.codecov.io/gh/pion/interceptor/commit/e1874104865b23ba465ecc505f959daf156cc2a5?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) to head [(`ad324ac`)](https://app.codecov.io/gh/pion/interceptor/commit/ad324ac50909578adb299b732cc5d27bce2ed7c1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> Report is 5 commits behind head on master.

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


```diff
@@            Coverage Diff             @@
##           master     #310      +/-   ##
==========================================
- Coverage   71.31%   69.38%   -1.94%     
==========================================
  Files          80       79       -1     
  Lines        4483     4530      +47     
==========================================
- Hits         3197     3143      -54     
- Misses       1153     1238      +85     
- Partials      133      149      +16     
```

| [Flag](https://app.codecov.io/gh/pion/interceptor/pull/310/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/310/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `?` | |
| [wasm](https://app.codecov.io/gh/pion/interceptor/pull/310/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | `69.38% <100.00%> (-0.02%)` | :arrow_down: |

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/310?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).

aalekseevx: Thank you @nils-ohlmeier :)

Patch:
diff --git a/pkg/nack/responder_interceptor.go b/pkg/nack/responder_interceptor.go
index 58e3430..8c21b07 100644
--- a/pkg/nack/responder_interceptor.go
+++ b/pkg/nack/responder_interceptor.go
@@ -113,6 +113,11 @@ func (n *ResponderInterceptor) BindLocalStream(info *interceptor.StreamInfo, wri
 	n.streamsMu.Unlock()
 
 	return interceptor.RTPWriterFunc(func(header *rtp.Header, payload []byte, attributes interceptor.Attributes) (int, error) {
+		// If this packet doesn't belong to the main SSRC, do not add it to rtpBuffer
+		if header.SSRC != info.SSRC {
+			return writer.Write(header, payload, attributes)
+		}
+
 		pkt, err := n.packetFactory.NewPacket(header, payload, info.SSRCRetransmission, info.PayloadTypeRetransmission)
 		if err != nil {
 			return 0, err


Test patch:
diff --git a/pkg/nack/responder_interceptor_test.go b/pkg/nack/responder_interceptor_test.go
index 019d85e..c48d4fe 100644
--- a/pkg/nack/responder_interceptor_test.go
+++ b/pkg/nack/responder_interceptor_test.go
@@ -57,7 +57,7 @@ func TestResponderInterceptor(t *testing.T) {
 			}()
 
 			for _, seqNum := range []uint16{10, 11, 12, 14, 15} {
-				require.NoError(t, stream.WriteRTP(&rtp.Packet{Header: rtp.Header{SequenceNumber: seqNum}}))
+				require.NoError(t, stream.WriteRTP(&rtp.Packet{Header: rtp.Header{SequenceNumber: seqNum, SSRC: 1}}))
 
 				select {
 				case p := <-stream.WrittenRTP():
@@ -252,7 +252,7 @@ func TestResponderInterceptor_RFC4588(t *testing.T) {
 	}()
 
 	for _, seqNum := range []uint16{10, 11, 12, 14, 15} {
-		require.NoError(t, stream.WriteRTP(&rtp.Packet{Header: rtp.Header{SequenceNumber: seqNum}}))
+		require.NoError(t, stream.WriteRTP(&rtp.Packet{Header: rtp.Header{SequenceNumber: seqNum, SSRC: 1}}))
 
 		select {
 		case p := <-stream.WrittenRTP():
@@ -272,7 +272,7 @@ func TestResponderInterceptor_RFC4588(t *testing.T) {
 		},
 	})
 
-	// seq number 13 was never sent, so it can't be resent
+	// seq number 13 was never sent, so it can't be present
 	for _, seqNum := range []uint16{11, 12, 15} {
 		select {
 		case p := <-stream.WrittenRTP():
@@ -290,3 +290,75 @@ func TestResponderInterceptor_RFC4588(t *testing.T) {
 	case <-time.After(10 * time.Millisecond):
 	}
 }
+
+func TestResponderInterceptor_BypassUnknownSSRCs(t *testing.T) {
+	f, err := NewResponderInterceptor(
+		ResponderSize(8),
+		ResponderLog(logging.NewDefaultLoggerFactory().NewLogger("test")),
+	)
+	require.NoError(t, err)
+
+	i, err := f.NewInterceptor("")
+	require.NoError(t, err)
+
+	stream := test.NewMockStream(&interceptor.StreamInfo{
+		SSRC:         1,
+		RTCPFeedback: []interceptor.RTCPFeedback{{Type: "nack"}},
+	}, i)
+	defer func() {
+		require.NoError(t, stream.Close())
+	}()
+
+	// Send some packets with both SSRCs to check that only SSRC=1 added to the buffer
+	for _, seqNum := range []uint16{10, 11, 12, 14, 15} {
+		require.NoError(t, stream.WriteRTP(&rtp.Packet{Header: rtp.Header{SequenceNumber: seqNum, SSRC: 1}}))
+		// This packet should be bypassed and not added to the buffer.
+		require.NoError(t, stream.WriteRTP(&rtp.Packet{Header: rtp.Header{SequenceNumber: seqNum, SSRC: 2}}))
+
+		select {
+		case p := <-stream.WrittenRTP():
+			require.Equal(t, seqNum, p.SequenceNumber)
+			require.Equal(t, uint32(1), p.SSRC)
+		case <-time.After(10 * time.Millisecond):
+			t.Fatal("written rtp packet not found")
+		}
+
+		select {
+		case p := <-stream.WrittenRTP():
+			require.Equal(t, seqNum, p.SequenceNumber)
+			require.Equal(t, uint32(2), p.SSRC)
+		case <-time.After(10 * time.Millisecond):
+			t.Fatal("written rtp packet not found")
+		}
+	}
+
+	// This packet should be bypassed and not added to the buffer.
+	require.NoError(t, stream.WriteRTP(&rtp.Packet{Header: rtp.Header{SequenceNumber: 13, SSRC: 2}}))
+	select {
+	case p := <-stream.WrittenRTP():
+		require.Equal(t, uint16(13), p.SequenceNumber)
+	case <-time.After(10 * time.Millisecond):
+		t.Fatal("written rtp packet not found")
+	}
+
+	stream.ReceiveRTCP([]rtcp.Packet{
+		&rtcp.TransportLayerNack{
+			MediaSSRC:  1,
+			SenderSSRC: 1,
+			Nacks: []rtcp.NackPair{
+				{PacketID: 11, LostPackets: 0b1011}, // sequence numbers: 11, 12, 13, 15
+			},
+		},
+	})
+
+	// seq number 13 was sent with different ssrc, it should not be present
+	for _, seqNum := range []uint16{11, 12, 15} {
+		select {
+		case p := <-stream.WrittenRTP():
+			require.Equal(t, uint32(1), p.SSRC)
+			require.Equal(t, seqNum, p.SequenceNumber)
+		case <-time.After(10 * time.Millisecond):
+			t.Fatal("written rtp packet not found")
+		}
+	}
+}