From a9bbd8684936dedbd4ca8f6402644d7a2e152cbe Mon Sep 17 00:00:00 2001 From: Bjorn Mellem Date: Fri, 2 Nov 2018 09:07:48 -0700 Subject: [PATCH] Add a configuration parameter for using the media transport for data channels. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a field |use_media_transport_for_data_channels| to RTCConfiguration. PeerConnection requires a media transport factory to be set if this bit is set. As with |use_media_transport|, the value may not be modified after setting the local or remote description. If either |use_media_transport| or |use_media_transport_for_data_channel| is set, PeerConnection uses its media transport factory when creating a JSEP transport controller. PeerConnection stops unconditionally using media transport in CreateVoiceChannel, as it may be present only for use in data channels. It uses the media transport if it is present and |use_media_transport| is set. Bug: webrtc:9719 Change-Id: I59d4ce8f7531fd19d9c17eefe033f063f663ebcc Reviewed-on: https://webrtc-review.googlesource.com/c/109041 Reviewed-by: Sami Kalliomäki Reviewed-by: Kári Helgason Reviewed-by: Steve Anton Reviewed-by: Peter Slatala Commit-Queue: Bjorn Mellem Cr-Commit-Position: refs/heads/master@{#25507} --- api/peerconnectioninterface.h | 9 +++ pc/peerconnection.cc | 40 +++++++++-- pc/peerconnection.h | 5 +- pc/peerconnection_media_unittest.cc | 68 +++++++++++++++++++ pc/peerconnectioninterface_unittest.cc | 2 + .../api/org/webrtc/PeerConnection.java | 12 ++++ sdk/android/src/jni/pc/peerconnection.cc | 3 + .../api/peerconnection/RTCConfiguration.h | 6 ++ .../api/peerconnection/RTCConfiguration.mm | 3 + 9 files changed, 141 insertions(+), 7 deletions(-) diff --git a/api/peerconnectioninterface.h b/api/peerconnectioninterface.h index 6504f534e3..80c3091d23 100644 --- a/api/peerconnectioninterface.h +++ b/api/peerconnectioninterface.h @@ -583,6 +583,15 @@ class PeerConnectionInterface : public rtc::RefCountInterface { // provided. bool use_media_transport = false; + // If MediaTransportFactory is provided in PeerConnectionFactory, this flag + // informs PeerConnection that it should use the MediaTransportInterface for + // data channels. It's invalid to set it to |true| if the + // MediaTransportFactory wasn't provided. Data channels over media + // transport are not compatible with RTP or SCTP data channels. Setting + // both |use_media_transport_for_data_channels| and + // |enable_rtp_data_channel| is invalid. + bool use_media_transport_for_data_channels = false; + // Defines advanced optional cryptographic settings related to SRTP and // frame encryption for native WebRTC. Setting this will overwrite any // settings set in PeerConnectionFactory (which is deprecated). diff --git a/pc/peerconnection.cc b/pc/peerconnection.cc index 08766445a9..3c5a025f22 100644 --- a/pc/peerconnection.cc +++ b/pc/peerconnection.cc @@ -706,6 +706,7 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( absl::optional network_preference; bool active_reset_srtp_params; bool use_media_transport; + bool use_media_transport_for_data_channels; absl::optional crypto_options; }; static_assert(sizeof(stuff_being_tested_for_equality) == sizeof(*this), @@ -756,6 +757,8 @@ bool PeerConnectionInterface::RTCConfiguration::operator==( network_preference == o.network_preference && active_reset_srtp_params == o.active_reset_srtp_params && use_media_transport == o.use_media_transport && + use_media_transport_for_data_channels == + o.use_media_transport_for_data_channels && crypto_options == o.crypto_options; } @@ -946,11 +949,13 @@ bool PeerConnection::Initialize( #endif config.active_reset_srtp_params = configuration.active_reset_srtp_params; - if (configuration.use_media_transport) { + if (configuration.use_media_transport || + configuration.use_media_transport_for_data_channels) { if (!factory_->media_transport_factory()) { RTC_DCHECK(false) - << "PeerConnecton is initialized with use_media_transport = true, " - << "but media transport factory is not set in PeerConnectioFactory"; + << "PeerConnecton is initialized with use_media_transport = true or " + << "use_media_transport_for_data_channels = true " + << "but media transport factory is not set in PeerConnectionFactory"; return false; } @@ -2919,6 +2924,22 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration, return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error); } + if (local_description() && + configuration.use_media_transport_for_data_channels != + configuration_.use_media_transport_for_data_channels) { + RTC_LOG(LS_ERROR) << "Can't change media_transport_for_data_channels " + "after calling SetLocalDescription."; + return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error); + } + + if (remote_description() && + configuration.use_media_transport_for_data_channels != + configuration_.use_media_transport_for_data_channels) { + RTC_LOG(LS_ERROR) << "Can't change media_transport_for_data_channels " + "after calling SetRemoteDescription."; + return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error); + } + if (local_description() && configuration.crypto_options != configuration_.crypto_options) { RTC_LOG(LS_ERROR) << "Can't change crypto_options after calling " @@ -2951,6 +2972,8 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration, modified_config.active_reset_srtp_params = configuration.active_reset_srtp_params; modified_config.use_media_transport = configuration.use_media_transport; + modified_config.use_media_transport_for_data_channels = + configuration.use_media_transport_for_data_channels; if (configuration != modified_config) { RTC_LOG(LS_ERROR) << "Modifying the configuration in an unsupported way."; return SafeSetError(RTCErrorType::INVALID_MODIFICATION, error); @@ -3009,8 +3032,10 @@ bool PeerConnection::SetConfiguration(const RTCConfiguration& configuration, transport_controller_->SetIceConfig(ParseIceConfig(modified_config)); transport_controller_->SetMediaTransportFactory( - modified_config.use_media_transport ? factory_->media_transport_factory() - : nullptr); + modified_config.use_media_transport || + modified_config.use_media_transport_for_data_channels + ? factory_->media_transport_factory() + : nullptr); if (configuration_.active_reset_srtp_params != modified_config.active_reset_srtp_params) { @@ -5597,7 +5622,10 @@ RTCError PeerConnection::CreateChannels(const SessionDescription& desc) { cricket::VoiceChannel* PeerConnection::CreateVoiceChannel( const std::string& mid) { RtpTransportInternal* rtp_transport = GetRtpTransport(mid); - MediaTransportInterface* media_transport = GetMediaTransport(mid); + MediaTransportInterface* media_transport = nullptr; + if (configuration_.use_media_transport) { + media_transport = GetMediaTransport(mid); + } cricket::VoiceChannel* voice_channel = channel_manager()->CreateVoiceChannel( call_.get(), configuration_.media_config, rtp_transport, media_transport, diff --git a/pc/peerconnection.h b/pc/peerconnection.h index e725798aca..4099992a0e 100644 --- a/pc/peerconnection.h +++ b/pc/peerconnection.h @@ -938,10 +938,13 @@ class PeerConnection : public PeerConnectionInternal, // to use media transport. Otherwise returns nullptr. MediaTransportInterface* GetMediaTransport(const std::string& mid) { auto media_transport = transport_controller_->GetMediaTransport(mid); - RTC_DCHECK(configuration_.use_media_transport == + RTC_DCHECK((configuration_.use_media_transport || + configuration_.use_media_transport_for_data_channels) == (media_transport != nullptr)) << "configuration_.use_media_transport=" << configuration_.use_media_transport + << ", configuration_.use_media_transport_for_data_channels=" + << configuration_.use_media_transport_for_data_channels << ", (media_transport != nullptr)=" << (media_transport != nullptr); return media_transport; } diff --git a/pc/peerconnection_media_unittest.cc b/pc/peerconnection_media_unittest.cc index e01ebb83ca..3c069aea7a 100644 --- a/pc/peerconnection_media_unittest.cc +++ b/pc/peerconnection_media_unittest.cc @@ -1129,6 +1129,74 @@ TEST_P(PeerConnectionMediaTest, MediaTransportPropagatedToVoiceEngine) { ASSERT_EQ(nullptr, callee_video->media_transport()); } +TEST_P(PeerConnectionMediaTest, MediaTransportOnlyForDataChannels) { + RTCConfiguration config; + + // Setup PeerConnection to use media transport for data channels. + config.use_media_transport_for_data_channels = true; + + // Force SDES. + config.enable_dtls_srtp = false; + + auto caller = CreatePeerConnectionWithAudioVideo(config); + auto callee = CreatePeerConnectionWithAudioVideo(config); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); + + auto caller_voice = caller->media_engine()->GetVoiceChannel(0); + auto callee_voice = callee->media_engine()->GetVoiceChannel(0); + ASSERT_TRUE(caller_voice); + ASSERT_TRUE(callee_voice); + + // Make sure media transport is not propagated to voice channel. + EXPECT_EQ(nullptr, caller_voice->media_transport()); + EXPECT_EQ(nullptr, callee_voice->media_transport()); +} + +TEST_P(PeerConnectionMediaTest, MediaTransportForMediaAndDataChannels) { + RTCConfiguration config; + + // Setup PeerConnection to use media transport for both media and data + // channels. + config.use_media_transport = true; + config.use_media_transport_for_data_channels = true; + + // Force SDES. + config.enable_dtls_srtp = false; + + auto caller = CreatePeerConnectionWithAudioVideo(config); + auto callee = CreatePeerConnectionWithAudioVideo(config); + + ASSERT_TRUE(callee->SetRemoteDescription(caller->CreateOfferAndSetAsLocal())); + ASSERT_TRUE(callee->SetLocalDescription(callee->CreateAnswer())); + + auto caller_voice = caller->media_engine()->GetVoiceChannel(0); + auto callee_voice = callee->media_engine()->GetVoiceChannel(0); + ASSERT_TRUE(caller_voice); + ASSERT_TRUE(callee_voice); + + // Make sure media transport is propagated to voice channel. + FakeMediaTransport* caller_voice_media_transport = + static_cast(caller_voice->media_transport()); + FakeMediaTransport* callee_voice_media_transport = + static_cast(callee_voice->media_transport()); + ASSERT_NE(nullptr, caller_voice_media_transport); + ASSERT_NE(nullptr, callee_voice_media_transport); + + // Make sure media transport is created with correct is_caller. + EXPECT_TRUE(caller_voice_media_transport->is_caller()); + EXPECT_FALSE(callee_voice_media_transport->is_caller()); + + // TODO(sukhanov): Propagate media transport to video channel. This test + // will fail once media transport is propagated to video channel and it will + // serve as a reminder to add a test for video channel propagation. + auto caller_video = caller->media_engine()->GetVideoChannel(0); + auto callee_video = callee->media_engine()->GetVideoChannel(0); + ASSERT_EQ(nullptr, caller_video->media_transport()); + ASSERT_EQ(nullptr, callee_video->media_transport()); +} + TEST_P(PeerConnectionMediaTest, MediaTransportNotPropagatedToVoiceEngine) { auto caller = CreatePeerConnectionWithAudioVideo(); auto callee = CreatePeerConnectionWithAudioVideo(); diff --git a/pc/peerconnectioninterface_unittest.cc b/pc/peerconnectioninterface_unittest.cc index 55932b6f40..12d192eba3 100644 --- a/pc/peerconnectioninterface_unittest.cc +++ b/pc/peerconnectioninterface_unittest.cc @@ -1431,12 +1431,14 @@ TEST_P(PeerConnectionInterfaceTest, GetConfigurationAfterSetConfiguration) { PeerConnectionInterface::RTCConfiguration config = pc_->GetConfiguration(); config.type = PeerConnectionInterface::kRelay; config.use_media_transport = true; + config.use_media_transport_for_data_channels = true; EXPECT_TRUE(pc_->SetConfiguration(config)); PeerConnectionInterface::RTCConfiguration returned_config = pc_->GetConfiguration(); EXPECT_EQ(PeerConnectionInterface::kRelay, returned_config.type); EXPECT_TRUE(returned_config.use_media_transport); + EXPECT_TRUE(returned_config.use_media_transport_for_data_channels); } TEST_P(PeerConnectionInterfaceTest, SetConfigurationFailsAfterClose) { diff --git a/sdk/android/api/org/webrtc/PeerConnection.java b/sdk/android/api/org/webrtc/PeerConnection.java index 1cb5717b4e..dac32c86f9 100644 --- a/sdk/android/api/org/webrtc/PeerConnection.java +++ b/sdk/android/api/org/webrtc/PeerConnection.java @@ -468,6 +468,12 @@ public class PeerConnection { */ public boolean useMediaTransport; + /* + * Experimental flag that enables a use of media transport for data channels. If this is true, + * the media transport factory MUST be provided to the PeerConnectionFactory. + */ + public boolean useMediaTransportForDataChannels; + /** * Defines advanced optional cryptographic settings related to SRTP and * frame encryption for native WebRTC. Setting this will overwrite any @@ -515,6 +521,7 @@ public class PeerConnection { sdpSemantics = SdpSemantics.PLAN_B; activeResetSrtpParams = false; useMediaTransport = false; + useMediaTransportForDataChannels = false; cryptoOptions = null; } @@ -720,6 +727,11 @@ public class PeerConnection { return useMediaTransport; } + @CalledByNative("RTCConfiguration") + boolean getUseMediaTransportForDataChannels() { + return useMediaTransportForDataChannels; + } + @Nullable @CalledByNative("RTCConfiguration") CryptoOptions getCryptoOptions() { diff --git a/sdk/android/src/jni/pc/peerconnection.cc b/sdk/android/src/jni/pc/peerconnection.cc index 0d51847575..162f809e6e 100644 --- a/sdk/android/src/jni/pc/peerconnection.cc +++ b/sdk/android/src/jni/pc/peerconnection.cc @@ -249,6 +249,9 @@ void JavaToNativeRTCConfiguration( Java_RTCConfiguration_getActiveResetSrtpParams(jni, j_rtc_config); rtc_config->use_media_transport = Java_RTCConfiguration_getUseMediaTransport(jni, j_rtc_config); + rtc_config->use_media_transport_for_data_channels = + Java_RTCConfiguration_getUseMediaTransportForDataChannels(jni, + j_rtc_config); rtc_config->crypto_options = JavaToNativeOptionalCryptoOptions(jni, j_crypto_options); } diff --git a/sdk/objc/api/peerconnection/RTCConfiguration.h b/sdk/objc/api/peerconnection/RTCConfiguration.h index fcaa07a4f5..f1400813d6 100644 --- a/sdk/objc/api/peerconnection/RTCConfiguration.h +++ b/sdk/objc/api/peerconnection/RTCConfiguration.h @@ -188,6 +188,12 @@ RTC_OBJC_EXPORT */ @property(nonatomic, assign) BOOL useMediaTransport; +/** + * If MediaTransportFactory is provided in PeerConnectionFactory, this flag informs PeerConnection + * that it should use the MediaTransportInterface for data channels. + */ +@property(nonatomic, assign) BOOL useMediaTransportForDataChannels; + /** * Defines advanced optional cryptographic settings related to SRTP and * frame encryption for native WebRTC. Setting this will overwrite any diff --git a/sdk/objc/api/peerconnection/RTCConfiguration.mm b/sdk/objc/api/peerconnection/RTCConfiguration.mm index 83f32b4c12..55bf26f38d 100644 --- a/sdk/objc/api/peerconnection/RTCConfiguration.mm +++ b/sdk/objc/api/peerconnection/RTCConfiguration.mm @@ -51,6 +51,7 @@ @synthesize turnCustomizer = _turnCustomizer; @synthesize activeResetSrtpParams = _activeResetSrtpParams; @synthesize useMediaTransport = _useMediaTransport; +@synthesize useMediaTransportForDataChannels = _useMediaTransportForDataChannels; @synthesize cryptoOptions = _cryptoOptions; - (instancetype)init { @@ -100,6 +101,7 @@ _iceBackupCandidatePairPingInterval = config.ice_backup_candidate_pair_ping_interval; _useMediaTransport = config.use_media_transport; + _useMediaTransportForDataChannels = config.use_media_transport_for_data_channels; _keyType = RTCEncryptionKeyTypeECDSA; _iceCandidatePoolSize = config.ice_candidate_pool_size; _shouldPruneTurnPorts = config.prune_turn_ports; @@ -199,6 +201,7 @@ nativeConfig->ice_backup_candidate_pair_ping_interval = _iceBackupCandidatePairPingInterval; nativeConfig->use_media_transport = _useMediaTransport; + nativeConfig->use_media_transport_for_data_channels = _useMediaTransportForDataChannels; rtc::KeyType keyType = [[self class] nativeEncryptionKeyTypeForKeyType:_keyType]; if (_certificate != nullptr) {