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 <tommi@webrtc.org>
Reviewed-by: Erik Språng <sprang@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#31193}
diff --git a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
index 1cb61f5..4f84b02 100644
--- a/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
+++ b/modules/rtp_rtcp/source/rtp_rtcp_impl.cc
@@ -774,8 +774,10 @@
}
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 60e2c8e..00ef526 100644
--- a/video/rtp_streams_synchronizer.h
+++ b/video/rtp_streams_synchronizer.h
@@ -25,6 +25,11 @@
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 0af17d5..19ca958 100644
--- a/video/video_receive_stream2.cc
+++ b/video/video_receive_stream2.cc
@@ -274,6 +274,7 @@
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 @@
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 @@
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 @@
}
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::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 @@
}
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 2a0c07c..22a2469 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 @@
void HandleEncodedFrame(std::unique_ptr<video_coding::EncodedFrame> 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 @@
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<void(const RecordableEncodedFrame&)>
@@ -225,6 +226,10 @@
// 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
