Make VP8 PictureID optional

In RFC 7741, PictureID is not mandatory and required only if SLI is used.
pion · Jun 14, 2023 · 9cfc943 · 9cfc943
1 parent d8a4cf0
commit 9cfc943
Show file tree

Hide file tree

Showing 2 changed files with 118 additions and 43 deletions.
diff --git 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])

diff --git 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) {