From c623495fd1ff90aada0eb625af91ec17843fefd0 Mon Sep 17 00:00:00 2001 From: Tommi Date: Fri, 8 May 2020 20:59:05 +0200 Subject: [PATCH] Remove playout delay lock. Now update the playout delay and related stats on the worker thread. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This was previously reviewed here: https://webrtc-review.googlesource.com/c/src/+/172929/ With the exception of reducing unnecessarily broad lock scope in one function in rtp_rtcp_impl.cc and added comments in rtp_streams_synchronizer.h Bug: webrtc:11489 Change-Id: I77807b5da2accfe774255d9409542d358f288993 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/174200 Commit-Queue: Tommi Reviewed-by: Erik Språng Cr-Commit-Position: refs/heads/master@{#31193} --- modules/rtp_rtcp/source/rtp_rtcp_impl.cc | 6 ++- video/rtp_streams_synchronizer.h | 5 +++ video/video_receive_stream2.cc | 49 ++++++++++++++---------- video/video_receive_stream2.h | 25 +++++++----- 4 files changed, 52 insertions(+), 33 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 1cb61f5a61..4f84b0247d 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -774,8 +774,10 @@ std::vector ModuleRtpRtcpImpl::BoundingSet(bool* tmmbr_owner) { } void ModuleRtpRtcpImpl::set_rtt_ms(int64_t rtt_ms) { - rtc::CritScope cs(&critical_section_rtt_); - rtt_ms_ = rtt_ms; + { + rtc::CritScope cs(&critical_section_rtt_); + rtt_ms_ = rtt_ms; + } if (rtp_sender_) { rtp_sender_->packet_history.SetRtt(rtt_ms); } diff --git a/video/rtp_streams_synchronizer.h b/video/rtp_streams_synchronizer.h index 60e2c8ee32..00ef526dc5 100644 --- a/video/rtp_streams_synchronizer.h +++ b/video/rtp_streams_synchronizer.h @@ -25,6 +25,11 @@ namespace webrtc { class Syncable; +// TODO(bugs.webrtc.org/11489): Remove dependency on ProcessThread/Module. +// Instead make this a single threaded class, constructed on a TQ and +// post a 1 sec timer there. There shouldn't be a need for locking internally +// and the callback from this class, should occur on the construction TQ +// which in turn means that the callback doesn't need locking either. class RtpStreamsSynchronizer : public Module { public: explicit RtpStreamsSynchronizer(Syncable* syncable_video); diff --git a/video/video_receive_stream2.cc b/video/video_receive_stream2.cc index 0af17d5a45..19ca958fc7 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -274,6 +274,7 @@ VideoReceiveStream2::~VideoReceiveStream2() { RTC_LOG(LS_INFO) << "~VideoReceiveStream2: " << config_.ToString(); Stop(); process_thread_->DeRegisterModule(&rtp_stream_sync_); + task_safety_flag_->SetNotAlive(); } void VideoReceiveStream2::SignalNetworkState(NetworkState state) { @@ -477,8 +478,6 @@ bool VideoReceiveStream2::SetBaseMinimumPlayoutDelayMs(int delay_ms) { return false; } - // TODO(bugs.webrtc.org/11489): Consider posting to worker. - rtc::CritScope cs(&playout_delay_lock_); base_minimum_playout_delay_ms_ = delay_ms; UpdatePlayoutDelays(); return true; @@ -486,8 +485,6 @@ bool VideoReceiveStream2::SetBaseMinimumPlayoutDelayMs(int delay_ms) { int VideoReceiveStream2::GetBaseMinimumPlayoutDelayMs() const { RTC_DCHECK_RUN_ON(&worker_sequence_checker_); - - rtc::CritScope cs(&playout_delay_lock_); return base_minimum_playout_delay_ms_; } @@ -549,21 +546,18 @@ void VideoReceiveStream2::OnCompleteFrame( } last_complete_frame_time_ms_ = time_now_ms; - // TODO(bugs.webrtc.org/11489): We grab the playout_delay_lock_ lock - // potentially twice. Consider checking both min/max and posting to worker if - // there's a change. If we always update playout delays on the worker, we - // don't need a lock. const PlayoutDelay& playout_delay = frame->EncodedImage().playout_delay_; - if (playout_delay.min_ms >= 0) { - rtc::CritScope cs(&playout_delay_lock_); - frame_minimum_playout_delay_ms_ = playout_delay.min_ms; - UpdatePlayoutDelays(); - } - - if (playout_delay.max_ms >= 0) { - rtc::CritScope cs(&playout_delay_lock_); - frame_maximum_playout_delay_ms_ = playout_delay.max_ms; - UpdatePlayoutDelays(); + if (playout_delay.min_ms >= 0 || playout_delay.max_ms >= 0) { + worker_thread_->PostTask(ToQueuedTask( + task_safety_flag_, + [min_ms = playout_delay.min_ms, max_ms = playout_delay.max_ms, this]() { + RTC_DCHECK_RUN_ON(&worker_sequence_checker_); + if (min_ms >= 0) + frame_minimum_playout_delay_ms_ = min_ms; + if (max_ms >= 0) + frame_maximum_playout_delay_ms_ = max_ms; + UpdatePlayoutDelays(); + })); } int64_t last_continuous_pid = frame_buffer_->InsertFrame(std::move(frame)); @@ -607,9 +601,21 @@ void VideoReceiveStream2::SetEstimatedPlayoutNtpTimestampMs( } void VideoReceiveStream2::SetMinimumPlayoutDelay(int delay_ms) { - RTC_DCHECK_RUN_ON(&module_process_sequence_checker_); - // TODO(bugs.webrtc.org/11489): Consider posting to worker. - rtc::CritScope cs(&playout_delay_lock_); + // TODO(bugs.webrtc.org/11489): Currently called back on the module process + // thread because of RtpStreamsSynchronizer or |rtp_stream_sync_|. Once we + // change RtpStreamsSynchronizer to be single threaded, this call should + // always occur on the worker thread. Use of |rtp_stream_sync_| should all + // move to the worker thread, which will remove a lot of locks and take + // blocking work off of the decoder thread. + if (!worker_thread_->IsCurrent()) { + RTC_DCHECK_RUN_ON(&module_process_sequence_checker_); + worker_thread_->PostTask( + ToQueuedTask(task_safety_flag_, + [delay_ms, this]() { SetMinimumPlayoutDelay(delay_ms); })); + return; + } + + RTC_DCHECK_RUN_ON(&worker_sequence_checker_); syncable_minimum_playout_delay_ms_ = delay_ms; UpdatePlayoutDelays(); } @@ -731,6 +737,7 @@ bool VideoReceiveStream2::IsReceivingKeyFrame(int64_t timestamp_ms) const { } void VideoReceiveStream2::UpdatePlayoutDelays() const { + RTC_DCHECK_RUN_ON(&worker_sequence_checker_); const int minimum_delay_ms = std::max({frame_minimum_playout_delay_ms_, base_minimum_playout_delay_ms_, syncable_minimum_playout_delay_ms_}); diff --git a/video/video_receive_stream2.h b/video/video_receive_stream2.h index 2a0c07c879..22a246956a 100644 --- a/video/video_receive_stream2.h +++ b/video/video_receive_stream2.h @@ -26,6 +26,7 @@ #include "modules/video_coding/video_receiver2.h" #include "rtc_base/synchronization/sequence_checker.h" #include "rtc_base/task_queue.h" +#include "rtc_base/task_utils/pending_task_safety_flag.h" #include "system_wrappers/include/clock.h" #include "video/receive_statistics_proxy2.h" #include "video/rtp_streams_synchronizer.h" @@ -134,8 +135,7 @@ class VideoReceiveStream2 : public webrtc::VideoReceiveStream, void HandleEncodedFrame(std::unique_ptr frame) RTC_RUN_ON(decode_queue_); void HandleFrameBufferTimeout() RTC_RUN_ON(decode_queue_); - void UpdatePlayoutDelays() const - RTC_EXCLUSIVE_LOCKS_REQUIRED(playout_delay_lock_); + void UpdatePlayoutDelays() const; void RequestKeyFrame(int64_t timestamp_ms) RTC_RUN_ON(decode_queue_); void HandleKeyFrameGeneration(bool received_frame_is_keyframe, int64_t now_ms) RTC_RUN_ON(decode_queue_); @@ -200,22 +200,23 @@ class VideoReceiveStream2 : public webrtc::VideoReceiveStream, const int max_wait_for_keyframe_ms_; const int max_wait_for_frame_ms_; - rtc::CriticalSection playout_delay_lock_; - // All of them tries to change current min_playout_delay on |timing_| but // source of the change request is different in each case. Among them the // biggest delay is used. -1 means use default value from the |timing_|. // // Minimum delay as decided by the RTP playout delay extension. - int frame_minimum_playout_delay_ms_ RTC_GUARDED_BY(playout_delay_lock_) = -1; - // Minimum delay as decided by the setLatency function in "webrtc/api". - int base_minimum_playout_delay_ms_ RTC_GUARDED_BY(playout_delay_lock_) = -1; - // Minimum delay as decided by the A/V synchronization feature. - int syncable_minimum_playout_delay_ms_ RTC_GUARDED_BY(playout_delay_lock_) = + int frame_minimum_playout_delay_ms_ RTC_GUARDED_BY(worker_sequence_checker_) = -1; + // Minimum delay as decided by the setLatency function in "webrtc/api". + int base_minimum_playout_delay_ms_ RTC_GUARDED_BY(worker_sequence_checker_) = + -1; + // Minimum delay as decided by the A/V synchronization feature. + int syncable_minimum_playout_delay_ms_ + RTC_GUARDED_BY(worker_sequence_checker_) = -1; // Maximum delay as decided by the RTP playout delay extension. - int frame_maximum_playout_delay_ms_ RTC_GUARDED_BY(playout_delay_lock_) = -1; + int frame_maximum_playout_delay_ms_ RTC_GUARDED_BY(worker_sequence_checker_) = + -1; // Function that is triggered with encoded frames, if not empty. std::function @@ -225,6 +226,10 @@ class VideoReceiveStream2 : public webrtc::VideoReceiveStream, // Defined last so they are destroyed before all other members. rtc::TaskQueue decode_queue_; + + // Used to signal destruction to potentially pending tasks. + PendingTaskSafetyFlag::Pointer task_safety_flag_ = + PendingTaskSafetyFlag::Create(); }; } // namespace internal } // namespace webrtc