Repository: pion/rtp
Pull request #228 (2023-07-16 18:45:12)

Fix API breakage on Header.Unmarshal
Return (int, error) and not just error

Sean-Der: @stv0g @aler9 This was the plan I believe? After this lands I can start tagging `v1` off of master branch again.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pion/rtp/pull/228?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
Patch coverage: **`83.33`**% and project coverage change: **`-1.00`** :warning:
> Comparison is base [(`5d59489`)](https://app.codecov.io/gh/pion/rtp/commit/5d59489081b941427ab4362c846175942728d328?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) 87.53% compared to head [(`f5cf1a5`)](https://app.codecov.io/gh/pion/rtp/pull/228?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) 86.54%.

> :exclamation: Current head f5cf1a5 differs from pull request most recent head 9478ee9. Consider uploading reports for the commit 9478ee9 to get more accurate results

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #228      +/-   ##
==========================================
- Coverage   87.53%   86.54%   -1.00%     
==========================================
  Files          20       20              
  Lines        1877     1880       +3     
==========================================
- Hits         1643     1627      -16     
- Misses        204      218      +14     
- Partials       30       35       +5     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `86.54% <83.33%> (-0.04%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://app.codecov.io/gh/pion/rtp/pull/228?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [packet.go](https://app.codecov.io/gh/pion/rtp/pull/228?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGFja2V0Lmdv) | `85.91% <83.33%> (-2.49%)` | :arrow_down: |

... and [2 files with indirect coverage changes](https://app.codecov.io/gh/pion/rtp/pull/228/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/pion/rtp/pull/228?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

aler9: @Sean-Der the change that you want to restore was part of the backward-incompatible PR #119, that was included in v1.7.0 and that i intentionally left out in my previous PR. Prior v1.7.0, signature of `Header.Unmarshal` was

https://github.com/pion/rtp/blob/4997f5536911192ea66dd8f47eae54934e9a1537/packet.go#L81

after v1.7.0, signature is

https://github.com/pion/rtp/blob/ccb8b11cd0efb85bef256e50a5b6b4c4ac74058a/packet.go#L78

Therefore, this PR restores compatibility with `v1.7.x` but breaks compatibility with `v1.6.5`. Probably it's better to merge it since version `v1.7.x` has surely more users than `v1.6.x`.

Sean-Der: @aler9 thanks for the explanation. What a mess :(

I will get some tooling setup to make sure this doesn’t happen again.

Patch:
diff --git a/AUTHORS.txt b/AUTHORS.txt
index 38685a9..727aaaa 100644
--- a/AUTHORS.txt
+++ b/AUTHORS.txt
@@ -30,8 +30,10 @@ Michael Uti <utimichael9@gmail.com>
 Raphael Derosso Pereira <raphaelpereira@gmail.com>
 Rob Lofthouse <ri.lofthouse@gmail.com>
 Robin Raymond <robin@goheadroom.com>
+Sean <sean@pion.ly>
 Sean DuBois <duboisea@twitch.tv>
 Sean DuBois <seaduboi@amazon.com>
+Sean DuBois <sean@pion.ly>
 Sean DuBois <sean@siobud.com>
 Simone Gotti <simone.gotti@gmail.com>
 Steffen Vogel <post@steffenvogel.de>
diff --git a/packet.go b/packet.go
index 2796a09..a47e5fc 100644
--- a/packet.go
+++ b/packet.go
@@ -82,9 +82,10 @@ func (p Packet) String() string {
 }
 
 // Unmarshal parses the passed byte slice and stores the result in the Header.
-func (h *Header) Unmarshal(buf []byte) error { //nolint:gocognit
+// 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 fmt.Errorf("%w: %d < %d", errHeaderSizeInsufficient, len(buf), headerLength)
+		return 0, fmt.Errorf("%w: %d < %d", errHeaderSizeInsufficient, len(buf), headerLength)
 	}
 
 	/*
@@ -112,9 +113,10 @@ func (h *Header) Unmarshal(buf []byte) error { //nolint:gocognit
 		h.CSRC = h.CSRC[:nCSRC]
 	}
 
-	n := csrcOffset + (nCSRC * csrcLength)
+	n = csrcOffset + (nCSRC * csrcLength)
 	if len(buf) < n {
-		return fmt.Errorf("size %d < %d: %w", len(buf), n, errHeaderSizeInsufficient)
+		return n, fmt.Errorf("size %d < %d: %w", len(buf), n,
+			errHeaderSizeInsufficient)
 	}
 
 	h.Marker = (buf[1] >> markerShift & markerMask) > 0
@@ -135,7 +137,7 @@ func (h *Header) Unmarshal(buf []byte) error { //nolint:gocognit
 
 	if h.Extension {
 		if expected := n + 4; len(buf) < expected {
-			return fmt.Errorf("size %d < %d: %w",
+			return n, fmt.Errorf("size %d < %d: %w",
 				len(buf), expected,
 				errHeaderSizeInsufficientForExtension,
 			)
@@ -147,7 +149,7 @@ func (h *Header) Unmarshal(buf []byte) error { //nolint:gocognit
 		n += 2
 
 		if expected := n + extensionLength; len(buf) < expected {
-			return fmt.Errorf("size %d < %d: %w",
+			return n, fmt.Errorf("size %d < %d: %w",
 				len(buf), expected,
 				errHeaderSizeInsufficientForExtension,
 			)
@@ -198,7 +200,8 @@ func (h *Header) Unmarshal(buf []byte) error { //nolint:gocognit
 
 		default: // RFC3550 Extension
 			if len(buf) < n+extensionLength {
-				return fmt.Errorf("%w: %d < %d", errHeaderSizeInsufficientForExtension, len(buf), n+extensionLength)
+				return n, fmt.Errorf("%w: %d < %d",
+					errHeaderSizeInsufficientForExtension, len(buf), n+extensionLength)
 			}
 
 			extension := Extension{id: 0, payload: buf[n : n+extensionLength]}
@@ -209,12 +212,13 @@ func (h *Header) Unmarshal(buf []byte) error { //nolint:gocognit
 
 	h.PayloadOffset = n
 
-	return nil
+	return n, nil
 }
 
 // Unmarshal parses the passed byte slice and stores the result in the Packet.
 func (p *Packet) Unmarshal(buf []byte) error {
-	if err := p.Header.Unmarshal(buf); err != nil {
+	_, err := p.Header.Unmarshal(buf)
+	if err != nil {
 		return err
 	}
 


Test patch:
diff --git a/packet_test.go b/packet_test.go
index 40cb1a0..be13efb 100644
--- a/packet_test.go
+++ b/packet_test.go
@@ -1345,7 +1345,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)
 			}