diff --git a/api/test/fake_datagram_transport.h b/api/test/fake_datagram_transport.h index 16cb8d5e7e..847b4d842a 100644 --- a/api/test/fake_datagram_transport.h +++ b/api/test/fake_datagram_transport.h @@ -26,9 +26,14 @@ constexpr size_t kMaxFakeDatagramSize = 1000; // or sending data. Only used for tests that need to stub out a transport. class FakeDatagramTransport : public DatagramTransportInterface { public: - FakeDatagramTransport(const MediaTransportSettings& settings, - std::string transport_parameters) - : settings_(settings), transport_parameters_(transport_parameters) {} + FakeDatagramTransport( + const MediaTransportSettings& settings, + std::string transport_parameters, + const std::function& + are_parameters_compatible) + : settings_(settings), + transport_parameters_(transport_parameters), + are_parameters_compatible_(are_parameters_compatible) {} ~FakeDatagramTransport() override { RTC_DCHECK(!state_callback_); } @@ -63,6 +68,16 @@ class FakeDatagramTransport : public DatagramTransportInterface { return transport_parameters_; } + RTCError SetRemoteTransportParameters( + absl::string_view remote_parameters) override { + if (are_parameters_compatible_(GetTransportParameters(), + remote_parameters)) { + return RTCError::OK(); + } + return RTCError(RTCErrorType::UNSUPPORTED_PARAMETER, + "Incompatible remote transport parameters"); + } + RTCError OpenChannel(int channel_id) override { return RTCError(RTCErrorType::UNSUPPORTED_OPERATION); } @@ -94,6 +109,8 @@ class FakeDatagramTransport : public DatagramTransportInterface { private: const MediaTransportSettings settings_; const std::string transport_parameters_; + const std::function + are_parameters_compatible_; rtc::PacketTransportInternal* packet_transport_ = nullptr; MediaTransportStateCallback* state_callback_ = nullptr; diff --git a/api/test/fake_media_transport.h b/api/test/fake_media_transport.h index ce2d88ce62..530394710a 100644 --- a/api/test/fake_media_transport.h +++ b/api/test/fake_media_transport.h @@ -51,11 +51,22 @@ class FakeMediaTransportFactory : public MediaTransportFactory { CreateDatagramTransport(rtc::Thread* network_thread, const MediaTransportSettings& settings) override { return std::unique_ptr( - new FakeDatagramTransport(settings, transport_offer_.value_or(""))); + new FakeDatagramTransport(settings, transport_offer_.value_or(""), + transport_parameters_comparison_)); + } + + void set_transport_parameters_comparison( + std::function comparison) { + transport_parameters_comparison_ = std::move(comparison); } private: const absl::optional transport_offer_; + std::function + transport_parameters_comparison_ = + [](absl::string_view local, absl::string_view remote) { + return local == remote; + }; }; } // namespace webrtc diff --git a/api/test/loopback_media_transport.cc b/api/test/loopback_media_transport.cc index 14b28acf4b..847ca4864a 100644 --- a/api/test/loopback_media_transport.cc +++ b/api/test/loopback_media_transport.cc @@ -57,6 +57,10 @@ class WrapperDatagramTransport : public DatagramTransportInterface { return wrapped_->GetTransportParameters(); } + RTCError SetRemoteTransportParameters(absl::string_view parameters) override { + return wrapped_->SetRemoteTransportParameters(parameters); + } + // Data channel overrides. RTCError OpenChannel(int channel_id) override { return wrapped_->OpenChannel(channel_id); @@ -299,6 +303,18 @@ MediaTransportPair::LoopbackDatagramTransport::GetTransportParameters() const { return transport_parameters_; } +RTCError +MediaTransportPair::LoopbackDatagramTransport::SetRemoteTransportParameters( + absl::string_view remote_parameters) { + RTC_DCHECK_RUN_ON(thread_); + if (transport_parameters_comparison_(GetTransportParameters(), + remote_parameters)) { + return RTCError::OK(); + } + return RTCError(RTCErrorType::UNSUPPORTED_PARAMETER, + "Incompatible remote transport parameters"); +} + RTCError MediaTransportPair::LoopbackDatagramTransport::OpenChannel( int channel_id) { return dc_transport_.OpenChannel(channel_id); diff --git a/api/test/loopback_media_transport.h b/api/test/loopback_media_transport.h index f2aed3e8e7..468965ba31 100644 --- a/api/test/loopback_media_transport.h +++ b/api/test/loopback_media_transport.h @@ -115,6 +115,22 @@ class MediaTransportPair { first_datagram_transport_.set_transport_parameters(params); } + void SetSecondDatagramTransportParameters(const std::string& params) { + second_datagram_transport_.set_transport_parameters(params); + } + + void SetFirstDatagramTransportParametersComparison( + std::function comparison) { + first_datagram_transport_.set_transport_parameters_comparison( + std::move(comparison)); + } + + void SetSecondDatagramTransportParametersComparison( + std::function comparison) { + second_datagram_transport_.set_transport_parameters_comparison( + std::move(comparison)); + } + void FlushAsyncInvokes() { first_datagram_transport_.FlushAsyncInvokes(); second_datagram_transport_.FlushAsyncInvokes(); @@ -186,6 +202,8 @@ class MediaTransportPair { size_t GetLargestDatagramSize() const override; void SetDatagramSink(DatagramSinkInterface* sink) override; std::string GetTransportParameters() const override; + RTCError SetRemoteTransportParameters( + absl::string_view remote_parameters) override; // Data channel overrides. RTCError OpenChannel(int channel_id) override; @@ -208,6 +226,15 @@ class MediaTransportPair { transport_parameters_ = value; } + void set_transport_parameters_comparison( + std::function comparison) { + thread_->Invoke( + RTC_FROM_HERE, [this, comparison = std::move(comparison)] { + RTC_DCHECK_RUN_ON(thread_); + transport_parameters_comparison_ = std::move(comparison); + }); + } + private: void DeliverDatagram(rtc::CopyOnWriteBuffer buffer); @@ -222,6 +249,9 @@ class MediaTransportPair { LoopbackDatagramTransport* other_; std::string transport_parameters_; + std::function + transport_parameters_comparison_ RTC_GUARDED_BY(thread_) = + [](absl::string_view a, absl::string_view b) { return a == b; }; absl::optional state_after_connect_; diff --git a/api/transport/datagram_transport_interface.h b/api/transport/datagram_transport_interface.h index dfac4578e9..01736b978d 100644 --- a/api/transport/datagram_transport_interface.h +++ b/api/transport/datagram_transport_interface.h @@ -128,6 +128,22 @@ class DatagramTransportInterface : public DataChannelTransportInterface { // the client, possibly removing any fields or parameters which the client // does not understand. virtual std::string GetTransportParameters() const = 0; + + // Sets remote transport parameters. |remote_params| is a serialized string + // of opaque parameters, understood by the datagram transport implementation. + // Returns an error if |remote_params| are not compatible with this transport. + // + // TODO(mellem): Make pure virtual. The default implementation maintains + // original negotiation behavior (negotiation falls back to RTP if the + // remote datagram transport fails to echo exactly the local parameters). + virtual RTCError SetRemoteTransportParameters( + absl::string_view remote_params) { + if (remote_params == GetTransportParameters()) { + return RTCError::OK(); + } + return RTCError(RTCErrorType::UNSUPPORTED_PARAMETER, + "Local and remote transport parameters do not match"); + } }; } // namespace webrtc diff --git a/p2p/base/transport_description_factory.cc b/p2p/base/transport_description_factory.cc index f417c5ad86..17152d1a04 100644 --- a/p2p/base/transport_description_factory.cc +++ b/p2p/base/transport_description_factory.cc @@ -110,10 +110,10 @@ std::unique_ptr TransportDescriptionFactory::CreateAnswer( return NULL; } - // Answers may only attach opaque parameters that exactly match parameters - // present in the offer. If the answerer cannot fully understand or accept - // the offered transport, it must reject it and fall back. - if (offer->opaque_parameters == options.opaque_parameters) { + // Answers may only attach opaque parameters if the offer contained them as + // well. The answer's parameters may differ, and it's up to the opaque + // transport implementation to decide if the difference is acceptable. + if (offer->opaque_parameters && options.opaque_parameters) { desc->opaque_parameters = options.opaque_parameters; } diff --git a/p2p/base/transport_description_factory_unittest.cc b/p2p/base/transport_description_factory_unittest.cc index f91cf6fe31..8359ffc1c9 100644 --- a/p2p/base/transport_description_factory_unittest.cc +++ b/p2p/base/transport_description_factory_unittest.cc @@ -259,30 +259,6 @@ TEST_F(TransportDescriptionFactoryTest, TestAnswerNoOpaqueTransportParameters) { EXPECT_EQ(answer->opaque_parameters, absl::nullopt); } -TEST_F(TransportDescriptionFactoryTest, - TestAnswerDifferentOpaqueTransportParameters) { - OpaqueTransportParameters offer_params; - offer_params.protocol = "fake"; - offer_params.parameters = "foobar"; - - TransportOptions options; - options.opaque_parameters = offer_params; - - std::unique_ptr offer = - f1_.CreateOffer(options, NULL, &ice_credentials_); - - OpaqueTransportParameters answer_params; - answer_params.protocol = "fake"; - answer_params.parameters = "baz"; - - options.opaque_parameters = answer_params; - std::unique_ptr answer = - f2_.CreateAnswer(offer.get(), options, true, NULL, &ice_credentials_); - - CheckDesc(answer.get(), "", "", "", ""); - EXPECT_EQ(answer->opaque_parameters, absl::nullopt); -} - TEST_F(TransportDescriptionFactoryTest, TestAnswerNoOpaqueTransportParametersInOffer) { std::unique_ptr offer = diff --git a/pc/jsep_transport.cc b/pc/jsep_transport.cc index 8a555f2c67..bc380402b1 100644 --- a/pc/jsep_transport.cc +++ b/pc/jsep_transport.cc @@ -765,10 +765,19 @@ void JsepTransport::NegotiateDatagramTransport(SdpType type) { return; // No need to negotiate the use of datagram transport. } - bool compatible_datagram_transport = - remote_description_->transport_desc.opaque_parameters && - remote_description_->transport_desc.opaque_parameters == - local_description_->transport_desc.opaque_parameters; + bool compatible_datagram_transport = false; + if (datagram_transport_ && + local_description_->transport_desc.opaque_parameters && + remote_description_->transport_desc.opaque_parameters) { + // If both descriptions have datagram transport parameters, and the remote + // parameters are accepted by the datagram transport, then use the datagram + // transport. Otherwise, fall back to RTP. + compatible_datagram_transport = + datagram_transport_ + ->SetRemoteTransportParameters(remote_description_->transport_desc + .opaque_parameters->parameters) + .ok(); + } bool use_datagram_transport_for_media = compatible_datagram_transport && diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index f62cd87bb1..bc7000f451 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -1116,8 +1116,19 @@ JsepTransportController::MaybeCreateDatagramTransport( config_.media_transport_factory->CreateDatagramTransport(network_thread_, settings); - // TODO(sukhanov): Proper error handling. - RTC_CHECK(datagram_transport_result.ok()); + if (!datagram_transport_result.ok()) { + // Datagram transport negotiation will fail and we'll fall back to RTP. + return nullptr; + } + + if (!datagram_transport_result.value() + ->SetRemoteTransportParameters( + transport_description->opaque_parameters->parameters) + .ok()) { + // Datagram transport negotiation failed (parameters are incompatible). + // Fall back to RTP. + return nullptr; + } return datagram_transport_result.MoveValue(); } diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc index d78597ccf1..18fdc209d1 100644 --- a/pc/jsep_transport_controller_unittest.cc +++ b/pc/jsep_transport_controller_unittest.cc @@ -1827,6 +1827,65 @@ TEST_P(JsepTransportControllerDatagramTest, OfferHasWrongTransportName) { absl::nullopt); } +TEST_P(JsepTransportControllerDatagramTest, IncompatibleAnswer) { + // Transport will claim that no parameters are compatible, even if they match + // exactly. + fake_media_transport_factory_.set_transport_parameters_comparison( + [](absl::string_view, absl::string_view) { return false; }); + + cricket::OpaqueTransportParameters fake_params = CreateTransportParameters(); + if (IsOfferer()) { + EXPECT_EQ(transport_controller_->GetTransportParameters(kAudioMid1), + fake_params); + EXPECT_EQ(transport_controller_->GetTransportParameters(kVideoMid1), + fake_params); + } + + auto offer = CreateSessionDescriptionForDatagramTransport(fake_params); + EXPECT_TRUE(SetDescription(SdpType::kOffer, offer.get()).ok()); + + auto answer = CreateSessionDescriptionForDatagramTransport(fake_params); + EXPECT_TRUE(SetDescription(SdpType::kAnswer, answer.get()).ok()); + + // The offerer and answerer have incompatible parameters, so the answerer + // rejects the offered parameters. + EXPECT_EQ(transport_controller_->GetTransportParameters(kAudioMid1), + absl::nullopt); + EXPECT_EQ(transport_controller_->GetTransportParameters(kVideoMid1), + absl::nullopt); +} + +TEST_P(JsepTransportControllerDatagramTest, CompatibleAnswer) { + // Transport will claim that no parameters are compatible, even if they are + // completely different. + fake_media_transport_factory_.set_transport_parameters_comparison( + [](absl::string_view, absl::string_view) { return true; }); + + cricket::OpaqueTransportParameters fake_params = CreateTransportParameters(); + if (IsOfferer()) { + EXPECT_EQ(transport_controller_->GetTransportParameters(kAudioMid1), + fake_params); + EXPECT_EQ(transport_controller_->GetTransportParameters(kVideoMid1), + fake_params); + } + + auto offer = CreateSessionDescriptionForDatagramTransport(fake_params); + EXPECT_TRUE(SetDescription(SdpType::kOffer, offer.get()).ok()); + + cricket::OpaqueTransportParameters answer_params; + answer_params.protocol = fake_params.protocol; + answer_params.parameters = "something different from offer"; + auto answer = CreateSessionDescriptionForDatagramTransport(answer_params); + EXPECT_TRUE(SetDescription(SdpType::kAnswer, answer.get()).ok()); + + // The offerer and answerer have compatible parameters, so the answerer + // accepts the offered parameters. + EXPECT_EQ(transport_controller_->GetTransportParameters(kAudioMid1), + fake_params); + EXPECT_EQ(transport_controller_->GetTransportParameters(kVideoMid1), + fake_params); +} + TEST_P(JsepTransportControllerDatagramTest, AnswerRejectsDatagram) { cricket::OpaqueTransportParameters fake_params = CreateTransportParameters(); if (IsOfferer()) { diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 399001f9f3..4c7ea0c4e7 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -3696,6 +3696,180 @@ TEST_P(PeerConnectionIntegrationTest, ASSERT_TRUE(ExpectNewFrames(media_expectations)); } +// Tests that the datagram transport to SCTP fallback works correctly when +// datagram transports do not advertise compatible transport parameters. +TEST_P(PeerConnectionIntegrationTest, + DatagramTransportIncompatibleParametersFallsBackToSctp) { + 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; + + // By default, only equal parameters are compatible. + loopback_media_transports()->SetFirstDatagramTransportParameters("foo"); + loopback_media_transports()->SetSecondDatagramTransportParameters("bar"); + + // 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); + + // Negotiation should fallback to SCTP, 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); + + // Both endpoints should agree to use SCTP for data channels. + EXPECT_NE(nullptr, caller()->pc()->GetSctpTransport()); + EXPECT_NE(nullptr, callee()->pc()->GetSctpTransport()); + + // 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)); +} + +// Tests that the datagram transport to SCTP fallback works correctly when +// only the answerer believes datagram transport parameters are incompatible. +TEST_P(PeerConnectionIntegrationTest, + DatagramTransportIncompatibleParametersOnAnswererFallsBackToSctp) { + 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; + + // By default, only equal parameters are compatible. + loopback_media_transports()->SetFirstDatagramTransportParameters("foo"); + loopback_media_transports()->SetSecondDatagramTransportParameters("bar"); + + // Set the offerer to accept different parameters, while the answerer rejects + // them. + loopback_media_transports()->SetFirstDatagramTransportParametersComparison( + [](absl::string_view a, absl::string_view b) { return true; }); + loopback_media_transports()->SetSecondDatagramTransportParametersComparison( + [](absl::string_view a, absl::string_view b) { return false; }); + + // 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); + + // Negotiation should fallback to SCTP, 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); + + // Both endpoints should agree to use SCTP for data channels. + EXPECT_NE(nullptr, caller()->pc()->GetSctpTransport()); + EXPECT_NE(nullptr, callee()->pc()->GetSctpTransport()); + + // 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)); +} + +// Tests that the data channel transport works correctly when datagram +// transports provide different, but compatible, transport parameters. +TEST_P(PeerConnectionIntegrationTest, + DatagramTransportCompatibleParametersDoNotFallbackToSctp) { + 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; + + // By default, only equal parameters are compatible. + loopback_media_transports()->SetFirstDatagramTransportParameters("foo"); + loopback_media_transports()->SetSecondDatagramTransportParameters("bar"); + + // Change the comparison used to treat these transport parameters are + // compatible (on both sides). + loopback_media_transports()->SetFirstDatagramTransportParametersComparison( + [](absl::string_view a, absl::string_view b) { return true; }); + loopback_media_transports()->SetSecondDatagramTransportParametersComparison( + [](absl::string_view a, absl::string_view b) { return true; }); + + 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); + + // Both endpoints should agree to use datagram transport for data channels. + EXPECT_EQ(nullptr, caller()->pc()->GetSctpTransport()); + EXPECT_EQ(nullptr, callee()->pc()->GetSctpTransport()); + + // 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)); +} + TEST_P(PeerConnectionIntegrationTest, DatagramTransportDataChannelWithMediaOnCaller) { // Configure the caller to attempt use of datagram transport for media and