Avoids invalid copy of audio buffer to task queue

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 d3b7917a5e4b7680a917a53bb1dadc2e8eb64147..8ba416d5d0516d936e08914b2303d2da05215c20 100644
--- a/webrtc/modules/audio_device/audio_device_buffer.cc
+++ b/webrtc/modules/audio_device/audio_device_buffer.cc
@@ -59,7 +59,9 @@ AudioDeviceBuffer::AudioDeviceBuffer()
       last_log_stat_time_(0),
       max_rec_level_(0),
       max_play_level_(0),
-      num_rec_level_is_zero_(0) {
+      num_rec_level_is_zero_(0),
+      rec_stat_count_(0),
+      play_stat_count_(0) {
   LOG(INFO) << "AudioDeviceBuffer::ctor";
 }
 
@@ -234,12 +236,12 @@ int32_t AudioDeviceBuffer::StopOutputFileRecording() {
 
 int32_t AudioDeviceBuffer::SetRecordedBuffer(const void* audio_buffer,
                                              size_t num_samples) {
-  const size_t rec_bytes_per_sample = [&] {
+  const size_t rec_channels = [&] {
     rtc::CritScope lock(&lock_);
-    return rec_bytes_per_sample_;
+    return rec_channels_;
   }();
   // Copy the complete input buffer to the local buffer.
-  const size_t size_in_bytes = num_samples * rec_bytes_per_sample;
+  const size_t size_in_bytes = num_samples * rec_channels * sizeof(int16_t);
   const size_t old_size = rec_buffer_.size();
   rec_buffer_.SetData(static_cast<const uint8_t*>(audio_buffer), size_in_bytes);
   // Keep track of the size of the recording buffer. Only updated when the
@@ -247,10 +249,19 @@ int32_t AudioDeviceBuffer::SetRecordedBuffer(const void* audio_buffer,
   if (old_size != rec_buffer_.size()) {
     LOG(LS_INFO) << "Size of recording buffer: " << rec_buffer_.size();
   }
+  // Derive a new level value (using max(abs())) twice per second.
+  int16_t max_abs = 0;
+  if (++rec_stat_count_ == 50) {

[kwiberg-webrtc, 2016/10/20 17:02:59]

Either DCHECK that the value isn't more than 50, or test >= 50 here. (Or both.)

[henrika_webrtc, 2016/10/21 08:40:22]

Sure thing but should it really be needed?

[kwiberg-webrtc, 2016/10/21 09:04:43]

Well, no. DCHECKs are, by design, never needed, since they only verify things
that you already know to be true. :-)

Think of it as machine-checkable documentation.

+    const size_t size = num_samples * rec_channels;
+    max_abs = WebRtcSpl_MaxAbsValueW16(
+        static_cast<int16_t*>(const_cast<void*>(audio_buffer)), size);

[kwiberg-webrtc, 2016/10/20 17:02:59]

You shouldn't need to cast away the constness. WebRtcSpl_MaxAbsValueW16 takes a
const array argument.

+    rec_stat_count_ = 0;
+  }
   // Update some stats but do it on the task queue to ensure that the members
-  // are modified and read on the same thread.
+  // are modified and read on the same thread. Note that |max_abs| will only be
+  // non-zero two times per second approximately.

[kwiberg-webrtc, 2016/10/20 17:02:59]

Don't you mean rec_stat_count_? Or that max_abs will only change ~twice a
second?

[henrika_webrtc, 2016/10/21 08:40:22]

I mean max_abs. Let me elaborate.

   task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::UpdateRecStats, this,
-                                 audio_buffer, num_samples));
+                                 max_abs, num_samples));
   return 0;
 }
 
@@ -291,14 +302,15 @@ int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
   last_playout_time_ = now_time;
   playout_diff_times_[diff_time]++;
 
