From 0da25a1c8e5de3b2f3c9e83280c96ae1d2ccb408 Mon Sep 17 00:00:00 2001 From: Johannes Kron Date: Wed, 6 Mar 2019 09:34:13 +0100 Subject: [PATCH] Update TransportSequenceNumberV2 extension to support fixed size The initial implementation forced the sender to use different sizes of the RTP header extension depending on if a feedback request is included or not. This can be a problem if the RTP header is pre- allocated. This CL changes this so that a static size of 4 bytes can be used for the TransportSequenceNumberV2 RTP header extension. The change in the protocol to get this to work is that FeedbackRequest::sequence_count == 0 means that no feedback is requested, and FeedbackRequest::sequence_count == 1 means that feedback is requested for the current packet only. Bug: webrtc:10262 Change-Id: Ia5134b3daf49f8a5b89f6c717894f6e055f39c8e Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/125420 Commit-Queue: Johannes Kron Reviewed-by: Karl Wiberg Reviewed-by: Sebastian Jansson Reviewed-by: Danil Chapovalov Cr-Commit-Position: refs/heads/master@{#26985} --- api/rtp_headers.h | 3 +- .../remote_estimator_proxy.cc | 5 +- .../remote_estimator_proxy_unittest.cc | 6 +- .../rtp_rtcp/source/rtp_header_extensions.cc | 24 ++++--- .../rtp_rtcp/source/rtp_header_extensions.h | 7 +- .../rtp_rtcp/source/rtp_packet_unittest.cc | 69 ++++++++++++++++++- 6 files changed, 96 insertions(+), 18 deletions(-) diff --git a/api/rtp_headers.h b/api/rtp_headers.h index 2621db9bd7..8ab560c95c 100644 --- a/api/rtp_headers.h +++ b/api/rtp_headers.h @@ -88,7 +88,8 @@ struct FeedbackRequest { // should be included. bool include_timestamps; // Include feedback of received packets in the range [sequence_number - - // sequence_count, sequence_number]. + // sequence_count + 1, sequence_number]. That is, no feedback will be sent if + // sequence_count is zero. int sequence_count; }; diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc index b1d7e70778..70eef90f77 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy.cc @@ -197,10 +197,13 @@ void RemoteEstimatorProxy::SendPeriodicFeedbacks() { void RemoteEstimatorProxy::SendFeedbackOnRequest( int64_t sequence_number, const FeedbackRequest& feedback_request) { + if (feedback_request.sequence_count == 0) { + return; + } rtcp::TransportFeedback feedback_packet(feedback_request.include_timestamps); int64_t first_sequence_number = - sequence_number - feedback_request.sequence_count; + sequence_number - feedback_request.sequence_count + 1; auto begin_iterator = packet_arrival_times_.lower_bound(first_sequence_number); auto end_iterator = packet_arrival_times_.upper_bound(sequence_number); diff --git a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc index 9cce167e86..71b6659993 100644 --- a/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc +++ b/modules/remote_bitrate_estimator/remote_estimator_proxy_unittest.cc @@ -377,7 +377,7 @@ TEST_F(RemoteEstimatorProxyOnRequestTest, RequestSinglePacketFeedback) { })); constexpr FeedbackRequest kSinglePacketFeedbackRequest = { - /*include_timestamps=*/true, /*sequence_count=*/0}; + /*include_timestamps=*/true, /*sequence_count=*/1}; IncomingPacket(kBaseSeq + 3, kBaseTimeMs + 3 * kMaxSmallDeltaMs, kSinglePacketFeedbackRequest); } @@ -407,7 +407,7 @@ TEST_F(RemoteEstimatorProxyOnRequestTest, RequestLastFivePacketFeedback) { })); constexpr FeedbackRequest kFivePacketsFeedbackRequest = { - /*include_timestamps=*/true, /*sequence_count=*/4}; + /*include_timestamps=*/true, /*sequence_count=*/5}; IncomingPacket(kBaseSeq + i, kBaseTimeMs + i * kMaxSmallDeltaMs, kFivePacketsFeedbackRequest); } @@ -436,7 +436,7 @@ TEST_F(RemoteEstimatorProxyOnRequestTest, })); constexpr FeedbackRequest kFivePacketsFeedbackRequest = { - /*include_timestamps=*/true, /*sequence_count=*/4}; + /*include_timestamps=*/true, /*sequence_count=*/5}; IncomingPacket(kBaseSeq + i, kBaseTimeMs + i * kMaxSmallDeltaMs, kFivePacketsFeedbackRequest); } diff --git a/modules/rtp_rtcp/source/rtp_header_extensions.cc b/modules/rtp_rtcp/source/rtp_header_extensions.cc index e531a886c6..1d76ff3f1e 100644 --- a/modules/rtp_rtcp/source/rtp_header_extensions.cc +++ b/modules/rtp_rtcp/source/rtp_header_extensions.cc @@ -166,10 +166,13 @@ bool TransportSequenceNumber::Write(rtc::ArrayView data, // +-+-+-+-+-+-+-+-+ // // 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. +// or not and |seq_count| determines how many packets the feedback packet should +// cover including the current packet. If |seq_count| is zero no feedback is +// requested. constexpr RTPExtensionType TransportSequenceNumberV2::kId; -constexpr uint8_t TransportSequenceNumberV2::kValueSizeBytesWithFeedbackRequest; +constexpr uint8_t TransportSequenceNumberV2::kValueSizeBytes; +constexpr uint8_t + TransportSequenceNumberV2::kValueSizeBytesWithoutFeedbackRequest; constexpr const char TransportSequenceNumberV2::kUri[]; constexpr uint16_t TransportSequenceNumberV2::kIncludeTimestampsBit; @@ -177,21 +180,24 @@ bool TransportSequenceNumberV2::Parse( rtc::ArrayView data, uint16_t* transport_sequence_number, absl::optional* feedback_request) { - if (data.size() != TransportSequenceNumber::kValueSizeBytes && - data.size() != kValueSizeBytesWithFeedbackRequest) + if (data.size() != kValueSizeBytes && + data.size() != kValueSizeBytesWithoutFeedbackRequest) return false; *transport_sequence_number = ByteReader::ReadBigEndian(data.data()); - if (data.size() == kValueSizeBytesWithFeedbackRequest) { + *feedback_request = absl::nullopt; + if (data.size() == kValueSizeBytes) { 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; + + // If |sequence_count| is zero no feedback is requested. + if (sequence_count != 0) { + *feedback_request = {include_timestamps, sequence_count}; + } } return true; } diff --git a/modules/rtp_rtcp/source/rtp_header_extensions.h b/modules/rtp_rtcp/source/rtp_header_extensions.h index e37388a3cd..e36c83f31b 100644 --- a/modules/rtp_rtcp/source/rtp_header_extensions.h +++ b/modules/rtp_rtcp/source/rtp_header_extensions.h @@ -94,7 +94,8 @@ class TransportSequenceNumberV2 { public: static constexpr RTPExtensionType kId = kRtpExtensionTransportSequenceNumber02; - static constexpr uint8_t kValueSizeBytesWithFeedbackRequest = 4; + static constexpr uint8_t kValueSizeBytes = 4; + static constexpr uint8_t kValueSizeBytesWithoutFeedbackRequest = 2; static constexpr const char kUri[] = "http://www.ietf.org/id/" "draft-holmer-rmcat-transport-wide-cc-extensions-02"; @@ -104,8 +105,8 @@ class TransportSequenceNumberV2 { static size_t ValueSize( uint16_t /*transport_sequence_number*/, const absl::optional& feedback_request) { - return feedback_request ? kValueSizeBytesWithFeedbackRequest - : TransportSequenceNumber::kValueSizeBytes; + return feedback_request ? kValueSizeBytes + : kValueSizeBytesWithoutFeedbackRequest; } static bool Write(rtc::ArrayView data, uint16_t transport_sequence_number, diff --git a/modules/rtp_rtcp/source/rtp_packet_unittest.cc b/modules/rtp_rtcp/source/rtp_packet_unittest.cc index 4700a776f5..84f5020ec3 100644 --- a/modules/rtp_rtcp/source/rtp_packet_unittest.cc +++ b/modules/rtp_rtcp/source/rtp_packet_unittest.cc @@ -851,7 +851,74 @@ TEST(RtpPacketTest, CreateAndParseTransportSequenceNumber) { EXPECT_EQ(received_transport_sequeunce_number, kTransportSequenceNumber); } -TEST(RtpPacketTest, CreateAndParseTransportSequenceNumberWithFeedbackRequest) { +TEST(RtpPacketTest, CreateAndParseTransportSequenceNumberV2) { + // Create a packet with transport sequence number V2 extension populated. + // No feedback request means that the extension will be two bytes unless it's + // pre-allocated. + 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, + absl::nullopt); + EXPECT_EQ(send_packet.GetRawExtension().size(), + 2u); + + // 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; + absl::optional received_feedback_request; + EXPECT_TRUE(receive_packet.GetExtension( + &received_transport_sequeunce_number, &received_feedback_request)); + EXPECT_EQ(received_transport_sequeunce_number, kTransportSequenceNumber); + EXPECT_FALSE(received_feedback_request); +} + +TEST(RtpPacketTest, CreateAndParseTransportSequenceNumberV2Preallocated) { + // Create a packet with transport sequence number V2 extension populated. + // No feedback request means that the extension could be two bytes, but since + // it's pre-allocated we don't know if it is with or without feedback request + // therefore the size is four bytes. + 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 kNoFeedbackRequest = + FeedbackRequest{/*include_timestamps=*/false, /*sequence_count=*/0}; + send_packet.ReserveExtension(); + send_packet.SetExtension(kTransportSequenceNumber, + kNoFeedbackRequest); + EXPECT_EQ(send_packet.GetRawExtension().size(), + 4u); + + // 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; + absl::optional received_feedback_request; + EXPECT_TRUE(receive_packet.GetExtension( + &received_transport_sequeunce_number, &received_feedback_request)); + EXPECT_EQ(received_transport_sequeunce_number, kTransportSequenceNumber); + EXPECT_FALSE(received_feedback_request); +} + +TEST(RtpPacketTest, + CreateAndParseTransportSequenceNumberV2WithFeedbackRequest) { // Create a packet with TransportSequenceNumberV2 extension populated. RtpPacketToSend::ExtensionManager extensions; constexpr int kExtensionId = 1;