Avoids invalid copy of audio buffer to task queue

webrtc/modules/audio_device/audio_device_buffer.cc

 /*
  *  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.
  */
 
@@ skipping 41 matching lines @@
       last_rec_callbacks_(0),
       play_callbacks_(0),
       last_play_callbacks_(0),
       rec_samples_(0),
       last_rec_samples_(0),
       play_samples_(0),
       last_play_samples_(0),
       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";
 }
 
 AudioDeviceBuffer::~AudioDeviceBuffer() {
   RTC_DCHECK(thread_checker_.CalledOnValidThread());
   LOG(INFO) << "AudioDeviceBuffer::~dtor";
 
   size_t total_diff_time = 0;
   int num_measurements = 0;
   LOG(INFO) << "[playout diff time => #measurements]";
@@ skipping 154 matching lines @@
   return 0;
 }
 
 int32_t AudioDeviceBuffer::StopOutputFileRecording() {
   LOG(LS_WARNING) << "Not implemented";
   return 0;
 }
 
 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
   // size changes, which is a rare event.
   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;
 }
 
 int32_t AudioDeviceBuffer::DeliverRecordedData() {
   rtc::CritScope lock(&lock_cb_);
   if (!audio_transport_cb_) {
     LOG(LS_WARNING) << "Invalid audio transport";
     return 0;
   }
   const size_t rec_bytes_per_sample = [&] {
@@ skipping 20 matching lines @@
   // position/index corresponds to time differences (in milliseconds) between
   // two successive playout callbacks, and the stored value is the number of
   // times a given time difference was found.
   int64_t now_time = rtc::TimeMillis();
   size_t diff_time = rtc::TimeDiff(now_time, last_playout_time_);
   // Truncate at 500ms to limit the size of the array.
   diff_time = std::min(kMaxDeltaTimeInMs, diff_time);
   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);
     LOG(LS_INFO) << "Size of playout buffer: " << play_buffer_.size();
   }
 
   rtc::CritScope lock(&lock_cb_);
 
   // It is currently supported to start playout without a valid audio
   // transport object. Leads to warning and silence.
   if (!audio_transport_cb_) {
     LOG(LS_WARNING) << "Invalid audio transport";
     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);
 }
 
 int32_t AudioDeviceBuffer::GetPlayoutData(void* audio_buffer) {
   RTC_DCHECK_GT(play_buffer_.size(), 0u);
   const size_t play_bytes_per_sample = [&] {
     rtc::CritScope lock(&lock_);
     return play_bytes_per_sample_;
   }();
   memcpy(audio_buffer, play_buffer_.data(), play_buffer_.size());
@@ skipping 73 matching lines @@
 
 void AudioDeviceBuffer::ResetPlayStats() {
   last_playout_time_ = rtc::TimeMillis();
   play_callbacks_ = 0;
   last_play_callbacks_ = 0;
   play_samples_ = 0;
   last_play_samples_ = 0;
   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;
   }
 }
 
 }  // namespace webrtc