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}

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;
 };
 

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);

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);
 }

modules/rtp_rtcp/source/rtp_header_extensions.cc

@@ -166,10 +166,13 @@ bool TransportSequenceNumber::Write(rtc::ArrayView<uint8_t> data,
 //  +-+-+-+-+-+-+-+-+
 //
 // The bit |T| determines whether the feedback should include timing information
-// or not and |seq_count| determines how many additional packets the feedback
+// or not and |seq_count| determines how many packets the feedback packet should
-// packet should cover.
+// 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<const uint8_t> data,
     uint16_t* transport_sequence_number,
     absl::optional<FeedbackRequest>* feedback_request) {
-  if (data.size() != TransportSequenceNumber::kValueSizeBytes &&
+  if (data.size() != kValueSizeBytes &&
-      data.size() != kValueSizeBytesWithFeedbackRequest)
+      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;
+
+    // If |sequence_count| is zero no feedback is requested.
+    if (sequence_count != 0) {
       *feedback_request = {include_timestamps, sequence_count};
-  } else {
+    }
-    *feedback_request = absl::nullopt;
   }
   return true;
 }

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<FeedbackRequest>& feedback_request) {
-    return feedback_request ? kValueSizeBytesWithFeedbackRequest
+    return feedback_request ? kValueSizeBytes
-                            : TransportSequenceNumber::kValueSizeBytes;
+                            : kValueSizeBytesWithoutFeedbackRequest;
   }
   static bool Write(rtc::ArrayView<uint8_t> data,
                     uint16_t transport_sequence_number,

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<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;