From 6a871d34878a0e301925e243ec994c7c2e88d136 Mon Sep 17 00:00:00 2001 From: Tommi Date: Sat, 9 May 2020 20:16:08 +0000 Subject: [PATCH] Revert "Remove playout delay lock." MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit c623495fd1ff90aada0eb625af91ec17843fefd0. Reason for revert: Need to look into failure in remoting_unittests in Chrome (Webrtc/ConnectionTest.SecondCaptureFailed/0). It looks like the order FrameBuffer2 calls into VCMTiming while receiving frames and updating playout delay values, needs to be synchronized better. Original change's description: > Remove playout delay lock. > Now update the playout delay and related stats on the worker thread. > > 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} TBR=tommi@webrtc.org,sprang@webrtc.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: webrtc:11489 Change-Id: I9149025d2fc10686314e6d4e89d1b92125650c36 Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/174757 Reviewed-by: Tommi Commit-Queue: Tommi Cr-Commit-Position: refs/heads/master@{#31197} --- 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 | 23 +++++------ 4 files changed, 32 insertions(+), 51 deletions(-) diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc index 4f84b0247d..1cb61f5a61 100644 --- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc +++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc @@ -774,10 +774,8 @@ 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 00ef526dc5..60e2c8ee32 100644 --- a/video/rtp_streams_synchronizer.h +++ b/video/rtp_streams_synchronizer.h @@ -25,11 +25,6 @@ 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 19ca958fc7..0af17d5a45 100644 --- a/video/video_receive_stream2.cc +++ b/video/video_receive_stream2.cc @@ -274,7 +274,6 @@ 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) { @@ -478,6 +477,8 @@ 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; @@ -485,6 +486,8 @@ 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_; } @@ -546,18 +549,21 @@ 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 || 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(); - })); + 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(); } int64_t last_continuous_pid = frame_buffer_->InsertFrame(std::move(frame)); @@ -601,21 +607,9 @@ void VideoReceiveStream2::SetEstimatedPlayoutNtpTimestampMs( } void VideoReceiveStream2::SetMinimumPlayoutDelay(int delay_ms) { - // 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_); + RTC_DCHECK_RUN_ON(&module_process_sequence_checker_); + // TODO(bugs.webrtc.org/11489): Consider posting to worker. + rtc::CritScope cs(&playout_delay_lock_); syncable_minimum_playout_delay_ms_ = delay_ms; UpdatePlayoutDelays(); } @@ -737,7 +731,6 @@ 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 22a246956a..2a0c07c879 100644 --- a/video/video_receive_stream2.h +++ b/video/video_receive_stream2.h @@ -26,7 +26,6 @@ #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" @@ -135,7 +134,8 @@ 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; + void UpdatePlayoutDelays() const + RTC_EXCLUSIVE_LOCKS_REQUIRED(playout_delay_lock_); 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,23 +200,22 @@ 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(worker_sequence_checker_) = - -1; + 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(worker_sequence_checker_) = - -1; + 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(worker_sequence_checker_) = -1; + int syncable_minimum_playout_delay_ms_ RTC_GUARDED_BY(playout_delay_lock_) = + -1; // Maximum delay as decided by the RTP playout delay extension. - int frame_maximum_playout_delay_ms_ RTC_GUARDED_BY(worker_sequence_checker_) = - -1; + int frame_maximum_playout_delay_ms_ RTC_GUARDED_BY(playout_delay_lock_) = -1; // Function that is triggered with encoded frames, if not empty. std::function @@ -226,10 +225,6 @@ 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