Use module_process_thread_ for thread checks in ChannelReceive.
ChannelReceive for audio has both a thread checker and pointer.
Both aren't needed, so this removes the checker. Moving forward
we should be able to guard more variables with checks and remove
the need for locks.
Removing module_process_thread_checker_ from AudioReceiveStream.
The checker was misleading and actually checked the worker thread.
Updating downstream code in ChannelReceive accordingly.
Bug: webrtc:11993
Change-Id: I93becd4989e5838412a4f079ba63cf67252daa84
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/212613
Commit-Queue: Tommi <tommi@webrtc.org>
Reviewed-by: Niels Moller <nisse@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#33616}
diff --git a/audio/audio_receive_stream.cc b/audio/audio_receive_stream.cc
index e99e39c..467647b 100644
--- a/audio/audio_receive_stream.cc
+++ b/audio/audio_receive_stream.cc
@@ -127,8 +127,6 @@
RTC_DCHECK(audio_state_);
RTC_DCHECK(channel_receive_);
- module_process_thread_checker_.Detach();
-
RTC_DCHECK(receiver_controller);
RTC_DCHECK(packet_router);
// Configure bandwidth estimation.
@@ -325,14 +323,10 @@
}
absl::optional<Syncable::Info> AudioReceiveStream::GetInfo() const {
- RTC_DCHECK_RUN_ON(&module_process_thread_checker_);
- absl::optional<Syncable::Info> info = channel_receive_->GetSyncInfo();
-
- if (!info)
- return absl::nullopt;
-
- info->current_delay_ms = channel_receive_->GetDelayEstimate();
- return info;
+ // TODO(bugs.webrtc.org/11993): This is called via RtpStreamsSynchronizer,
+ // expect to be called on the network thread.
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
+ return channel_receive_->GetSyncInfo();
}
bool AudioReceiveStream::GetPlayoutRtpTimestamp(uint32_t* rtp_timestamp,
@@ -350,7 +344,9 @@
}
bool AudioReceiveStream::SetMinimumPlayoutDelay(int delay_ms) {
- RTC_DCHECK_RUN_ON(&module_process_thread_checker_);
+ // TODO(bugs.webrtc.org/11993): This is called via RtpStreamsSynchronizer,
+ // expect to be called on the network thread.
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
return channel_receive_->SetMinimumPlayoutDelay(delay_ms);
}
diff --git a/audio/audio_receive_stream.h b/audio/audio_receive_stream.h
index 545a120..a8438c2 100644
--- a/audio/audio_receive_stream.h
+++ b/audio/audio_receive_stream.h
@@ -109,7 +109,6 @@
AudioState* audio_state() const;
SequenceChecker worker_thread_checker_;
- SequenceChecker module_process_thread_checker_;
webrtc::AudioReceiveStream::Config config_;
rtc::scoped_refptr<webrtc::AudioState> audio_state_;
SourceTracker source_tracker_;
diff --git a/audio/channel_receive.cc b/audio/channel_receive.cc
index dcc2d25..4278215 100644
--- a/audio/channel_receive.cc
+++ b/audio/channel_receive.cc
@@ -200,7 +200,7 @@
// parts with single-threaded semantics, and thereby reduce the need for
// locks.
SequenceChecker worker_thread_checker_;
- SequenceChecker module_process_thread_checker_;
+
// Methods accessed from audio and video threads are checked for sequential-
// only access. We don't necessarily own and control these threads, so thread
// checkers cannot be used. E.g. Chromium may transfer "ownership" from one
@@ -261,8 +261,7 @@
// frame.
int64_t capture_start_ntp_time_ms_ RTC_GUARDED_BY(ts_stats_lock_);
- // uses
- ProcessThread* _moduleProcessThreadPtr;
+ ProcessThread* const module_process_thread_;
AudioDeviceModule* _audioDeviceModulePtr;
float _outputGain RTC_GUARDED_BY(volume_settings_mutex_);
@@ -499,17 +498,14 @@
rtp_ts_wraparound_handler_(new rtc::TimestampWrapAroundHandler()),
capture_start_rtp_time_stamp_(-1),
capture_start_ntp_time_ms_(-1),
- _moduleProcessThreadPtr(module_process_thread),
+ module_process_thread_(module_process_thread),
_audioDeviceModulePtr(audio_device_module),
_outputGain(1.0f),
associated_send_channel_(nullptr),
frame_decryptor_(frame_decryptor),
crypto_options_(crypto_options),
absolute_capture_time_receiver_(clock) {
- // TODO(nisse): Use _moduleProcessThreadPtr instead?
- module_process_thread_checker_.Detach();
-
- RTC_DCHECK(module_process_thread);
+ RTC_DCHECK(module_process_thread_);
RTC_DCHECK(audio_device_module);
acm_receiver_.ResetInitialDelay();
@@ -536,39 +532,43 @@
rtp_rtcp_->SetSendingMediaStatus(false);
rtp_rtcp_->SetRemoteSSRC(remote_ssrc_);
- _moduleProcessThreadPtr->RegisterModule(rtp_rtcp_.get(), RTC_FROM_HERE);
-
// Ensure that RTCP is enabled for the created channel.
rtp_rtcp_->SetRTCPStatus(RtcpMode::kCompound);
+
+ // TODO(tommi): This should be an implementation detail of ModuleRtpRtcpImpl2
+ // and the pointer to the process thread should be there (which also localizes
+ // the problem of getting rid of that dependency).
+ module_process_thread_->RegisterModule(rtp_rtcp_.get(), RTC_FROM_HERE);
}
ChannelReceive::~ChannelReceive() {
RTC_DCHECK(construction_thread_.IsCurrent());
+ // Unregister the module before stopping playout etc, to match the order
+ // things were set up in the ctor.
+ module_process_thread_->DeRegisterModule(rtp_rtcp_.get());
+
// Resets the delegate's callback to ChannelReceive::OnReceivedPayloadData.
if (frame_transformer_delegate_)
frame_transformer_delegate_->Reset();
StopPlayout();
-
- if (_moduleProcessThreadPtr)
- _moduleProcessThreadPtr->DeRegisterModule(rtp_rtcp_.get());
}
void ChannelReceive::SetSink(AudioSinkInterface* sink) {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
MutexLock lock(&callback_mutex_);
audio_sink_ = sink;
}
void ChannelReceive::StartPlayout() {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
MutexLock lock(&playing_lock_);
playing_ = true;
}
void ChannelReceive::StopPlayout() {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
MutexLock lock(&playing_lock_);
playing_ = false;
_outputAudioLevel.ResetLevelFullRange();
@@ -576,13 +576,13 @@
absl::optional<std::pair<int, SdpAudioFormat>> ChannelReceive::GetReceiveCodec()
const {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
return acm_receiver_.LastDecoder();
}
void ChannelReceive::SetReceiveCodecs(
const std::map<int, SdpAudioFormat>& codecs) {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
for (const auto& kv : codecs) {
RTC_DCHECK_GE(kv.second.clockrate_hz, 1000);
payload_type_frequencies_[kv.first] = kv.second.clockrate_hz;
@@ -592,6 +592,9 @@
// May be called on either worker thread or network thread.
void ChannelReceive::OnRtpPacket(const RtpPacketReceived& packet) {
+ // TODO(bugs.webrtc.org/11993): Expect to be called exclusively on the
+ // network thread. Once that's done, the same applies to
+ // UpdatePlayoutTimestamp and
int64_t now_ms = rtc::TimeMillis();
{
@@ -680,6 +683,9 @@
// May be called on either worker thread or network thread.
void ChannelReceive::ReceivedRTCPPacket(const uint8_t* data, size_t length) {
+ // TODO(bugs.webrtc.org/11993): Expect to be called exclusively on the
+ // network thread.
+
// Store playout timestamp for the received RTCP packet
UpdatePlayoutTimestamp(true, rtc::TimeMillis());
@@ -716,29 +722,29 @@
}
int ChannelReceive::GetSpeechOutputLevelFullRange() const {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
return _outputAudioLevel.LevelFullRange();
}
double ChannelReceive::GetTotalOutputEnergy() const {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
return _outputAudioLevel.TotalEnergy();
}
double ChannelReceive::GetTotalOutputDuration() const {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
return _outputAudioLevel.TotalDuration();
}
void ChannelReceive::SetChannelOutputVolumeScaling(float scaling) {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
MutexLock lock(&volume_settings_mutex_);
_outputGain = scaling;
}
void ChannelReceive::RegisterReceiverCongestionControlObjects(
PacketRouter* packet_router) {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
RTC_DCHECK(packet_router);
RTC_DCHECK(!packet_router_);
constexpr bool remb_candidate = false;
@@ -747,14 +753,14 @@
}
void ChannelReceive::ResetReceiverCongestionControlObjects() {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
RTC_DCHECK(packet_router_);
packet_router_->RemoveReceiveRtpModule(rtp_rtcp_.get());
packet_router_ = nullptr;
}
CallReceiveStatistics ChannelReceive::GetRTCPStatistics() const {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
CallReceiveStatistics stats;
// The jitter statistics is updated for each received RTP packet and is based
@@ -814,7 +820,7 @@
}
void ChannelReceive::SetNACKStatus(bool enable, int max_packets) {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
// None of these functions can fail.
if (enable) {
rtp_receive_statistics_->SetMaxReorderingThreshold(max_packets);
@@ -835,14 +841,14 @@
void ChannelReceive::SetAssociatedSendChannel(
const ChannelSendInterface* channel) {
// TODO(bugs.webrtc.org/11993): Expect to be called on the network thread.
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
MutexLock lock(&assoc_send_channel_lock_);
associated_send_channel_ = channel;
}
void ChannelReceive::SetDepacketizerToDecoderFrameTransformer(
rtc::scoped_refptr<webrtc::FrameTransformerInterface> frame_transformer) {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
// Depending on when the channel is created, the transformer might be set
// twice. Don't replace the delegate if it was already initialized.
if (!frame_transformer || frame_transformer_delegate_)
@@ -852,28 +858,36 @@
NetworkStatistics ChannelReceive::GetNetworkStatistics(
bool get_and_clear_legacy_stats) const {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
NetworkStatistics stats;
acm_receiver_.GetNetworkStatistics(&stats, get_and_clear_legacy_stats);
return stats;
}
AudioDecodingCallStats ChannelReceive::GetDecodingCallStatistics() const {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
AudioDecodingCallStats stats;
acm_receiver_.GetDecodingCallStatistics(&stats);
return stats;
}
uint32_t ChannelReceive::GetDelayEstimate() const {
- RTC_DCHECK(worker_thread_checker_.IsCurrent() ||
- module_process_thread_checker_.IsCurrent());
- MutexLock lock(&video_sync_lock_);
- return acm_receiver_.FilteredCurrentDelayMs() + playout_delay_ms_;
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
+
+ uint32_t playout_delay;
+ {
+ MutexLock lock(&video_sync_lock_);
+ playout_delay = playout_delay_ms_;
+ }
+ // Return the current jitter buffer delay + playout delay.
+ return acm_receiver_.FilteredCurrentDelayMs() + playout_delay;
}
bool ChannelReceive::SetMinimumPlayoutDelay(int delay_ms) {
- RTC_DCHECK(module_process_thread_checker_.IsCurrent());
+ // TODO(bugs.webrtc.org/11993): This should run on the network thread.
+ // We get here via RtpStreamsSynchronizer. Once that's done, many (all?) of
+ // these locks aren't needed.
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
// Limit to range accepted by both VoE and ACM, so we're at least getting as
// close as possible, instead of failing.
delay_ms = rtc::SafeClamp(delay_ms, kVoiceEngineMinMinPlayoutDelayMs,
@@ -909,7 +923,7 @@
absl::optional<int64_t>
ChannelReceive::GetCurrentEstimatedPlayoutNtpTimestampMs(int64_t now_ms) const {
- RTC_DCHECK(worker_thread_checker_.IsCurrent());
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
MutexLock lock(&video_sync_lock_);
if (!playout_timestamp_ntp_ || !playout_timestamp_ntp_time_ms_)
return absl::nullopt;
@@ -927,7 +941,10 @@
}
absl::optional<Syncable::Info> ChannelReceive::GetSyncInfo() const {
- RTC_DCHECK(module_process_thread_checker_.IsCurrent());
+ // TODO(bugs.webrtc.org/11993): This should run on the network thread.
+ // We get here via RtpStreamsSynchronizer. Once that's done, many of
+ // these locks aren't needed.
+ RTC_DCHECK_RUN_ON(&worker_thread_checker_);
Syncable::Info info;
if (rtp_rtcp_->RemoteNTP(&info.capture_time_ntp_secs,
&info.capture_time_ntp_frac,
@@ -936,6 +953,7 @@
&info.capture_time_source_clock) != 0) {
return absl::nullopt;
}
+
{
MutexLock lock(&sync_info_lock_);
if (!last_received_rtp_timestamp_ || !last_received_rtp_system_time_ms_) {
@@ -944,10 +962,20 @@
info.latest_received_capture_timestamp = *last_received_rtp_timestamp_;
info.latest_receive_time_ms = *last_received_rtp_system_time_ms_;
}
+
+ int jitter_buffer_delay = acm_receiver_.FilteredCurrentDelayMs();
+ {
+ MutexLock lock(&video_sync_lock_);
+ info.current_delay_ms = jitter_buffer_delay + playout_delay_ms_;
+ }
+
return info;
}
void ChannelReceive::UpdatePlayoutTimestamp(bool rtcp, int64_t now_ms) {
+ // TODO(bugs.webrtc.org/11993): Expect to be called exclusively on the
+ // network thread. Once that's done, we won't need video_sync_lock_.
+
jitter_buffer_playout_timestamp_ = acm_receiver_.GetPlayoutTimestamp();
if (!jitter_buffer_playout_timestamp_) {
