Adds thread safety annotations to the AudioDeviceBuffer class

webrtc/modules/audio_device/audio_device_buffer.h

Index: webrtc/modules/audio_device/audio_device_buffer.h
diff --git a/webrtc/modules/audio_device/audio_device_buffer.h b/webrtc/modules/audio_device/audio_device_buffer.h
index 7e9f3e3eec30105a5971727002fe9b8bb39d87eb..1cd04acc157a69cd6fb53da0290a42a2fb4f10dd 100644
--- a/webrtc/modules/audio_device/audio_device_buffer.h
+++ b/webrtc/modules/audio_device/audio_device_buffer.h
@@ -14,6 +14,7 @@
 #include "webrtc/base/buffer.h"
 #include "webrtc/base/criticalsection.h"
 #include "webrtc/base/task_queue.h"
+#include "webrtc/base/thread_annotations.h"
 #include "webrtc/base/thread_checker.h"
 #include "webrtc/modules/audio_device/include/audio_device.h"
 #include "webrtc/system_wrappers/include/file_wrapper.h"
@@ -47,27 +48,27 @@ class AudioDeviceBuffer {
   void StopPlayout();
   void StopRecording();
 
-  int32_t SetRecordingSampleRate(uint32_t fsHz);
-  int32_t SetPlayoutSampleRate(uint32_t fsHz);
+  int32_t SetRecordingSampleRate(uint32_t fsHz) LOCKS_EXCLUDED(lock_);
+  int32_t SetPlayoutSampleRate(uint32_t fsHz) LOCKS_EXCLUDED(lock_);
   int32_t RecordingSampleRate() const;
   int32_t PlayoutSampleRate() const;
 
-  int32_t SetRecordingChannels(size_t channels);
-  int32_t SetPlayoutChannels(size_t channels);
+  int32_t SetRecordingChannels(size_t channels) LOCKS_EXCLUDED(lock_);
+  int32_t SetPlayoutChannels(size_t channels) LOCKS_EXCLUDED(lock_);
   size_t RecordingChannels() const;
   size_t PlayoutChannels() const;
   int32_t SetRecordingChannel(const AudioDeviceModule::ChannelType channel);
   int32_t RecordingChannel(AudioDeviceModule::ChannelType& channel) const;
 
   virtual int32_t SetRecordedBuffer(const void* audio_buffer,
-                                    size_t num_samples);
+                                    size_t num_samples) LOCKS_EXCLUDED(lock_);
   int32_t SetCurrentMicLevel(uint32_t level);
   virtual void SetVQEData(int play_delay_ms, int rec_delay_ms, int clock_drift);
-  virtual int32_t DeliverRecordedData();
+  virtual int32_t DeliverRecordedData() LOCKS_EXCLUDED(lock_);
   uint32_t NewMicLevel() const;
 
-  virtual int32_t RequestPlayoutData(size_t num_samples);
-  virtual int32_t GetPlayoutData(void* audio_buffer);
+  virtual int32_t RequestPlayoutData(size_t num_samples) LOCKS_EXCLUDED(lock_);
+  virtual int32_t GetPlayoutData(void* audio_buffer) LOCKS_EXCLUDED(lock_);
 
   // TODO(henrika): these methods should not be used and does not contain any
   // valid implementation. Investigate the possibility to either remove them
