From 35ba0c5cd58f1d922b68ba06cfe2ccd5bb407a59 Mon Sep 17 00:00:00 2001 From: Harald Alvestrand Date: Thu, 5 May 2022 07:37:41 +0000 Subject: [PATCH] Check that PC is configured for media before doing media operations. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If media_engine is not passed in init parameters, the PC can't handle media, but can be used for datachannels. This CL adds testing that datachannels work without media engine, and adds failure returns to PeerConnection APIs that manipulate media when media engine is not present. Bug: webrtc:13931 Change-Id: Iecdf17a0a0bb89e0ad39eb74d6ed077303b875c2 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/261246 Commit-Queue: Harald Alvestrand Reviewed-by: Henrik Boström Cr-Commit-Position: refs/heads/main@{#36778} --- pc/data_channel_integrationtest.cc | 10 +++---- pc/peer_connection.cc | 38 +++++++++++++++++++++++++++ pc/peer_connection.h | 6 +++-- pc/peer_connection_integrationtest.cc | 7 +++++ pc/test/integration_test_helpers.h | 30 +++++++++++++++++---- 5 files changed, 79 insertions(+), 12 deletions(-) diff --git a/pc/data_channel_integrationtest.cc b/pc/data_channel_integrationtest.cc index 9d18adeb7f..2a936335b9 100644 --- a/pc/data_channel_integrationtest.cc +++ b/pc/data_channel_integrationtest.cc @@ -202,7 +202,7 @@ TEST_P(DataChannelIntegrationTest, EndToEndCallWithSctpDataChannel) { // data channel only, and sends messages of various sizes. TEST_P(DataChannelIntegrationTest, EndToEndCallWithSctpDataChannelVariousSizes) { - ASSERT_TRUE(CreatePeerConnectionWrappers()); + ASSERT_TRUE(CreatePeerConnectionWrappersWithoutMediaEngine()); ConnectFakeSignaling(); // Expect that data channel created on caller side will show up for callee as // well. @@ -241,7 +241,7 @@ TEST_P(DataChannelIntegrationTest, // data channel only, and sends empty messages TEST_P(DataChannelIntegrationTest, EndToEndCallWithSctpDataChannelEmptyMessages) { - ASSERT_TRUE(CreatePeerConnectionWrappers()); + ASSERT_TRUE(CreatePeerConnectionWrappersWithoutMediaEngine()); ConnectFakeSignaling(); // Expect that data channel created on caller side will show up for callee as // well. @@ -291,7 +291,7 @@ TEST_P(DataChannelIntegrationTest, // this test does not use TURN. const size_t kLowestSafePayloadSizeLimit = 1225; - ASSERT_TRUE(CreatePeerConnectionWrappers()); + ASSERT_TRUE(CreatePeerConnectionWrappersWithoutMediaEngine()); ConnectFakeSignaling(); // Expect that data channel created on caller side will show up for callee as // well. @@ -328,7 +328,7 @@ TEST_P(DataChannelIntegrationTest, EndToEndCallWithSctpDataChannelHarmfulMtu) { // The size of the smallest message that fails to be delivered. const size_t kMessageSizeThatIsNotDelivered = 1157; - ASSERT_TRUE(CreatePeerConnectionWrappers()); + ASSERT_TRUE(CreatePeerConnectionWrappersWithoutMediaEngine()); ConnectFakeSignaling(); caller()->CreateDataChannel(); caller()->CreateAndSetAndSignalOffer(); @@ -429,7 +429,7 @@ TEST_P(DataChannelIntegrationTest, StressTestUnorderedSctpDataChannel) { virtual_socket_server()->set_delay_stddev(5); virtual_socket_server()->UpdateDelayDistribution(); // Normal procedure, but with unordered data channel config. - ASSERT_TRUE(CreatePeerConnectionWrappers()); + ASSERT_TRUE(CreatePeerConnectionWrappersWithoutMediaEngine()); ConnectFakeSignaling(); webrtc::DataChannelInit init; init.ordered = false; diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 19cb1933cc..f914fc8365 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc @@ -829,11 +829,16 @@ bool PeerConnection::AddStream(MediaStreamInterface* local_stream) { RTC_CHECK(!IsUnifiedPlan()) << "AddStream is not available with Unified Plan " "SdpSemantics. Please use AddTrack instead."; TRACE_EVENT0("webrtc", "PeerConnection::AddStream"); + if (!ConfiguredForMedia()) { + RTC_LOG(LS_ERROR) << "AddStream: Not configured for media"; + return false; + } return sdp_handler_->AddStream(local_stream); } void PeerConnection::RemoveStream(MediaStreamInterface* local_stream) { RTC_DCHECK_RUN_ON(signaling_thread()); + RTC_DCHECK(ConfiguredForMedia()); RTC_CHECK(!IsUnifiedPlan()) << "RemoveStream is not available with Unified " "Plan SdpSemantics. Please use RemoveTrack " "instead."; @@ -846,6 +851,10 @@ RTCErrorOr> PeerConnection::AddTrack( const std::vector& stream_ids) { RTC_DCHECK_RUN_ON(signaling_thread()); TRACE_EVENT0("webrtc", "PeerConnection::AddTrack"); + if (!ConfiguredForMedia()) { + LOG_AND_RETURN_ERROR(RTCErrorType::UNSUPPORTED_OPERATION, + "Not configured for media"); + } if (!track) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, "Track is null."); } @@ -874,6 +883,10 @@ RTCErrorOr> PeerConnection::AddTrack( RTCError PeerConnection::RemoveTrackOrError( rtc::scoped_refptr sender) { RTC_DCHECK_RUN_ON(signaling_thread()); + if (!ConfiguredForMedia()) { + LOG_AND_RETURN_ERROR(RTCErrorType::UNSUPPORTED_OPERATION, + "Not configured for media"); + } if (!sender) { LOG_AND_RETURN_ERROR(RTCErrorType::INVALID_PARAMETER, "Sender is null."); } @@ -923,6 +936,11 @@ PeerConnection::FindTransceiverBySender( RTCErrorOr> PeerConnection::AddTransceiver( rtc::scoped_refptr track) { + if (!ConfiguredForMedia()) { + LOG_AND_RETURN_ERROR(RTCErrorType::UNSUPPORTED_OPERATION, + "Not configured for media"); + } + return AddTransceiver(track, RtpTransceiverInit()); } @@ -944,6 +962,10 @@ PeerConnection::AddTransceiver( rtc::scoped_refptr track, const RtpTransceiverInit& init) { RTC_DCHECK_RUN_ON(signaling_thread()); + if (!ConfiguredForMedia()) { + LOG_AND_RETURN_ERROR(RTCErrorType::UNSUPPORTED_OPERATION, + "Not configured for media"); + } RTC_CHECK(IsUnifiedPlan()) << "AddTransceiver is only available with Unified Plan SdpSemantics"; if (!track) { @@ -970,6 +992,10 @@ RTCErrorOr> PeerConnection::AddTransceiver(cricket::MediaType media_type, const RtpTransceiverInit& init) { RTC_DCHECK_RUN_ON(signaling_thread()); + if (!ConfiguredForMedia()) { + LOG_AND_RETURN_ERROR(RTCErrorType::UNSUPPORTED_OPERATION, + "Not configured for media"); + } RTC_CHECK(IsUnifiedPlan()) << "AddTransceiver is only available with Unified Plan SdpSemantics"; if (!(media_type == cricket::MEDIA_TYPE_AUDIO || @@ -987,6 +1013,10 @@ PeerConnection::AddTransceiver( const RtpTransceiverInit& init, bool update_negotiation_needed) { RTC_DCHECK_RUN_ON(signaling_thread()); + if (!ConfiguredForMedia()) { + LOG_AND_RETURN_ERROR(RTCErrorType::UNSUPPORTED_OPERATION, + "Not configured for media"); + } RTC_DCHECK((media_type == cricket::MEDIA_TYPE_AUDIO || media_type == cricket::MEDIA_TYPE_VIDEO)); if (track) { @@ -1095,6 +1125,10 @@ rtc::scoped_refptr PeerConnection::CreateSender( const std::string& kind, const std::string& stream_id) { RTC_DCHECK_RUN_ON(signaling_thread()); + if (!ConfiguredForMedia()) { + RTC_LOG(LS_ERROR) << "Not configured for media"; + return nullptr; + } RTC_CHECK(!IsUnifiedPlan()) << "CreateSender is not available with Unified " "Plan SdpSemantics. Please use AddTransceiver " "instead."; @@ -1677,6 +1711,10 @@ void PeerConnection::AddAdaptationResource( call_->AddAdaptationResource(resource); } +bool PeerConnection::ConfiguredForMedia() const { + return context_->channel_manager()->media_engine(); +} + bool PeerConnection::StartRtcEventLog(std::unique_ptr output, int64_t output_period_ms) { return worker_thread()->Invoke( diff --git a/pc/peer_connection.h b/pc/peer_connection.h index 52ad382d95..2907cb565b 100644 --- a/pc/peer_connection.h +++ b/pc/peer_connection.h @@ -430,6 +430,10 @@ class PeerConnection : public PeerConnectionInternal, RTC_RUN_ON(network_thread()); void TeardownDataChannelTransport_n() override RTC_RUN_ON(network_thread()); + const FieldTrialsView& trials() const override { return *trials_; } + + bool ConfiguredForMedia() const; + // Functions made public for testing. void ReturnHistogramVeryQuicklyForTesting() { RTC_DCHECK_RUN_ON(signaling_thread()); @@ -437,8 +441,6 @@ class PeerConnection : public PeerConnectionInternal, } void RequestUsagePatternReportForTesting(); - const FieldTrialsView& trials() const override { return *trials_; } - protected: // Available for rtc::scoped_refptr creation PeerConnection(rtc::scoped_refptr context, diff --git a/pc/peer_connection_integrationtest.cc b/pc/peer_connection_integrationtest.cc index 7866e075e8..6017f54dd4 100644 --- a/pc/peer_connection_integrationtest.cc +++ b/pc/peer_connection_integrationtest.cc @@ -3675,6 +3675,13 @@ TEST_P(PeerConnectionIntegrationTest, NewTracksDoNotCauseNewCandidates) { ASSERT_TRUE_WAIT(SignalingStateStable(), kDefaultTimeout); } +TEST_P(PeerConnectionIntegrationTest, MediaCallWithoutMediaEngineFails) { + ASSERT_TRUE(CreatePeerConnectionWrappersWithoutMediaEngine()); + // AddTrack should fail. + EXPECT_FALSE( + caller()->pc()->AddTrack(caller()->CreateLocalAudioTrack(), {}).ok()); +} + INSTANTIATE_TEST_SUITE_P( PeerConnectionIntegrationTest, PeerConnectionIntegrationInteropTest, diff --git a/pc/test/integration_test_helpers.h b/pc/test/integration_test_helpers.h index d8713196d0..525e2d4909 100644 --- a/pc/test/integration_test_helpers.h +++ b/pc/test/integration_test_helpers.h @@ -735,7 +735,8 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, rtc::Thread* worker_thread, std::unique_ptr event_log_factory, bool reset_encoder_factory, - bool reset_decoder_factory) { + bool reset_decoder_factory, + bool create_media_engine) { // There's an error in this test code if Init ends up being called twice. RTC_DCHECK(!peer_connection_); RTC_DCHECK(!peer_connection_factory_); @@ -782,8 +783,10 @@ class PeerConnectionIntegrationWrapper : public webrtc::PeerConnectionObserver, media_deps.trials = pc_factory_dependencies.trials.get(); - pc_factory_dependencies.media_engine = - cricket::CreateMediaEngine(std::move(media_deps)); + if (create_media_engine) { + pc_factory_dependencies.media_engine = + cricket::CreateMediaEngine(std::move(media_deps)); + } pc_factory_dependencies.call_factory = webrtc::CreateCallFactory(); if (event_log_factory) { event_log_factory_ = event_log_factory.get(); @@ -1435,7 +1438,8 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { webrtc::PeerConnectionDependencies dependencies, std::unique_ptr event_log_factory, bool reset_encoder_factory, - bool reset_decoder_factory) { + bool reset_decoder_factory, + bool create_media_engine = true) { RTCConfiguration modified_config; if (config) { modified_config = *config; @@ -1451,7 +1455,7 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { if (!client->Init(options, &modified_config, std::move(dependencies), network_thread_.get(), worker_thread_.get(), std::move(event_log_factory), reset_encoder_factory, - reset_decoder_factory)) { + reset_decoder_factory, create_media_engine)) { return nullptr; } return client; @@ -1590,6 +1594,22 @@ class PeerConnectionIntegrationBaseTest : public ::testing::Test { return caller_ && callee_; } + bool CreatePeerConnectionWrappersWithoutMediaEngine() { + caller_ = CreatePeerConnectionWrapper( + "Caller", nullptr, nullptr, webrtc::PeerConnectionDependencies(nullptr), + nullptr, + /*reset_encoder_factory=*/false, + /*reset_decoder_factory=*/false, + /*create_media_engine=*/false); + callee_ = CreatePeerConnectionWrapper( + "Callee", nullptr, nullptr, webrtc::PeerConnectionDependencies(nullptr), + nullptr, + /*reset_encoder_factory=*/false, + /*reset_decoder_factory=*/false, + /*create_media_engine=*/false); + return caller_ && callee_; + } + cricket::TestTurnServer* CreateTurnServer( rtc::SocketAddress internal_address, rtc::SocketAddress external_address,