From 575498ffc208e243ccadc9e3153655545f3c136c Mon Sep 17 00:00:00 2001 From: Emil Lundmark Date: Fri, 27 Aug 2021 17:01:56 +0200 Subject: [PATCH] Tweak VP8 payload to comply with RFC 7741 This updates the VP8 payload diagrams to be compliant with RFC 7741. It also fixes some minor inconsistencies with PID, previously referred to as PartID. Bug: None Change-Id: I33eb57d96f3d95b01ef5f0afa21a9dc54b41db2d Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/230243 Reviewed-by: Danil Chapovalov Commit-Queue: Emil Lundmark Cr-Commit-Position: refs/heads/main@{#34859} --- modules/rtp_rtcp/source/rtp_format_vp8.cc | 31 ++++++++++-------- .../source/rtp_format_vp8_test_helper.cc | 26 ++++++++------- .../source/video_rtp_depacketizer_vp8.cc | 31 ++++++++++-------- .../video_rtp_depacketizer_vp8_unittest.cc | 32 ++++++++++++------- 4 files changed, 68 insertions(+), 52 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_format_vp8.cc b/modules/rtp_rtcp/source/rtp_format_vp8.cc index 5005c00fb6..ae5f4e50a4 100644 --- a/modules/rtp_rtcp/source/rtp_format_vp8.cc +++ b/modules/rtp_rtcp/source/rtp_format_vp8.cc @@ -94,22 +94,25 @@ bool RtpPacketizerVp8::NextPacket(RtpPacketToSend* packet) { return true; } -// Write the VP8 payload descriptor. -// 0 -// 0 1 2 3 4 5 6 7 8 -// +-+-+-+-+-+-+-+-+-+ -// |X| |N|S| PART_ID | -// +-+-+-+-+-+-+-+-+-+ -// X: |I|L|T|K| | (mandatory if any of the below are used) -// +-+-+-+-+-+-+-+-+-+ -// I: |PictureID (16b)| (optional) -// +-+-+-+-+-+-+-+-+-+ -// L: | TL0PIC_IDX | (optional) -// +-+-+-+-+-+-+-+-+-+ -// T/K: |TID:Y| KEYIDX | (optional) -// +-+-+-+-+-+-+-+-+-+ RtpPacketizerVp8::RawHeader RtpPacketizerVp8::BuildHeader( const RTPVideoHeaderVP8& header) { + // VP8 payload descriptor + // https://datatracker.ietf.org/doc/html/rfc7741#section-4.2 + // + // 0 1 2 3 4 5 6 7 + // +-+-+-+-+-+-+-+-+ + // |X|R|N|S|R| PID | (REQUIRED) + // +-+-+-+-+-+-+-+-+ + // X: |I|L|T|K| RSV | (OPTIONAL) + // +-+-+-+-+-+-+-+-+ + // I: |M| PictureID | (OPTIONAL) + // +-+-+-+-+-+-+-+-+ + // | PictureID | + // +-+-+-+-+-+-+-+-+ + // L: | TL0PICIDX | (OPTIONAL) + // +-+-+-+-+-+-+-+-+ + // T/K: |TID|Y| KEYIDX | (OPTIONAL) + // +-+-+-+-+-+-+-+-+ RTC_DCHECK(ValidateHeader(header)); RawHeader result; diff --git a/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc b/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc index 174e24c712..0088ff8f31 100644 --- a/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc +++ b/modules/rtp_rtcp/source/rtp_format_vp8_test_helper.cc @@ -14,22 +14,17 @@ #include "test/gmock.h" #include "test/gtest.h" -namespace webrtc { -namespace { - -using ::testing::ElementsAreArray; - -constexpr RtpPacketToSend::ExtensionManager* kNoExtensions = nullptr; - -// Payload descriptor +// VP8 payload descriptor +// https://datatracker.ietf.org/doc/html/rfc7741#section-4.2 +// // 0 1 2 3 4 5 6 7 // +-+-+-+-+-+-+-+-+ -// |X|R|N|S|PartID | (REQUIRED) +// |X|R|N|S|R| PID | (REQUIRED) // +-+-+-+-+-+-+-+-+ -// X: |I|L|T|K| RSV | (OPTIONAL) +// X: |I|L|T|K| RSV | (OPTIONAL) +// +-+-+-+-+-+-+-+-+ +// I: |M| PictureID | (OPTIONAL) // +-+-+-+-+-+-+-+-+ -// |M| PictureID | -// I: +-+-+-+-+-+-+-+-+ (OPTIONAL) // | PictureID | // +-+-+-+-+-+-+-+-+ // L: | TL0PICIDX | (OPTIONAL) @@ -37,6 +32,13 @@ constexpr RtpPacketToSend::ExtensionManager* kNoExtensions = nullptr; // T/K: |TID|Y| KEYIDX | (OPTIONAL) // +-+-+-+-+-+-+-+-+ +namespace webrtc { +namespace { + +using ::testing::ElementsAreArray; + +constexpr RtpPacketToSend::ExtensionManager* kNoExtensions = nullptr; + int Bit(uint8_t byte, int position) { return (byte >> position) & 0x01; } diff --git a/modules/rtp_rtcp/source/video_rtp_depacketizer_vp8.cc b/modules/rtp_rtcp/source/video_rtp_depacketizer_vp8.cc index 7c128fe2bc..d6bd33c24d 100644 --- a/modules/rtp_rtcp/source/video_rtp_depacketizer_vp8.cc +++ b/modules/rtp_rtcp/source/video_rtp_depacketizer_vp8.cc @@ -19,29 +19,34 @@ #include "rtc_base/checks.h" #include "rtc_base/logging.h" -// VP8 format: +// VP8 payload descriptor +// https://datatracker.ietf.org/doc/html/rfc7741#section-4.2 // -// Payload descriptor // 0 1 2 3 4 5 6 7 // +-+-+-+-+-+-+-+-+ -// |X|R|N|S|PartID | (REQUIRED) +// |X|R|N|S|R| PID | (REQUIRED) // +-+-+-+-+-+-+-+-+ -// X: |I|L|T|K| RSV | (OPTIONAL) +// X: |I|L|T|K| RSV | (OPTIONAL) // +-+-+-+-+-+-+-+-+ -// I: | PictureID | (OPTIONAL) +// I: |M| PictureID | (OPTIONAL) +// +-+-+-+-+-+-+-+-+ +// | PictureID | // +-+-+-+-+-+-+-+-+ // L: | TL0PICIDX | (OPTIONAL) // +-+-+-+-+-+-+-+-+ -// T/K: |TID:Y| KEYIDX | (OPTIONAL) +// T/K: |TID|Y| KEYIDX | (OPTIONAL) // +-+-+-+-+-+-+-+-+ // -// Payload header (considered part of the actual payload, sent to decoder) +// VP8 payload header. Considered part of the actual payload, sent to decoder. +// https://datatracker.ietf.org/doc/html/rfc7741#section-4.3 +// // 0 1 2 3 4 5 6 7 // +-+-+-+-+-+-+-+-+ // |Size0|H| VER |P| // +-+-+-+-+-+-+-+-+ -// | ... | -// + + +// : ... : +// +-+-+-+-+-+-+-+-+ + namespace webrtc { namespace { @@ -56,7 +61,7 @@ int ParseVP8Descriptor(RTPVideoHeaderVP8* vp8, bool extension = (*data & 0x80) ? true : false; // X bit vp8->nonReference = (*data & 0x20) ? true : false; // N bit vp8->beginningOfPartition = (*data & 0x10) ? true : false; // S bit - vp8->partitionId = (*data & 0x0F); // PartID field + vp8->partitionId = (*data & 0x07); // PID field data++; parsed_bytes++; @@ -160,10 +165,8 @@ int VideoRtpDepacketizerVp8::ParseRtpPayload( if (descriptor_size == kFailedToParse) return kFailedToParse; - if (vp8_header.partitionId > 8) { - // Weak check for corrupt payload_data: PartID MUST NOT be larger than 8. - return kFailedToParse; - } + RTC_DCHECK_LT(vp8_header.partitionId, 8); + video_header->is_first_packet_in_frame = vp8_header.beginningOfPartition && vp8_header.partitionId == 0; diff --git a/modules/rtp_rtcp/source/video_rtp_depacketizer_vp8_unittest.cc b/modules/rtp_rtcp/source/video_rtp_depacketizer_vp8_unittest.cc index 4837ecae25..77469cf935 100644 --- a/modules/rtp_rtcp/source/video_rtp_depacketizer_vp8_unittest.cc +++ b/modules/rtp_rtcp/source/video_rtp_depacketizer_vp8_unittest.cc @@ -17,32 +17,40 @@ #include "test/gmock.h" #include "test/gtest.h" -namespace webrtc { -namespace { - -// Payload descriptor +// VP8 payload descriptor +// https://datatracker.ietf.org/doc/html/rfc7741#section-4.2 +// // 0 1 2 3 4 5 6 7 // +-+-+-+-+-+-+-+-+ -// |X|R|N|S|PartID | (REQUIRED) +// |X|R|N|S|R| PID | (REQUIRED) // +-+-+-+-+-+-+-+-+ -// X: |I|L|T|K| RSV | (OPTIONAL) +// X: |I|L|T|K| RSV | (OPTIONAL) // +-+-+-+-+-+-+-+-+ -// I: | PictureID | (OPTIONAL) +// I: |M| PictureID | (OPTIONAL) +// +-+-+-+-+-+-+-+-+ +// | PictureID | // +-+-+-+-+-+-+-+-+ // L: | TL0PICIDX | (OPTIONAL) // +-+-+-+-+-+-+-+-+ -// T/K: |TID:Y| KEYIDX | (OPTIONAL) +// T/K: |TID|Y| KEYIDX | (OPTIONAL) // +-+-+-+-+-+-+-+-+ // -// Payload header +// VP8 payload header. Considered part of the actual payload, sent to decoder. +// https://datatracker.ietf.org/doc/html/rfc7741#section-4.3 +// // 0 1 2 3 4 5 6 7 // +-+-+-+-+-+-+-+-+ // |Size0|H| VER |P| // +-+-+-+-+-+-+-+-+ -// : : +// : ... : +// +-+-+-+-+-+-+-+-+ + +namespace webrtc { +namespace { + TEST(VideoRtpDepacketizerVp8Test, BasicHeader) { uint8_t packet[4] = {0}; - packet[0] = 0b0001'0100; // S = 1, PartID = 4. + packet[0] = 0b0001'0100; // S = 1, partition ID = 4. packet[1] = 0x01; // P frame. RTPVideoHeader video_header; @@ -145,7 +153,7 @@ TEST(VideoRtpDepacketizerVp8Test, KeyIdx) { TEST(VideoRtpDepacketizerVp8Test, MultipleExtensions) { uint8_t packet[10] = {0}; - packet[0] = 0b1010'0110; // X and N bit set, partID = 6 + packet[0] = 0b1010'0110; // X and N bit set, partition ID = 6 packet[1] = 0b1111'0000; packet[2] = 0x80 | 0x12; // PictureID, high 7 bits. packet[3] = 0x34; // PictureID, low 8 bits.