diff --git a/experiments/field_trials.py b/experiments/field_trials.py index dda60dca6b..06969ebdf1 100755 --- a/experiments/field_trials.py +++ b/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)), diff --git a/p2p/base/fake_packet_transport.h b/p2p/base/fake_packet_transport.h index df6c62ac1f..a7388f52cf 100644 --- a/p2p/base/fake_packet_transport.h +++ b/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); diff --git a/pc/BUILD.gn b/pc/BUILD.gn index fd60ce2505..4ba9883ec7 100644 --- a/pc/BUILD.gn +++ b/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", diff --git a/pc/channel_unittest.cc b/pc/channel_unittest.cc index ade298bebb..a6513914e9 100644 --- a/pc/channel_unittest.cc +++ b/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( - rtcp_packet_transport == nullptr); + rtcp_packet_transport == nullptr, field_trials_); SendTask(network_thread_, [&rtp_transport, rtp_packet_transport, rtcp_packet_transport] { diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index 7ad8e2d018..65334d6782 100644 --- a/pc/jsep_transport_controller.cc +++ b/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(rtcp_packet_transport == nullptr); + auto unencrypted_rtp_transport = std::make_unique( + 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); diff --git a/pc/rtp_transport.cc b/pc/rtp_transport.cc index ab6058f7a1..b2fc9b21f6 100644 --- a/pc/rtp_transport.cc +++ b/pc/rtp_transport.cc @@ -15,7 +15,6 @@ #include #include -#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,9 +150,13 @@ bool RtpTransport::SendPacket(bool rtcp, int ret = transport->SendPacket(packet->cdata(), packet->size(), options, flags); if (ret != static_cast(packet->size())) { - if (transport->GetError() == ENOTCONN) { - RTC_LOG(LS_WARNING) << "Got ENOTCONN from transport."; - SetReadyToSend(rtcp, false); + 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; } diff --git a/pc/rtp_transport.h b/pc/rtp_transport.h index 1cfb16db45..ebb7aa64d7 100644 --- a/pc/rtp_transport.h +++ b/pc/rtp_transport.h @@ -17,6 +17,7 @@ #include #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; diff --git a/pc/rtp_transport_unittest.cc b/pc/rtp_transport_unittest.cc index 5257b428ab..a3ab768548 100644 --- a/pc/rtp_transport_unittest.cc +++ b/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); diff --git a/pc/srtp_transport.cc b/pc/srtp_transport.cc index e18855739b..dd4fb14765 100644 --- a/pc/srtp_transport.cc +++ b/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,