From bf282bc3d67fc85d0118bdf8dac2ba50dc23b7f9 Mon Sep 17 00:00:00 2001 From: Artem Titov Date: Mon, 25 Apr 2022 18:40:09 +0200 Subject: [PATCH] [PCLF] Store params as value and return const ref This is the first step in the next refactoring: 1. Make general peer params immutable and accessible only via const ref. 2. Extract params which can be changed during the call 3. Expose mutators for mutable params protecting them with proper locking. Bug: b/213863770 Change-Id: Ie3ac17918f1fed3b8ec84992f8b0afc402bce7f4 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/260003 Reviewed-by: Mirko Bonadei Commit-Queue: Artem Titov Cr-Commit-Position: refs/heads/main@{#36647} --- test/pc/e2e/media/media_helper.cc | 12 ++--- test/pc/e2e/peer_connection_quality_test.cc | 52 ++++++++++----------- test/pc/e2e/test_peer.cc | 7 ++- test/pc/e2e/test_peer.h | 13 ++++-- 4 files changed, 44 insertions(+), 40 deletions(-) diff --git a/test/pc/e2e/media/media_helper.cc b/test/pc/e2e/media/media_helper.cc index 7985bd0123..1689905a94 100644 --- a/test/pc/e2e/media/media_helper.cc +++ b/test/pc/e2e/media/media_helper.cc @@ -33,10 +33,10 @@ using CapturingDeviceIndex = ::webrtc::webrtc_pc_e2e:: } // namespace void MediaHelper::MaybeAddAudio(TestPeer* peer) { - if (!peer->params()->audio_config) { + if (!peer->params2().audio_config) { return; } - const AudioConfig& audio_config = peer->params()->audio_config.value(); + const AudioConfig& audio_config = peer->params2().audio_config.value(); rtc::scoped_refptr source = peer->pc_factory()->CreateAudioSource(audio_config.audio_options); rtc::scoped_refptr track = @@ -51,15 +51,15 @@ void MediaHelper::MaybeAddAudio(TestPeer* peer) { std::vector> MediaHelper::MaybeAddVideo(TestPeer* peer) { // Params here valid because of pre-run validation. - Params* params = peer->params(); + const Params& params = peer->params2(); std::vector> out; - for (size_t i = 0; i < params->video_configs.size(); ++i) { - auto video_config = params->video_configs[i]; + for (size_t i = 0; i < params.video_configs.size(); ++i) { + auto video_config = params.video_configs[i]; // Setup input video source into peer connection. std::unique_ptr capturer = CreateVideoCapturer( video_config, peer->ReleaseVideoSource(i), video_quality_analyzer_injection_helper_->CreateFramePreprocessor( - params->name.value(), video_config)); + params.name.value(), video_config)); bool is_screencast = video_config.content_hint == VideoTrackInterface::ContentHint::kText || video_config.content_hint == diff --git a/test/pc/e2e/peer_connection_quality_test.cc b/test/pc/e2e/peer_connection_quality_test.cc index 8729eeacc2..48411164f3 100644 --- a/test/pc/e2e/peer_connection_quality_test.cc +++ b/test/pc/e2e/peer_connection_quality_test.cc @@ -287,8 +287,8 @@ void PeerConnectionE2EQualityTest::Run(RunParams run_params) { video_quality_analyzer_injection_helper_->Start( test_case_name_, - std::vector{alice_->params()->name.value(), - bob_->params()->name.value()}, + std::vector{alice_->params2().name.value(), + bob_->params2().name.value()}, video_analyzer_threads); audio_quality_analyzer_->Start(test_case_name_, &analyzer_helper_); for (auto& reporter : quality_metrics_reporters_) { @@ -296,15 +296,15 @@ void PeerConnectionE2EQualityTest::Run(RunParams run_params) { } // Start RTCEventLog recording if requested. - if (alice_->params()->rtc_event_log_path) { + if (alice_->params2().rtc_event_log_path) { auto alice_rtc_event_log = std::make_unique( - alice_->params()->rtc_event_log_path.value()); + alice_->params2().rtc_event_log_path.value()); alice_->pc()->StartRtcEventLog(std::move(alice_rtc_event_log), webrtc::RtcEventLog::kImmediateOutput); } - if (bob_->params()->rtc_event_log_path) { + if (bob_->params2().rtc_event_log_path) { auto bob_rtc_event_log = std::make_unique( - bob_->params()->rtc_event_log_path.value()); + bob_->params2().rtc_event_log_path.value()); bob_->pc()->StartRtcEventLog(std::move(bob_rtc_event_log), webrtc::RtcEventLog::kImmediateOutput); } @@ -317,8 +317,8 @@ void PeerConnectionE2EQualityTest::Run(RunParams run_params) { return kAliveMessageLogInterval; }); - RTC_LOG(LS_INFO) << "Configuration is done. Now " << *alice_->params()->name - << " is calling to " << *bob_->params()->name << "..."; + RTC_LOG(LS_INFO) << "Configuration is done. Now " << *alice_->params2().name + << " is calling to " << *bob_->params2().name << "..."; // Setup stats poller. std::vector observers = { @@ -327,8 +327,8 @@ void PeerConnectionE2EQualityTest::Run(RunParams run_params) { for (auto& reporter : quality_metrics_reporters_) { observers.push_back(reporter.get()); } - StatsPoller stats_poller(observers, {{*alice_->params()->name, alice_.get()}, - {*bob_->params()->name, bob_.get()}}); + StatsPoller stats_poller(observers, {{*alice_->params2().name, alice_.get()}, + {*bob_->params2().name, bob_.get()}}); executor_->ScheduleActivity(TimeDelta::Zero(), kStatsUpdateInterval, [&stats_poller](TimeDelta) { stats_poller.PollStatsAndNotifyObservers(); @@ -456,7 +456,7 @@ void PeerConnectionE2EQualityTest::SetupCallOnSignalingThread( RtpTransceiverInit receive_only_transceiver_init; receive_only_transceiver_init.direction = RtpTransceiverDirection::kRecvOnly; int alice_transceivers_counter = 0; - if (bob_->params()->audio_config) { + if (bob_->params2().audio_config) { // Setup receive audio transceiver if Bob has audio to send. If we'll need // multiple audio streams, then we need transceiver for each Bob's audio // stream. @@ -468,13 +468,13 @@ void PeerConnectionE2EQualityTest::SetupCallOnSignalingThread( } size_t alice_video_transceivers_non_simulcast_counter = 0; - for (auto& video_config : alice_->params()->video_configs) { + for (auto& video_config : alice_->params2().video_configs) { RtpTransceiverInit transceiver_params; if (video_config.simulcast_config) { transceiver_params.direction = RtpTransceiverDirection::kSendOnly; - // Because simulcast enabled `alice_->params()->video_codecs` has only 1 + // Because simulcast enabled `alice_->params2().video_codecs` has only 1 // element. - if (alice_->params()->video_codecs[0].name == cricket::kVp8CodecName) { + if (alice_->params2().video_codecs[0].name == cricket::kVp8CodecName) { // For Vp8 simulcast we need to add as many RtpEncodingParameters to the // track as many simulcast streams requested. If they specified in // `video_config.simulcast_config` it should be copied from there. @@ -510,7 +510,7 @@ void PeerConnectionE2EQualityTest::SetupCallOnSignalingThread( // Add receive only transceivers in case Bob has more video_configs than // Alice. for (size_t i = alice_video_transceivers_non_simulcast_counter; - i < bob_->params()->video_configs.size(); ++i) { + i < bob_->params2().video_configs.size(); ++i) { RTCErrorOr> result = alice_->AddTransceiver(cricket::MediaType::MEDIA_TYPE_VIDEO, receive_only_transceiver_init); @@ -535,15 +535,15 @@ void PeerConnectionE2EQualityTest::TearDownCallOnSignalingThread() { void PeerConnectionE2EQualityTest::SetPeerCodecPreferences(TestPeer* peer) { std::vector with_rtx_video_capabilities = FilterVideoCodecCapabilities( - peer->params()->video_codecs, true, peer->params()->use_ulp_fec, - peer->params()->use_flex_fec, + peer->params2().video_codecs, true, peer->params2().use_ulp_fec, + peer->params2().use_flex_fec, peer->pc_factory() ->GetRtpSenderCapabilities(cricket::MediaType::MEDIA_TYPE_VIDEO) .codecs); std::vector without_rtx_video_capabilities = FilterVideoCodecCapabilities( - peer->params()->video_codecs, false, peer->params()->use_ulp_fec, - peer->params()->use_flex_fec, + peer->params2().video_codecs, false, peer->params2().use_ulp_fec, + peer->params2().use_flex_fec, peer->pc_factory() ->GetRtpSenderCapabilities(cricket::MediaType::MEDIA_TYPE_VIDEO) .codecs); @@ -572,7 +572,7 @@ PeerConnectionE2EQualityTest::CreateSignalingInterceptor( std::map stream_label_to_simulcast_streams_count; // We add only Alice here, because simulcast/svc is supported only from the // first peer. - for (auto& video_config : alice_->params()->video_configs) { + for (auto& video_config : alice_->params2().video_configs) { if (video_config.simulcast_config) { stream_label_to_simulcast_streams_count.insert( {*video_config.stream_label, @@ -621,7 +621,7 @@ void PeerConnectionE2EQualityTest::ExchangeOfferAnswer( offer->ToString(&log_output); RTC_LOG(LS_INFO) << "Original offer: " << log_output; LocalAndRemoteSdp patch_result = signaling_interceptor->PatchOffer( - std::move(offer), alice_->params()->video_codecs[0]); + std::move(offer), alice_->params2().video_codecs[0]); patch_result.local_sdp->ToString(&log_output); RTC_LOG(LS_INFO) << "Offer to set as local description: " << log_output; patch_result.remote_sdp->ToString(&log_output); @@ -638,7 +638,7 @@ void PeerConnectionE2EQualityTest::ExchangeOfferAnswer( answer->ToString(&log_output); RTC_LOG(LS_INFO) << "Original answer: " << log_output; patch_result = signaling_interceptor->PatchAnswer( - std::move(answer), bob_->params()->video_codecs[0]); + std::move(answer), bob_->params2().video_codecs[0]); patch_result.local_sdp->ToString(&log_output); RTC_LOG(LS_INFO) << "Answer to set as local description: " << log_output; patch_result.remote_sdp->ToString(&log_output); @@ -661,7 +661,7 @@ void PeerConnectionE2EQualityTest::ExchangeIceCandidates( for (auto& candidate : alice_candidates) { std::string candidate_str; RTC_CHECK(candidate->ToString(&candidate_str)); - RTC_LOG(LS_INFO) << *alice_->params()->name + RTC_LOG(LS_INFO) << *alice_->params2().name << " ICE candidate(mid= " << candidate->sdp_mid() << "): " << candidate_str; } @@ -672,7 +672,7 @@ void PeerConnectionE2EQualityTest::ExchangeIceCandidates( for (auto& candidate : bob_candidates) { std::string candidate_str; RTC_CHECK(candidate->ToString(&candidate_str)); - RTC_LOG(LS_INFO) << *bob_->params()->name + RTC_LOG(LS_INFO) << *bob_->params2().name << " ICE candidate(mid= " << candidate->sdp_mid() << "): " << candidate_str; } @@ -707,11 +707,11 @@ void PeerConnectionE2EQualityTest::TearDownCall() { } void PeerConnectionE2EQualityTest::ReportGeneralTestResults() { - test::PrintResult(*alice_->params()->name + "_connected", "", test_case_name_, + test::PrintResult(*alice_->params2().name + "_connected", "", test_case_name_, alice_connected_, "unitless", /*important=*/false, test::ImproveDirection::kBiggerIsBetter); - test::PrintResult(*bob_->params()->name + "_connected", "", test_case_name_, + test::PrintResult(*bob_->params2().name + "_connected", "", test_case_name_, bob_connected_, "unitless", /*important=*/false, test::ImproveDirection::kBiggerIsBetter); diff --git a/test/pc/e2e/test_peer.cc b/test/pc/e2e/test_peer.cc index 78347edfdb..02bbafc21a 100644 --- a/test/pc/e2e/test_peer.cc +++ b/test/pc/e2e/test_peer.cc @@ -50,8 +50,7 @@ bool TestPeer::SetRemoteDescription( pc()->SetRemoteDescription(std::move(desc), observer); RTC_CHECK(observer->is_called()); if (!observer->error().ok()) { - RTC_LOG(LS_ERROR) << *params_->name - << ": Failed to set remote description: " + RTC_LOG(LS_ERROR) << *params_.name << ": Failed to set remote description: " << observer->error().message(); if (error_out) { *error_out = observer->error().message(); @@ -96,11 +95,11 @@ TestPeer::TestPeer( std::vector video_sources, rtc::scoped_refptr audio_processing, std::unique_ptr worker_thread) - : worker_thread_(std::move(worker_thread)), + : params_(std::move(*params)), + worker_thread_(std::move(worker_thread)), wrapper_(std::make_unique(std::move(pc_factory), std::move(pc), std::move(observer))), - params_(std::move(params)), video_sources_(std::move(video_sources)), audio_processing_(audio_processing) {} diff --git a/test/pc/e2e/test_peer.h b/test/pc/e2e/test_peer.h index 0aadeb61aa..310725e83d 100644 --- a/test/pc/e2e/test_peer.h +++ b/test/pc/e2e/test_peer.h @@ -33,7 +33,12 @@ namespace webrtc_pc_e2e { // Describes a single participant in the call. class TestPeer final { public: - Params* params() const { return params_.get(); } + // TODO(titovartem): remove when downstream projects will stop using it. + Params* params() { return ¶ms_; } + + // TODO(titovartem): rename to params after removing the method above. + const Params& params2() const { return params_; } + PeerConfigurerImpl::VideoSource ReleaseVideoSource(size_t i) { RTC_CHECK(wrapper_) << "TestPeer is already closed"; return std::move(video_sources_[i]); @@ -57,11 +62,11 @@ class TestPeer final { void CreateOffer( rtc::scoped_refptr observer) { RTC_CHECK(wrapper_) << "TestPeer is already closed"; - pc()->CreateOffer(observer.release(), params_->rtc_offer_answer_options); + pc()->CreateOffer(observer.release(), params_.rtc_offer_answer_options); } std::unique_ptr CreateOffer() { RTC_CHECK(wrapper_) << "TestPeer is already closed"; - return wrapper_->CreateOffer(params_->rtc_offer_answer_options); + return wrapper_->CreateOffer(params_.rtc_offer_answer_options); } std::unique_ptr CreateAnswer() { @@ -145,10 +150,10 @@ class TestPeer final { std::unique_ptr worker_thread); private: + Params params_; // Keeps ownership of worker thread. It has to be destroyed after `wrapper_`. std::unique_ptr worker_thread_; std::unique_ptr wrapper_; - std::unique_ptr params_; std::vector video_sources_; rtc::scoped_refptr audio_processing_;