-  const size_t play_bytes_per_sample = [&] {
+  const size_t play_channels = [&] {
     rtc::CritScope lock(&lock_);
-    return play_bytes_per_sample_;
+    return play_channels_;
   }();
 
   // 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
   // method.
+  const size_t play_bytes_per_sample = play_channels * sizeof(int16_t);
   const size_t size_in_bytes = num_samples * play_bytes_per_sample;
   if (play_buffer_.size() != size_in_bytes) {
     play_buffer_.SetSize(size_in_bytes);
@@ -314,20 +326,30 @@ int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
     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;
   size_t num_samples_out(0);
   uint32_t res = audio_transport_cb_->NeedMorePlayData(
-      num_samples, play_bytes_per_sample_, play_channels_, play_sample_rate_,
+      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";
   }
 
-  // Update some stats but do it on the task queue to ensure that access of
-  // members is serialized hence avoiding usage of locks.
+  // Derive a new level value (using max(abs())) twice per second.
+  int16_t max_abs = 0;
+  if (++play_stat_count_ == 50) {

[kwiberg-webrtc, 2016/10/20 17:02:59]

DCHECK and/or >=

[henrika_webrtc, 2016/10/21 08:40:22]

Done.

+    const size_t size = num_samples * play_channels;
+    max_abs = WebRtcSpl_MaxAbsValueW16(
+        reinterpret_cast<int16_t*>(play_buffer_.data()), size);

[kwiberg-webrtc, 2016/10/20 17:02:59]

You use static_cast in the other loop. Please be consistent.

[henrika_webrtc, 2016/10/21 08:40:22]

Will fix.

+    play_stat_count_ = 0;
+  }
+  // Update some stats but do it on the task queue to ensure that the members
+  // are modified and read on the same thread. Note that |max_abs| will only be
+  // non-zero two times per second approximately.

[kwiberg-webrtc, 2016/10/20 17:02:59]

Comment wrong here too?

[henrika_webrtc, 2016/10/21 08:40:22]

Actually no but let me explain better.

   task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::UpdatePlayStats, this,
-                                 play_buffer_.data(), num_samples_out));
+                                 max_abs, num_samples_out));
   return static_cast<int32_t>(num_samples_out);
 }
 
@@ -421,39 +443,21 @@ void AudioDeviceBuffer::ResetPlayStats() {
   max_play_level_ = 0;
 }
 
-void AudioDeviceBuffer::UpdateRecStats(const void* audio_buffer,
-                                       size_t num_samples) {
+void AudioDeviceBuffer::UpdateRecStats(int16_t max_abs, size_t num_samples) {
   RTC_DCHECK(task_queue_.IsCurrent());
   ++rec_callbacks_;
   rec_samples_ += num_samples;
-
-  // Find the max absolute value in an audio packet twice per second and update
-  // |max_rec_level_| to track the largest value.
-  if (rec_callbacks_ % 50 == 0) {
-    int16_t max_abs = WebRtcSpl_MaxAbsValueW16(
-        static_cast<int16_t*>(const_cast<void*>(audio_buffer)),
-        num_samples * rec_channels_);
-    if (max_abs > max_rec_level_) {
-      max_rec_level_ = max_abs;
-    }
+  if (max_abs > max_rec_level_) {
+    max_rec_level_ = max_abs;
   }
 }
 
-void AudioDeviceBuffer::UpdatePlayStats(const void* audio_buffer,
-                                        size_t num_samples) {
+void AudioDeviceBuffer::UpdatePlayStats(int16_t max_abs, size_t num_samples) {
   RTC_DCHECK(task_queue_.IsCurrent());
   ++play_callbacks_;
   play_samples_ += num_samples;
-
-  // Find the max absolute value in an audio packet twice per second and update
-  // |max_play_level_| to track the largest value.
-  if (play_callbacks_ % 50 == 0) {
-    int16_t max_abs = WebRtcSpl_MaxAbsValueW16(
-        static_cast<int16_t*>(const_cast<void*>(audio_buffer)),
-        num_samples * play_channels_);
-    if (max_abs > max_play_level_) {
-      max_play_level_ = max_abs;
-    }
+  if (max_abs > max_play_level_) {
+    max_play_level_ = max_abs;
   }
 }