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..0dd46345a3904f77906fee6fcb4a29da72949893 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),
@@ -84,7 +82,11 @@ AudioDeviceBuffer::~AudioDeviceBuffer() {
 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;
 }
@@ -204,6 +206,7 @@ void AudioDeviceBuffer::StopRecording() {
 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;
 }
@@ -211,31 +214,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_);

[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;
 }
 
@@ -256,10 +262,12 @@ 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_;
 }
 
@@ -309,6 +317,7 @@ int32_t AudioDeviceBuffer::StopOutputFileRecording() {
 
 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_;
@@ -348,21 +357,26 @@ int32_t AudioDeviceBuffer::SetRecordedBuffer(const void* audio_buffer,
 }
 
 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;
@@ -373,6 +387,7 @@ int32_t AudioDeviceBuffer::DeliverRecordedData() {
 }
 
 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
@@ -388,6 +403,10 @@ int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
     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
@@ -400,25 +419,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 +457,13 @@ int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
 }
 
 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);
 }
@@ -480,13 +497,23 @@ void AudioDeviceBuffer::LogStats(LogState state) {
   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 << ", "
@@ -496,7 +523,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 << ", "