Now uses rtc::Buffer in AudioDeviceBuffer

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 5c4a0638b321fbd9f804a311134dfdaa352b8071..5805db7743c7f7ccc43d4c84ee1604dfa019d33f 100644
--- a/webrtc/modules/audio_device/audio_device_buffer.h
+++ b/webrtc/modules/audio_device/audio_device_buffer.h
@@ -11,6 +11,7 @@
 #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/task_queue.h"
 #include "webrtc/base/thread_checker.h"
@@ -19,8 +20,6 @@
 #include "webrtc/typedefs.h"
 
 namespace webrtc {
-class CriticalSectionWrapper;
-
 // Delta times between two successive playout callbacks are limited to this
 // value before added to an internal array.
 const size_t kMaxDeltaTimeInMs = 500;
@@ -73,12 +72,6 @@ class AudioDeviceBuffer {
   int32_t SetTypingStatus(bool typing_status);
 
  private:
-  // Playout and recording parameters can change on the fly. e.g. at device
-  // switch. These methods ensures that the callback methods always use the
-  // latest parameters.
-  void UpdatePlayoutParameters();
-  void UpdateRecordingParameters();
-
   // Posts the first delayed task in the task queue and starts the periodic
   // timer.
   void StartTimer();
@@ -106,8 +99,8 @@ class AudioDeviceBuffer {
 
   // TODO(henrika): given usage of thread checker, it should be possible to
   // remove all locks in this class.
-  rtc::CriticalSection _critSect;
-  rtc::CriticalSection _critSectCb;
+  rtc::CriticalSection lock_;
+  rtc::CriticalSection lock_cb_;

[kwiberg-webrtc, 2016/09/14 08:39:05]

This class uses none of our very cool and very handy compiler-checked lock
annotations (available for both mutexes and thread checkers). It's a good idea
to always use them in new code, and start using them in old code when you have
to touch locking code, because it's too easy to make mistakes when verifying
correctness manually.

Example CL here, if you need it: https://codereview.webrtc.org/2286343002/

Since this CL reduces the scope that's protected by locks in several member
functions, I'd strongly recommend that you add annotations (for all member
variables; annotating just a subset is seldom a good compromise). Because that's
going to touch a fair number of lines, it may make sense to do it in a separate
CL.

[henrika_webrtc, 2016/09/14 10:26:54]

Would love to improve this area. Hope you are OK with if I do that in a separate
CLs. Feels better to use a title where it is clear what the change does. Thanks
for the reference ;-)

PS I might also be able to remove some of these locks based on knowledge how we
call the APIs.

[kwiberg-webrtc, 2016/09/14 11:17:17]

Acknowledged.

 
   // 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
@@ -125,28 +118,17 @@ class AudioDeviceBuffer {
   size_t rec_channels_;
   size_t play_channels_;
 
-  // selected recording channel (left/right/both)
-  AudioDeviceModule::ChannelType rec_channel_;
-
   // Number of bytes per audio sample (2 or 4).
   size_t rec_bytes_per_sample_;
   size_t play_bytes_per_sample_;
 
-  // Number of audio samples/bytes per 10ms.
-  size_t rec_samples_per_10ms_;
-  size_t rec_bytes_per_10ms_;
-  size_t play_samples_per_10ms_;
-  size_t play_bytes_per_10ms_;
-
-  // Buffer used for recorded audio samples. Size is currently fixed
-  // but it should be changed to be dynamic and correspond to
-  // |play_bytes_per_10ms_|. TODO(henrika): avoid using fixed (max) size.
-  std::unique_ptr<int8_t[]> rec_buffer_;
-
-  // Buffer used for audio samples to be played out. Size is currently fixed
-  // but it should be changed to be dynamic and correspond to
-  // |play_bytes_per_10ms_|. TODO(henrika): avoid using fixed (max) size.
-  std::unique_ptr<int8_t[]> play_buffer_;
+  // Byte buffer used for recorded audio samples. Size can be changed
+  // dynamically.
+  rtc::Buffer rec_buffer_;
+
+  // Buffer used for audio samples to be played out. Size can be changed
+  // dynamically.
+  rtc::Buffer play_buffer_;

[kwiberg-webrtc, 2016/09/14 08:39:05]

The dynamic resizing is obvious from the type, so just e.g.

  rtc::Buffer rec_buffer_;   // Recorded audio samples.
  rtc::Buffer play_buffer_;  // Audio samples to be played out.

ought to be sufficient.

Also, from the way these are used in the .cc file, it seems like they should
contain a bunch of int16_t rather than bytes---i.e., they should be
rtc::BufferT<int16_t>.

[henrika_webrtc, 2016/09/14 10:26:54]

This class has always worked with bytes but I must agree that int16 feels
better. The declarations for input buffers uses void* and same for output (not
perfect I know but it would touch many files to change that). Let me try to
change to int16 and see what that looks like. I think it can be beneficial.

[kwiberg-webrtc, 2016/09/14 11:17:17]

Acknowledged.

[henrika_webrtc, 2016/10/17 09:31:10]

Tried it but it is rather tricky actually since several interfaces (that also
are used in Chrome) assumes byte vectors. 

I would like to skip this part now and come back with a change in a separate CL
with a more clear heading.

Thanks.

 
   // AGC parameters.
   uint32_t current_mic_level_;