Dont signal ReadyToSend in RtpTransport::SendPacket

Before this cl, ReadyToSend signaled false if sending a packet failed and transport->GetError() returns ECONN.
ECONN may be reported by the TCP connection (TcpConnection) if the remote closed the connection. TcpConnection will attempt to reconnect and should change the writable state if it fail.
Changing the state in the context of sending packets may cause recursive
calls and seems to cause problems with incorrect states.
It is simpler if RtpTransport::SendPacket ignore these failures and
upper layers treat these lost packets similar to if the packets had been
lost via UDP.
For safety, this change can be reverted by field trial WebRTC-SetReadyToSendFalseIfSendFail/Enabled/.

Bug: webrtc:361124449 b/359989715
Change-Id: I8e7016dfb4301862286215c4512aa8ac03a16685
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/360120
Reviewed-by: Harald Alvestrand <hta@webrtc.org>
Commit-Queue: Per Kjellander <perkj@webrtc.org>
Reviewed-by: Jonas Oreland <jonaso@webrtc.org>
Cr-Commit-Position: refs/heads/main@{#42868}

experiments/field_trials.py

@@ -134,6 +134,9 @@ ACTIVE_FIELD_TRIALS: FrozenSet[FieldTrial] = frozenset([
     FieldTrial('WebRTC-RtcEventLogEncodeNetEqSetMinimumDelayKillSwitch',
                42225058,
                date(2024, 4, 1)),
+    FieldTrial('WebRTC-SetReadyToSendFalseIfSendFail',
+               361124449,
+               date(2024, 12, 1)),
     FieldTrial('WebRTC-SetCodecPreferences-ReceiveOnlyFilterInsteadOfThrow',
                40644399,
                date(2024, 12, 1)),

p2p/base/fake_packet_transport.h

@@ -61,7 +61,7 @@ class FakePacketTransport : public PacketTransportInternal {
                  size_t len,
                  const PacketOptions& options,
                  int flags) override {
-    if (!dest_) {
+    if (!dest_ || error_ != 0) {
       return -1;
     }
     CopyOnWriteBuffer packet(data, len);

pc/BUILD.gn

@@ -509,6 +509,7 @@ rtc_source_set("rtp_transport") {
     ":rtp_transport_internal",
     ":session_description",
     "../api:array_view",
+    "../api:field_trials_view",
     "../api/task_queue:pending_task_safety_flag",
     "../api/units:timestamp",
     "../call:rtp_receiver",

pc/channel_unittest.cc

@@ -340,7 +340,7 @@ class ChannelTest : public ::testing::Test, public sigslot::has_slots<> {
       rtc::PacketTransportInternal* rtp_packet_transport,
       rtc::PacketTransportInternal* rtcp_packet_transport) {
     auto rtp_transport = std::make_unique<webrtc::RtpTransport>(
-        rtcp_packet_transport == nullptr);
+        rtcp_packet_transport == nullptr, field_trials_);
 
     SendTask(network_thread_,
              [&rtp_transport, rtp_packet_transport, rtcp_packet_transport] {

pc/jsep_transport_controller.cc

@@ -504,8 +504,8 @@ JsepTransportController::CreateUnencryptedRtpTransport(
     rtc::PacketTransportInternal* rtp_packet_transport,
     rtc::PacketTransportInternal* rtcp_packet_transport) {
   RTC_DCHECK_RUN_ON(network_thread_);
-  auto unencrypted_rtp_transport =
-      std::make_unique<RtpTransport>(rtcp_packet_transport == nullptr);
+  auto unencrypted_rtp_transport = std::make_unique<RtpTransport>(
+      rtcp_packet_transport == nullptr, env_.field_trials());
   unencrypted_rtp_transport->SetRtpPacketTransport(rtp_packet_transport);
   if (rtcp_packet_transport) {
     unencrypted_rtp_transport->SetRtcpPacketTransport(rtcp_packet_transport);

pc/rtp_transport.cc

@@ -15,7 +15,6 @@
 #include <cstdint>
 #include <utility>
 
-#include "absl/strings/string_view.h"
 #include "api/array_view.h"
 #include "api/units/timestamp.h"
 #include "media/base/rtp_utils.h"
@@ -80,8 +79,6 @@ void RtpTransport::SetRtpPacketTransport(
   }
 
   rtp_packet_transport_ = new_packet_transport;
-  // Assumes the transport is ready to send if it is writable. If we are wrong,
-  // ready to send will be updated the next time we try to send.
   SetReadyToSend(false,
                  rtp_packet_transport_ && rtp_packet_transport_->writable());
 }
@@ -119,8 +116,7 @@ void RtpTransport::SetRtcpPacketTransport(
   }
   rtcp_packet_transport_ = new_packet_transport;
 
-  // Assumes the transport is ready to send if it is writable. If we are wrong,
-  // ready to send will be updated the next time we try to send.
+  // Assumes the transport is ready to send if it is writable.
   SetReadyToSend(true,
                  rtcp_packet_transport_ && rtcp_packet_transport_->writable());
 }
@@ -154,10 +150,14 @@ bool RtpTransport::SendPacket(bool rtcp,
   int ret = transport->SendPacket(packet->cdata<char>(), packet->size(),
                                   options, flags);
   if (ret != static_cast<int>(packet->size())) {
+    if (set_ready_to_send_false_if_send_fail_) {
+      // TODO: webrtc:361124449 - Remove SetReadyToSend if field trial
+      // WebRTC-SetReadyToSendFalseIfSendFail succeed 2024-12-01.
       if (transport->GetError() == ENOTCONN) {
         RTC_LOG(LS_WARNING) << "Got ENOTCONN from transport.";
         SetReadyToSend(rtcp, false);
       }
+    }
     return false;
   }
   return true;

pc/rtp_transport.h

@@ -17,6 +17,7 @@
 #include <string>
 
 #include "absl/types/optional.h"
+#include "api/field_trials_view.h"
 #include "api/task_queue/pending_task_safety_flag.h"
 #include "api/units/timestamp.h"
 #include "call/rtp_demuxer.h"
@@ -47,8 +48,10 @@ class RtpTransport : public RtpTransportInternal {
   RtpTransport(const RtpTransport&) = delete;
   RtpTransport& operator=(const RtpTransport&) = delete;
 
-  explicit RtpTransport(bool rtcp_mux_enabled)
-      : rtcp_mux_enabled_(rtcp_mux_enabled) {}
+  RtpTransport(bool rtcp_mux_enabled, const FieldTrialsView& field_trials)
+      : set_ready_to_send_false_if_send_fail_(
+            field_trials.IsEnabled("WebRTC-SetReadyToSendFalseIfSendFail")),
+        rtcp_mux_enabled_(rtcp_mux_enabled) {}
 
   bool rtcp_mux_enabled() const override { return rtcp_mux_enabled_; }
   void SetRtcpMuxEnabled(bool enable) override;
@@ -125,6 +128,7 @@ class RtpTransport : public RtpTransportInternal {
 
   bool IsTransportWritable();
 
+  const bool set_ready_to_send_false_if_send_fail_;
   bool rtcp_mux_enabled_;
 
   rtc::PacketTransportInternal* rtp_packet_transport_ = nullptr;

pc/rtp_transport_unittest.cc

@@ -18,11 +18,14 @@
 #include "rtc_base/containers/flat_set.h"
 #include "rtc_base/gunit.h"
 #include "rtc_base/third_party/sigslot/sigslot.h"
+#include "test/explicit_key_value_config.h"
 #include "test/gtest.h"
 #include "test/run_loop.h"
 
 namespace webrtc {
 
+using test::ExplicitKeyValueConfig;
+
 constexpr bool kMuxDisabled = false;
 constexpr bool kMuxEnabled = true;
 constexpr uint16_t kLocalNetId = 1;
@@ -82,7 +85,8 @@ class SignalObserver : public sigslot::has_slots<> {
 };
 
 TEST(RtpTransportTest, SettingRtcpAndRtpSignalsReady) {
-  RtpTransport transport(kMuxDisabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
+
   SignalObserver observer(&transport);
   rtc::FakePacketTransport fake_rtcp("fake_rtcp");
   fake_rtcp.SetWritable(true);
@@ -96,7 +100,7 @@ TEST(RtpTransportTest, SettingRtcpAndRtpSignalsReady) {
 }
 
 TEST(RtpTransportTest, SettingRtpAndRtcpSignalsReady) {
-  RtpTransport transport(kMuxDisabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
   SignalObserver observer(&transport);
   rtc::FakePacketTransport fake_rtcp("fake_rtcp");
   fake_rtcp.SetWritable(true);
@@ -110,7 +114,7 @@ TEST(RtpTransportTest, SettingRtpAndRtcpSignalsReady) {
 }
 
 TEST(RtpTransportTest, SettingRtpWithRtcpMuxEnabledSignalsReady) {
-  RtpTransport transport(kMuxEnabled);
+  RtpTransport transport(kMuxEnabled, ExplicitKeyValueConfig(""));
   SignalObserver observer(&transport);
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   fake_rtp.SetWritable(true);
@@ -120,7 +124,7 @@ TEST(RtpTransportTest, SettingRtpWithRtcpMuxEnabledSignalsReady) {
 }
 
 TEST(RtpTransportTest, DisablingRtcpMuxSignalsNotReady) {
-  RtpTransport transport(kMuxEnabled);
+  RtpTransport transport(kMuxEnabled, ExplicitKeyValueConfig(""));
   SignalObserver observer(&transport);
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   fake_rtp.SetWritable(true);
@@ -133,7 +137,7 @@ TEST(RtpTransportTest, DisablingRtcpMuxSignalsNotReady) {
 }
 
 TEST(RtpTransportTest, EnablingRtcpMuxSignalsReady) {
-  RtpTransport transport(kMuxDisabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
   SignalObserver observer(&transport);
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   fake_rtp.SetWritable(true);
@@ -147,7 +151,7 @@ TEST(RtpTransportTest, EnablingRtcpMuxSignalsReady) {
 
 // Tests the SignalNetworkRoute is fired when setting a packet transport.
 TEST(RtpTransportTest, SetRtpTransportWithNetworkRouteChanged) {
-  RtpTransport transport(kMuxDisabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
   SignalObserver observer(&transport);
   rtc::FakePacketTransport fake_rtp("fake_rtp");
 
@@ -176,7 +180,7 @@ TEST(RtpTransportTest, SetRtpTransportWithNetworkRouteChanged) {
 }
 
 TEST(RtpTransportTest, SetRtcpTransportWithNetworkRouteChanged) {
-  RtpTransport transport(kMuxDisabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
   SignalObserver observer(&transport);
   rtc::FakePacketTransport fake_rtcp("fake_rtcp");
 
@@ -209,7 +213,7 @@ TEST(RtpTransportTest, SetRtcpTransportWithNetworkRouteChanged) {
 TEST(RtpTransportTest, RtcpPacketSentOverCorrectTransport) {
   // If the RTCP-mux is not enabled, RTCP packets are expected to be sent over
   // the RtcpPacketTransport.
-  RtpTransport transport(kMuxDisabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
   rtc::FakePacketTransport fake_rtcp("fake_rtcp");
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   transport.SetRtcpPacketTransport(&fake_rtcp);  // rtcp ready
@@ -231,7 +235,7 @@ TEST(RtpTransportTest, RtcpPacketSentOverCorrectTransport) {
 }
 
 TEST(RtpTransportTest, ChangingReadyToSendStateOnlySignalsWhenChanged) {
-  RtpTransport transport(kMuxEnabled);
+  RtpTransport transport(kMuxEnabled, ExplicitKeyValueConfig(""));
   TransportObserver observer(&transport);
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   fake_rtp.SetWritable(true);
@@ -256,7 +260,7 @@ TEST(RtpTransportTest, ChangingReadyToSendStateOnlySignalsWhenChanged) {
 // Test that SignalPacketReceived fires with rtcp=true when a RTCP packet is
 // received.
 TEST(RtpTransportTest, SignalDemuxedRtcp) {
-  RtpTransport transport(kMuxDisabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   fake_rtp.SetDestination(&fake_rtp, true);
   transport.SetRtpPacketTransport(&fake_rtp);
@@ -279,7 +283,7 @@ static const int kRtpLen = 12;
 // Test that SignalPacketReceived fires with rtcp=false when a RTP packet with a
 // handled payload type is received.
 TEST(RtpTransportTest, SignalHandledRtpPayloadType) {
-  RtpTransport transport(kMuxDisabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   fake_rtp.SetDestination(&fake_rtp, true);
   transport.SetRtpPacketTransport(&fake_rtp);
@@ -302,7 +306,7 @@ TEST(RtpTransportTest, SignalHandledRtpPayloadType) {
 }
 
 TEST(RtpTransportTest, ReceivedPacketEcnMarkingPropagatedToDemuxedPacket) {
-  RtpTransport transport(kMuxDisabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
   // Setup FakePacketTransport to send packets to itself.
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   fake_rtp.SetDestination(&fake_rtp, true);
@@ -327,7 +331,7 @@ TEST(RtpTransportTest, ReceivedPacketEcnMarkingPropagatedToDemuxedPacket) {
 // Test that SignalPacketReceived does not fire when a RTP packet with an
 // unhandled payload type is received.
 TEST(RtpTransportTest, DontSignalUnhandledRtpPayloadType) {
-  RtpTransport transport(kMuxDisabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   fake_rtp.SetDestination(&fake_rtp, true);
   transport.SetRtpPacketTransport(&fake_rtp);
@@ -348,10 +352,36 @@ TEST(RtpTransportTest, DontSignalUnhandledRtpPayloadType) {
   transport.UnregisterRtpDemuxerSink(&observer);
 }
 
+TEST(RtpTransportTest, DontChangeReadyToSendStateOnSendFailure) {
+  // ReadyToSendState should only care about if transport is writable unless the
+  // field trial WebRTC-SetReadyToSendFalseIfSendFail/Enabled/ is set.
+  RtpTransport transport(kMuxEnabled, ExplicitKeyValueConfig(""));
+  TransportObserver observer(&transport);
+
+  rtc::FakePacketTransport fake_rtp("fake_rtp");
+  fake_rtp.SetDestination(&fake_rtp, true);
+  transport.SetRtpPacketTransport(&fake_rtp);
+  fake_rtp.SetWritable(true);
+  EXPECT_TRUE(observer.ready_to_send());
+  EXPECT_EQ(observer.ready_to_send_signal_count(), 1);
+  rtc::CopyOnWriteBuffer packet;
+  EXPECT_TRUE(transport.SendRtpPacket(&packet, rtc::PacketOptions(), 0));
+
+  // The fake RTP will return -1 due to ENOTCONN.
+  fake_rtp.SetError(ENOTCONN);
+  EXPECT_FALSE(transport.SendRtpPacket(&packet, rtc::PacketOptions(), 0));
+  // Ready to send state should not have changed.
+  EXPECT_TRUE(observer.ready_to_send());
+  EXPECT_EQ(observer.ready_to_send_signal_count(), 1);
+}
+
 TEST(RtpTransportTest, RecursiveSetSendDoesNotCrash) {
   const int kShortTimeout = 100;
   test::RunLoop loop;
-  RtpTransport transport(kMuxEnabled);
+
+  RtpTransport transport(
+      kMuxEnabled,
+      ExplicitKeyValueConfig("WebRTC-SetReadyToSendFalseIfSendFail/Enabled/"));
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   transport.SetRtpPacketTransport(&fake_rtp);
   TransportObserver observer(&transport);
@@ -375,7 +405,7 @@ TEST(RtpTransportTest, RecursiveSetSendDoesNotCrash) {
 TEST(RtpTransportTest, RecursiveOnSentPacketDoesNotCrash) {
   const int kShortTimeout = 100;
   test::RunLoop loop;
-  RtpTransport transport(kMuxEnabled);
+  RtpTransport transport(kMuxDisabled, ExplicitKeyValueConfig(""));
   rtc::FakePacketTransport fake_rtp("fake_rtp");
   transport.SetRtpPacketTransport(&fake_rtp);
   fake_rtp.SetDestination(&fake_rtp, true);

pc/srtp_transport.cc

@@ -35,7 +35,8 @@ namespace webrtc {
 
 SrtpTransport::SrtpTransport(bool rtcp_mux_enabled,
                              const FieldTrialsView& field_trials)
-    : RtpTransport(rtcp_mux_enabled), field_trials_(field_trials) {}
+    : RtpTransport(rtcp_mux_enabled, field_trials),
+      field_trials_(field_trials) {}
 
 bool SrtpTransport::SendRtpPacket(rtc::CopyOnWriteBuffer* packet,
                                   const rtc::PacketOptions& options,