Repository: pion/interceptor
Pull request #339 (2025-06-10 06:20:31)

Handle Header.PaddingSize in PacketFactoryCopy
PacketFactoryCopy.NewPacket does not handle case when padding size is set in Header.PaddingSize and no padding is in payload. It would try to remove it and either it would create broken packet or return error. It should not do this when PaddingSize field is set.

This is another part of fix for https://github.com/pion/webrtc/issues/2403


Patch:
diff --git a/internal/rtpbuffer/packet_factory.go b/internal/rtpbuffer/packet_factory.go
index c0e3468..a9a8d36 100644
--- a/internal/rtpbuffer/packet_factory.go
+++ b/internal/rtpbuffer/packet_factory.go
@@ -102,15 +102,19 @@ func (m *PacketFactoryCopy) NewPacket(
 		// Rewrite the sequence number.
 		retainablePacket.header.SequenceNumber = m.rtxSequencer.NextSequenceNumber()
 		// Remove padding if present.
-		if retainablePacket.header.Padding && retainablePacket.payload != nil && len(retainablePacket.payload) > 0 {
-			paddingLength := int(retainablePacket.payload[len(retainablePacket.payload)-1])
-			retainablePacket.header.Padding = false
-
-			if paddingLength > len(retainablePacket.payload) {
-				return nil, errPaddingOverflow
+		if retainablePacket.header.Padding {
+			// Older versions of pion/rtp didn't have the Header.PaddingSize field and as a workaround
+			// users had to add padding to the payload. We need to handle this case here.
+			if retainablePacket.header.PaddingSize == 0 && len(retainablePacket.payload) > 0 {
+				paddingLength := int(retainablePacket.payload[len(retainablePacket.payload)-1])
+				if paddingLength > len(retainablePacket.payload) {
+					return nil, errPaddingOverflow
+				}
+				retainablePacket.payload = (*retainablePacket.buffer)[:len(retainablePacket.payload)-paddingLength]
 			}
 
-			retainablePacket.payload = (*retainablePacket.buffer)[:len(retainablePacket.payload)-paddingLength]
+			retainablePacket.header.Padding = false
+			retainablePacket.header.PaddingSize = 0
 		}
 	}
 


Test patch:
diff --git a/internal/rtpbuffer/rtpbuffer_test.go b/internal/rtpbuffer/rtpbuffer_test.go
index 18b83ce..f00a07c 100644
--- a/internal/rtpbuffer/rtpbuffer_test.go
+++ b/internal/rtpbuffer/rtpbuffer_test.go
@@ -226,7 +226,7 @@ func TestRTPBuffer_Padding(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, uint16(1), sb.size)
 
-	t.Run("valid padding is stripped", func(t *testing.T) {
+	t.Run("valid padding in payload is stripped", func(t *testing.T) {
 		origPayload := []byte{116, 101, 115, 116}
 		expected := []byte{0, 1, 116, 101, 115, 116}
 
@@ -239,6 +239,7 @@ func TestRTPBuffer_Padding(t *testing.T) {
 		pkt, err := pm.NewPacket(&rtp.Header{
 			SequenceNumber: 1,
 			Padding:        true,
+			PaddingSize:    0,
 		}, padded, 1, 1)
 		require.NoError(t, err)
 
@@ -255,6 +256,30 @@ func TestRTPBuffer_Padding(t *testing.T) {
 		require.Equal(t, expected, actual, "payload content after trimming")
 	})
 
+	t.Run("valid paddingsize in header is cleared", func(t *testing.T) {
+		origPayload := []byte{116, 101, 115, 116}
+		expected := []byte{0, 1, 116, 101, 115, 116}
+
+		pkt, err := pm.NewPacket(&rtp.Header{
+			SequenceNumber: 1,
+			Padding:        true,
+			PaddingSize:    120,
+		}, origPayload, 1, 1)
+		require.NoError(t, err)
+
+		sb.Add(pkt)
+
+		retrieved := sb.Get(1)
+		require.NotNil(t, retrieved)
+		defer retrieved.Release()
+
+		require.False(t, retrieved.Header().Padding, "P-bit should be cleared after trimming")
+
+		actual := retrieved.Payload()
+		require.Equal(t, len(expected), len(actual), "payload length after trimming")
+		require.Equal(t, expected, actual, "payload content after trimming")
+	})
+
 	t.Run("overflow padding returns io.ErrShortBuffer", func(t *testing.T) {
 		overflow := []byte{0, 1, 200}