Skip to content

Commit a8b82f7

Browse files
sirzoorosirzooro
committed
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.
1 parent ea0d786 commit a8b82f7

3 files changed

Lines changed: 146 additions & 24 deletions

File tree

‎packet.go‎

Lines changed: 54 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,18 +29,27 @@ type Header struct {
2929
ExtensionProfile uint16
3030
Extensions []Extension
3131

32+
// PaddingLength is the length of the padding in bytes. It is not part of the RTP header
33+
// (it is sent in the last byte of RTP packet padding), but logically it belongs here.
34+
PaddingSize byte
35+
3236
// Deprecated: will be removed in a future version.
3337
PayloadOffset int
3438
}
3539

3640
// Packet represents an RTP Packet.
3741
type Packet struct {
3842
Header
39-
Payload []byte
40-
PaddingSize byte
43+
Payload []byte
44+
45+
PaddingSize byte // Deprecated: will be removed in a future version. Use Header.PaddingSize instead.
4146

4247
// Deprecated: will be removed in a future version.
4348
Raw []byte
49+
50+
// Please do not add any new field directly to Packet struct unless you know that it is safe.
51+
// pion internally passes Header and Payload separately, what causes bugs like
52+
// https://github.com/pion/webrtc/issues/2403 .
4453
}
4554

