Revert "dcsctp: Avoid bundling FORWARD-TSN and DATA chunks"

This proved to be not very efficient unfortunately, so revert it and
keep bundling FORWARD-TSN with other packets to be more efficient.

https://github.com/sctplab/usrsctp/issues/597 is still unresolved.

Note that this is not a clean revert; The logic to rate limit the
sending of FORWARD-TSN is kept, as it still makes sense.

This partly reverts commit 0ca62e3752149ad37f73bf074db0a5f8fcaf6585.

Bug: webrtc:12961
Change-Id: I42728434290e7ece19e9c23f24ef6f3d3b171315
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/259520
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#36584}

net/dcsctp/socket/dcsctp_socket_test.cc

@@ -27,7 +27,6 @@
 #include "net/dcsctp/packet/chunk/data_chunk.h"
 #include "net/dcsctp/packet/chunk/data_common.h"
 #include "net/dcsctp/packet/chunk/error_chunk.h"
-#include "net/dcsctp/packet/chunk/forward_tsn_chunk.h"
 #include "net/dcsctp/packet/chunk/heartbeat_ack_chunk.h"
 #include "net/dcsctp/packet/chunk/heartbeat_request_chunk.h"
 #include "net/dcsctp/packet/chunk/idata_chunk.h"
@@ -2023,89 +2022,6 @@ TEST_P(DcSctpSocketParametrizedTest, SendsOnlyLargePackets) {
   MaybeHandoverSocketAndSendMessage(a, std::move(z));
 }
 
-TEST_P(DcSctpSocketParametrizedTest, DoesntBundleForwardTsnWithData) {
-  SocketUnderTest a("A");
-  auto z = std::make_unique<SocketUnderTest>("Z");
-
-  ConnectSockets(a, *z);
-  z = MaybeHandoverSocket(std::move(z));
-
-  // Force an RTT measurement using heartbeats.
-  AdvanceTime(a, *z, a.options.heartbeat_interval);
-
-  // HEARTBEAT
-  std::vector<uint8_t> hb_req_a = a.cb.ConsumeSentPacket();
-  std::vector<uint8_t> hb_req_z = z->cb.ConsumeSentPacket();
-
-  constexpr DurationMs kRtt = DurationMs(80);
-  AdvanceTime(a, *z, kRtt);
-  z->socket.ReceivePacket(hb_req_a);
-  a.socket.ReceivePacket(hb_req_z);
-
-  // HEARTBEAT_ACK
-  a.socket.ReceivePacket(z->cb.ConsumeSentPacket());
-  z->socket.ReceivePacket(a.cb.ConsumeSentPacket());
-
-  SendOptions send_options;
-  send_options.max_retransmissions = 0;
-  std::vector<uint8_t> payload(a.options.mtu - 100);
-
-  // Send an initial message that is received, but the SACK was lost
-  a.socket.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options);
-  // DATA
-  z->socket.ReceivePacket(a.cb.ConsumeSentPacket());
-  // SACK (lost)
-  std::vector<uint8_t> sack = z->cb.ConsumeSentPacket();
-
-  // Queue enough messages to fill the congestion window.
-  do {
-    a.socket.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options);
-  } while (!a.cb.ConsumeSentPacket().empty());
-
-  // Enqueue at least one more.
-  a.socket.Send(DcSctpMessage(StreamID(1), PPID(51), payload), send_options);
-
-  // Let all of them expire by T3-RTX and inspect what's sent.
-  AdvanceTime(a, *z, a.options.rto_initial);
-
-  std::vector<uint8_t> sent1 = a.cb.ConsumeSentPacket();
-  ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet1, SctpPacket::Parse(sent1));
-
-  EXPECT_THAT(packet1.descriptors(), SizeIs(1));
-  EXPECT_EQ(packet1.descriptors()[0].type, ForwardTsnChunk::kType);
-
-  std::vector<uint8_t> sent2 = a.cb.ConsumeSentPacket();
-  ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet2, SctpPacket::Parse(sent2));
-
-  EXPECT_GE(packet2.descriptors().size(), 1u);
-  EXPECT_EQ(packet2.descriptors()[0].type, DataChunk::kType);
-
-  // Drop all remaining packets that A has sent.
-  while (!a.cb.ConsumeSentPacket().empty()) {
-  }
-
-  // Replay the SACK, and see if a FORWARD-TSN is sent again.
-  a.socket.ReceivePacket(sack);
-
-  // It shouldn't be sent as not enough time has passed yet. Instead, more
-  // DATA chunks are sent, that are in the queue.
-  std::vector<uint8_t> sent3 = a.cb.ConsumeSentPacket();
-  ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet3, SctpPacket::Parse(sent3));
-
-  EXPECT_GE(packet2.descriptors().size(), 1u);
-  EXPECT_EQ(packet3.descriptors()[0].type, DataChunk::kType);
-
-  // Now let RTT time pass, to allow a FORWARD-TSN to be sent again.
-  AdvanceTime(a, *z, kRtt);
-  a.socket.ReceivePacket(sack);
-
-  std::vector<uint8_t> sent4 = a.cb.ConsumeSentPacket();
-  ASSERT_HAS_VALUE_AND_ASSIGN(SctpPacket packet4, SctpPacket::Parse(sent4));
-
-  EXPECT_THAT(packet4.descriptors(), SizeIs(1));
-  EXPECT_EQ(packet4.descriptors()[0].type, ForwardTsnChunk::kType);
-}
-
 TEST(DcSctpSocketTest, SendMessagesAfterHandover) {
   SocketUnderTest a("A");
   auto z = std::make_unique<SocketUnderTest>("Z");

net/dcsctp/socket/transmission_control_block.cc

@@ -104,9 +104,6 @@ void TransmissionControlBlock::MaybeSendForwardTsn(SctpPacket::Builder& builder,
 
 void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder,
                                                    TimeMs now) {
-  // FORWARD-TSNs are sent as separate packets to avoid bugs.webrtc.org/12961.
-  MaybeSendForwardTsn(builder, now);
-
   for (int packet_idx = 0;
        packet_idx < options_.max_burst && retransmission_queue_.can_send_data();
        ++packet_idx) {
@@ -131,6 +128,7 @@ void TransmissionControlBlock::SendBufferedPackets(SctpPacket::Builder& builder,
         builder.Add(data_tracker_.CreateSelectiveAck(
             reassembly_queue_.remaining_bytes()));
       }
+      MaybeSendForwardTsn(builder, now);
       absl::optional<ReConfigChunk> reconfig =
           stream_reset_handler_.MakeStreamResetRequest();
       if (reconfig.has_value()) {