Repository: pion/rtp
Pull request #142 (2021-07-08 00:48:16)

Make VP8 PictureID optional
In RFC 7741, PictureID is not mandatory and required only if SLI is used.

## Refs
https://github.com/pion/rtp/pull/126#issuecomment-834139647
Will be enabled in pion/webrtc by https://github.com/pion/webrtc/pull/1875


codecov[bot]: # [Codecov](https://codecov.io/gh/pion/rtp/pull/142?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#142](https://codecov.io/gh/pion/rtp/pull/142?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (9301dac) into [master](https://codecov.io/gh/pion/rtp/commit/e325dcae6088dc345f3a16c844241651fc58d499?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (e325dca) will **increase** coverage by `1.07%`.
> The diff coverage is `89.47%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/rtp/pull/142/graphs/tree.svg?width=650&height=150&src=pr&token=4KrbiBsspj&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)](https://codecov.io/gh/pion/rtp/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   88.80%   89.88%   +1.07%     
==========================================
  Files          13       13              
  Lines         670      672       +2     
==========================================
+ Hits          595      604       +9     
+ Misses         57       50       -7     
  Partials       18       18              
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `89.88% <89.47%> (+1.67%)` | :arrow_up: |

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://codecov.io/gh/pion/rtp/pull/142?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [codecs/vp8\_packet.go](https://codecov.io/gh/pion/rtp/pull/142/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29kZWNzL3ZwOF9wYWNrZXQuZ28=) | `94.25% <89.47%> (+13.07%)` | :arrow_up: |
| [packet.go](https://codecov.io/gh/pion/rtp/pull/142/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGFja2V0Lmdv) | `83.17% <0.00%> (-1.93%)` | :arrow_down: |

------

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


Patch:
diff --git a/codecs/vp8_packet.go b/codecs/vp8_packet.go
index b602ef5..24e7200 100644
--- a/codecs/vp8_packet.go
+++ b/codecs/vp8_packet.go
@@ -2,7 +2,8 @@ package codecs
 
 // VP8Payloader payloads VP8 packets
 type VP8Payloader struct {
-	pictureID uint16
+	EnablePictureID bool
+	pictureID       uint16
 }
 
 const (
@@ -33,12 +34,14 @@ func (p *VP8Payloader) Payload(mtu int, payload []byte) [][]byte {
 	 */
 
 	usingHeaderSize := vp8HeaderSize
-	switch {
-	case p.pictureID == 0:
-	case p.pictureID < 128:
-		usingHeaderSize = vp8HeaderSize + 2
-	default:
-		usingHeaderSize = vp8HeaderSize + 3
+	if p.EnablePictureID {
+		switch {
+		case p.pictureID == 0:
+		case p.pictureID < 128:
+			usingHeaderSize = vp8HeaderSize + 2
+		default:
+			usingHeaderSize = vp8HeaderSize + 3
+		}
 	}
 
 	maxFragmentSize := mtu - usingHeaderSize
@@ -62,17 +65,19 @@ func (p *VP8Payloader) Payload(mtu int, payload []byte) [][]byte {
 			out[0] = 0x10
 			first = false
 		}
-		switch usingHeaderSize {
-		case vp8HeaderSize:
-		case vp8HeaderSize + 2:
-			out[0] |= 0x80
-			out[1] |= 0x80
-			out[2] |= uint8(p.pictureID & 0x7F)
-		case vp8HeaderSize + 3:
-			out[0] |= 0x80
-			out[1] |= 0x80
-			out[2] |= 0x80 | uint8((p.pictureID>>8)&0x7F)
-			out[3] |= uint8(p.pictureID & 0xFF)
+		if p.EnablePictureID {
+			switch usingHeaderSize {
+			case vp8HeaderSize:
+			case vp8HeaderSize + 2:
+				out[0] |= 0x80
+				out[1] |= 0x80
+				out[2] |= uint8(p.pictureID & 0x7F)
+			case vp8HeaderSize + 3:
+				out[0] |= 0x80
+				out[1] |= 0x80
+				out[2] |= 0x80 | uint8((p.pictureID>>8)&0x7F)
+				out[3] |= uint8(p.pictureID & 0xFF)
+			}
 		}
 
 		copy(out[usingHeaderSize:], payloadData[payloadDataIndex:payloadDataIndex+currentFragmentSize])


Test patch:
diff --git a/codecs/vp8_packet_test.go b/codecs/vp8_packet_test.go
index 1c062c3..37d160d 100644
--- a/codecs/vp8_packet_test.go
+++ b/codecs/vp8_packet_test.go
@@ -2,6 +2,7 @@ package codecs
 
 import (
 	"errors"
+	"reflect"
 	"testing"
 )
 
@@ -109,33 +110,102 @@ func TestVP8Packet_Unmarshal(t *testing.T) {
 }
 
 func TestVP8Payloader_Payload(t *testing.T) {
-	pck := VP8Payloader{}
-	payload := []byte{0x90, 0x90, 0x90}
-
-	// Positive MTU, nil payload
-	res := pck.Payload(1, nil)
-	if len(res) != 0 {
-		t.Fatal("Generated payload should be empty")
-	}
-
-	// Positive MTU, small payload
-	// MTU of 1 results in fragment size of 0
-	res = pck.Payload(1, payload)
-	if len(res) != 0 {
-		t.Fatal("Generated payload should be empty")
-	}
+	testCases := map[string]struct {
+		payloader VP8Payloader
+		mtu       int
+		payload   [][]byte
+		expected  [][][]byte
+	}{
+		"WithoutPictureID": {
+			payloader: VP8Payloader{},
+			mtu:       2,
+			payload: [][]byte{
+				{0x90, 0x90, 0x90},
+				{0x91, 0x91},
+			},
+			expected: [][][]byte{
+				{{0x10, 0x90}, {0x00, 0x90}, {0x00, 0x90}},
+				{{0x10, 0x91}, {0x00, 0x91}},
+			},
+		},
+		"WithPictureID_1byte": {
+			payloader: VP8Payloader{
+				EnablePictureID: true,
+				pictureID:       0x20,
+			},
+			mtu: 5,
+			payload: [][]byte{
+				{0x90, 0x90, 0x90},
+				{0x91, 0x91},
+			},
+			expected: [][][]byte{
+				{
+					{0x90, 0x80, 0x20, 0x90, 0x90},
+					{0x80, 0x80, 0x20, 0x90},
+				},
+				{
+					{0x90, 0x80, 0x21, 0x91, 0x91},
+				},
+			},
+		},
+		"WithPictureID_2bytes": {
+			payloader: VP8Payloader{
+				EnablePictureID: true,
+				pictureID:       0x120,
+			},
+			mtu: 6,
+			payload: [][]byte{
+				{0x90, 0x90, 0x90},
+				{0x91, 0x91},
+			},
+			expected: [][][]byte{
+				{
+					{0x90, 0x80, 0x81, 0x20, 0x90, 0x90},
+					{0x80, 0x80, 0x81, 0x20, 0x90},
+				},
+				{
+					{0x90, 0x80, 0x81, 0x21, 0x91, 0x91},
+				},
+			},
+		},
+	}
+	for name, testCase := range testCases {
+		testCase := testCase
+		t.Run(name, func(t *testing.T) {
+			pck := testCase.payloader
+
+			for i := range testCase.payload {
+				res := pck.Payload(testCase.mtu, testCase.payload[i])
+				if !reflect.DeepEqual(testCase.expected[i], res) {
+					t.Fatalf("Generated packet[%d] differs, expected: %v, got: %v", i, testCase.expected[i], res)
+				}
+			}
+		})
+	}
+
+	t.Run("Error", func(t *testing.T) {
+		pck := VP8Payloader{}
+		payload := []byte{0x90, 0x90, 0x90}
+
+		// Positive MTU, nil payload
+		res := pck.Payload(1, nil)
+		if len(res) != 0 {
+			t.Fatal("Generated payload should be empty")
+		}
 
-	// Negative MTU, small payload
-	res = pck.Payload(-1, payload)
-	if len(res) != 0 {
-		t.Fatal("Generated payload should be empty")
-	}
+		// Positive MTU, small payload
+		// MTU of 1 results in fragment size of 0
+		res = pck.Payload(1, payload)
+		if len(res) != 0 {
+			t.Fatal("Generated payload should be empty")
+		}
 
-	// Positive MTU, small payload
-	res = pck.Payload(2, payload)
-	if len(res) != len(payload) {
-		t.Fatal("Generated payload should be the same size as original payload size")
-	}
+		// Negative MTU, small payload
+		res = pck.Payload(-1, payload)
+		if len(res) != 0 {
+			t.Fatal("Generated payload should be empty")
+		}
+	})
 }
 
 func TestVP8PartitionHeadChecker_IsPartitionHead(t *testing.T) {