4655
const (
@@ -219,11 +228,12 @@ func (p *Packet) Unmarshal(buf []byte) error {
219228
if end <= n {
220229
return errTooSmall
221230
}
222-
p.PaddingSize = buf[end-1]
223-
end -= int(p.PaddingSize)
231+
p.Header.PaddingSize = buf[end-1]
232+
end -= int(p.Header.PaddingSize)
224233
} else {
225-
p.PaddingSize = 0
234+
p.Header.PaddingSize = 0
226235
}
236+
p.PaddingSize = p.Header.PaddingSize
227237
if end < n {
228238
return errTooSmall
229239
}
@@ -490,7 +500,7 @@ func (p Packet) Marshal() (buf []byte, err error) {
490500

491501
// MarshalTo serializes the packet and writes to the buffer.
492502
func (p *Packet) MarshalTo(buf []byte) (n int, err error) {
493-
if p.Header.Padding && p.PaddingSize == 0 {
503+
if p.Header.Padding && p.paddingSize() == 0 {
494504
return 0, errInvalidRTPPadding
495505
}
496506

@@ -499,23 +509,28 @@ func (p *Packet) MarshalTo(buf []byte) (n int, err error) {
499509
return 0, err
500510
}
501511

512+
return marshalPayloadAndPaddingTo(buf, n, &p.Header, p.Payload, p.paddingSize())
513+
}
514+
515+
func marshalPayloadAndPaddingTo(buf []byte, offset int, header *Header, payload []byte, paddingSize byte,
516+
) (n int, err error) {
502517
// Make sure the buffer is large enough to hold the packet.
503-
if n+len(p.Payload)+int(p.PaddingSize) > len(buf) {
518+
if offset+len(payload)+int(paddingSize) > len(buf) {
504519
return 0, io.ErrShortBuffer
505520
}
506521

507-
m := copy(buf[n:], p.Payload)
522+
m := copy(buf[offset:], payload)
508523

509-
if p.Header.Padding {
510-
buf[n+m+int(p.PaddingSize-1)] = p.PaddingSize
524+
if header.Padding {
525+
buf[offset+m+int(paddingSize-1)] = paddingSize
511526
}
512527

513-
return n + m + int(p.PaddingSize), nil
528+
return offset + m + int(paddingSize), nil
514529
}
515530

516531
// MarshalSize returns the size of the packet once marshaled.
517532
func (p Packet) MarshalSize() int {
518-
return p.Header.MarshalSize() + len(p.Payload) + int(p.PaddingSize)
533+
return p.Header.MarshalSize() + len(p.Payload) + int(p.paddingSize())
519534
}
520535

521536
// Clone returns a deep copy of p.
@@ -552,3 +567,30 @@ func (h Header) Clone() Header {
552567

553568
return clone
554569
}
570+
571+
func (p *Packet) paddingSize() byte {
572+
if p.Header.PaddingSize > 0 {
573+
return p.Header.PaddingSize
574+
}
575+
576+
return p.PaddingSize
577+
}
578+
579+
// MarshalPacketTo serializes the header and payload into bytes.
580+
// Parts of pion code passes RTP header and payload separately, so this function
581+
// is provided to help with that.
582+
func MarshalPacketTo(buf []byte, header *Header, payload []byte) (int, error) {
583+
n, err := header.MarshalTo(buf)
584+
if err != nil {
585+
return 0, err
586+
}
587+
588+
return marshalPayloadAndPaddingTo(buf, n, header, payload, header.PaddingSize)
589+
}
590+
591+
// PacketMarshalSize returns the size of the header and payload once marshaled.
592+
// Parts of pion code passes RTP header and payload separately, so this function
593+
// is provided to help with that.
594+
func PacketMarshalSize(header *Header, payload []byte) int {
595+
return header.MarshalSize() + len(payload) + int(header.PaddingSize)
596+
}

‎packet_test.go‎

Lines changed: 91 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
3737
Timestamp: 3653407706,
3838
SSRC: 476325762,
3939
CSRC: []uint32{},
40+
PaddingSize: 0,
4041
},
41-
Payload: rawPkt[20:],
42-
PaddingSize: 0,
42+
Payload: rawPkt[20:],
4343
}
4444

4545
// Unmarshal to the used Packet should work as well.
@@ -79,6 +79,7 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
7979
Timestamp: 3653407706,
8080
SSRC: 476325762,
8181
CSRC: []uint32{},
82+
PaddingSize: 4,
8283
},
8384
Payload: rawPkt[20:21],
8485
PaddingSize: 4,
@@ -108,9 +109,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
108109
Timestamp: 3653407706,
109110
SSRC: 476325762,
110111
CSRC: []uint32{},
112+
PaddingSize: 0,
111113
},
112-
Payload: rawPkt[20:],
113-
PaddingSize: 0,
114+
Payload: rawPkt[20:],
114115
}
115116
assert.NoError(t, packet.Unmarshal(rawPkt))
116117
assert.Equal(t, packet, parsedPacket)
@@ -137,6 +138,7 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
137138
Timestamp: 3653407706,
138139
SSRC: 476325762,
139140
CSRC: []uint32{},
141+
PaddingSize: 5,
140142
},
141143
Payload: []byte{},
142144
PaddingSize: 5,
@@ -167,9 +169,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
167169
Timestamp: 3653407706,
168170
SSRC: 476325762,
169171
CSRC: []uint32{},
172+
PaddingSize: 0,
170173
},
171-
Payload: []byte{},
172-
PaddingSize: 0,
174+
Payload: []byte{},
173175
}
174176
err := packet.Unmarshal(rawPkt)
175177
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
197199
Timestamp: 3653407706,
198200
SSRC: 476325762,
199201
CSRC: []uint32{},
202+
PaddingSize: 4,
200203
},
201-
Payload: rawPkt[20:21],
202-
PaddingSize: 4,
204+
Payload: rawPkt[20:21],
203205
}
204206
buf, err := parsedPacket.Marshal()
205207
assert.NoError(t, err)
@@ -227,9 +229,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
227229
Timestamp: 3653407706,
228230
SSRC: 476325762,
229231
CSRC: []uint32{},
232+
PaddingSize: 5,
230233
},
231-
Payload: []byte{},
232-
PaddingSize: 5,
234+
Payload: []byte{},
233235
}
234236
buf, err = parsedPacket.Marshal()
235237
assert.NoError(t, err)
@@ -257,9 +259,9 @@ func TestBasic(t *testing.T) { // nolint:maintidx,cyclop
257259
Timestamp: 3653407706,
258260
SSRC: 476325762,
259261
CSRC: []uint32{},
262+
PaddingSize: 5,
260263
},
261-
Payload: []byte{},
262-
PaddingSize: 5,
264+
Payload: []byte{},
263265
}
264266
buf, err = parsedPacket.Marshal()
265267
assert.NoError(t, err)
@@ -1255,6 +1257,83 @@ func TestClonePacket(t *testing.T) {
12551257
assert.NotEqual(t, clone.Payload[0], 0x1F, "Expected payload to be unchanged")
12561258
}
12571259

