Adds thread safety annotations to the AudioDeviceBuffer class

webrtc/modules/audio_device/audio_device_buffer.cc

 /*
  *  Copyright (c) 2012 The WebRTC project authors. All Rights Reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 28 matching lines @@
 
 AudioDeviceBuffer::AudioDeviceBuffer()
     : audio_transport_cb_(nullptr),
       task_queue_(kTimerQueueName),
       playing_(false),
       recording_(false),
       rec_sample_rate_(0),
       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),
       play_delay_ms_(0),
       rec_delay_ms_(0),
       clock_drift_(0),
       num_stat_reports_(0),
       rec_callbacks_(0),
       last_rec_callbacks_(0),
       play_callbacks_(0),
@@ skipping 16 matching lines @@
 AudioDeviceBuffer::~AudioDeviceBuffer() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   RTC_DCHECK(!playing_);
   RTC_DCHECK(!recording_);
   LOG(INFO) << "AudioDeviceBuffer::~dtor";
 }
 
 int32_t AudioDeviceBuffer::RegisterAudioCallback(
     AudioTransport* audio_callback) {
   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;
+  }
+  RTC_DCHECK_RUNS_SERIALIZED(&race_checker_);
   audio_transport_cb_ = audio_callback;
   return 0;
 }
 
 void AudioDeviceBuffer::StartPlayout() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   // 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.
   if (playing_) {
@@ skipping 99 matching lines @@
     const int only_zeros = static_cast<int>(only_silence_recorded_);
     RTC_HISTOGRAM_BOOLEAN("WebRTC.Audio.RecordedOnlyZeros", only_zeros);
     LOG(INFO) << "HISTOGRAM(WebRTC.Audio.RecordedOnlyZeros): " << only_zeros;
   }
   LOG(INFO) << "total recording time: " << time_since_start;
 }
 
 int32_t AudioDeviceBuffer::SetRecordingSampleRate(uint32_t fsHz) {
   LOG(INFO) << "SetRecordingSampleRate(" << fsHz << ")";
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  rtc::CritScope lock(&lock_);

[kwiberg-webrtc, 2016/11/01 15:53:50]

If every call to this function is on the same thread, why do you need to lock?
You should only need one of these, I think.

[henrika_webrtc, 2016/11/02 10:29:17]

Please correct me if I am wrong but: I want to protect rec_sample_rate_ given an
assumption that it is set on one thread (the creating thread) and read on
another thread (OS/audio specific). So we need a lock. But, I want to be sure
that the lock is needed and therefore I add the thread 
check.

Wrong thinking?

[kwiberg-webrtc, 2016/11/02 11:27:56]

Yes, for that situation you need a lock.

I don't see what you want the thread checker to do, though. It will verify that
you're always on the same thread when you call it. How does this help here? If I
understand correctly, you want to verify that two different methods are always
called from different threads.

   rec_sample_rate_ = fsHz;
   return 0;
 }
 
 int32_t AudioDeviceBuffer::SetPlayoutSampleRate(uint32_t fsHz) {
   LOG(INFO) << "SetPlayoutSampleRate(" << fsHz << ")";
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
+  rtc::CritScope lock(&lock_);

[kwiberg-webrtc, 2016/11/01 15:53:50]

Again, you shouldn't need both the thread checker and the lock, right? (This
happens more times below. The only legitimate reason I can think of is if you
have one member protected by the thread checker and one protected by the lock,
and you need to access both at the same time; it's the same sort of circumstance
under which you'd need to acquire to locks.)

[henrika_webrtc, 2016/11/02 10:29:18]

see above

   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;
 }
 
 int32_t AudioDeviceBuffer::SetRecordingChannel(
     const AudioDeviceModule::ChannelType channel) {
   LOG(INFO) << "SetRecordingChannel(" << channel << ")";
   LOG(LS_WARNING) << "Not implemented";
   // Add DCHECK to ensure that user does not try to use this API with a non-
   // default parameter.
   RTC_DCHECK_EQ(channel, AudioDeviceModule::kChannelBoth);
   return -1;
 }
 
 int32_t AudioDeviceBuffer::RecordingChannel(
     AudioDeviceModule::ChannelType& channel) const {
   LOG(LS_WARNING) << "Not implemented";
   return -1;
 }
 
 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) {
   current_mic_level_ = level;
   return 0;
 }
 
 int32_t AudioDeviceBuffer::SetTypingStatus(bool typing_status) {
   typing_status_ = typing_status;
@@ skipping 29 matching lines @@
   return 0;
 }
 
 int32_t AudioDeviceBuffer::StopOutputFileRecording() {
   LOG(LS_WARNING) << "Not implemented";
   return 0;
 }
 
 int32_t AudioDeviceBuffer::SetRecordedBuffer(const void* audio_buffer,
                                              size_t num_samples) {
+  RTC_DCHECK_RUNS_SERIALIZED(&race_checker_);
   const size_t rec_channels = [&] {
     rtc::CritScope lock(&lock_);
     return rec_channels_;
   }();
   // Copy the complete input buffer to the local buffer.
   const size_t size_in_bytes = num_samples * rec_channels * sizeof(int16_t);
   const size_t old_size = rec_buffer_.size();
   rec_buffer_.SetData(static_cast<const uint8_t*>(audio_buffer), size_in_bytes);
   // Keep track of the size of the recording buffer. Only updated when the
   // size changes, which is a rare event.
@@ skipping 19 matching lines @@
   // Update some stats but do it on the task queue to ensure that the members
   // are modified and read on the same thread. Note that |max_abs| will be
   // zero in most calls and then have no effect of the stats. It is only updated
   // approximately two times per second and can then change the stats.
   task_queue_.PostTask(
       [this, max_abs, num_samples] { UpdateRecStats(max_abs, num_samples); });
   return 0;
 }
 
 int32_t AudioDeviceBuffer::DeliverRecordedData() {
-  rtc::CritScope lock(&lock_cb_);
+  RTC_DCHECK_RUNS_SERIALIZED(&race_checker_);
   if (!audio_transport_cb_) {
     LOG(LS_WARNING) << "Invalid audio transport";
     return 0;
   }
-  const size_t rec_bytes_per_sample = [&] {
+  const size_t rec_channels = [&] {
     rtc::CritScope lock(&lock_);
-    return rec_bytes_per_sample_;
+    return rec_channels_;
   }();
+  const size_t rec_sample_rate = [&] {
+    rtc::CritScope lock(&lock_);
+    return rec_sample_rate_;
+  }();

[kwiberg-webrtc, 2016/11/01 15:53:50]

By taking the lock twice like this, you pay twice the performance cost, and also
risk getting a pair of values that never existed simultaneously (if someone
changes stuff after you release the lock the first time, before you acquire it
for the second time). It's better to read both variables at once, I think (but
you can't easily use this cute lambda trick to do so until C++17).

[henrika_webrtc, 2016/11/02 10:29:18]

Removed lambda. 

+  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;
   } else {
     LOG(LS_ERROR) << "RecordedDataIsAvailable() failed";
   }
   return 0;
 }
 
 int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
+  RTC_DCHECK_RUNS_SERIALIZED(&race_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
   // times a given time difference was found.
   int64_t now_time = rtc::TimeMillis();
   size_t diff_time = rtc::TimeDiff(now_time, last_playout_time_);
   // Truncate at 500ms to limit the size of the array.
   diff_time = std::min(kMaxDeltaTimeInMs, diff_time);
   last_playout_time_ = now_time;
   playout_diff_times_[diff_time]++;
 
   const size_t play_channels = [&] {
     rtc::CritScope lock(&lock_);
     return play_channels_;
   }();
+  const size_t play_sample_rate = [&] {
+    rtc::CritScope lock(&lock_);
+    return 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
   // method.
   const size_t play_bytes_per_sample = play_channels * sizeof(int16_t);
   const size_t size_in_bytes = num_samples * play_bytes_per_sample;
   if (play_buffer_.size() != size_in_bytes) {
     play_buffer_.SetSize(size_in_bytes);
     LOG(LS_INFO) << "Size of playout buffer: " << play_buffer_.size();
   }
 
   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.
   int16_t max_abs = 0;
   RTC_DCHECK_LT(play_stat_count_, 50);
   if (++play_stat_count_ >= 50) {
     const size_t size = num_samples * play_channels;
     // Returns the largest absolute value in a signed 16-bit vector.
     max_abs = WebRtcSpl_MaxAbsValueW16(
         reinterpret_cast<const int16_t*>(play_buffer_.data()), size);
     play_stat_count_ = 0;
   }
   // Update some stats but do it on the task queue to ensure that the members
   // are modified and read on the same thread. Note that |max_abs| will be
   // zero in most calls and then have no effect of the stats. It is only updated
   // approximately two times per second and can then change the stats.
   task_queue_.PostTask([this, max_abs, num_samples_out] {
     UpdatePlayStats(max_abs, num_samples_out);
   });
   return static_cast<int32_t>(num_samples_out);
 }
 
 int32_t AudioDeviceBuffer::GetPlayoutData(void* audio_buffer) {
+  RTC_DCHECK_RUNS_SERIALIZED(&race_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);
 }
 
 void AudioDeviceBuffer::StartPeriodicLogging() {
   task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::LogStats, this,
                                  AudioDeviceBuffer::LOG_START));
 }
 
 void AudioDeviceBuffer::StopPeriodicLogging() {
@@ skipping 13 matching lines @@
     // Stop logging and posting new tasks.
     return;
   } else if (state == AudioDeviceBuffer::LOG_ACTIVE) {
     // Default state. Just keep on logging.
   }
 
   int64_t next_callback_time = now_time + kTimerIntervalInMilliseconds;
   int64_t time_since_last = rtc::TimeDiff(now_time, last_timer_task_time_);
   last_timer_task_time_ = now_time;
 
+  // Read |play_sample_rate_| and |rec_sample_rate_| under exclusive lock.

[kwiberg-webrtc, 2016/11/01 15:53:50]

The comment isn't necessary.

[henrika_webrtc, 2016/11/02 10:29:18]

Done.

+  const size_t play_sample_rate = [&] {
+    rtc::CritScope lock(&lock_);
+    return play_sample_rate_;
+  }();
+  const size_t rec_sample_rate = [&] {
+    rtc::CritScope lock(&lock_);
+    return 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 << ", "
               << "rate: " << static_cast<int>(rate + 0.5) << ", "
               << "level: " << max_rec_level_;
 
     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 << ", "
               << "rate: " << static_cast<int>(rate + 0.5) << ", "
               << "level: " << max_play_level_;
   }
 
   last_rec_callbacks_ = rec_callbacks_;
   last_play_callbacks_ = play_callbacks_;
   last_rec_samples_ = rec_samples_;
@@ skipping 40 matching lines @@
 void AudioDeviceBuffer::UpdatePlayStats(int16_t max_abs, size_t num_samples) {
   RTC_DCHECK(task_queue_.IsCurrent());
   ++play_callbacks_;
   play_samples_ += num_samples;
   if (max_abs > max_play_level_) {
     max_play_level_ = max_abs;
   }
 }
 
 }  // namespace webrtc