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.

Author: sirzooro

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
 		}
 	}
 

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}