Adds thread safety annotations to the AudioDeviceBuffer class

webrtc/modules/audio_device/audio_device_buffer.h

 /*
  *  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.
  */
 
 #ifndef WEBRTC_MODULES_AUDIO_DEVICE_AUDIO_DEVICE_BUFFER_H_
 #define WEBRTC_MODULES_AUDIO_DEVICE_AUDIO_DEVICE_BUFFER_H_
 
 #include "webrtc/base/buffer.h"
 #include "webrtc/base/criticalsection.h"
+#include "webrtc/base/race_checker.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"
 #include "webrtc/typedefs.h"
 
 namespace webrtc {
 // Delta times between two successive playout callbacks are limited to this
 // value before added to an internal array.
 const size_t kMaxDeltaTimeInMs = 500;
 // TODO(henrika): remove when no longer used by external client.
@@ skipping 13 matching lines @@
   virtual ~AudioDeviceBuffer();
 
   void SetId(uint32_t id) {};
   int32_t RegisterAudioCallback(AudioTransport* audio_callback);
 
   void StartPlayout();
   void StartRecording();
   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
   // or add a proper implementation if needed.
   int32_t StartInputFileRecording(const char fileName[kAdmMaxFileNameSize]);
   int32_t StopInputFileRecording();
   int32_t StartOutputFileRecording(const char fileName[kAdmMaxFileNameSize]);
   int32_t StopOutputFileRecording();
 
   int32_t SetTypingStatus(bool typing_status);
@@ skipping 17 matching lines @@
   void UpdateRecStats(int16_t max_abs, size_t num_samples);
   void UpdatePlayStats(int16_t max_abs, size_t num_samples);
 
   // Clears all members tracking stats for recording and playout.
   // These methods both run on the task queue.
   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/01 15:53:50]

No member variable is annotated as being protected by thread_checker_. Is it not
used?

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

As we discussed. Will try to fix that.

 
+  // Verifies that access to some members are serialized.
+  rtc::RaceChecker race_checker_;
+
+  rtc::CriticalSection lock_;
+
   // Raw pointer to AudioTransport instance. Supplied to RegisterAudioCallback()
-  // and it must outlive this object.
-  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_;
+  // 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_ GUARDED_BY(race_checker_);
 
   // 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.
   rtc::TaskQueue task_queue_;

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

I presume this object is itself thread safe, so that you don't need to protect
it by a separate critical section?

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

Yes, dtor and ctor are thread checked and I can't see any reason why a lock is
needed.

 
   // 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_;

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

These are not annotated. It's usually good to make sure that *every* member
variable (apart from the mutex(es)...) are annotated, or have a comment
explaining why they don't need to be protected.

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

Acknowledged.

 
   // 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_ GUARDED_BY(race_checker_);
 
   // Buffer used for audio samples to be played out. Size can be changed
   // dynamically.
-  rtc::Buffer play_buffer_;
+  rtc::Buffer play_buffer_ GUARDED_BY(race_checker_);
 
   // AGC parameters.
   uint32_t current_mic_level_;
   uint32_t new_mic_level_;

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

More unannotated variables. Please annotate all of them. (It's not that a
partial annotation isn't useful, it's just that if one member needs protection,
very likely they all do, so there's usually no point in not annotating
everything.)

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

Acknowledged.

 
   // Contains true of a key-press has been detected.
   bool typing_status_;
 
   // Delay values used by the AEC.
   int play_delay_ms_;
   int rec_delay_ms_;
 
   // Contains a clock-drift measurement.
   int clock_drift_;
@@ skipping 61 matching lines @@
   uint64_t rec_start_time_;
 
   // Set to true at construction and modified to false as soon as one audio-
   // level estimate larger than zero is detected.
   bool only_silence_recorded_;
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_DEVICE_AUDIO_DEVICE_BUFFER_H_