@@ -103,19 +104,20 @@ class AudioDeviceBuffer {
   void ResetRecStats();
   void ResetPlayStats();
 
-  // Ensures that methods are called on the same thread as the thread that
-  // creates this object.
   rtc::ThreadChecker thread_checker_;

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

Could this one get a descriptive name too?

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

Done.

+  rtc::ThreadChecker playout_thread_checker_;
+  rtc::ThreadChecker recording_thread_checker_;
+
+  rtc::CriticalSection lock_;

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

OK, *three* thread checkers... *and* a lock. I'd like a comment that explains
this. (You're being called from three different threads, and some data is
private to each thread, while some data is shared between threads and needs to
be locked?)

I have a feeling that this indicates that it would be useful to split this class
into (at least) three pieces, one for each thread, but that's out of scope for
this CL.

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

It is actually not that complicated. Let me try to explain that inline.

 
   // Raw pointer to AudioTransport instance. Supplied to RegisterAudioCallback()
-  // and it must outlive this object.
+  // and it must outlive this object. It is not possible to change this member
+  // while any media is active. It is possible to start media without calling
+  // RegisterAudioCallback() but that will lead to ignored audio callbacks in
+  // both directions where native audio will be acive but no audio samples will
+  // be transported.
   AudioTransport* audio_transport_cb_;
 
-  // TODO(henrika): given usage of thread checker, it should be possible to
-  // remove all locks in this class.
-  rtc::CriticalSection lock_;
-  rtc::CriticalSection lock_cb_;
-
   // Task queue used to invoke LogStats() periodically. Tasks are executed on a
   // worker thread but it does not necessarily have to be the same thread for
   // each task.
@@ -124,74 +126,79 @@ class AudioDeviceBuffer {
   // Keeps track of if playout/recording are active or not. A combination
   // of these states are used to determine when to start and stop the timer.
   // Only used on the creating thread and not used to control any media flow.
-  bool playing_;
-  bool recording_;
+  bool playing_ ACCESS_ON(thread_checker_);
+  bool recording_ ACCESS_ON(thread_checker_);
 
   // Sample rate in Hertz.
-  uint32_t rec_sample_rate_;
-  uint32_t play_sample_rate_;
+  uint32_t rec_sample_rate_ GUARDED_BY(lock_);
+  uint32_t play_sample_rate_ GUARDED_BY(lock_);
 
   // Number of audio channels.
-  size_t rec_channels_;
-  size_t play_channels_;
-
-  // Number of bytes per audio sample (2 or 4).
-  size_t rec_bytes_per_sample_;
-  size_t play_bytes_per_sample_;
+  size_t rec_channels_ GUARDED_BY(lock_);
+  size_t play_channels_ GUARDED_BY(lock_);
 
   // Byte buffer used for recorded audio samples. Size can be changed
   // dynamically.
-  rtc::Buffer rec_buffer_;
+  rtc::Buffer rec_buffer_ ACCESS_ON(recording_thread_checker_);
 
   // Buffer used for audio samples to be played out. Size can be changed
   // dynamically.
-  rtc::Buffer play_buffer_;
+  rtc::Buffer play_buffer_ ACCESS_ON(playout_thread_checker_);
 
   // AGC parameters.
-  uint32_t current_mic_level_;
-  uint32_t new_mic_level_;
+  uint32_t current_mic_level_ ACCESS_ON(recording_thread_checker_);
+  uint32_t new_mic_level_ ACCESS_ON(recording_thread_checker_);
 
   // Contains true of a key-press has been detected.
-  bool typing_status_;
+  bool typing_status_ ACCESS_ON(recording_thread_checker_);
 
   // Delay values used by the AEC.
-  int play_delay_ms_;
-  int rec_delay_ms_;
+  int play_delay_ms_ ACCESS_ON(recording_thread_checker_);
+  int rec_delay_ms_ ACCESS_ON(recording_thread_checker_);
 
   // Contains a clock-drift measurement.
-  int clock_drift_;
+  int clock_drift_ ACCESS_ON(recording_thread_checker_);
 
   // Counts number of times LogStats() has been called.
-  size_t num_stat_reports_;
+  size_t num_stat_reports_ ACCESS_ON(task_queue_);

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

Aha, a task queue annotation. Excellent!

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

;-)

 
   // Total number of recording callbacks where the source provides 10ms audio
   // data each time.
-  uint64_t rec_callbacks_;
+  uint64_t rec_callbacks_ ACCESS_ON(task_queue_);
 
   // Total number of recording callbacks stored at the last timer task.
-  uint64_t last_rec_callbacks_;
+  uint64_t last_rec_callbacks_ ACCESS_ON(task_queue_);
 
   // Total number of playback callbacks where the sink asks for 10ms audio
   // data each time.
-  uint64_t play_callbacks_;
+  uint64_t play_callbacks_ ACCESS_ON(task_queue_);
 
   // Total number of playout callbacks stored at the last timer task.
-  uint64_t last_play_callbacks_;
+  uint64_t last_play_callbacks_ ACCESS_ON(task_queue_);
 
   // Total number of recorded audio samples.
-  uint64_t rec_samples_;
+  uint64_t rec_samples_ ACCESS_ON(task_queue_);
 
   // Total number of recorded samples stored at the previous timer task.
-  uint64_t last_rec_samples_;
+  uint64_t last_rec_samples_ ACCESS_ON(task_queue_);
 
   // Total number of played audio samples.
-  uint64_t play_samples_;
+  uint64_t play_samples_ ACCESS_ON(task_queue_);
 
   // Total number of played samples stored at the previous timer task.
-  uint64_t last_play_samples_;
+  uint64_t last_play_samples_ ACCESS_ON(task_queue_);
+
+  // Contains max level (max(abs(x))) of recorded audio packets over the last
+  // 10 seconds where a new measurement is done twice per second. The level
+  // is reset to zero at each call to LogStats().
+  int16_t max_rec_level_ ACCESS_ON(task_queue_);
+
+  // Contains max level of recorded audio packets over the last 10 seconds
+  // where a new measurement is done twice per second.
+  int16_t max_play_level_ ACCESS_ON(task_queue_);
 
   // Time stamp of last timer task (drives logging).
-  uint64_t last_timer_task_time_;
+  uint64_t last_timer_task_time_ ACCESS_ON(task_queue_);
 
   // Time stamp of last playout callback.
   uint64_t last_playout_time_;
@@ -203,25 +210,14 @@ class AudioDeviceBuffer {
   // destruction when no audio is running.
   uint32_t playout_diff_times_[kMaxDeltaTimeInMs + 1] = {0};
 
-  // Contains max level (max(abs(x))) of recorded audio packets over the last
-  // 10 seconds where a new measurement is done twice per second. The level
-  // is reset to zero at each call to LogStats(). Only modified on the task
-  // queue thread.
-  int16_t max_rec_level_;
-
-  // Contains max level of recorded audio packets over the last 10 seconds
-  // where a new measurement is done twice per second.
-  int16_t max_play_level_;
-
   // Counts number of audio callbacks modulo 50 to create a signal when
   // a new storage of audio stats shall be done.
-  // Only updated on the OS-specific audio thread that drives audio.
-  int16_t rec_stat_count_;
-  int16_t play_stat_count_;
+  int16_t rec_stat_count_ ACCESS_ON(recording_thread_checker_);
+  int16_t play_stat_count_ ACCESS_ON(playout_thread_checker_);
 
   // Time stamps of when playout and recording starts.
-  uint64_t play_start_time_;
-  uint64_t rec_start_time_;
+  uint64_t play_start_time_ ACCESS_ON(thread_checker_);
+  uint64_t rec_start_time_ ACCESS_ON(thread_checker_);;
 
   // Set to true at construction and modified to false as soon as one audio-
   // level estimate larger than zero is detected.