Repository: pion/rtp
Pull request #106 (2020-12-12 16:16:46)

Fix Unmarshal to used struct
Extensions field was not cleared on Unmarshal and data was appended
to the old extension slice.
This fixes Extensions field to have correct number of data even if
the slice has some data before calling Unmarshal.

Reuse Extensions and CSRC fields.

### master branch:
```
BenchmarkUnmarshal/SharedStruct-32      6965502    158 ns/op    370 B/op    1 allocs/op
**^Unmarshal result was wrong**
BenchmarkUnmarshal/NewStruct-32         9876006    121 ns/op    104 B/op    3 allocs/op
```
### this:
```
BenchmarkUnmarshal/SharedStruct-32     61906088   19.1 ns/op      0 B/op    0 allocs/op
BenchmarkUnmarshal/NewStruct-32         9698794    123 ns/op    104 B/op    3 allocs/op
```

codecov[bot]: # [Codecov](https://codecov.io/gh/pion/rtp/pull/106?src=pr&el=h1) Report
> Merging [#106](https://codecov.io/gh/pion/rtp/pull/106?src=pr&el=desc) (bde8a26) into [master](https://codecov.io/gh/pion/rtp/commit/c8b3d3ec7ff956797d8d29c5ab1cfb054c9e4028?el=desc) (c8b3d3e) will **increase** coverage by `0.73%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/rtp/pull/106/graphs/tree.svg?width=650&height=150&src=pr&token=4KrbiBsspj)](https://codecov.io/gh/pion/rtp/pull/106?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #106      +/-   ##
==========================================
+ Coverage   90.52%   91.26%   +0.73%     
==========================================
  Files          13       13              
  Lines         612      618       +6     
==========================================
+ Hits          554      564      +10     
+ Misses         41       37       -4     
  Partials       17       17              
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `91.26% <100.00%> (+0.73%)` | :arrow_up: |
| wasm | `90.61% <100.00%> (+0.09%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/rtp/pull/106?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [packet.go](https://codecov.io/gh/pion/rtp/pull/106/diff?src=pr&el=tree#diff-cGFja2V0Lmdv) | `85.64% <100.00%> (+2.39%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/rtp/pull/106?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/rtp/pull/106?src=pr&el=footer). Last update [c8b3d3e...bde8a26](https://codecov.io/gh/pion/rtp/pull/106?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/packet.go b/packet.go
index aa2c232..b237b0a 100644
--- a/packet.go
+++ b/packet.go
@@ -101,9 +101,14 @@ 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
-	h.CSRC = make([]uint32, rawPacket[0]&ccMask)
+	nCSRC := int(rawPacket[0] & ccMask)
+	if cap(h.CSRC) < nCSRC || h.CSRC == nil {
+		h.CSRC = make([]uint32, nCSRC)
+	} else {
+		h.CSRC = h.CSRC[:nCSRC]
+	}
 
-	currOffset := csrcOffset + (len(h.CSRC) * csrcLength)
+	currOffset := csrcOffset + (nCSRC * csrcLength)
 	if len(rawPacket) < currOffset {
 		return fmt.Errorf("size %d < %d: %w", len(rawPacket), currOffset, errHeaderSizeInsufficient)
 	}
@@ -120,6 +125,10 @@ func (h *Header) Unmarshal(rawPacket []byte) error { //nolint:gocognit
 		h.CSRC[i] = binary.BigEndian.Uint32(rawPacket[offset:])
 	}
 
+	if h.Extensions != nil {
+		h.Extensions = h.Extensions[:0]
+	}
+
 	if h.Extension {
 		if expected := currOffset + 4; len(rawPacket) < expected {
 			return fmt.Errorf("size %d < %d: %w",


Test patch:
diff --git a/packet_test.go b/packet_test.go
index 0dff52a..a1cfa8e 100644
--- a/packet_test.go
+++ b/packet_test.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"encoding/hex"
 	"errors"
+	"fmt"
 	"reflect"
 	"testing"
 )
@@ -41,31 +42,36 @@ func TestBasic(t *testing.T) {
 		Raw:     rawPkt,
 	}
 
-	if err := p.Unmarshal(rawPkt); err != nil {
-		t.Error(err)
-	} else if !reflect.DeepEqual(p, parsedPacket) {
-		t.Errorf("TestBasic unmarshal: got %#v, want %#v", p, parsedPacket)
-	}
+	// Unmarshal to the used Packet should work as well.
+	for i := 0; i < 2; i++ {
+		t.Run(fmt.Sprintf("Run%d", i+1), func(t *testing.T) {
+			if err := p.Unmarshal(rawPkt); err != nil {
+				t.Error(err)
+			} else if !reflect.DeepEqual(p, parsedPacket) {
+				t.Errorf("TestBasic unmarshal: got %#v, want %#v", p, parsedPacket)
+			}
 
-	if parsedPacket.Header.MarshalSize() != 20 {
-		t.Errorf("wrong computed header marshal size")
-	} else if parsedPacket.MarshalSize() != len(rawPkt) {
-		t.Errorf("wrong computed marshal size")
-	}
+			if parsedPacket.Header.MarshalSize() != 20 {
+				t.Errorf("wrong computed header marshal size")
+			} else if parsedPacket.MarshalSize() != len(rawPkt) {
+				t.Errorf("wrong computed marshal size")
+			}
 
-	if p.PayloadOffset != 20 {
-		t.Errorf("wrong payload offset: %d != %d", p.PayloadOffset, 20)
-	}
+			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)
-	}
+			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)
+			if p.PayloadOffset != 20 {
+				t.Errorf("wrong payload offset: %d != %d", p.PayloadOffset, 20)
+			}
+		})
 	}
 }
 
@@ -1219,3 +1225,46 @@ func BenchmarkMarshalTo(b *testing.B) {
 		}
 	}
 }
+
+func BenchmarkUnmarshal(b *testing.B) {
+	pkt := Packet{
+		Header: Header{
+			Extension:        true,
+			CSRC:             []uint32{1, 2},
+			ExtensionProfile: extensionProfileTwoByte,
+			Extensions: []Extension{
+				{id: 1, payload: []byte{3, 4}},
+				{id: 2, payload: []byte{5, 6}},
+			},
+		},
+		Payload: []byte{
+			0x07, 0x08, 0x09, 0x0a,
+		},
+	}
+	rawPkt, errMarshal := pkt.Marshal()
+	if errMarshal != nil {
+		b.Fatal(errMarshal)
+	}
+
+	b.Run("SharedStruct", func(b *testing.B) {
+		p := &Packet{}
+
+		b.ReportAllocs()
+		b.ResetTimer()
+		for i := 0; i < b.N; i++ {
+			if err := p.Unmarshal(rawPkt); err != nil {
+				b.Fatal(err)
+			}
+		}
+	})
+	b.Run("NewStruct", func(b *testing.B) {
+		b.ReportAllocs()
+		b.ResetTimer()
+		for i := 0; i < b.N; i++ {
+			p := &Packet{}
+			if err := p.Unmarshal(rawPkt); err != nil {
+				b.Fatal(err)
+			}
+		}
+	})
+}