Moved PaddingSize field from Packet to Header

Parts of pion code passes RTP header and payload via separate parameters
instead of using Packet struct to pass them together. As a result value
of padding size field may be lost. This change adds PaddingSize field to
the Header struct and marks one in Packet as a deprecated.

This is part of fix for pion/webrtc#2403 .
Another change is needed for pion/srtp to resolve it fully.

Author: sirzooro

packet.go

@@ -29,18 +29,27 @@ type Header struct {
 	ExtensionProfile uint16
 	Extensions       []Extension
 
+	// PaddingLength is the length of the padding in bytes. It is not part of the RTP header
+	// (it is sent in the last byte of RTP packet padding), but logically it belongs here.
+	PaddingSize byte
+
 	// Deprecated: will be removed in a future version.
 	PayloadOffset int
 }
 
 // Packet represents an RTP Packet.
 type Packet struct {
 	Header
-	Payload     []byte
-	PaddingSize byte
+	Payload []byte
+
+	PaddingSize byte // Deprecated: will be removed in a future version. Use Header.PaddingSize instead.
 
 	// Deprecated: will be removed in a future version.
 	Raw []byte
+
+	// Please do not add any new field directly to Packet struct unless you know that it is safe.
+	// pion internally passes Header and Payload separately, what causes bugs like
+	// https://github.com/pion/webrtc/issues/2403 .
 }
 
 const (
@@ -219,11 +228,12 @@ func (p *Packet) Unmarshal(buf []byte) error {
 		if end <= n {
 			return errTooSmall
 		}
-		p.PaddingSize = buf[end-1]
-		end -= int(p.PaddingSize)
+		p.Header.PaddingSize = buf[end-1]
+		end -= int(p.Header.PaddingSize)
 	} else {
-		p.PaddingSize = 0
+		p.Header.PaddingSize = 0
 	}
+	p.PaddingSize = p.Header.PaddingSize
 	if end < n {
 		return errTooSmall
 	}
@@ -490,7 +500,7 @@ func (p Packet) Marshal() (buf []byte, err error) {
 
 // MarshalTo serializes the packet and writes to the buffer.
 func (p *Packet) MarshalTo(buf []byte) (n int, err error) {
-	if p.Header.Padding && p.PaddingSize == 0 {
+	if p.Header.Padding && p.paddingSize() == 0 {
 		return 0, errInvalidRTPPadding
 	}
 
@@ -499,23 +509,28 @@ func (p *Packet) MarshalTo(buf []byte) (n int, err error) {
 		return 0, err
 	}
 
+	return marshalPayloadAndPaddingTo(buf, n, &p.Header, p.Payload, p.paddingSize())
+}
+
+func marshalPayloadAndPaddingTo(buf []byte, offset int, header *Header, payload []byte, paddingSize byte,
+) (n int, err error) {
 	// Make sure the buffer is large enough to hold the packet.
-	if n+len(p.Payload)+int(p.PaddingSize) > len(buf) {
+	if offset+len(payload)+int(paddingSize) > len(buf) {
 		return 0, io.ErrShortBuffer
 	}
 
-	m := copy(buf[n:], p.Payload)
+	m := copy(buf[offset:], payload)
 
-	if p.Header.Padding {
-		buf[n+m+int(p.PaddingSize-1)] = p.PaddingSize
+	if header.Padding {
+		buf[offset+m+int(paddingSize-1)] = paddingSize
 	}
 
-	return n + m + int(p.PaddingSize), nil
+	return offset + m + int(paddingSize), nil
 }
 
 // MarshalSize returns the size of the packet once marshaled.
 func (p Packet) MarshalSize() int {
-	return p.Header.MarshalSize() + len(p.Payload) + int(p.PaddingSize)
+	return p.Header.MarshalSize() + len(p.Payload) + int(p.paddingSize())
 }
 
 // Clone returns a deep copy of p.
@@ -552,3 +567,30 @@ func (h Header) Clone() Header {
 
 	return clone
 }
+
+func (p *Packet) paddingSize() byte {
+	if p.Header.PaddingSize > 0 {
+		return p.Header.PaddingSize
+	}
+
+	return p.PaddingSize
+}
+
+// MarshalPacketTo serializes the header and payload into bytes.
+// Parts of pion code passes RTP header and payload separately, so this function
+// is provided to help with that.
+func MarshalPacketTo(buf []byte, header *Header, payload []byte) (int, error) {
+	n, err := header.MarshalTo(buf)
+	if err != nil {
+		return 0, err
+	}
+
+	return marshalPayloadAndPaddingTo(buf, n, header, payload, header.PaddingSize)
+}
+
+// PacketMarshalSize returns the size of the header and payload once marshaled.
+// Parts of pion code passes RTP header and payload separately, so this function
+// is provided to help with that.
+func PacketMarshalSize(header *Header, payload []byte) int {
+	return header.MarshalSize() + len(payload) + int(header.PaddingSize)
+}

packet_test.go

@@ -37,9 +37,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
 			Timestamp:      3653407706,
 			SSRC:           476325762,
 			CSRC:           []uint32{},
+			PaddingSize:    0,
 		},
-		Payload:     rawPkt[20:],
-		PaddingSize: 0,
+		Payload: rawPkt[20:],
 	}
 
 	// Unmarshal to the used Packet should work as well.
@@ -79,6 +79,7 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
 			Timestamp:      3653407706,
 			SSRC:           476325762,
 			CSRC:           []uint32{},
+			PaddingSize:    4,
 		},
 		Payload:     rawPkt[20:21],
 		PaddingSize: 4,
@@ -108,9 +109,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
 			Timestamp:      3653407706,
 			SSRC:           476325762,
 			CSRC:           []uint32{},
+			PaddingSize:    0,
 		},
-		Payload:     rawPkt[20:],
-		PaddingSize: 0,
+		Payload: rawPkt[20:],
 	}
 	assert.NoError(t, packet.Unmarshal(rawPkt))
 	assert.Equal(t, packet, parsedPacket)
