From 57510d7a84f36391fae1fa44a418661e1751e571 Mon Sep 17 00:00:00 2001 From: ffmiyo Date: Mon, 8 Feb 2021 22:46:50 +0800 Subject: [PATCH 1/2] Refactor Packet struct to match RFC format Remove PayloadOffset from Header. Don't keep unnecessary full data bytes inside Packet. Packet Marshal and Unmarshal method API stay the same. Fixes #90 --- README.md | 1 + packet.go | 130 +++++++++++++++++++++------------------------ packet_test.go | 43 +-------------- packetizer_test.go | 1 - 4 files changed, 63 insertions(+), 112 deletions(-) diff --git a/README.md b/README.md index 22b4ece..0dffc17 100644 --- a/README.md +++ b/README.md @@ -51,6 +51,7 @@ Check out the **[contributing wiki](https://github.com/pion/webrtc/wiki/Contribu * [Robin Raymond](https://github.com/robin-raymond) * [debiandebiandebian](https://github.com/debiandebiandebian) * [Juliusz Chroboczek](https://github.com/jech) +* [ffmiyo](https://github.com/ffmiyo) ### License MIT License - see [LICENSE](LICENSE) for full text diff --git a/packet.go b/packet.go index b237b0a..b94a2f0 100644 --- a/packet.go +++ b/packet.go @@ -13,13 +13,11 @@ type Extension struct { } // Header represents an RTP packet header -// NOTE: PayloadOffset is populated by Marshal/Unmarshal and should not be modified type Header struct { Version uint8 Padding bool Extension bool Marker bool - PayloadOffset int PayloadType uint8 SequenceNumber uint16 Timestamp uint32 @@ -30,10 +28,8 @@ type Header struct { } // Packet represents an RTP Packet -// NOTE: Raw is populated by Marshal/Unmarshal and should not be modified type Packet struct { Header - Raw []byte Payload []byte } @@ -77,10 +73,11 @@ func (p Packet) String() string { return out } -// Unmarshal parses the passed byte slice and stores the result in the Header this method is called upon -func (h *Header) Unmarshal(rawPacket []byte) error { //nolint:gocognit - if len(rawPacket) < headerLength { - return fmt.Errorf("%w: %d < %d", errHeaderSizeInsufficient, len(rawPacket), headerLength) +// Unmarshal parses the passed byte slice and stores the result in the Header. +// It returns the number of bytes read n and any error. +func (h *Header) Unmarshal(buf []byte) (n int, err error) { //nolint:gocognit + if len(buf) < headerLength { + return 0, fmt.Errorf("%w: %d < %d", errHeaderSizeInsufficient, len(buf), headerLength) } /* @@ -98,31 +95,32 @@ func (h *Header) Unmarshal(rawPacket []byte) error { //nolint:gocognit * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ */ - h.Version = rawPacket[0] >> versionShift & versionMask - h.Padding = (rawPacket[0] >> paddingShift & paddingMask) > 0 - h.Extension = (rawPacket[0] >> extensionShift & extensionMask) > 0 - nCSRC := int(rawPacket[0] & ccMask) + h.Version = buf[0] >> versionShift & versionMask + h.Padding = (buf[0] >> paddingShift & paddingMask) > 0 + h.Extension = (buf[0] >> extensionShift & extensionMask) > 0 + nCSRC := int(buf[0] & ccMask) if cap(h.CSRC) < nCSRC || h.CSRC == nil { h.CSRC = make([]uint32, nCSRC) } else { h.CSRC = h.CSRC[:nCSRC] } - currOffset := csrcOffset + (nCSRC * csrcLength) - if len(rawPacket) < currOffset { - return fmt.Errorf("size %d < %d: %w", len(rawPacket), currOffset, errHeaderSizeInsufficient) + n = csrcOffset + (nCSRC * csrcLength) + if len(buf) < n { + return n, fmt.Errorf("size %d < %d: %w", len(buf), n, + errHeaderSizeInsufficient) } - h.Marker = (rawPacket[1] >> markerShift & markerMask) > 0 - h.PayloadType = rawPacket[1] & ptMask + h.Marker = (buf[1] >> markerShift & markerMask) > 0 + h.PayloadType = buf[1] & ptMask - h.SequenceNumber = binary.BigEndian.Uint16(rawPacket[seqNumOffset : seqNumOffset+seqNumLength]) - h.Timestamp = binary.BigEndian.Uint32(rawPacket[timestampOffset : timestampOffset+timestampLength]) - h.SSRC = binary.BigEndian.Uint32(rawPacket[ssrcOffset : ssrcOffset+ssrcLength]) + h.SequenceNumber = binary.BigEndian.Uint16(buf[seqNumOffset : seqNumOffset+seqNumLength]) + h.Timestamp = binary.BigEndian.Uint32(buf[timestampOffset : timestampOffset+timestampLength]) + h.SSRC = binary.BigEndian.Uint32(buf[ssrcOffset : ssrcOffset+ssrcLength]) for i := range h.CSRC { offset := csrcOffset + (i * csrcLength) - h.CSRC[i] = binary.BigEndian.Uint32(rawPacket[offset:]) + h.CSRC[i] = binary.BigEndian.Uint32(buf[offset:]) } if h.Extensions != nil { @@ -130,21 +128,21 @@ func (h *Header) Unmarshal(rawPacket []byte) error { //nolint:gocognit } if h.Extension { - if expected := currOffset + 4; len(rawPacket) < expected { - return fmt.Errorf("size %d < %d: %w", - len(rawPacket), expected, + if expected := n + 4; len(buf) < expected { + return n, fmt.Errorf("size %d < %d: %w", + len(buf), expected, errHeaderSizeInsufficientForExtension, ) } - h.ExtensionProfile = binary.BigEndian.Uint16(rawPacket[currOffset:]) - currOffset += 2 - extensionLength := int(binary.BigEndian.Uint16(rawPacket[currOffset:])) * 4 - currOffset += 2 + h.ExtensionProfile = binary.BigEndian.Uint16(buf[n:]) + n += 2 + extensionLength := int(binary.BigEndian.Uint16(buf[n:])) * 4 + n += 2 - if expected := currOffset + extensionLength; len(rawPacket) < expected { - return fmt.Errorf("size %d < %d: %w", - len(rawPacket), expected, + if expected := n + extensionLength; len(buf) < expected { + return n, fmt.Errorf("size %d < %d: %w", + len(buf), expected, errHeaderSizeInsufficientForExtension, ) } @@ -152,70 +150,67 @@ func (h *Header) Unmarshal(rawPacket []byte) error { //nolint:gocognit switch h.ExtensionProfile { // RFC 8285 RTP One Byte Header Extension case extensionProfileOneByte: - end := currOffset + extensionLength - for currOffset < end { - if rawPacket[currOffset] == 0x00 { // padding - currOffset++ + end := n + extensionLength + for n < end { + if buf[n] == 0x00 { // padding + n++ continue } - extid := rawPacket[currOffset] >> 4 - len := int(rawPacket[currOffset]&^0xF0 + 1) - currOffset++ + extid := buf[n] >> 4 + len := int(buf[n]&^0xF0 + 1) + n++ if extid == extensionIDReserved { break } - extension := Extension{id: extid, payload: rawPacket[currOffset : currOffset+len]} + extension := Extension{id: extid, payload: buf[n : n+len]} h.Extensions = append(h.Extensions, extension) - currOffset += len + n += len } // RFC 8285 RTP Two Byte Header Extension case extensionProfileTwoByte: - end := currOffset + extensionLength - for currOffset < end { - if rawPacket[currOffset] == 0x00 { // padding - currOffset++ + end := n + extensionLength + for n < end { + if buf[n] == 0x00 { // padding + n++ continue } - extid := rawPacket[currOffset] - currOffset++ + extid := buf[n] + n++ - len := int(rawPacket[currOffset]) - currOffset++ + len := int(buf[n]) + n++ - extension := Extension{id: extid, payload: rawPacket[currOffset : currOffset+len]} + extension := Extension{id: extid, payload: buf[n : n+len]} h.Extensions = append(h.Extensions, extension) - currOffset += len + n += len } default: // RFC3550 Extension - if len(rawPacket) < currOffset+extensionLength { - return fmt.Errorf("%w: %d < %d", errHeaderSizeInsufficientForExtension, len(rawPacket), currOffset+extensionLength) + if len(buf) < n+extensionLength { + return n, fmt.Errorf("%w: %d < %d", + errHeaderSizeInsufficientForExtension, len(buf), n+extensionLength) } - extension := Extension{id: 0, payload: rawPacket[currOffset : currOffset+extensionLength]} + extension := Extension{id: 0, payload: buf[n : n+extensionLength]} h.Extensions = append(h.Extensions, extension) - currOffset += len(h.Extensions[0].payload) + n += len(h.Extensions[0].payload) } } - - h.PayloadOffset = currOffset - - return nil + return n, nil } -// Unmarshal parses the passed byte slice and stores the result in the Packet this method is called upon -func (p *Packet) Unmarshal(rawPacket []byte) error { - if err := p.Header.Unmarshal(rawPacket); err != nil { +// Unmarshal parses the passed byte slice and stores the result in the Packet. +func (p *Packet) Unmarshal(buf []byte) error { + n, err := p.Header.Unmarshal(buf) + if err != nil { return err } - - p.Payload = rawPacket[p.PayloadOffset:] - p.Raw = rawPacket + p.Payload = buf[n:] return nil } @@ -227,7 +222,6 @@ func (h *Header) Marshal() (buf []byte, err error) { if err != nil { return nil, err } - return buf[:n], nil } @@ -253,7 +247,8 @@ func (h *Header) MarshalTo(buf []byte) (n int, err error) { return 0, io.ErrShortBuffer } - // The first byte contains the version, padding bit, extension bit, and csrc size + // The first byte contains the version, padding bit, extension bit, + // and csrc size. buf[0] = (h.Version << versionShift) | uint8(len(h.CSRC)) if h.Padding { buf[0] |= 1 << paddingShift @@ -324,8 +319,6 @@ func (h *Header) MarshalTo(buf []byte) (n int, err error) { } } - h.PayloadOffset = n - return n, nil } @@ -479,7 +472,6 @@ func (p *Packet) MarshalTo(buf []byte) (n int, err error) { } m := copy(buf[n:], p.Payload) - p.Raw = buf[:n+m] return n + m, nil } diff --git a/packet_test.go b/packet_test.go index a1cfa8e..76dff9f 100644 --- a/packet_test.go +++ b/packet_test.go @@ -31,7 +31,6 @@ func TestBasic(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 20, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -39,7 +38,6 @@ func TestBasic(t *testing.T) { CSRC: []uint32{}, }, Payload: rawPkt[20:], - Raw: rawPkt, } // Unmarshal to the used Packet should work as well. @@ -57,20 +55,12 @@ func TestBasic(t *testing.T) { t.Errorf("wrong computed marshal size") } - if p.PayloadOffset != 20 { - t.Errorf("wrong payload offset: %d != %d", p.PayloadOffset, 20) - } - raw, err := p.Marshal() if err != nil { t.Error(err) } else if !reflect.DeepEqual(raw, rawPkt) { t.Errorf("TestBasic marshal: got %#v, want %#v", raw, rawPkt) } - - if p.PayloadOffset != 20 { - t.Errorf("wrong payload offset: %d != %d", p.PayloadOffset, 20) - } }) } } @@ -134,7 +124,6 @@ func TestRFC8285OneByteExtension(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 18, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -142,7 +131,6 @@ func TestRFC8285OneByteExtension(t *testing.T) { CSRC: []uint32{}, }, Payload: rawPkt[20:], - Raw: rawPkt, } dstData, _ := p.Marshal() @@ -198,7 +186,6 @@ func TestRFC8285OneByteTwoExtensionOfTwoBytes(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -206,7 +193,6 @@ func TestRFC8285OneByteTwoExtensionOfTwoBytes(t *testing.T) { CSRC: []uint32{}, }, Payload: rawPkt[20:], - Raw: rawPkt, } dstData, _ := p.Marshal() @@ -323,7 +309,6 @@ func TestRFC8285OneByteMultipleExtensions(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -331,7 +316,6 @@ func TestRFC8285OneByteMultipleExtensions(t *testing.T) { CSRC: []uint32{}, }, Payload: rawPkt[28:], - Raw: rawPkt, } dstData, _ := p.Marshal() @@ -367,7 +351,6 @@ func TestRFC8285TwoByteExtension(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 42, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -375,7 +358,6 @@ func TestRFC8285TwoByteExtension(t *testing.T) { CSRC: []uint32{}, }, Payload: rawPkt[44:], - Raw: rawPkt, } dstData, _ := p.Marshal() @@ -470,7 +452,6 @@ func TestRFC8285TwoByteMultipleExtensionsWithLargeExtension(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 40, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -478,7 +459,6 @@ func TestRFC8285TwoByteMultipleExtensionsWithLargeExtension(t *testing.T) { CSRC: []uint32{}, }, Payload: rawPkt[40:], - Raw: rawPkt, } dstData, _ := p.Marshal() @@ -497,7 +477,6 @@ func TestRFC8285GetExtensionReturnsNilWhenExtensionsDisabled(t *testing.T) { Marker: true, Extension: false, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -529,7 +508,6 @@ func TestRFC8285DelExtension(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -579,7 +557,6 @@ func TestRFC8285GetExtensionIDs(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -615,7 +592,6 @@ func TestRFC8285GetExtensionIDsReturnsErrorWhenExtensionsDisabled(t *testing.T) Marker: true, Extension: false, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -641,7 +617,6 @@ func TestRFC8285DelExtensionReturnsErrorWhenExtensionsDisabled(t *testing.T) { Marker: true, Extension: false, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -667,7 +642,6 @@ func TestRFC8285OneByteSetExtensionShouldEnableExensionsWhenAdding(t *testing.T) Marker: true, Extension: false, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -710,7 +684,6 @@ func TestRFC8285OneByteSetExtensionShouldSetCorrectExtensionProfileFor16ByteExte Marker: true, Extension: false, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -752,7 +725,6 @@ func TestRFC8285OneByteSetExtensionShouldUpdateExistingExension(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -793,7 +765,6 @@ func TestRFC8285OneByteSetExtensionShouldErrorWhenInvalidIDProvided(t *testing.T }}, }, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -851,7 +822,6 @@ func TestRFC8285OneByteSetExtensionShouldErrorWhenPayloadTooLarge(t *testing.T) }}, }, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -879,7 +849,6 @@ func TestRFC8285TwoByteSetExtensionShouldEnableExensionsWhenAdding(t *testing.T) Marker: true, Extension: false, Version: 2, - PayloadOffset: 31, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -931,7 +900,6 @@ func TestRFC8285TwoByteSetExtensionShouldUpdateExistingExension(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -975,7 +943,6 @@ func TestRFC8285TwoByteSetExtensionShouldErrorWhenPayloadTooLarge(t *testing.T) }}, }, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -1033,7 +1000,6 @@ func TestRFC3550SetExtensionShouldErrorWhenNonZero(t *testing.T) { }}, }, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -1065,7 +1031,6 @@ func TestRFC3550SetExtensionShouldRaiseErrorWhenSettingNonzeroID(t *testing.T) { Extension: true, ExtensionProfile: 0x1111, Version: 2, - PayloadOffset: 26, PayloadType: 96, SequenceNumber: 27023, Timestamp: 3653407706, @@ -1134,7 +1099,7 @@ func TestUnmarshal_ErrorHandling(t *testing.T) { testCase := testCase t.Run(name, func(t *testing.T) { h := &Header{} - err := h.Unmarshal(testCase.input) + _, err := h.Unmarshal(testCase.input) if !errors.Is(err, testCase.err) { t.Errorf("Expected error: %v, got: %v", testCase.err, err) } @@ -1153,9 +1118,6 @@ func TestRoundtrip(t *testing.T) { if err := p.Unmarshal(rawPkt); err != nil { t.Fatal(err) } - if !bytes.Equal(rawPkt, p.Raw) { - t.Errorf("p.Raw must be same as rawPkt.\n p.Raw: %+v,\nrawPkt: %+v", p.Raw, rawPkt) - } if !bytes.Equal(payload, p.Payload) { t.Errorf("p.Payload must be same as payload.\n payload: %+v,\np.Payload: %+v", payload, p.Payload, @@ -1169,9 +1131,6 @@ func TestRoundtrip(t *testing.T) { if !bytes.Equal(rawPkt, buf) { t.Errorf("buf must be same as rawPkt.\n buf: %+v,\nrawPkt: %+v", buf, rawPkt) } - if !bytes.Equal(rawPkt, p.Raw) { - t.Errorf("p.Raw must be same as rawPkt.\n p.Raw: %+v,\nrawPkt: %+v", p.Raw, rawPkt) - } if !bytes.Equal(payload, p.Payload) { t.Errorf("p.Payload must be same as payload.\n payload: %+v,\np.Payload: %+v", payload, p.Payload, diff --git a/packetizer_test.go b/packetizer_test.go index 5d2331a..baf1dbf 100644 --- a/packetizer_test.go +++ b/packetizer_test.go @@ -43,7 +43,6 @@ func TestPacketizer_AbsSendTime(t *testing.T) { Padding: false, Extension: true, Marker: true, - PayloadOffset: 0, // not set by Packetize() at now PayloadType: 98, SequenceNumber: 1234, Timestamp: 45678, From 31e5b0c0644ee00c4f995c43de5edc2a7699f27f Mon Sep 17 00:00:00 2001 From: aler9 <46489434+aler9@users.noreply.github.com> Date: Sun, 16 May 2021 18:33:48 +0200 Subject: [PATCH 2/2] Add /v2 v2 is needed since #119 drops some fields. --- README.md | 1 + go.mod | 2 +- packetizer_test.go | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 0dffc17..789e9ac 100644 --- a/README.md +++ b/README.md @@ -52,6 +52,7 @@ Check out the **[contributing wiki](https://github.com/pion/webrtc/wiki/Contribu * [debiandebiandebian](https://github.com/debiandebiandebian) * [Juliusz Chroboczek](https://github.com/jech) * [ffmiyo](https://github.com/ffmiyo) +* [Alessandro Ros](https://github.com/aler9) ### License MIT License - see [LICENSE](LICENSE) for full text diff --git a/go.mod b/go.mod index 412ae63..19a0cc0 100644 --- a/go.mod +++ b/go.mod @@ -1,4 +1,4 @@ -module github.com/pion/rtp +module github.com/pion/rtp/v2 go 1.13 diff --git a/packetizer_test.go b/packetizer_test.go index baf1dbf..1a4e307 100644 --- a/packetizer_test.go +++ b/packetizer_test.go @@ -6,7 +6,7 @@ import ( "testing" "time" - "github.com/pion/rtp/codecs" + "github.com/pion/rtp/v2/codecs" ) func TestPacketizer(t *testing.T) {