From 54047bea1bf00aae0d8da31eb07dd3663d5fb545 Mon Sep 17 00:00:00 2001 From: Johannes Kron Date: Thu, 21 Feb 2019 14:09:20 +0000 Subject: [PATCH] Reland "Extend TransportSequenceNumber RTP header extension" This reverts commit 109b5fb5f5b2f46e1798c91c4a024ce26f57f0b0. Reason for revert: The failing libfuzzer was fixed in commit d6c6f16063b81fc60206618ba06198e34ee0eacb Original change's description: > Revert "Extend TransportSequenceNumber RTP header extension" > > This reverts commit 28c7362bc485d22bdc8c744bc725022780187a96. > > Reason for revert: It breaks Linux64 Release (libfuzzer): > https://logs.chromium.org/logs/webrtc/buildbucket/cr-buildbucket.appspot.com/8921003137877469920/+/steps/compile/0/stdout > > Original change's description: > > Extend TransportSequenceNumber RTP header extension > > > > Extend TransportSequenceNumber RTP header extension to support > > feedback on sender request. > > > > Bug: webrtc:10262 > > Change-Id: Ibc1cf18162d15cd102e951c9dc697d8ea536ebb6 > > Reviewed-on: https://webrtc-review.googlesource.com/c/123233 > > Reviewed-by: Danil Chapovalov > > Reviewed-by: Alex Loiko > > Commit-Queue: Johannes Kron > > Cr-Commit-Position: refs/heads/master@{#26766} > > TBR=danilchap@webrtc.org,aleloi@webrtc.org,kron@webrtc.org > > Change-Id: Ie8a73f5fdffd99919ceaa1ae8911a1645f2077e9 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: webrtc:10262 > Reviewed-on: https://webrtc-review.googlesource.com/c/123522 > Reviewed-by: Mirko Bonadei > Commit-Queue: Mirko Bonadei > Cr-Commit-Position: refs/heads/master@{#26767} TBR=danilchap@webrtc.org,mbonadei@webrtc.org,aleloi@webrtc.org,kron@webrtc.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:10262 Change-Id: I0f854299a46c042cfbdf8b8cc8cd965a228142c8 Reviewed-on: https://webrtc-review.googlesource.com/c/123764 Reviewed-by: Johannes Kron Reviewed-by: Danil Chapovalov Reviewed-by: Alex Loiko Commit-Queue: Johannes Kron Cr-Commit-Position: refs/heads/master@{#26798} --- modules/rtp_rtcp/include/rtp_rtcp_defines.h | 1 + .../source/rtp_header_extension_map.cc | 1 + .../rtp_rtcp/source/rtp_header_extensions.cc | 80 +++++++++++++++++-- .../rtp_rtcp/source/rtp_header_extensions.h | 35 +++++++- .../rtp_rtcp/source/rtp_packet_received.cc | 3 + .../rtp_rtcp/source/rtp_packet_unittest.cc | 58 ++++++++++++++ modules/rtp_rtcp/source/rtp_utility.cc | 4 + test/fuzzers/BUILD.gn | 1 + test/fuzzers/rtp_packet_fuzzer.cc | 8 ++ 9 files changed, 181 insertions(+), 10 deletions(-) diff --git a/modules/rtp_rtcp/include/rtp_rtcp_defines.h b/modules/rtp_rtcp/include/rtp_rtcp_defines.h index 82328980e7..03dcda3c3c 100644 --- a/modules/rtp_rtcp/include/rtp_rtcp_defines.h +++ b/modules/rtp_rtcp/include/rtp_rtcp_defines.h @@ -55,6 +55,7 @@ enum RTPExtensionType : int { kRtpExtensionAbsoluteSendTime, kRtpExtensionVideoRotation, kRtpExtensionTransportSequenceNumber, + kRtpExtensionTransportSequenceNumber02, kRtpExtensionPlayoutDelay, kRtpExtensionVideoContentType, kRtpExtensionVideoTiming, diff --git a/modules/rtp_rtcp/source/rtp_header_extension_map.cc b/modules/rtp_rtcp/source/rtp_header_extension_map.cc index 4eec8d3cb8..b4aaf3ff58 100644 --- a/modules/rtp_rtcp/source/rtp_header_extension_map.cc +++ b/modules/rtp_rtcp/source/rtp_header_extension_map.cc @@ -35,6 +35,7 @@ constexpr ExtensionInfo kExtensions[] = { CreateExtensionInfo(), CreateExtensionInfo(), CreateExtensionInfo(), + CreateExtensionInfo(), CreateExtensionInfo(), CreateExtensionInfo(), CreateExtensionInfo(), diff --git a/modules/rtp_rtcp/source/rtp_header_extensions.cc b/modules/rtp_rtcp/source/rtp_header_extensions.cc index 38c2d5ac7d..e531a886c6 100644 --- a/modules/rtp_rtcp/source/rtp_header_extensions.cc +++ b/modules/rtp_rtcp/source/rtp_header_extensions.cc @@ -126,27 +126,93 @@ bool TransmissionOffset::Write(rtc::ArrayView data, int32_t rtp_time) { return true; } +// TransportSequenceNumber +// // 0 1 2 // 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ -// | ID | L=1 |transport wide sequence number | +// | ID | L=1 |transport-wide sequence number | // +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ constexpr RTPExtensionType TransportSequenceNumber::kId; constexpr uint8_t TransportSequenceNumber::kValueSizeBytes; constexpr const char TransportSequenceNumber::kUri[]; bool TransportSequenceNumber::Parse(rtc::ArrayView data, - uint16_t* value) { - if (data.size() != 2) + uint16_t* transport_sequence_number) { + if (data.size() != kValueSizeBytes) return false; - *value = ByteReader::ReadBigEndian(data.data()); + *transport_sequence_number = ByteReader::ReadBigEndian(data.data()); return true; } bool TransportSequenceNumber::Write(rtc::ArrayView data, - uint16_t value) { - RTC_DCHECK_EQ(data.size(), 2); - ByteWriter::WriteBigEndian(data.data(), value); + uint16_t transport_sequence_number) { + RTC_DCHECK_EQ(data.size(), ValueSize(transport_sequence_number)); + ByteWriter::WriteBigEndian(data.data(), transport_sequence_number); + return true; +} + +// TransportSequenceNumberV2 +// +// In addition to the format used for TransportSequencNumber, V2 also supports +// the following packet format where two extra bytes are used to specify that +// the sender requests immediate feedback. +// 0 1 2 3 +// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// | ID | L=3 |transport-wide sequence number |T| seq count | +// +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ +// |seq count cont.| +// +-+-+-+-+-+-+-+-+ +// +// The bit |T| determines whether the feedback should include timing information +// or not and |seq_count| determines how many additional packets the feedback +// packet should cover. +constexpr RTPExtensionType TransportSequenceNumberV2::kId; +constexpr uint8_t TransportSequenceNumberV2::kValueSizeBytesWithFeedbackRequest; +constexpr const char TransportSequenceNumberV2::kUri[]; +constexpr uint16_t TransportSequenceNumberV2::kIncludeTimestampsBit; + +bool TransportSequenceNumberV2::Parse( + rtc::ArrayView data, + uint16_t* transport_sequence_number, + absl::optional* feedback_request) { + if (data.size() != TransportSequenceNumber::kValueSizeBytes && + data.size() != kValueSizeBytesWithFeedbackRequest) + return false; + + *transport_sequence_number = ByteReader::ReadBigEndian(data.data()); + + if (data.size() == kValueSizeBytesWithFeedbackRequest) { + uint16_t feedback_request_raw = + ByteReader::ReadBigEndian(data.data() + 2); + bool include_timestamps = + (feedback_request_raw & kIncludeTimestampsBit) != 0; + uint16_t sequence_count = feedback_request_raw & ~kIncludeTimestampsBit; + *feedback_request = {include_timestamps, sequence_count}; + } else { + *feedback_request = absl::nullopt; + } + return true; +} + +bool TransportSequenceNumberV2::Write( + rtc::ArrayView data, + uint16_t transport_sequence_number, + const absl::optional& feedback_request) { + RTC_DCHECK_EQ(data.size(), + ValueSize(transport_sequence_number, feedback_request)); + + ByteWriter::WriteBigEndian(data.data(), transport_sequence_number); + + if (feedback_request) { + RTC_DCHECK_GE(feedback_request->sequence_count, 0); + RTC_DCHECK_LT(feedback_request->sequence_count, kIncludeTimestampsBit); + uint16_t feedback_request_raw = + feedback_request->sequence_count | + (feedback_request->include_timestamps ? kIncludeTimestampsBit : 0); + ByteWriter::WriteBigEndian(data.data() + 2, feedback_request_raw); + } return true; } diff --git a/modules/rtp_rtcp/source/rtp_header_extensions.h b/modules/rtp_rtcp/source/rtp_header_extensions.h index 9f4a28b701..e37388a3cd 100644 --- a/modules/rtp_rtcp/source/rtp_header_extensions.h +++ b/modules/rtp_rtcp/source/rtp_header_extensions.h @@ -81,9 +81,38 @@ class TransportSequenceNumber { static constexpr const char kUri[] = "http://www.ietf.org/id/" "draft-holmer-rmcat-transport-wide-cc-extensions-01"; - static bool Parse(rtc::ArrayView data, uint16_t* value); - static size_t ValueSize(uint16_t value) { return kValueSizeBytes; } - static bool Write(rtc::ArrayView data, uint16_t value); + static bool Parse(rtc::ArrayView data, + uint16_t* transport_sequence_number); + static size_t ValueSize(uint16_t /*transport_sequence_number*/) { + return kValueSizeBytes; + } + static bool Write(rtc::ArrayView data, + uint16_t transport_sequence_number); +}; + +class TransportSequenceNumberV2 { + public: + static constexpr RTPExtensionType kId = + kRtpExtensionTransportSequenceNumber02; + static constexpr uint8_t kValueSizeBytesWithFeedbackRequest = 4; + static constexpr const char kUri[] = + "http://www.ietf.org/id/" + "draft-holmer-rmcat-transport-wide-cc-extensions-02"; + static bool Parse(rtc::ArrayView data, + uint16_t* transport_sequence_number, + absl::optional* feedback_request); + static size_t ValueSize( + uint16_t /*transport_sequence_number*/, + const absl::optional& feedback_request) { + return feedback_request ? kValueSizeBytesWithFeedbackRequest + : TransportSequenceNumber::kValueSizeBytes; + } + static bool Write(rtc::ArrayView data, + uint16_t transport_sequence_number, + const absl::optional& feedback_request); + + private: + static constexpr uint16_t kIncludeTimestampsBit = 1 << 15; }; class VideoOrientation { diff --git a/modules/rtp_rtcp/source/rtp_packet_received.cc b/modules/rtp_rtcp/source/rtp_packet_received.cc index f80fad68e0..2a36b7bd1b 100644 --- a/modules/rtp_rtcp/source/rtp_packet_received.cc +++ b/modules/rtp_rtcp/source/rtp_packet_received.cc @@ -52,6 +52,9 @@ void RtpPacketReceived::GetHeader(RTPHeader* header) const { header->extension.hasAbsoluteSendTime = GetExtension(&header->extension.absoluteSendTime); header->extension.hasTransportSequenceNumber = + GetExtension( + &header->extension.transportSequenceNumber, + &header->extension.feedback_request) || GetExtension( &header->extension.transportSequenceNumber); header->extension.hasAudioLevel = GetExtension( diff --git a/modules/rtp_rtcp/source/rtp_packet_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_unittest.cc index 0d24272f5f..4700a776f5 100644 --- a/modules/rtp_rtcp/source/rtp_packet_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_unittest.cc @@ -827,4 +827,62 @@ TEST(RtpPacketTest, CreateAndParseColorSpaceExtensionWithoutHdrMetadata) { TestCreateAndParseColorSpaceExtension(/*with_hdr_metadata=*/false); } +TEST(RtpPacketTest, CreateAndParseTransportSequenceNumber) { + // Create a packet with transport sequence number extension populated. + RtpPacketToSend::ExtensionManager extensions; + constexpr int kExtensionId = 1; + extensions.Register(kExtensionId); + RtpPacketToSend send_packet(&extensions); + send_packet.SetPayloadType(kPayloadType); + send_packet.SetSequenceNumber(kSeqNum); + send_packet.SetTimestamp(kTimestamp); + send_packet.SetSsrc(kSsrc); + + constexpr int kTransportSequenceNumber = 12345; + send_packet.SetExtension(kTransportSequenceNumber); + + // Serialize the packet and then parse it again. + RtpPacketReceived receive_packet(&extensions); + EXPECT_TRUE(receive_packet.Parse(send_packet.Buffer())); + + uint16_t received_transport_sequeunce_number; + EXPECT_TRUE(receive_packet.GetExtension( + &received_transport_sequeunce_number)); + EXPECT_EQ(received_transport_sequeunce_number, kTransportSequenceNumber); +} + +TEST(RtpPacketTest, CreateAndParseTransportSequenceNumberWithFeedbackRequest) { + // Create a packet with TransportSequenceNumberV2 extension populated. + RtpPacketToSend::ExtensionManager extensions; + constexpr int kExtensionId = 1; + extensions.Register(kExtensionId); + RtpPacketToSend send_packet(&extensions); + send_packet.SetPayloadType(kPayloadType); + send_packet.SetSequenceNumber(kSeqNum); + send_packet.SetTimestamp(kTimestamp); + send_packet.SetSsrc(kSsrc); + + constexpr int kTransportSequenceNumber = 12345; + constexpr absl::optional kFeedbackRequest = + FeedbackRequest{/*include_timestamps=*/true, /*sequence_count=*/3}; + send_packet.SetExtension(kTransportSequenceNumber, + kFeedbackRequest); + + // Serialize the packet and then parse it again. + RtpPacketReceived receive_packet(&extensions); + EXPECT_TRUE(receive_packet.Parse(send_packet.Buffer())); + + // Parse transport sequence number and feedback request. + uint16_t received_transport_sequeunce_number; + absl::optional received_feedback_request; + EXPECT_TRUE(receive_packet.GetExtension( + &received_transport_sequeunce_number, &received_feedback_request)); + EXPECT_EQ(received_transport_sequeunce_number, kTransportSequenceNumber); + ASSERT_TRUE(received_feedback_request); + EXPECT_EQ(received_feedback_request->include_timestamps, + kFeedbackRequest->include_timestamps); + EXPECT_EQ(received_feedback_request->sequence_count, + kFeedbackRequest->sequence_count); +} + } // namespace webrtc diff --git a/modules/rtp_rtcp/source/rtp_utility.cc b/modules/rtp_rtcp/source/rtp_utility.cc index 12038237b7..8811bf1d21 100644 --- a/modules/rtp_rtcp/source/rtp_utility.cc +++ b/modules/rtp_rtcp/source/rtp_utility.cc @@ -433,6 +433,10 @@ void RtpHeaderParser::ParseOneByteExtensionHeader( header->extension.hasTransportSequenceNumber = true; break; } + case kRtpExtensionTransportSequenceNumber02: + RTC_LOG(WARNING) << "TransportSequenceNumberV2 unsupported by rtp " + "header parser."; + break; case kRtpExtensionPlayoutDelay: { if (len != 2) { RTC_LOG(LS_WARNING) << "Incorrect playout delay len: " << len; diff --git a/test/fuzzers/BUILD.gn b/test/fuzzers/BUILD.gn index 797c27d917..6e723e1548 100644 --- a/test/fuzzers/BUILD.gn +++ b/test/fuzzers/BUILD.gn @@ -230,6 +230,7 @@ webrtc_fuzzer_test("rtp_packet_fuzzer") { ] deps = [ "../../modules/rtp_rtcp:rtp_rtcp_format", + "//third_party/abseil-cpp/absl/types:optional", ] seed_corpus = "corpora/rtp-corpus" } diff --git a/test/fuzzers/rtp_packet_fuzzer.cc b/test/fuzzers/rtp_packet_fuzzer.cc index d2de72d5dc..730124cb9a 100644 --- a/test/fuzzers/rtp_packet_fuzzer.cc +++ b/test/fuzzers/rtp_packet_fuzzer.cc @@ -10,6 +10,7 @@ #include +#include "absl/types/optional.h" #include "modules/rtp_rtcp/include/rtp_header_extension_map.h" #include "modules/rtp_rtcp/source/rtp_generic_frame_descriptor_extension.h" #include "modules/rtp_rtcp/source/rtp_header_extensions.h" @@ -86,6 +87,13 @@ void FuzzOneInput(const uint8_t* data, size_t size) { uint16_t seqnum; packet.GetExtension(&seqnum); break; + case kRtpExtensionTransportSequenceNumber02: { + uint16_t seqnum; + absl::optional feedback_request; + packet.GetExtension(&seqnum, + &feedback_request); + break; + } case kRtpExtensionPlayoutDelay: PlayoutDelay playout; packet.GetExtension(&playout);