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 <kron@webrtc.org>
Reviewed-by: Karl Wiberg <kwiberg@webrtc.org>
Reviewed-by: Sebastian Jansson <srte@webrtc.org>
Reviewed-by: Danil Chapovalov <danilchap@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26985}diff --git a/api/rtp_headers.h b/api/rtp_headers.h
index 2621db9..8ab560c 100644
--- a/api/rtp_headers.h
+++ b/api/rtp_headers.h
@@ -88,7 +88,8 @@
// 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 b1d7e70..70eef90 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::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 9cce167..71b6659 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 @@
}));
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 @@
}));
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 @@
}));
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 e531a88..1d76ff3 100644
--- a/modules/rtp_rtcp/source/rtp_header_extensions.cc
+++ b/modules/rtp_rtcp/source/rtp_header_extensions.cc
@@ -166,10 +166,13 @@
// +-+-+-+-+-+-+-+-+
//
// 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 @@
rtc::ArrayView<const uint8_t> data,
uint16_t* transport_sequence_number,
absl::optional<FeedbackRequest>* feedback_request) {
- if (data.size() != TransportSequenceNumber::kValueSizeBytes &&
- data.size() != kValueSizeBytesWithFeedbackRequest)
+ if (data.size() != kValueSizeBytes &&
+ data.size() != kValueSizeBytesWithoutFeedbackRequest)
return false;
*transport_sequence_number = ByteReader<uint16_t>::ReadBigEndian(data.data());
- if (data.size() == kValueSizeBytesWithFeedbackRequest) {
+ *feedback_request = absl::nullopt;
+ if (data.size() == kValueSizeBytes) {
uint16_t feedback_request_raw =
ByteReader<uint16_t>::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 e37388a..e36c83f 100644
--- a/modules/rtp_rtcp/source/rtp_header_extensions.h
+++ b/modules/rtp_rtcp/source/rtp_header_extensions.h
@@ -94,7 +94,8 @@
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 @@
static size_t ValueSize(
uint16_t /*transport_sequence_number*/,
const absl::optional<FeedbackRequest>& feedback_request) {
- return feedback_request ? kValueSizeBytesWithFeedbackRequest
- : TransportSequenceNumber::kValueSizeBytes;
+ return feedback_request ? kValueSizeBytes
+ : kValueSizeBytesWithoutFeedbackRequest;
}
static bool Write(rtc::ArrayView<uint8_t> 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 4700a77..84f5020 100644
--- a/modules/rtp_rtcp/source/rtp_packet_unittest.cc
+++ b/modules/rtp_rtcp/source/rtp_packet_unittest.cc
@@ -851,7 +851,74 @@
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<TransportSequenceNumberV2>(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<TransportSequenceNumberV2>(kTransportSequenceNumber,
+ absl::nullopt);
+ EXPECT_EQ(send_packet.GetRawExtension<TransportSequenceNumberV2>().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<FeedbackRequest> received_feedback_request;
+ EXPECT_TRUE(receive_packet.GetExtension<TransportSequenceNumberV2>(
+ &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<TransportSequenceNumberV2>(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<FeedbackRequest> kNoFeedbackRequest =
+ FeedbackRequest{/*include_timestamps=*/false, /*sequence_count=*/0};
+ send_packet.ReserveExtension<TransportSequenceNumberV2>();
+ send_packet.SetExtension<TransportSequenceNumberV2>(kTransportSequenceNumber,
+ kNoFeedbackRequest);
+ EXPECT_EQ(send_packet.GetRawExtension<TransportSequenceNumberV2>().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<FeedbackRequest> received_feedback_request;
+ EXPECT_TRUE(receive_packet.GetExtension<TransportSequenceNumberV2>(
+ &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;