Fetch sender parameters asynchronously during GetStats Resolve an issue where calling GetStats could block while fetching send parameters. Previously, fetching sender parameters relied on `GetParametersInternal`, which blocks when cached parameters are unavailable. This moves the retrieval of sender RTP parameters to the worker thread. Parameters are now fetched directly from the media send channel during the asynchronous stats collection phase. This change avoids the thread blocking on the signaling thread and re-enables the `RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS` checks. Bug: webrtc:492108787 Change-Id: I941bed643ff46c4e52b96ae9fbc08a64d7cff7c6 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/456580 Reviewed-by: Henrik Boström <hbos@webrtc.org> Commit-Queue: Tomas Gunnarsson <tommi@webrtc.org> Cr-Commit-Position: refs/heads/main@{#47166}
diff --git a/pc/peer_connection.cc b/pc/peer_connection.cc index 78a9142..e2a4ade 100644 --- a/pc/peer_connection.cc +++ b/pc/peer_connection.cc
@@ -1375,10 +1375,7 @@ TRACE_EVENT0("webrtc", "PeerConnection::GetStats"); RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(callback); - // TODO: bugs.webrtc.org/492108787 - Re-enable this when fixing the - // call to `sender->internal()->GetParametersInternal()` which can - // block when cached parameters aren't available. - // RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS(); + RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS(); scoped_refptr<RtpSenderInternal> internal_sender; if (selector) { for (const auto& proxy_transceiver : @@ -1410,10 +1407,7 @@ TRACE_EVENT0("webrtc", "PeerConnection::GetStats"); RTC_DCHECK_RUN_ON(signaling_thread()); RTC_DCHECK(callback); - // TODO: bugs.webrtc.org/492108787 - Re-enable this when fixing the - // call to `sender->internal()->GetParametersInternal()` which can - // block when cached parameters aren't available. - // RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS(); + RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS(); scoped_refptr<RtpReceiverInternal> internal_receiver; if (selector) { for (const auto& proxy_transceiver :
diff --git a/pc/rtc_stats_collector.cc b/pc/rtc_stats_collector.cc index ea0ae61..6a2325b 100644 --- a/pc/rtc_stats_collector.cc +++ b/pc/rtc_stats_collector.cc
@@ -2266,10 +2266,10 @@ absl::AnyInvocable<RTCStatsCollector::WorkerThreadResult()> RTCStatsCollector::PrepareTransceiverStatsInfosAndCallStats_s_w() { RTC_DCHECK_RUN_ON(signaling_thread_); + RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS(); std::vector<RtpTransceiverStatsInfo> transceiver_stats_infos; std::vector<TransceiverReferences> transceiver_references; - std::vector<std::vector<RtpParameters>> all_sender_parameters; // These are used to invoke GetStats for all the media channels together in // one worker thread hop. std::map<VoiceMediaSendChannelInterface*, VoiceMediaSendInfo> @@ -2297,16 +2297,12 @@ scoped_refptr<RtpTransceiver>(transceiver)}; RTC_ALLOW_PLAN_B_DEPRECATION_BEGIN() - std::vector<RtpParameters> sender_parameters; for (const auto& sender : transceiver->senders()) { stats.sender_infos.push_back( {.ssrc = sender->ssrc(), .attachment_id = sender->internal()->AttachmentId(), .media_type = sender->media_type()}); - sender_parameters.push_back(sender->internal()->GetParametersInternal( - /*may_use_cache=*/true, /*with_all_layers=*/true)); } - all_sender_parameters.push_back(std::move(sender_parameters)); for (const auto& receiver : transceiver->receivers()) { stats.receiver_infos.push_back( {.track_id = receiver->track() ? receiver->track()->id() : "", @@ -2344,7 +2340,6 @@ // which also needs info from the worker thread. return [this, transceiver_stats_infos = std::move(transceiver_stats_infos), transceiver_references = std::move(transceiver_references), - all_sender_parameters = std::move(all_sender_parameters), voice_send_stats = std::move(voice_send_stats), voice_receive_stats = std::move(voice_receive_stats), video_send_stats = std::move(video_send_stats), @@ -2354,7 +2349,6 @@ WorkerThreadResult worker_result; worker_result.results.transceiver_stats_infos = std::move(transceiver_stats_infos); - worker_result.results.sender_parameters = std::move(all_sender_parameters); worker_result.transceiver_references = std::move(transceiver_references); for (auto& pair : voice_send_stats) { @@ -2394,6 +2388,27 @@ continue; } + std::vector<RtpParameters> sender_parameters; + for (const auto& sender : stats.sender_infos) { + RtpParameters params; + if (sender.ssrc != 0 && stats.has_channel) { + auto& transceiver = refs.transceiver; + MediaSendChannelInterface* media_channel = + (stats.media_type == MediaType::AUDIO) + ? static_cast<MediaSendChannelInterface*>( + transceiver->voice_media_send_channel()) + : static_cast<MediaSendChannelInterface*>( + transceiver->video_media_send_channel()); + if (media_channel) { + params = media_channel->GetRtpSendParameters(sender.ssrc); + } + } + // `sender_parameters` must be the same size as `stats.sender_infos` to + // allow 1:1 mapping when TrackMediaInfoMap is constructed. Empty params + // are safely ignored since they have no encodings/SSRCs. + sender_parameters.push_back(std::move(params)); + } + std::vector<RtpParameters> receiver_parameters; for (const auto& receiver : refs.receivers) { receiver_parameters.push_back(receiver->GetParameters()); @@ -2422,8 +2437,7 @@ stats.track_media_info_map = std::make_unique<TrackMediaInfoMap>( std::move(voice_media_info), std::move(video_media_info), - std::move(stats.sender_infos), - std::move(worker_result.results.sender_parameters[i]), + std::move(stats.sender_infos), std::move(sender_parameters), std::move(stats.receiver_infos), std::move(receiver_parameters)); if (stats.media_type == MediaType::AUDIO) { has_audio_receiver |= stats.has_receivers;
diff --git a/pc/rtc_stats_collector.h b/pc/rtc_stats_collector.h index 5103c6e..c873006 100644 --- a/pc/rtc_stats_collector.h +++ b/pc/rtc_stats_collector.h
@@ -156,13 +156,13 @@ private: struct StatsGatheringResults { std::vector<RtpTransceiverStatsInfo> transceiver_stats_infos; - std::vector<std::vector<RtpParameters>> sender_parameters; Call::Stats call_stats; std::optional<AudioDeviceModule::Stats> audio_device_stats; }; struct WorkerThreadResult { StatsGatheringResults results; + std::vector<std::vector<RtpParameters>> sender_parameters; std::vector<TransceiverReferences> transceiver_references; }; struct CollectionContext;
diff --git a/pc/rtc_stats_collector_unittest.cc b/pc/rtc_stats_collector_unittest.cc index e3ffa34..c044f87 100644 --- a/pc/rtc_stats_collector_unittest.cc +++ b/pc/rtc_stats_collector_unittest.cc
@@ -26,6 +26,7 @@ #include "absl/strings/str_replace.h" #include "api/audio/audio_device.h" #include "api/audio/audio_processing_statistics.h" +#include "api/audio_options.h" #include "api/candidate.h" #include "api/data_channel_interface.h" #include "api/dtls_transport_interface.h" @@ -54,6 +55,7 @@ #include "common_video/include/quality_limitation_reason.h" #include "json/reader.h" #include "json/value.h" +#include "media/base/fake_media_engine.h" #include "media/base/media_channel.h" #include "media/base/stream_params.h" #include "modules/rtp_rtcp/include/report_block_data.h" @@ -65,6 +67,7 @@ #include "p2p/base/transport_description.h" #include "pc/media_stream.h" #include "pc/peer_connection_internal.h" +#include "pc/rtp_sender.h" #include "pc/sctp_data_channel.h" #include "pc/stream_collection.h" #include "pc/test/fake_audio_track.h" @@ -2693,14 +2696,14 @@ auto video_sender = stats_->SetupLocalTrackAndSender( MediaType::VIDEO, "LocalVideoTrackID", 1, true, /*attachment_id=*/50); - EXPECT_CALL(*video_sender, GetParametersInternal(_, _)).WillRepeatedly([] { - RtpParameters params; - params.encodings.push_back(RtpEncodingParameters()); - params.encodings[0].ssrc = 1; - params.encodings.push_back(RtpEncodingParameters()); - params.encodings[1].ssrc = 2; - return params; - }); + + // Set up the fake video channel to return the expected RtpParameters + // containing both simulcast encodings for the primary SSRC. + StreamParams sp; + sp.add_ssrc(1); + sp.add_ssrc(2); + sp.ssrc_groups.push_back(SsrcGroup(kSimSsrcGroupSemantics, {1, 2})); + video_media_channels.first->AddSendStream(sp); scoped_refptr<const RTCStatsReport> report = stats_->GetStatsReport(main_thread_); @@ -3823,6 +3826,52 @@ EXPECT_FALSE(receiver_report->Get(graph.media_source_id)); } +TEST_P(RTCStatsCollectorTest, + GetStatsDoesNotBlockWhenFetchingSenderParameters) { + // We must create a new FakePeerConnectionForStats with a separate worker + // thread because the test base uses the current thread for all three, which + // bypasses the thread hop and avoids the + // RTC_DCHECK_DISALLOW_THREAD_BLOCKING_CALLS check. + auto worker_thread = Thread::Create(); + worker_thread->Start(); + pc_ = make_ref_counted<FakePeerConnectionForStats>(env_, worker_thread.get()); + SetStatsTimestampWithEnvironmentClock(GetParam()); + // Create a track and a real sender to ensure we test the actual blocking + // behavior in RtpSenderBase::GetParametersInternal. + scoped_refptr<MediaStreamTrackInterface> track = CreateFakeTrack( + MediaType::AUDIO, "audioTrack", MediaStreamTrackInterface::kLive); + + auto fake_media_channel = std::make_unique<FakeVoiceMediaSendChannel>( + webrtc::AudioOptions(), pc_->network_thread()); + + scoped_refptr<AudioRtpSender> sender = AudioRtpSender::Create( + env_, pc_->signaling_thread(), pc_->worker_thread(), "sender_id", + /*stats=*/nullptr, /*set_streams_observer=*/nullptr, + /*media_channel=*/nullptr); + + worker_thread->BlockingCall([&] { + fake_media_channel->AddSendStream(StreamParams::CreateLegacy(1234)); + sender->SetMediaChannel(fake_media_channel.get()); + }); + + sender->SetSsrc(1234); + sender->SetTrack(track.get()); + + RTC_ALLOW_PLAN_B_DEPRECATION_BEGIN(); + pc_->AddSender(sender); + RTC_ALLOW_PLAN_B_DEPRECATION_END(); + + scoped_refptr<const RTCStatsReport> report; + stats_->stats_collector().GetStatsReport( + RTCStatsObtainer::Create(&report, [this] { main_thread_.Quit(); })); + main_thread_.Run(); + + sender->Stop(); + sender = nullptr; + stats_.reset(); + pc_ = nullptr; +} + TEST_P(RTCStatsCollectorTest, GetStatsWithNullSenderSelector) { ExampleStatsGraph graph = SetupExampleStatsGraphForSelectorTests(); scoped_refptr<const RTCStatsReport> empty_report;