diff --git a/pc/composite_data_channel_transport.cc b/pc/composite_data_channel_transport.cc index 185dd1e23a..e66febc12b 100644 --- a/pc/composite_data_channel_transport.cc +++ b/pc/composite_data_channel_transport.cc @@ -24,6 +24,12 @@ CompositeDataChannelTransport::CompositeDataChannelTransport( } } +CompositeDataChannelTransport::~CompositeDataChannelTransport() { + for (auto transport : transports_) { + transport->SetDataSink(nullptr); + } +} + void CompositeDataChannelTransport::SetSendTransport( DataChannelTransportInterface* send_transport) { if (!absl::c_linear_search(transports_, send_transport)) { diff --git a/pc/composite_data_channel_transport.h b/pc/composite_data_channel_transport.h index ccff4fe7ab..b2a40fdb7a 100644 --- a/pc/composite_data_channel_transport.h +++ b/pc/composite_data_channel_transport.h @@ -26,6 +26,7 @@ class CompositeDataChannelTransport : public DataChannelTransportInterface, public: explicit CompositeDataChannelTransport( std::vector transports); + ~CompositeDataChannelTransport() override; // Specifies which transport to be used for sending. Must be called before // sending data. diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index b95dc22cfb..7c83e85e87 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -114,7 +114,6 @@ JsepTransport::JsepTransport( unencrypted_rtp_transport_(std::move(unencrypted_rtp_transport)), sdes_transport_(std::move(sdes_transport)), dtls_srtp_transport_(std::move(dtls_srtp_transport)), - datagram_rtp_transport_(std::move(datagram_rtp_transport)), rtp_dtls_transport_( rtp_dtls_transport ? new rtc::RefCountedObject( std::move(rtp_dtls_transport)) @@ -134,6 +133,7 @@ JsepTransport::JsepTransport( : nullptr), media_transport_(std::move(media_transport)), datagram_transport_(std::move(datagram_transport)), + datagram_rtp_transport_(std::move(datagram_rtp_transport)), data_channel_transport_(data_channel_transport) { RTC_DCHECK(ice_transport_); RTC_DCHECK(rtp_dtls_transport_); @@ -178,11 +178,9 @@ JsepTransport::JsepTransport( } JsepTransport::~JsepTransport() { - // Disconnect media transport state callbacks and make sure we delete media - // transport before ICE. + // Disconnect media transport state callbacks. if (media_transport_) { media_transport_->SetMediaTransportStateCallback(nullptr); - media_transport_.reset(); } if (sctp_transport_) { @@ -196,10 +194,6 @@ JsepTransport::~JsepTransport() { rtcp_dtls_transport_->Clear(); } - // Delete datagram transport before ICE, but after its RTP transport. - datagram_rtp_transport_.reset(); - datagram_transport_.reset(); - // ICE will be the last transport to be deleted. } diff --git a/pc/jsep_transport.h b/pc/jsep_transport.h index 868f7b92c6..b6199f8d0f 100644 --- a/pc/jsep_transport.h +++ b/pc/jsep_transport.h @@ -378,8 +378,6 @@ class JsepTransport : public sigslot::has_slots<>, RTC_GUARDED_BY(accessor_lock_); std::unique_ptr dtls_srtp_transport_ RTC_GUARDED_BY(accessor_lock_); - std::unique_ptr datagram_rtp_transport_ - RTC_GUARDED_BY(accessor_lock_); // If multiple RTP transports are in use, |composite_rtp_transport_| will be // passed to callers. This is only valid for offer-only, receive-only @@ -417,6 +415,9 @@ class JsepTransport : public sigslot::has_slots<>, std::unique_ptr datagram_transport_ RTC_GUARDED_BY(accessor_lock_); + std::unique_ptr datagram_rtp_transport_ + RTC_GUARDED_BY(accessor_lock_); + // Non-SCTP data channel transport. Set to one of |media_transport_| or // |datagram_transport_| if that transport should be used for data chanels. // Unset if neither should be used for data channels. diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 33fc9b9b9d..465dca12bc 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -3599,6 +3599,54 @@ TEST_P(PeerConnectionIntegrationTest, ASSERT_TRUE(ExpectNewFrames(media_expectations)); } +// Tests that the data channel transport works correctly when datagram transport +// negotiation succeeds and does not fall back to SCTP. +TEST_P(PeerConnectionIntegrationTest, + DatagramTransportDataChannelDoesNotFallbackToSctp) { + PeerConnectionInterface::RTCConfiguration rtc_config; + rtc_config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyRequire; + rtc_config.bundle_policy = PeerConnectionInterface::kBundlePolicyMaxBundle; + rtc_config.use_datagram_transport_for_data_channels = true; + + // Configure one endpoint to use datagram transport for data channels while + // the other does not. + ASSERT_TRUE(CreatePeerConnectionWrappersWithConfigAndMediaTransportFactory( + rtc_config, rtc_config, loopback_media_transports()->first_factory(), + loopback_media_transports()->second_factory())); + ConnectFakeSignaling(); + + // The caller offers a data channel using either datagram transport or SCTP. + caller()->CreateDataChannel(); + caller()->AddAudioVideoTracks(); + callee()->AddAudioVideoTracks(); + caller()->CreateAndSetAndSignalOffer(); + ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); + + // Ensure that the data channel transport is ready. + loopback_media_transports()->SetState(webrtc::MediaTransportState::kWritable); + loopback_media_transports()->FlushAsyncInvokes(); + + // Negotiation should succeed, allowing the data channel to be established. + ASSERT_NE(nullptr, caller()->data_channel()); + ASSERT_TRUE_WAIT(callee()->data_channel() != nullptr, kDefaultTimeout); + EXPECT_TRUE_WAIT(caller()->data_observer()->IsOpen(), kDefaultTimeout); + EXPECT_TRUE_WAIT(callee()->data_observer()->IsOpen(), kDefaultTimeout); + + // Ensure data can be sent in both directions. + std::string data = "hello world"; + caller()->data_channel()->Send(DataBuffer(data)); + EXPECT_EQ_WAIT(data, callee()->data_observer()->last_message(), + kDefaultTimeout); + callee()->data_channel()->Send(DataBuffer(data)); + EXPECT_EQ_WAIT(data, caller()->data_observer()->last_message(), + kDefaultTimeout); + + // Ensure that failure of the datagram negotiation doesn't impede media flow. + MediaExpectations media_expectations; + media_expectations.ExpectBidirectionalAudioAndVideo(); + ASSERT_TRUE(ExpectNewFrames(media_expectations)); +} + #endif // HAVE_SCTP // This test sets up a call between two parties with a datagram transport data @@ -3620,7 +3668,7 @@ TEST_P(PeerConnectionIntegrationTest, DatagramTransportDataChannelEndToEnd) { caller()->CreateAndSetAndSignalOffer(); ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); - // Ensure that the media transport is ready. + // Ensure that the data channel transport is ready. loopback_media_transports()->SetState(webrtc::MediaTransportState::kWritable); loopback_media_transports()->FlushAsyncInvokes(); @@ -3706,7 +3754,7 @@ TEST_P(PeerConnectionIntegrationTest, caller()->CreateAndSetAndSignalOffer(); ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); - // Ensure that the media transport is ready. + // Ensure that the data channel transport is ready. loopback_media_transports()->SetState(webrtc::MediaTransportState::kWritable); loopback_media_transports()->FlushAsyncInvokes(); @@ -3743,7 +3791,7 @@ TEST_P(PeerConnectionIntegrationTest, caller()->CreateAndSetAndSignalOffer(); ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); - // Ensure that the media transport is ready. + // Ensure that the data channel transport is ready. loopback_media_transports()->SetState(webrtc::MediaTransportState::kWritable); loopback_media_transports()->FlushAsyncInvokes();