Adds thread safety annotations to the AudioDeviceBuffer class

webrtc/modules/audio_device/audio_device_buffer.cc

Index: webrtc/modules/audio_device/audio_device_buffer.cc
diff --git a/webrtc/modules/audio_device/audio_device_buffer.cc b/webrtc/modules/audio_device/audio_device_buffer.cc
index ec6a8be490b08d513e6997ea76216d7d4223a6cc..9a3f21ce5044019ebb72aaa03428d2add5f99c9e 100644
--- a/webrtc/modules/audio_device/audio_device_buffer.cc
+++ b/webrtc/modules/audio_device/audio_device_buffer.cc
@@ -46,8 +46,6 @@ AudioDeviceBuffer::AudioDeviceBuffer()
       play_sample_rate_(0),
       rec_channels_(0),
       play_channels_(0),
-      rec_bytes_per_sample_(0),
-      play_bytes_per_sample_(0),
       current_mic_level_(0),
       new_mic_level_(0),
       typing_status_(false),
@@ -63,9 +61,10 @@ AudioDeviceBuffer::AudioDeviceBuffer()
       last_rec_samples_(0),
       play_samples_(0),
       last_play_samples_(0),
-      last_timer_task_time_(0),
       max_rec_level_(0),
       max_play_level_(0),
+      last_timer_task_time_(0),
+      last_playout_time_(0),
       rec_stat_count_(0),
       play_stat_count_(0),
       play_start_time_(0),