@@ -137,6 +138,7 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
 			Timestamp:      3653407706,
 			SSRC:           476325762,
 			CSRC:           []uint32{},
+			PaddingSize:    5,
 		},
 		Payload:     []byte{},
 		PaddingSize: 5,
@@ -167,9 +169,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
 			Timestamp:      3653407706,
 			SSRC:           476325762,
 			CSRC:           []uint32{},
+			PaddingSize:    0,
 		},
-		Payload:     []byte{},
-		PaddingSize: 0,
+		Payload: []byte{},
 	}
 	err := packet.Unmarshal(rawPkt)
 	assert.Error(t, err, "Unmarshal did not error on packet with excessive padding")
@@ -197,9 +199,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
 			Timestamp:      3653407706,
 			SSRC:           476325762,
 			CSRC:           []uint32{},
+			PaddingSize:    4,
 		},
-		Payload:     rawPkt[20:21],
-		PaddingSize: 4,
+		Payload: rawPkt[20:21],
 	}
 	buf, err := parsedPacket.Marshal()
 	assert.NoError(t, err)
@@ -227,9 +229,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
 			Timestamp:      3653407706,
 			SSRC:           476325762,
 			CSRC:           []uint32{},
+			PaddingSize:    5,
 		},
-		Payload:     []byte{},
-		PaddingSize: 5,
+		Payload: []byte{},
 	}
 	buf, err = parsedPacket.Marshal()
 	assert.NoError(t, err)
@@ -257,9 +259,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
 			Timestamp:      3653407706,
 			SSRC:           476325762,
 			CSRC:           []uint32{},
+			PaddingSize:    5,
 		},
-		Payload:     []byte{},
-		PaddingSize: 5,
+		Payload: []byte{},
 	}
 	buf, err = parsedPacket.Marshal()
 	assert.NoError(t, err)
@@ -1255,6 +1257,83 @@ func TestClonePacket(t *testing.T) {
 	assert.NotEqual(t, clone.Payload[0], 0x1F, "Expected payload to be unchanged")
 }
 
+func TestMarshalRTPPacketFuncs(t *testing.T) {
+	// packet with only padding
+	rawPkt := []byte{
+		0xb0, 0xe0, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,
+		0x27, 0x82, 0x00, 0x01, 0x00, 0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x05,
+	}
+	parsedPacket := &Packet{
+		Header: Header{
+			Padding:          true,
+			Marker:           true,
+			Extension:        true,
+			ExtensionProfile: 1,
+			Extensions: []Extension{
+				{0, []byte{
+					0xFF, 0xFF, 0xFF, 0xFF,
+				}},
+			},
+			Version:        2,
+			PayloadType:    96,
+			SequenceNumber: 27023,
+			Timestamp:      3653407706,
+			SSRC:           476325762,
+			CSRC:           []uint32{},
+			PaddingSize:    5,
+		},
+		Payload: []byte{},
+	}
+
+	buf := make([]byte, 100)
+	n, err := MarshalPacketTo(buf, &parsedPacket.Header, parsedPacket.Payload)
+	assert.NoError(t, err)
+	assert.Equal(t, len(rawPkt), n)
+	assert.Equal(t, rawPkt, buf[:n])
+	assert.Equal(t, n, PacketMarshalSize(&parsedPacket.Header, parsedPacket.Payload))
+}
+
+func TestDeprecatedPaddingSizeField(t *testing.T) {
+	// packet with only padding
+	rawPkt := []byte{
+		0xb0, 0xe0, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,
+		0x27, 0x82, 0x00, 0x01, 0x00, 0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x05,
+	}
+	parsedPacket := &Packet{
+		Header: Header{
+			Padding:          true,
+			Marker:           true,
+			Extension:        true,
+			ExtensionProfile: 1,
+			Extensions: []Extension{
+				{0, []byte{
+					0xFF, 0xFF, 0xFF, 0xFF,
+				}},
+			},
+			Version:        2,
+			PayloadType:    96,
+			SequenceNumber: 27023,
+			Timestamp:      3653407706,
+			SSRC:           476325762,
+			CSRC:           []uint32{},
+		},
+		Payload:     []byte{},
+		PaddingSize: 5,
+	}
+
+	buf, err := parsedPacket.Marshal()
+	assert.NoError(t, err)
+	assert.Equal(t, rawPkt, buf)
+	assert.EqualValues(t, 0, parsedPacket.Header.PaddingSize)
+
+	assert.Equal(t, len(rawPkt), parsedPacket.MarshalSize())
+	assert.EqualValues(t, 0, parsedPacket.Header.PaddingSize)
+
+	parsedPacket2 := parsedPacket.Clone()
+	assert.EqualValues(t, 5, parsedPacket2.PaddingSize)
+	assert.EqualValues(t, 0, parsedPacket2.Header.PaddingSize)
+}
+
 func BenchmarkMarshal(b *testing.B) {
 	rawPkt := []byte{
 		0x90, 0x60, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,

packetizer_test.go

@@ -107,6 +107,7 @@ func TestPacketizer_Roundtrip(t *testing.T) {
 		assert.Equal(t, expectedPkt.Payload, pkt.Payload)
 
 		pkt.PaddingSize = 0
+		pkt.Header.PaddingSize = 0
 		assert.Equal(t, expectedPkt, pkt)
 	}
 }