From 105ded358bb0df4a771a4509870e89b068b906d4 Mon Sep 17 00:00:00 2001 From: "Piotr (Peter) Slatala" Date: Wed, 27 Feb 2019 14:26:15 -0800 Subject: [PATCH] Pass the x-mt line from SDP to the media transport If x-mt line is present (one or more), and the first line is dedicated for the media transport that we support, pass the config down to this media transport. In the future we will do 3 changes: 1) Add MediaTransportFactory::IsSupported(config) to let the implementation decide whether the current factory can support a given setting 2) Add support for multiple x-mt lines. Right now the support is minimal: we only look at the first line (because we only allow single media transport factory). In the future, when RtpMediaTransport is introduced, this may and will change. 3) Allow multiple MediaTransportFactories and add fallback to RTP if media transport is not supported. Current solution provides backward compatibility for the 2 above extensions. Bug: webrtc:9719 Change-Id: I82a469fecda57effc95d7d8191f4a9e4a01d199c Reviewed-on: https://webrtc-review.googlesource.com/c/124800 Commit-Queue: Peter Slatala Reviewed-by: Bjorn Mellem Reviewed-by: Steve Anton Cr-Commit-Position: refs/heads/master@{#26882} --- api/test/fake_media_transport.h | 5 ++ pc/jsep_transport_controller.cc | 19 +++++- pc/jsep_transport_controller.h | 7 +- pc/jsep_transport_controller_unittest.cc | 81 ++++++++++++++++++++++++ 4 files changed, 107 insertions(+), 5 deletions(-) diff --git a/api/test/fake_media_transport.h b/api/test/fake_media_transport.h index 16cd1bad87..6f87dfcb01 100644 --- a/api/test/fake_media_transport.h +++ b/api/test/fake_media_transport.h @@ -98,6 +98,9 @@ class FakeMediaTransport : public MediaTransportInterface { int target_rate_observers_size() { return target_rate_observers_.size(); } + // Settings that were passed down to fake media transport. + const MediaTransportSettings& settings() { return settings_; } + private: const MediaTransportSettings settings_; MediaTransportStateCallback* state_callback_; @@ -110,6 +113,8 @@ class FakeMediaTransportFactory : public MediaTransportFactory { FakeMediaTransportFactory() = default; ~FakeMediaTransportFactory() = default; + std::string GetTransportName() const override { return "fake"; } + RTCErrorOr> CreateMediaTransport( rtc::PacketTransportInternal* packet_transport, rtc::Thread* network_thread, diff --git a/pc/jsep_transport_controller.cc b/pc/jsep_transport_controller.cc index eb78f3473a..c1b8e4ed88 100644 --- a/pc/jsep_transport_controller.cc +++ b/pc/jsep_transport_controller.cc @@ -596,7 +596,7 @@ RTCError JsepTransportController::ApplyDescription_n( (IsBundled(content_info.name) && content_info.name != *bundled_mid())) { continue; } - error = MaybeCreateJsepTransport(local, content_info); + error = MaybeCreateJsepTransport(local, content_info, *description); if (!error.ok()) { return error; } @@ -958,6 +958,7 @@ cricket::JsepTransport* JsepTransportController::GetJsepTransportByName( std::unique_ptr JsepTransportController::MaybeCreateMediaTransport( const cricket::ContentInfo& content_info, + const cricket::SessionDescription& description, bool local, cricket::IceTransportInternal* ice_transport) { if (!is_media_transport_factory_enabled_) { @@ -998,6 +999,7 @@ JsepTransportController::MaybeCreateMediaTransport( return nullptr; } + // TODO(psla): This code will be removed in favor of media transport settings. // Note that we ignore here lifetime and length. // In fact we take those bits (inline, lifetime and length) and keep it as // part of key derivation. @@ -1031,6 +1033,7 @@ JsepTransportController::MaybeCreateMediaTransport( rtc::ArrayView( reinterpret_cast(label.data()), label.size()), kDerivedKeyByteSize); + // TODO(psla): End of the code to be removed. // We want to crash the app if we don't have a key, and not silently fall // back to the unsecure communication. @@ -1042,6 +1045,15 @@ JsepTransportController::MaybeCreateMediaTransport( if (config_.use_media_transport_for_media) { settings.event_log = config_.event_log; } + + // Assume there is only one media transport (or if more, use the first one). + if (!local && !description.MediaTransportSettings().empty() && + config_.media_transport_factory->GetTransportName() == + description.MediaTransportSettings()[0].transport_name) { + settings.remote_transport_parameters = + description.MediaTransportSettings()[0].transport_setting; + } + auto media_transport_result = config_.media_transport_factory->CreateMediaTransport( ice_transport, network_thread_, settings); @@ -1054,7 +1066,8 @@ JsepTransportController::MaybeCreateMediaTransport( RTCError JsepTransportController::MaybeCreateJsepTransport( bool local, - const cricket::ContentInfo& content_info) { + const cricket::ContentInfo& content_info, + const cricket::SessionDescription& description) { RTC_DCHECK(network_thread_->IsCurrent()); cricket::JsepTransport* transport = GetJsepTransportByName(content_info.name); if (transport) { @@ -1073,7 +1086,7 @@ RTCError JsepTransportController::MaybeCreateJsepTransport( CreateIceTransport(content_info.name, /*rtcp=*/false); std::unique_ptr media_transport = - MaybeCreateMediaTransport(content_info, local, ice.get()); + MaybeCreateMediaTransport(content_info, description, local, ice.get()); std::unique_ptr rtp_dtls_transport = CreateDtlsTransport(std::move(ice)); diff --git a/pc/jsep_transport_controller.h b/pc/jsep_transport_controller.h index f28c10c2db..1363907377 100644 --- a/pc/jsep_transport_controller.h +++ b/pc/jsep_transport_controller.h @@ -283,14 +283,17 @@ class JsepTransportController : public sigslot::has_slots<> { // Transport is created either during SetLocalDescription (|local| == true) or // during SetRemoteDescription (|local| == false). Passing |local| helps to // differentiate initiator (caller) from answerer (callee). - RTCError MaybeCreateJsepTransport(bool local, - const cricket::ContentInfo& content_info); + RTCError MaybeCreateJsepTransport( + bool local, + const cricket::ContentInfo& content_info, + const cricket::SessionDescription& description); // Creates media transport if config wants to use it, and pre-shared key is // provided in content info. It modifies the config to disable media transport // if pre-shared key is not provided. std::unique_ptr MaybeCreateMediaTransport( const cricket::ContentInfo& content_info, + const cricket::SessionDescription& description, bool local, cricket::IceTransportInternal* ice_transport); void MaybeDestroyJsepTransport(const std::string& mid); diff --git a/pc/jsep_transport_controller_unittest.cc b/pc/jsep_transport_controller_unittest.cc index 43ba4ad6b4..590f57dab2 100644 --- a/pc/jsep_transport_controller_unittest.cc +++ b/pc/jsep_transport_controller_unittest.cc @@ -514,6 +514,87 @@ TEST_F(JsepTransportControllerTest, GetMediaTransportInCallee) { << "Because media transport is used, expected no-op DTLS transport."; } +TEST_F(JsepTransportControllerTest, GetMediaTransportInCalleePassesSdp) { + 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()); + description->AddMediaTransportSetting("fake", "this-is-a-test-setting"); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kOffer, description.get()) + .ok()); + + FakeMediaTransport* media_transport = static_cast( + transport_controller_->GetMediaTransport(kAudioMid1)); + + ASSERT_NE(nullptr, media_transport); + + EXPECT_EQ("this-is-a-test-setting", + media_transport->settings().remote_transport_parameters); +} + +// Caller ignores its own outgoing parameters. +TEST_F(JsepTransportControllerTest, + GetMediaTransportInCallerIgnoresXmtSection) { + 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()); + description->AddMediaTransportSetting("fake", "this-is-a-test-setting"); + EXPECT_TRUE(transport_controller_ + ->SetLocalDescription(SdpType::kOffer, description.get()) + .ok()); + + FakeMediaTransport* media_transport = static_cast( + transport_controller_->GetMediaTransport(kAudioMid1)); + + ASSERT_NE(nullptr, media_transport); + + // Remote parameters are nullopt, because we are the offerer (we don't) + // have the remote transport parameters, only ours. + EXPECT_EQ(absl::nullopt, + media_transport->settings().remote_transport_parameters); +} + +TEST_F(JsepTransportControllerTest, + GetMediaTransportInCalleeIgnoresDifferentTransport) { + 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()); + description->AddMediaTransportSetting("not-a-fake-transport", + "this-is-a-test-setting"); + EXPECT_TRUE(transport_controller_ + ->SetRemoteDescription(SdpType::kOffer, description.get()) + .ok()); + + FakeMediaTransport* media_transport = static_cast( + transport_controller_->GetMediaTransport(kAudioMid1)); + + ASSERT_NE(nullptr, media_transport); + + EXPECT_EQ(absl::nullopt, + media_transport->settings().remote_transport_parameters); +} + TEST_F(JsepTransportControllerTest, GetMediaTransportIsNotSetIfNoSdes) { FakeMediaTransportFactory fake_media_transport_factory; JsepTransportController::Config config;