Now uses rtc::Buffer in AudioDeviceBuffer

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/task_queue.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 {
-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;
 // TODO(henrika): remove when no longer used by external client.
 const size_t kMaxBufferSizeBytes = 3840;  // 10ms in stereo @ 96kHz
 
 class AudioDeviceObserver;
 
 class AudioDeviceBuffer {
  public:
@@ skipping 32 matching lines @@
   // 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);
 
  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();
 
   // Called periodically on the internal thread created by the TaskQueue.
   void LogStats();
 
   // Clears all members tracking stats for recording and playout.
   void ResetRecStats();
   void ResetPlayStats();
 
   // Updates counters in each play/record callback but does it on the task
   // queue to ensure that they can be read by LogStats() without any locks since
   // each task is serialized by the task queue.
   void UpdateRecStats(const void* audio_buffer, size_t num_samples);
   void UpdatePlayStats(const void* audio_buffer, size_t num_samples);
 
   // Ensures that methods are called on the same thread as the thread that
   // creates this object.
   rtc::ThreadChecker thread_checker_;
 
   // 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 _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
   // each task.
   rtc::TaskQueue task_queue_;
 
   // Ensures that the timer is only started once.
   bool timer_has_started_;
 
   // Sample rate in Hertz.
   uint32_t rec_sample_rate_;
   uint32_t play_sample_rate_;
 
   // Number of audio channels.
   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_;
+  // Byte buffer used for recorded audio samples. Size can be changed
+  // dynamically.
+  rtc::Buffer rec_buffer_;
 
-  // 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_;
+  // 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_;
   uint32_t new_mic_level_;
 
   // Contains true of a key-press has been detected.
   bool typing_status_;
 
   // Delay values used by the AEC.
   int play_delay_ms_;
@@ skipping 58 matching lines @@
   // all level measurements since the last log has been exactly zero.
   // In other words: this counter is incremented only if 20 measurements
   // (two per second) in a row equals zero. The member is only incremented on
   // the task queue and max once every 10th second.
   size_t num_rec_level_is_zero_;
 };
 
 }  // namespace webrtc
 
 #endif  // WEBRTC_MODULES_AUDIO_DEVICE_AUDIO_DEVICE_BUFFER_H_