Repository: pion/rtp
Pull request #150 (2021-08-22 21:03:30)

Make the depacketizer more consistent
With the recent changes to depacketizer, partition tail checking is done in the Depacketizer, while partition tail checking is done in the PartitionHeadChecker.  This is inconsistent and error-prone.

In https://github.com/pion/webrtc/pull/1882, I proposed moving both methods into samplebuilder options; this was rejected, arguing that codecs should be more convenient to use.  In this PR, we move both methods into Depacketizer, which obsoletes PartitionHeadCheckers.

codecov[bot]: # [Codecov](https://codecov.io/gh/pion/rtp/pull/150?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
> Merging [#150](https://codecov.io/gh/pion/rtp/pull/150?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (b874c61) into [master](https://codecov.io/gh/pion/rtp/commit/b670290abfddd1a47b7f90bd968e24c9bfd1c6ea?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) (b670290) will **decrease** coverage by `0.15%`.
> The diff coverage is `72.72%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/rtp/pull/150/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/150?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

```diff
@@            Coverage Diff             @@
##           master     #150      +/-   ##
==========================================
- Coverage   93.17%   93.02%   -0.16%     
==========================================
  Files          14       14              
  Lines         938      932       -6     
==========================================
- Hits          874      867       -7     
  Misses         46       46              
- Partials       18       19       +1     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `93.02% <72.72%> (-0.16%)` | :arrow_down: |
| wasm | `92.59% <72.72%> (-0.16%)` | :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://codecov.io/gh/pion/rtp/pull/150?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [codecs/opus\_packet.go](https://codecov.io/gh/pion/rtp/pull/150/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29kZWNzL29wdXNfcGFja2V0Lmdv) | `100.00% <ø> (+6.25%)` | :arrow_up: |
| [codecs/common.go](https://codecov.io/gh/pion/rtp/pull/150/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29kZWNzL2NvbW1vbi5nbw==) | `66.66% <33.33%> (-33.34%)` | :arrow_down: |
| [codecs/vp8\_packet.go](https://codecov.io/gh/pion/rtp/pull/150/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29kZWNzL3ZwOF9wYWNrZXQuZ28=) | `92.94% <50.00%> (-1.32%)` | :arrow_down: |
| [codecs/h264\_packet.go](https://codecov.io/gh/pion/rtp/pull/150/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29kZWNzL2gyNjRfcGFja2V0Lmdv) | `97.58% <100.00%> (ø)` | |
| [codecs/vp9\_packet.go](https://codecov.io/gh/pion/rtp/pull/150/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-Y29kZWNzL3ZwOV9wYWNrZXQuZ28=) | `93.23% <100.00%> (+0.64%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/rtp/pull/150?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/150?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Last update [b670290...b874c61](https://codecov.io/gh/pion/rtp/pull/150?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/common.go b/codecs/common.go
index 39336d2..af5632a 100644
--- a/codecs/common.go
+++ b/codecs/common.go
@@ -6,3 +6,21 @@ func min(a, b int) int {
 	}
 	return b
 }
+
+// audioDepacketizer is a mixin for audio codec depacketizers
+type audioDepacketizer struct{}
+
+func (d *audioDepacketizer) IsPartitionTail(marker bool, payload []byte) bool {
+	return true
+}
+
+func (d *audioDepacketizer) IsPartitionHead(payload []byte) bool {
+	return true
+}
+
+// videoDepacketizer is a mixin for video codec depacketizers
+type videoDepacketizer struct{}
+
+func (d *videoDepacketizer) IsPartitionTail(marker bool, payload []byte) bool {
+	return marker
+}
diff --git a/codecs/h264_packet.go b/codecs/h264_packet.go
index 40d207f..11a82fe 100644
--- a/codecs/h264_packet.go
+++ b/codecs/h264_packet.go
@@ -189,6 +189,8 @@ func (p *H264Payloader) Payload(mtu uint16, payload []byte) [][]byte {
 type H264Packet struct {
 	IsAVC     bool
 	fuaBuffer []byte
+
+	videoDepacketizer
 }
 
 func (p *H264Packet) doPackaging(nalu []byte) []byte {
@@ -265,18 +267,18 @@ func (p *H264Packet) Unmarshal(payload []byte) ([]byte, error) {
 	return nil, fmt.Errorf("%w: %d", errUnhandledNALUType, naluType)
 }
 
-// H264PartitionHeadChecker checks H264 partition head
+// H264PartitionHeadChecker is obsolete
 type H264PartitionHeadChecker struct{}
 
 // IsPartitionHead checks if this is the head of a packetized nalu stream.
-func (*H264PartitionHeadChecker) IsPartitionHead(packet []byte) bool {
-	if packet == nil || len(packet) < 2 {
+func (*H264Packet) IsPartitionHead(payload []byte) bool {
+	if len(payload) < 2 {
 		return false
 	}
 
-	if packet[0]&naluTypeBitmask == fuaNALUType ||
-		packet[0]&naluTypeBitmask == fubNALUType {
-		return packet[1]&fuStartBitmask != 0
+	if payload[0]&naluTypeBitmask == fuaNALUType ||
+		payload[0]&naluTypeBitmask == fubNALUType {
+		return payload[1]&fuStartBitmask != 0
 	}
 
 	return true
diff --git a/codecs/opus_packet.go b/codecs/opus_packet.go
index dfae73c..cd5ea33 100644
--- a/codecs/opus_packet.go
+++ b/codecs/opus_packet.go
@@ -17,12 +17,8 @@ func (p *OpusPayloader) Payload(mtu uint16, payload []byte) [][]byte {
 // OpusPacket represents the Opus header that is stored in the payload of an RTP Packet
 type OpusPacket struct {
 	Payload []byte
-}
 
-// IsDetectedFinalPacketInSequence returns true as all opus packets are always
-// final in a sequence
-func (p *OpusPacket) IsDetectedFinalPacketInSequence(rtpPacketMarketBit bool) bool {
-	return true
+	audioDepacketizer
 }
 
 // Unmarshal parses the passed byte slice and stores the result in the OpusPacket this method is called upon
@@ -37,14 +33,5 @@ func (p *OpusPacket) Unmarshal(packet []byte) ([]byte, error) {
 	return packet, nil
 }
 
-// OpusPartitionHeadChecker checks Opus partition head
+// OpusPartitionHeadChecker is obsolete
 type OpusPartitionHeadChecker struct{}
-
-// IsPartitionHead checks whether if this is a head of the Opus partition
-func (*OpusPartitionHeadChecker) IsPartitionHead(packet []byte) bool {
-	p := &OpusPacket{}
-	if _, err := p.Unmarshal(packet); err != nil {
-		return false
-	}
-	return true
-}
diff --git a/codecs/vp8_packet.go b/codecs/vp8_packet.go
index d9d986d..cd86929 100644
--- a/codecs/vp8_packet.go
+++ b/codecs/vp8_packet.go
@@ -115,12 +115,8 @@ type VP8Packet struct {
 	KEYIDX    uint8  /* 5 bits temporal key frame index */
 
 	Payload []byte
-}
 
-// IsDetectedFinalPacketInSequence returns true of the packet passed in has the
-// marker bit set indicated the end of a packet sequence
-func (p *VP8Packet) IsDetectedFinalPacketInSequence(rtpPacketMarketBit bool) bool {
-	return rtpPacketMarketBit
+	videoDepacketizer
 }
 
 // Unmarshal parses the passed byte slice and stores the result in the VP8Packet this method is called upon
@@ -193,14 +189,13 @@ func (p *VP8Packet) Unmarshal(payload []byte) ([]byte, error) {
 	return p.Payload, nil
 }
 
-// VP8PartitionHeadChecker checks VP8 partition head
+// VP8PartitionHeadChecker is obsolete
 type VP8PartitionHeadChecker struct{}
 
 // IsPartitionHead checks whether if this is a head of the VP8 partition
-func (*VP8PartitionHeadChecker) IsPartitionHead(packet []byte) bool {
-	p := &VP8Packet{}
-	if _, err := p.Unmarshal(packet); err != nil {
+func (*VP8Packet) IsPartitionHead(payload []byte) bool {
+	if len(payload) < 1 {
 		return false
 	}
-	return p.S == 1
+	return (payload[0] & 0x10) != 0
 }
diff --git a/codecs/vp9_packet.go b/codecs/vp9_packet.go
index 75cf4eb..917e630 100644
--- a/codecs/vp9_packet.go
+++ b/codecs/vp9_packet.go
@@ -148,12 +148,8 @@ type VP9Packet struct {
 	PGPDiff [][]uint8 // Reference indecies of pictures in a Picture Group
 
 	Payload []byte
-}
 
-// IsDetectedFinalPacketInSequence returns true of the packet passed in has the
-// marker bit set indicated the end of a packet sequence
-func (p *VP9Packet) IsDetectedFinalPacketInSequence(rtpPacketMarketBit bool) bool {
-	return rtpPacketMarketBit
+	videoDepacketizer
 }
 
 // Unmarshal parses the passed byte slice and stores the result in the VP9Packet this method is called upon
@@ -380,14 +376,13 @@ func (p *VP9Packet) parseSSData(packet []byte, pos int) (int, error) {
 	return pos, nil
 }
 
-// VP9PartitionHeadChecker checks VP9 partition head
+// VP9PartitionHeadChecker is obsolete
 type VP9PartitionHeadChecker struct{}
 
 // IsPartitionHead checks whether if this is a head of the VP9 partition
-func (*VP9PartitionHeadChecker) IsPartitionHead(packet []byte) bool {
-	p := &VP9Packet{}
-	if _, err := p.Unmarshal(packet); err != nil {
+func (*VP9Packet) IsPartitionHead(payload []byte) bool {
+	if len(payload) < 1 {
 		return false
 	}
-	return p.B
+	return (payload[0] & 0x08) != 0
 }
diff --git a/depacketizer.go b/depacketizer.go
index e578d71..c66d2e3 100644
--- a/depacketizer.go
+++ b/depacketizer.go
@@ -2,6 +2,12 @@ package rtp
 
 // Depacketizer depacketizes a RTP payload, removing any RTP specific data from the payload
 type Depacketizer interface {
-	IsDetectedFinalPacketInSequence(rtpPacketMarketBit bool) bool
 	Unmarshal(packet []byte) ([]byte, error)
+	// Checks if the packet is at the beginning of a partition.  This
+	// should return false if the result could not be determined, in
+	// which case the caller will detect timestamp discontinuities.
+	IsPartitionHead(payload []byte) bool
+	// Checks if the packet is at the end of a partition.  This should
+	// return false if the result could not be determined.
+	IsPartitionTail(marker bool, payload []byte) bool
 }


Test patch:
diff --git a/codecs/h264_packet_test.go b/codecs/h264_packet_test.go
index 608b020..47826d7 100644
--- a/codecs/h264_packet_test.go
+++ b/codecs/h264_packet_test.go
@@ -180,45 +180,45 @@ func TestH264Packet_Unmarshal(t *testing.T) {
 	}
 }
 
-func TestH264PartitionHeadChecker_IsPartitionHead(t *testing.T) {
-	h264PartitionHeadChecker := H264PartitionHeadChecker{}
+func TestH264IsPartitionHead(t *testing.T) {
+	h264 := H264Packet{}
 
-	if h264PartitionHeadChecker.IsPartitionHead(nil) {
+	if h264.IsPartitionHead(nil) {
 		t.Fatal("nil must not be a partition head")
 	}
 
 	emptyNalu := []byte{}
-	if h264PartitionHeadChecker.IsPartitionHead(emptyNalu) {
+	if h264.IsPartitionHead(emptyNalu) {
 		t.Fatal("empty nalu must not be a partition head")
 	}
 
 	singleNalu := []byte{1, 0}
-	if h264PartitionHeadChecker.IsPartitionHead(singleNalu) == false {
+	if h264.IsPartitionHead(singleNalu) == false {
 		t.Fatal("single nalu must be a partition head")
 	}
 
 	stapaNalu := []byte{stapaNALUType, 0}
-	if h264PartitionHeadChecker.IsPartitionHead(stapaNalu) == false {
+	if h264.IsPartitionHead(stapaNalu) == false {
 		t.Fatal("stapa nalu must be a partition head")
 	}
 
 	fuaStartNalu := []byte{fuaNALUType, fuStartBitmask}
-	if h264PartitionHeadChecker.IsPartitionHead(fuaStartNalu) == false {
+	if h264.IsPartitionHead(fuaStartNalu) == false {
 		t.Fatal("fua start nalu must be a partition head")
 	}
 
 	fuaEndNalu := []byte{fuaNALUType, fuEndBitmask}
-	if h264PartitionHeadChecker.IsPartitionHead(fuaEndNalu) {
+	if h264.IsPartitionHead(fuaEndNalu) {
 		t.Fatal("fua end nalu must not be a partition head")
 	}
 
 	fubStartNalu := []byte{fubNALUType, fuStartBitmask}
-	if h264PartitionHeadChecker.IsPartitionHead(fubStartNalu) == false {
+	if h264.IsPartitionHead(fubStartNalu) == false {
 		t.Fatal("fub start nalu must be a partition head")
 	}
 
 	fubEndNalu := []byte{fubNALUType, fuEndBitmask}
-	if h264PartitionHeadChecker.IsPartitionHead(fubEndNalu) {
+	if h264.IsPartitionHead(fubEndNalu) {
 		t.Fatal("fub end nalu must not be a partition head")
 	}
 }
diff --git a/codecs/opus_packet_test.go b/codecs/opus_packet_test.go
index 1c09a5a..e68ff57 100644
--- a/codecs/opus_packet_test.go
+++ b/codecs/opus_packet_test.go
@@ -59,15 +59,10 @@ func TestOpusPayloader_Payload(t *testing.T) {
 	}
 }
 
-func TestOpusPartitionHeadChecker_IsPartitionHead(t *testing.T) {
-	checker := &OpusPartitionHeadChecker{}
-	t.Run("SmallPacket", func(t *testing.T) {
-		if checker.IsPartitionHead([]byte{}) {
-			t.Fatal("Small packet should not be the head of a new partition")
-		}
-	})
+func TestOpusIsPartitionHead(t *testing.T) {
+	opus := &OpusPacket{}
 	t.Run("NormalPacket", func(t *testing.T) {
-		if !checker.IsPartitionHead([]byte{0x00, 0x00}) {
+		if !opus.IsPartitionHead([]byte{0x00, 0x00}) {
 			t.Fatal("All OPUS RTP packet should be the head of a new partition")
 		}
 	})
diff --git a/codecs/vp8_packet_test.go b/codecs/vp8_packet_test.go
index 7bfe0b6..b173cfb 100644
--- a/codecs/vp8_packet_test.go
+++ b/codecs/vp8_packet_test.go
@@ -202,20 +202,20 @@ func TestVP8Payloader_Payload(t *testing.T) {
 	})
 }
 
-func TestVP8PartitionHeadChecker_IsPartitionHead(t *testing.T) {
-	checker := &VP8PartitionHeadChecker{}
+func TestVP8IsPartitionHead(t *testing.T) {
+	vp8 := &VP8Packet{}
 	t.Run("SmallPacket", func(t *testing.T) {
-		if checker.IsPartitionHead([]byte{0x00}) {
+		if vp8.IsPartitionHead([]byte{0x00}) {
 			t.Fatal("Small packet should not be the head of a new partition")
 		}
 	})
 	t.Run("SFlagON", func(t *testing.T) {
-		if !checker.IsPartitionHead([]byte{0x10, 0x00, 0x00, 0x00}) {
+		if !vp8.IsPartitionHead([]byte{0x10, 0x00, 0x00, 0x00}) {
 			t.Fatal("Packet with S flag should be the head of a new partition")
 		}
 	})
 	t.Run("SFlagOFF", func(t *testing.T) {
-		if checker.IsPartitionHead([]byte{0x00, 0x00, 0x00, 0x00}) {
+		if vp8.IsPartitionHead([]byte{0x00, 0x00, 0x00, 0x00}) {
 			t.Fatal("Packet without S flag should not be the head of a new partition")
 		}
 	})
diff --git a/codecs/vp9_packet_test.go b/codecs/vp9_packet_test.go
index a1f44af..65861c6 100644
--- a/codecs/vp9_packet_test.go
+++ b/codecs/vp9_packet_test.go
@@ -299,18 +299,18 @@ func TestVP9Payloader_Payload(t *testing.T) {
 	})
 }
 
-func TestVP9PartitionHeadChecker_IsPartitionHead(t *testing.T) {
-	checker := &VP9PartitionHeadChecker{}
+func TestVP9IsPartitionHead(t *testing.T) {
+	vp9 := &VP9Packet{}
 	t.Run("SmallPacket", func(t *testing.T) {
-		if checker.IsPartitionHead([]byte{}) {
+		if vp9.IsPartitionHead([]byte{}) {
 			t.Fatal("Small packet should not be the head of a new partition")
 		}
 	})
 	t.Run("NormalPacket", func(t *testing.T) {
-		if !checker.IsPartitionHead([]byte{0x18, 0x00, 0x00}) {
+		if !vp9.IsPartitionHead([]byte{0x18, 0x00, 0x00}) {
 			t.Error("VP9 RTP packet with B flag should be head of a new partition")
 		}
-		if checker.IsPartitionHead([]byte{0x10, 0x00, 0x00}) {
+		if vp9.IsPartitionHead([]byte{0x10, 0x00, 0x00}) {
 			t.Error("VP9 RTP packet without B flag should not be head of a new partition")
 		}
 	})