@@ -75,7 +74,7 @@ AudioDeviceBuffer::AudioDeviceBuffer()
 }
 
 AudioDeviceBuffer::~AudioDeviceBuffer() {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   RTC_DCHECK(!playing_);
   RTC_DCHECK(!recording_);
   LOG(INFO) << "AudioDeviceBuffer::~dtor";
@@ -83,14 +82,18 @@ AudioDeviceBuffer::~AudioDeviceBuffer() {
 
 int32_t AudioDeviceBuffer::RegisterAudioCallback(
     AudioTransport* audio_callback) {
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   LOG(INFO) << __FUNCTION__;
-  rtc::CritScope lock(&lock_cb_);
+  if (playing_ || recording_) {
+    LOG(LS_ERROR) << "Failed to set audio transport since media was active";
+    return -1;
+  }
   audio_transport_cb_ = audio_callback;
   return 0;
 }
 
 void AudioDeviceBuffer::StartPlayout() {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   // TODO(henrika): allow for usage of DCHECK(!playing_) here instead. Today the
   // ADM allows calling Start(), Start() by ignoring the second call but it
   // makes more sense to only allow one call.
@@ -98,6 +101,7 @@ void AudioDeviceBuffer::StartPlayout() {
     return;
   }
   LOG(INFO) << __FUNCTION__;
+  playout_thread_checker_.DetachFromThread();
   // Clear members tracking playout stats and do it on the task queue.
   task_queue_.PostTask([this] { ResetPlayStats(); });
   // Start a periodic timer based on task queue if not already done by the
@@ -108,16 +112,19 @@ void AudioDeviceBuffer::StartPlayout() {
   const uint64_t now_time = rtc::TimeMillis();
   // Clear members that are only touched on the main (creating) thread.
   play_start_time_ = now_time;
-  last_playout_time_ = now_time;
   playing_ = true;
+  // This member is updated on the audio thread but it safe to initialize
+  // here since the media has not started yet (known by design).

[kwiberg-webrtc, 2016/11/02 14:14:57]

So RTC_DCHECK_RUN_ON the audio thread, and detach it afterwards? That way, you
can annotate last_playout_time_ as being protected by that thread checker.

[henrika_webrtc, 2016/11/02 16:23:24]

Not sure if I understand; care to elaborate?

+  last_playout_time_ = now_time;
 }
 
 void AudioDeviceBuffer::StartRecording() {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   if (recording_) {
     return;
   }
   LOG(INFO) << __FUNCTION__;
+  recording_thread_checker_.DetachFromThread();
   // Clear members tracking recording stats and do it on the task queue.
   task_queue_.PostTask([this] { ResetRecStats(); });
   // Start a periodic timer based on task queue if not already done by the
@@ -135,7 +142,7 @@ void AudioDeviceBuffer::StartRecording() {
 }
 
 void AudioDeviceBuffer::StopPlayout() {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   if (!playing_) {
     return;
   }
@@ -172,7 +179,7 @@ void AudioDeviceBuffer::StopPlayout() {
 }
 
 void AudioDeviceBuffer::StopRecording() {
-  RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  RTC_DCHECK_RUN_ON(&thread_checker_);
   if (!recording_) {
     return;
   }
@@ -204,6 +211,7 @@ void AudioDeviceBuffer::StopRecording() {
 int32_t AudioDeviceBuffer::SetRecordingSampleRate(uint32_t fsHz) {
   LOG(INFO) << "SetRecordingSampleRate(" << fsHz << ")";
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  rtc::CritScope lock(&lock_);
   rec_sample_rate_ = fsHz;
   return 0;
 }
@@ -211,31 +219,34 @@ int32_t AudioDeviceBuffer::SetRecordingSampleRate(uint32_t fsHz) {
 int32_t AudioDeviceBuffer::SetPlayoutSampleRate(uint32_t fsHz) {
   LOG(INFO) << "SetPlayoutSampleRate(" << fsHz << ")";
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  rtc::CritScope lock(&lock_);
   play_sample_rate_ = fsHz;
   return 0;
 }
 
 int32_t AudioDeviceBuffer::RecordingSampleRate() const {
+  rtc::CritScope lock(&lock_);
   return rec_sample_rate_;
 }
 
 int32_t AudioDeviceBuffer::PlayoutSampleRate() const {
+  rtc::CritScope lock(&lock_);
   return play_sample_rate_;
 }
 
 int32_t AudioDeviceBuffer::SetRecordingChannels(size_t channels) {
   LOG(INFO) << "SetRecordingChannels(" << channels << ")";
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   rtc::CritScope lock(&lock_);
   rec_channels_ = channels;
-  rec_bytes_per_sample_ = sizeof(int16_t) * channels;
   return 0;
 }
 
 int32_t AudioDeviceBuffer::SetPlayoutChannels(size_t channels) {
   LOG(INFO) << "SetPlayoutChannels(" << channels << ")";
+  RTC_DCHECK(thread_checker_.CalledOnValidThread());
   rtc::CritScope lock(&lock_);
   play_channels_ = channels;
-  play_bytes_per_sample_ = sizeof(int16_t) * channels;
   return 0;
 }
 
@@ -256,30 +267,36 @@ int32_t AudioDeviceBuffer::RecordingChannel(
 }
 
 size_t AudioDeviceBuffer::RecordingChannels() const {
+  rtc::CritScope lock(&lock_);
   return rec_channels_;
 }
 
 size_t AudioDeviceBuffer::PlayoutChannels() const {
+  rtc::CritScope lock(&lock_);
   return play_channels_;
 }
 
 int32_t AudioDeviceBuffer::SetCurrentMicLevel(uint32_t level) {
+  RTC_DCHECK_RUN_ON(&recording_thread_checker_);
   current_mic_level_ = level;
   return 0;
 }
 
 int32_t AudioDeviceBuffer::SetTypingStatus(bool typing_status) {
+  RTC_DCHECK_RUN_ON(&recording_thread_checker_);
   typing_status_ = typing_status;
   return 0;
 }
 
 uint32_t AudioDeviceBuffer::NewMicLevel() const {
+  RTC_DCHECK_RUN_ON(&recording_thread_checker_);
   return new_mic_level_;
 }
 
 void AudioDeviceBuffer::SetVQEData(int play_delay_ms,
                                    int rec_delay_ms,
                                    int clock_drift) {
+  RTC_DCHECK_RUN_ON(&recording_thread_checker_);
   play_delay_ms_ = play_delay_ms;
   rec_delay_ms_ = rec_delay_ms;
   clock_drift_ = clock_drift;
@@ -309,6 +326,7 @@ int32_t AudioDeviceBuffer::StopOutputFileRecording() {
 
 int32_t AudioDeviceBuffer::SetRecordedBuffer(const void* audio_buffer,
                                              size_t num_samples) {
+  RTC_DCHECK_RUN_ON(&recording_thread_checker_);
   const size_t rec_channels = [&] {
     rtc::CritScope lock(&lock_);
     return rec_channels_;
@@ -348,21 +366,25 @@ int32_t AudioDeviceBuffer::SetRecordedBuffer(const void* audio_buffer,
 }
 
 int32_t AudioDeviceBuffer::DeliverRecordedData() {
-  rtc::CritScope lock(&lock_cb_);
+  RTC_DCHECK_RUN_ON(&recording_thread_checker_);
   if (!audio_transport_cb_) {
     LOG(LS_WARNING) << "Invalid audio transport";
     return 0;
   }
-  const size_t rec_bytes_per_sample = [&] {
+  size_t rec_channels = 0;
+  size_t rec_sample_rate = 0;

[kwiberg-webrtc, 2016/11/02 14:14:57]

Better to not initialize. That way, the reader can tell that they must be set
further down. And MSan will know if you try to read the without setting them
first.

[henrika_webrtc, 2016/11/02 16:23:24]

Done.

+  {
     rtc::CritScope lock(&lock_);
-    return rec_bytes_per_sample_;
-  }();
+    rec_channels = rec_channels_;
+    rec_sample_rate = rec_sample_rate_;
+  }
+  const size_t rec_bytes_per_sample = rec_channels * sizeof(int16_t);
   uint32_t new_mic_level(0);
   uint32_t total_delay_ms = play_delay_ms_ + rec_delay_ms_;
   size_t num_samples = rec_buffer_.size() / rec_bytes_per_sample;
   int32_t res = audio_transport_cb_->RecordedDataIsAvailable(
-      rec_buffer_.data(), num_samples, rec_bytes_per_sample_, rec_channels_,
-      rec_sample_rate_, total_delay_ms, clock_drift_, current_mic_level_,
+      rec_buffer_.data(), num_samples, rec_bytes_per_sample, rec_channels,
+      rec_sample_rate, total_delay_ms, clock_drift_, current_mic_level_,
       typing_status_, new_mic_level);
   if (res != -1) {
     new_mic_level_ = new_mic_level;
@@ -373,6 +395,7 @@ int32_t AudioDeviceBuffer::DeliverRecordedData() {
 }
 
 int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
+  RTC_DCHECK_RUN_ON(&playout_thread_checker_);
   // Measure time since last function call and update an array where the
   // position/index corresponds to time differences (in milliseconds) between
   // two successive playout callbacks, and the stored value is the number of
@@ -384,10 +407,13 @@ int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
   last_playout_time_ = now_time;
   playout_diff_times_[diff_time]++;
 
-  const size_t play_channels = [&] {
+  size_t play_channels = 0;
+  size_t play_sample_rate = 0;

[kwiberg-webrtc, 2016/11/02 14:14:57]

Same here.

[henrika_webrtc, 2016/11/02 16:23:24]

Done.

+  {
     rtc::CritScope lock(&lock_);
-    return play_channels_;
-  }();
+    play_channels = play_channels_;
+    play_sample_rate = play_sample_rate_;
+  }
 
   // The consumer can change the request size on the fly and we therefore
   // resize the buffer accordingly. Also takes place at the first call to this
@@ -400,25 +426,21 @@ int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
   }
 
   size_t num_samples_out(0);
-  {
-    rtc::CritScope lock(&lock_cb_);
-
-    // It is currently supported to start playout without a valid audio
-    // transport object. Leads to warning and silence.
-    if (!audio_transport_cb_) {
-      LOG(LS_WARNING) << "Invalid audio transport";
-      return 0;
-    }
+  // It is currently supported to start playout without a valid audio
+  // transport object. Leads to warning and silence.
+  if (!audio_transport_cb_) {
+    LOG(LS_WARNING) << "Invalid audio transport";
+    return 0;
+  }
 
-    // Retrieve new 16-bit PCM audio data using the audio transport instance.
-    int64_t elapsed_time_ms = -1;
-    int64_t ntp_time_ms = -1;
-    uint32_t res = audio_transport_cb_->NeedMorePlayData(
-        num_samples, play_bytes_per_sample_, play_channels, play_sample_rate_,
-        play_buffer_.data(), num_samples_out, &elapsed_time_ms, &ntp_time_ms);
-    if (res != 0) {
-      LOG(LS_ERROR) << "NeedMorePlayData() failed";
-    }
+  // Retrieve new 16-bit PCM audio data using the audio transport instance.
+  int64_t elapsed_time_ms = -1;
+  int64_t ntp_time_ms = -1;
+  uint32_t res = audio_transport_cb_->NeedMorePlayData(
+      num_samples, play_bytes_per_sample, play_channels, play_sample_rate,
+      play_buffer_.data(), num_samples_out, &elapsed_time_ms, &ntp_time_ms);
+  if (res != 0) {
+    LOG(LS_ERROR) << "NeedMorePlayData() failed";
   }
 
   // Derive a new level value twice per second.
@@ -442,11 +464,13 @@ int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
 }
 
 int32_t AudioDeviceBuffer::GetPlayoutData(void* audio_buffer) {
+  RTC_DCHECK_RUN_ON(&playout_thread_checker_);
   RTC_DCHECK_GT(play_buffer_.size(), 0u);
-  const size_t play_bytes_per_sample = [&] {
+  const size_t play_channels = [&] {
     rtc::CritScope lock(&lock_);
-    return play_bytes_per_sample_;
+    return play_channels_;
   }();
+  const size_t play_bytes_per_sample = play_channels * sizeof(int16_t);
   memcpy(audio_buffer, play_buffer_.data(), play_buffer_.size());
   return static_cast<int32_t>(play_buffer_.size() / play_bytes_per_sample);
 }
@@ -462,7 +486,7 @@ void AudioDeviceBuffer::StopPeriodicLogging() {
 }
 
 void AudioDeviceBuffer::LogStats(LogState state) {
-  RTC_DCHECK(task_queue_.IsCurrent());
+  RTC_DCHECK_RUN_ON(&task_queue_);
   int64_t now_time = rtc::TimeMillis();
   if (state == AudioDeviceBuffer::LOG_START) {
     // Reset counters at start. We will not add any logging in this state but
@@ -480,13 +504,21 @@ void AudioDeviceBuffer::LogStats(LogState state) {
   int64_t time_since_last = rtc::TimeDiff(now_time, last_timer_task_time_);
   last_timer_task_time_ = now_time;
 
+  size_t play_sample_rate = 0;
+  size_t rec_sample_rate = 0;

[kwiberg-webrtc, 2016/11/02 14:14:57]

Same here.

[henrika_webrtc, 2016/11/02 16:23:24]

Done.

+  {
+    rtc::CritScope lock(&lock_);
+    play_sample_rate = play_sample_rate_;
+    rec_sample_rate = rec_sample_rate_;
+  };
+
   // Log the latest statistics but skip the first round just after state was
   // set to LOG_START. Hence, first printed log will be after ~10 seconds.
   if (++num_stat_reports_ > 1 && time_since_last > 0) {
     uint32_t diff_samples = rec_samples_ - last_rec_samples_;
     float rate = diff_samples / (static_cast<float>(time_since_last) / 1000.0);
     LOG(INFO) << "[REC : " << time_since_last << "msec, "
-              << rec_sample_rate_ / 1000
+              << rec_sample_rate / 1000
               << "kHz] callbacks: " << rec_callbacks_ - last_rec_callbacks_
               << ", "
               << "samples: " << diff_samples << ", "
@@ -496,7 +528,7 @@ void AudioDeviceBuffer::LogStats(LogState state) {
     diff_samples = play_samples_ - last_play_samples_;
     rate = diff_samples / (static_cast<float>(time_since_last) / 1000.0);
     LOG(INFO) << "[PLAY: " << time_since_last << "msec, "
-              << play_sample_rate_ / 1000
+              << play_sample_rate / 1000
               << "kHz] callbacks: " << play_callbacks_ - last_play_callbacks_
               << ", "
               << "samples: " << diff_samples << ", "
@@ -521,7 +553,7 @@ void AudioDeviceBuffer::LogStats(LogState state) {
 }
 
 void AudioDeviceBuffer::ResetRecStats() {
-  RTC_DCHECK(task_queue_.IsCurrent());
+  RTC_DCHECK_RUN_ON(&task_queue_);
   rec_callbacks_ = 0;
   last_rec_callbacks_ = 0;
   rec_samples_ = 0;
@@ -530,7 +562,7 @@ void AudioDeviceBuffer::ResetRecStats() {
 }
 
 void AudioDeviceBuffer::ResetPlayStats() {
-  RTC_DCHECK(task_queue_.IsCurrent());
+  RTC_DCHECK_RUN_ON(&task_queue_);
   play_callbacks_ = 0;
   last_play_callbacks_ = 0;
   play_samples_ = 0;
@@ -539,7 +571,7 @@ void AudioDeviceBuffer::ResetPlayStats() {
 }
 
 void AudioDeviceBuffer::UpdateRecStats(int16_t max_abs, size_t num_samples) {
-  RTC_DCHECK(task_queue_.IsCurrent());
+  RTC_DCHECK_RUN_ON(&task_queue_);
   ++rec_callbacks_;
   rec_samples_ += num_samples;
   if (max_abs > max_rec_level_) {
@@ -548,7 +580,7 @@ void AudioDeviceBuffer::UpdateRecStats(int16_t max_abs, size_t num_samples) {
 }
 
 void AudioDeviceBuffer::UpdatePlayStats(int16_t max_abs, size_t num_samples) {
-  RTC_DCHECK(task_queue_.IsCurrent());
+  RTC_DCHECK_RUN_ON(&task_queue_);
   ++play_callbacks_;
   play_samples_ += num_samples;
   if (max_abs > max_play_level_) {