Only create no-op DTLS if media transport is used for both media and data
Currently it's possible that no-op DTLS is created if media transport is only used for data channels.
Changing it so that no-op DTLS is only created when both media & data will flow through media transport.
Bug: webrtc:9719
Change-Id: I87f27fc778ea21b12f2904bad1452d893f66b541
Reviewed-on: https://webrtc-review.googlesource.com/c/119909
Commit-Queue: Peter Slatala <psla@webrtc.org>
Reviewed-by: Steve Anton <steveanton@webrtc.org>
Reviewed-by: Anton Sukhanov <sukhanov@webrtc.org>
Reviewed-by: Bjorn Mellem <mellem@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#26416}diff --git a/api/peer_connection_interface.h b/api/peer_connection_interface.h
index 3115e25..a429519 100644
--- a/api/peer_connection_interface.h
+++ b/api/peer_connection_interface.h
@@ -602,9 +602,9 @@
bool active_reset_srtp_params = false;
// If MediaTransportFactory is provided in PeerConnectionFactory, this flag
- // informs PeerConnection that it should use the MediaTransportInterface.
- // It's invalid to set it to |true| if the MediaTransportFactory wasn't
- // provided.
+ // informs PeerConnection that it should use the MediaTransportInterface for
+ // media (audio/video). It's invalid to set it to |true| if the
+ // MediaTransportFactory wasn't provided.
bool use_media_transport = false;
// If MediaTransportFactory is provided in PeerConnectionFactory, this flag
diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc
index 084cec5..9cecc10 100644
--- a/pc/jsep_transport_controller.cc
+++ b/pc/jsep_transport_controller.cc
@@ -402,13 +402,24 @@
}
}
-void JsepTransportController::SetMediaTransportFactory(
- MediaTransportFactory* media_transport_factory) {
- RTC_DCHECK(media_transport_factory == config_.media_transport_factory ||
+void JsepTransportController::SetMediaTransportSettings(
+ bool use_media_transport_for_media,
+ bool use_media_transport_for_data_channels) {
+ RTC_DCHECK(use_media_transport_for_media ==
+ config_.use_media_transport_for_media ||
jsep_transports_by_name_.empty())
- << "You can only call SetMediaTransportFactory before "
- "JsepTransportController created its first transport.";
- config_.media_transport_factory = media_transport_factory;
+ << "You can only change media transport configuration before creating "
+ "the first transport.";
+
+ RTC_DCHECK(use_media_transport_for_data_channels ==
+ config_.use_media_transport_for_data_channels ||
+ jsep_transports_by_name_.empty())
+ << "You can only change media transport configuration before creating "
+ "the first transport.";
+
+ config_.use_media_transport_for_media = use_media_transport_for_media;
+ config_.use_media_transport_for_data_channels =
+ use_media_transport_for_data_channels;
}
std::unique_ptr<cricket::IceTransportInternal>
@@ -433,7 +444,12 @@
RTC_DCHECK(network_thread_->IsCurrent());
std::unique_ptr<cricket::DtlsTransportInternal> dtls;
- if (is_media_transport_factory_enabled_ && config_.media_transport_factory) {
+ // If media transport is used for both media and data channels,
+ // then we don't need to create DTLS.
+ // Otherwise, DTLS is still created.
+ if (is_media_transport_factory_enabled_ && config_.media_transport_factory &&
+ config_.use_media_transport_for_media &&
+ config_.use_media_transport_for_data_channels) {
dtls = absl::make_unique<cricket::NoOpDtlsTransport>(
std::move(ice), config_.crypto_options);
} else if (config_.external_transport_factory) {
@@ -954,6 +970,11 @@
return nullptr;
}
+ if (!config_.use_media_transport_for_media &&
+ !config_.use_media_transport_for_data_channels) {
+ return nullptr;
+ }
+
absl::optional<cricket::CryptoParams> selected_crypto_for_media_transport;
if (content_info.media_description() &&
!content_info.media_description()->cryptos().empty()) {
diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h
index 08dea61..f28c10c 100644
--- a/pc/jsep_transport_controller.h
+++ b/pc/jsep_transport_controller.h
@@ -83,10 +83,19 @@
bool active_reset_srtp_params = false;
RtcEventLog* event_log = nullptr;
+ // Whether media transport is used for media.
+ bool use_media_transport_for_media = false;
+
+ // Whether media transport is used for data channels.
+ bool use_media_transport_for_data_channels = false;
+
// Optional media transport factory (experimental). If provided it will be
- // used to create media_transport and will be used to send / receive
- // audio and video frames instead of RTP. Note that currently
- // media_transport co-exists with RTP / RTCP transports and uses the same
+ // used to create media_transport (as long as either
+ // |use_media_transport_for_media| or
+ // |use_media_transport_for_data_channels| is set to true). However, whether
+ // it will be used to send / receive audio and video frames instead of RTP
+ // is determined by |use_media_transport_for_media|. Note that currently
+ // media_transport co-exists with RTP / RTCP transports and may use the same
// underlying ICE transport.
MediaTransportFactory* media_transport_factory = nullptr;
};
@@ -173,10 +182,11 @@
void SetActiveResetSrtpParams(bool active_reset_srtp_params);
// Allows to overwrite the settings from config. You may set or reset the
- // media transport factory on the jsep transport controller, as long as you
- // did not call 'GetMediaTransport' or 'MaybeCreateJsepTransport'. Once Jsep
- // transport is created, you can't change this setting.
- void SetMediaTransportFactory(MediaTransportFactory* media_transport_factory);
+ // media transport configuration on the jsep transport controller, as long as
+ // you did not call 'GetMediaTransport' or 'MaybeCreateJsepTransport'. Once
+ // Jsep transport is created, you can't change this setting.
+ void SetMediaTransportSettings(bool use_media_transport_for_media,
+ bool use_media_transport_for_data_channels);
// All of these signals are fired on the signaling thread.
diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc
index d6a31c8..43ba4ad 100644
--- a/pc/jsep_transport_controller_unittest.cc
+++ b/pc/jsep_transport_controller_unittest.cc
@@ -415,12 +415,48 @@
EXPECT_EQ(nullptr, transport_controller_->GetMediaTransport(kAudioMid1));
}
+TEST_F(JsepTransportControllerTest,
+ DtlsIsStillCreatedIfMediaTransportIsOnlyUsedForDataChannels) {
+ FakeMediaTransportFactory fake_media_transport_factory;
+ JsepTransportController::Config config;
+
+ config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyRequire;
+ config.media_transport_factory = &fake_media_transport_factory;
+ config.use_media_transport_for_data_channels = true;
+ CreateJsepTransportController(config);
+ auto description = CreateSessionDescriptionWithBundleGroup();
+ AddCryptoSettings(description.get());
+
+ EXPECT_TRUE(transport_controller_
+ ->SetLocalDescription(SdpType::kOffer, description.get())
+ .ok());
+
+ FakeMediaTransport* media_transport = static_cast<FakeMediaTransport*>(
+ transport_controller_->GetMediaTransport(kAudioMid1));
+
+ ASSERT_NE(nullptr, media_transport);
+
+ // After SetLocalDescription, media transport should be created as caller.
+ EXPECT_TRUE(media_transport->is_caller());
+ EXPECT_TRUE(media_transport->pre_shared_key().has_value());
+
+ // Return nullptr for non-existing mids.
+ EXPECT_EQ(nullptr, transport_controller_->GetMediaTransport(kVideoMid2));
+
+ EXPECT_EQ(cricket::ICE_CANDIDATE_COMPONENT_RTP,
+ transport_controller_->GetDtlsTransport(kAudioMid1)->component())
+ << "Media transport for media was not enabled, and so DTLS transport "
+ "should be created.";
+}
+
TEST_F(JsepTransportControllerTest, GetMediaTransportInCaller) {
FakeMediaTransportFactory fake_media_transport_factory;
JsepTransportController::Config config;
config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyRequire;
config.media_transport_factory = &fake_media_transport_factory;
+ config.use_media_transport_for_data_channels = true;
+ config.use_media_transport_for_media = true;
CreateJsepTransportController(config);
auto description = CreateSessionDescriptionWithBundleGroup();
AddCryptoSettings(description.get());
@@ -452,6 +488,8 @@
config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyRequire;
config.media_transport_factory = &fake_media_transport_factory;
+ config.use_media_transport_for_data_channels = true;
+ config.use_media_transport_for_media = true;
CreateJsepTransportController(config);
auto description = CreateSessionDescriptionWithBundleGroup();
AddCryptoSettings(description.get());
@@ -482,6 +520,7 @@
config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyNegotiate;
config.media_transport_factory = &fake_media_transport_factory;
+ config.use_media_transport_for_media = true;
CreateJsepTransportController(config);
auto description = CreateSessionDescriptionWithoutBundle();
EXPECT_TRUE(transport_controller_
@@ -512,6 +551,7 @@
config.rtcp_mux_policy = PeerConnectionInterface::kRtcpMuxPolicyRequire;
config.media_transport_factory = &fake_media_transport_factory;
+ config.use_media_transport_for_media = true;
CreateJsepTransportController(config);
auto description = CreateSessionDescriptionWithoutBundle();
AddCryptoSettings(description.get());
@@ -821,10 +861,12 @@
}
TEST_F(JsepTransportControllerTest,
- SignalConnectionStateConnectedWithMediaTransport) {
+ SignalConnectionStateConnectedWithMediaTransportAndNoDtls) {
FakeMediaTransportFactory fake_media_transport_factory;
JsepTransportController::Config config;
config.media_transport_factory = &fake_media_transport_factory;
+ config.use_media_transport_for_data_channels = true;
+ config.use_media_transport_for_media = true;
CreateJsepTransportController(config);
// Media Transport is only used with bundle.
@@ -865,10 +907,63 @@
}
TEST_F(JsepTransportControllerTest,
+ SignalConnectionStateConnectedWithMediaTransport) {
+ FakeMediaTransportFactory fake_media_transport_factory;
+ JsepTransportController::Config config;
+ config.media_transport_factory = &fake_media_transport_factory;
+ config.use_media_transport_for_media = true;
+ CreateJsepTransportController(config);
+
+ // Media Transport is only used with bundle.
+ auto description = CreateSessionDescriptionWithBundleGroup();
+ AddCryptoSettings(description.get());
+ EXPECT_TRUE(transport_controller_
+ ->SetLocalDescription(SdpType::kOffer, description.get())
+ .ok());
+
+ auto fake_audio_dtls = static_cast<FakeDtlsTransport*>(
+ transport_controller_->GetDtlsTransport(kAudioMid1));
+ auto fake_video_dtls = static_cast<FakeDtlsTransport*>(
+ transport_controller_->GetDtlsTransport(kVideoMid1));
+
+ auto fake_audio_ice = static_cast<cricket::FakeIceTransport*>(
+ transport_controller_->GetDtlsTransport(kAudioMid1)->ice_transport());
+ auto fake_video_ice = static_cast<cricket::FakeIceTransport*>(
+ transport_controller_->GetDtlsTransport(kVideoMid1)->ice_transport());
+ fake_audio_ice->SetConnectionCount(2);
+ fake_audio_ice->SetConnectionCount(1);
+ fake_video_ice->SetConnectionCount(2);
+ fake_video_ice->SetConnectionCount(1);
+ fake_audio_ice->SetWritable(true);
+ fake_video_ice->SetWritable(true);
+ fake_audio_dtls->SetWritable(true);
+ fake_video_dtls->SetWritable(true);
+
+ // Still not connected, because we are waiting for media transport.
+ EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_,
+ kTimeout);
+
+ FakeMediaTransport* media_transport = static_cast<FakeMediaTransport*>(
+ transport_controller_->GetMediaTransport(kAudioMid1));
+
+ media_transport->SetState(webrtc::MediaTransportState::kWritable);
+ EXPECT_EQ_WAIT(cricket::kIceConnectionConnecting, connection_state_,
+ kTimeout);
+
+ // Still waiting for the second media transport.
+ media_transport = static_cast<FakeMediaTransport*>(
+ transport_controller_->GetMediaTransport(kVideoMid1));
+ media_transport->SetState(webrtc::MediaTransportState::kWritable);
+
+ EXPECT_EQ_WAIT(cricket::kIceConnectionConnected, connection_state_, kTimeout);
+}
+
+TEST_F(JsepTransportControllerTest,
SignalConnectionStateFailedWhenMediaTransportClosed) {
FakeMediaTransportFactory fake_media_transport_factory;
JsepTransportController::Config config;
config.media_transport_factory = &fake_media_transport_factory;
+ config.use_media_transport_for_media = true;
CreateJsepTransportController(config);
auto description = CreateSessionDescriptionWithoutBundle();
AddCryptoSettings(description.get());
@@ -876,6 +971,11 @@
->SetLocalDescription(SdpType::kOffer, description.get())
.ok());
+ auto fake_audio_dtls = static_cast<FakeDtlsTransport*>(
+ transport_controller_->GetDtlsTransport(kAudioMid1));
+ auto fake_video_dtls = static_cast<FakeDtlsTransport*>(
+ transport_controller_->GetDtlsTransport(kVideoMid1));
+
auto fake_audio_ice = static_cast<cricket::FakeIceTransport*>(
transport_controller_->GetDtlsTransport(kAudioMid1)->ice_transport());
auto fake_video_ice = static_cast<cricket::FakeIceTransport*>(
@@ -888,6 +988,9 @@
fake_video_ice->SetConnectionCount(2);
fake_video_ice->SetConnectionCount(1);
+ fake_audio_dtls->SetWritable(true);
+ fake_video_dtls->SetWritable(true);
+
FakeMediaTransport* media_transport = static_cast<FakeMediaTransport*>(
transport_controller_->GetMediaTransport(kAudioMid1));
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc
index 4cdab68..2b4a48d 100644
--- a/pc/peer_connection.cc
+++ b/pc/peer_connection.cc
@@ -989,6 +989,9 @@
return false;
}
+ config.use_media_transport_for_media = configuration.use_media_transport;
+ config.use_media_transport_for_data_channels =
+ configuration.use_media_transport_for_data_channels;
config.media_transport_factory = factory_->media_transport_factory();
}
@@ -3207,11 +3210,9 @@
}
transport_controller_->SetIceConfig(ParseIceConfig(modified_config));
- transport_controller_->SetMediaTransportFactory(
- modified_config.use_media_transport ||
- modified_config.use_media_transport_for_data_channels
- ? factory_->media_transport_factory()
- : nullptr);
+ transport_controller_->SetMediaTransportSettings(
+ modified_config.use_media_transport,
+ modified_config.use_media_transport_for_data_channels);
if (configuration_.active_reset_srtp_params !=
modified_config.active_reset_srtp_params) {
@@ -6748,7 +6749,11 @@
sctp_transport_->SetDtlsTransport(dtls_transport);
}
- call_->MediaTransportChange(media_transport);
+ if (configuration_.use_media_transport) {
+ // Only pass media transport to call object if media transport is used
+ // for media (and not data channel).
+ call_->MediaTransportChange(media_transport);
+ }
return ret;
}
diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc
index dd7f1d8..4091006 100644
--- a/pc/peer_connection_integrationtest.cc
+++ b/pc/peer_connection_integrationtest.cc
@@ -3638,6 +3638,28 @@
EXPECT_GE(first_stats.sent_video_frames, second_stats.received_video_frames);
}
+TEST_P(PeerConnectionIntegrationTest,
+ MediaTransportDataChannelUsesRtpBidirectionalVideo) {
+ PeerConnectionInterface::RTCConfiguration rtc_config;
+ rtc_config.use_media_transport = false;
+ rtc_config.use_media_transport_for_data_channels = true;
+ rtc_config.enable_dtls_srtp = false; // SDES is required for media transport.
+ ASSERT_TRUE(CreatePeerConnectionWrappersWithConfigAndMediaTransportFactory(
+ rtc_config, rtc_config, loopback_media_transports()->first_factory(),
+ loopback_media_transports()->second_factory()));
+ ConnectFakeSignaling();
+
+ caller()->AddVideoTrack();
+ callee()->AddVideoTrack();
+ // Start offer/answer exchange and wait for it to complete.
+ caller()->CreateAndSetAndSignalOffer();
+ ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout);
+
+ MediaExpectations media_expectations;
+ media_expectations.ExpectBidirectionalVideo();
+ ASSERT_TRUE(ExpectNewFrames(media_expectations));
+}
+
// Test that the ICE connection and gathering states eventually reach
// "complete".
TEST_P(PeerConnectionIntegrationTest, IceStatesReachCompletion) {