1260+
func TestMarshalRTPPacketFuncs(t *testing.T) {
1261+
// packet with only padding
1262+
rawPkt := []byte{
1263+
0xb0, 0xe0, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,
1264+
0x27, 0x82, 0x00, 0x01, 0x00, 0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x05,
1265+
}
1266+
parsedPacket := &Packet{
1267+
Header: Header{
1268+
Padding: true,
1269+
Marker: true,
1270+
Extension: true,
1271+
ExtensionProfile: 1,
1272+
Extensions: []Extension{
1273+
{0, []byte{
1274+
0xFF, 0xFF, 0xFF, 0xFF,
1275+
}},
1276+
},
1277+
Version: 2,
1278+
PayloadType: 96,
1279+
SequenceNumber: 27023,
1280+
Timestamp: 3653407706,
1281+
SSRC: 476325762,
1282+
CSRC: []uint32{},
1283+
PaddingSize: 5,
1284+
},
1285+
Payload: []byte{},
1286+
}
1287+
1288+
buf := make([]byte, 100)
1289+
n, err := MarshalPacketTo(buf, &parsedPacket.Header, parsedPacket.Payload)
1290+
assert.NoError(t, err)
1291+
assert.Equal(t, len(rawPkt), n)
1292+
assert.Equal(t, rawPkt, buf[:n])
1293+
assert.Equal(t, n, PacketMarshalSize(&parsedPacket.Header, parsedPacket.Payload))
1294+
}
1295+
1296+
func TestDeprecatedPaddingSizeField(t *testing.T) {
1297+
// packet with only padding
1298+
rawPkt := []byte{
1299+
0xb0, 0xe0, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,
1300+
0x27, 0x82, 0x00, 0x01, 0x00, 0x01, 0xFF, 0xFF, 0xFF, 0xFF, 0x00, 0x00, 0x00, 0x00, 0x05,
1301+
}
1302+
parsedPacket := &Packet{
1303+
Header: Header{
1304+
Padding: true,
1305+
Marker: true,
1306+
Extension: true,
1307+
ExtensionProfile: 1,
1308+
Extensions: []Extension{
1309+
{0, []byte{
1310+
0xFF, 0xFF, 0xFF, 0xFF,
1311+
}},
1312+
},
1313+
Version: 2,
1314+
PayloadType: 96,
1315+
SequenceNumber: 27023,
1316+
Timestamp: 3653407706,
1317+
SSRC: 476325762,
1318+
CSRC: []uint32{},
1319+
},
1320+
Payload: []byte{},
1321+
PaddingSize: 5,
1322+
}
1323+
1324+
buf, err := parsedPacket.Marshal()
1325+
assert.NoError(t, err)
1326+
assert.Equal(t, rawPkt, buf)
1327+
assert.EqualValues(t, 0, parsedPacket.Header.PaddingSize)
1328+
1329+
assert.Equal(t, len(rawPkt), parsedPacket.MarshalSize())
1330+
assert.EqualValues(t, 0, parsedPacket.Header.PaddingSize)
1331+
1332+
parsedPacket2 := parsedPacket.Clone()
1333+
assert.EqualValues(t, 5, parsedPacket2.PaddingSize)
1334+
assert.EqualValues(t, 0, parsedPacket2.Header.PaddingSize)
1335+
}
1336+
12581337
func BenchmarkMarshal(b *testing.B) {
12591338
rawPkt := []byte{
12601339
0x90, 0x60, 0x69, 0x8f, 0xd9, 0xc2, 0x93, 0xda, 0x1c, 0x64,

‎packetizer_test.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,7 @@ func TestPacketizer_Roundtrip(t *testing.T) {
107107
assert.Equal(t, expectedPkt.Payload, pkt.Payload)
108108

109109
pkt.PaddingSize = 0
110+
pkt.Header.PaddingSize = 0
110111
assert.Equal(t, expectedPkt, pkt)
111112
}
112113
}

0 commit comments

Comments
 (0)