AudioDeviceBuffer now uses 16-bit buffers

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 19d45b28569feaa9688449a73fb788e22915f9c2..2ed305a6a4f19264ced87c37d2986406f9c78b68 100644
--- a/webrtc/modules/audio_device/audio_device_buffer.cc
+++ b/webrtc/modules/audio_device/audio_device_buffer.cc
@@ -22,8 +22,6 @@
 #include "webrtc/modules/audio_device/audio_device_config.h"
 #include "webrtc/system_wrappers/include/metrics.h"
 
-#include "webrtc/base/platform_thread.h"
-
 namespace webrtc {
 
 static const char kTimerQueueName[] = "AudioDeviceBufferTimer";
@@ -304,9 +302,11 @@ int32_t AudioDeviceBuffer::SetRecordedBuffer(const void* audio_buffer,
                                              size_t num_samples) {
   RTC_DCHECK_RUN_ON(&recording_thread_checker_);
   // Copy the complete input buffer to the local buffer.
-  const size_t size_in_bytes = num_samples * rec_channels_ * sizeof(int16_t);
+  const size_t sample_frame_size = rec_channels_;
+  const size_t packet_size_in_frames = sample_frame_size * num_samples;

[kwiberg-webrtc, 2016/11/08 12:19:58]

No... Since a frame consists of 1 or more samples (the same number as the number
of channels), the packet size in frames should be the packet size in samples
DIVIDED BY the number of samples per frame. It looks like what you're doing here
is converting the number of samples per packet per channel ("num_samples") into
the total number of samples per packet ("packet_size_in_frames"). If this is the
case, the variables are confusingly named, and I would suggest something like

  num_samples           -> samples_per_channel
  sample_frame_size     -> remove, and just use rec_channels_
  packet_size_in_frames -> total_samples = samples_per_channel * rec_channels_

Or is this a case of the reviewer being confused?

[henrika_webrtc, 2016/11/08 12:33:59]

I am using a standard notation from how audio samples are denoted.
It is used in e.g Core Audio on MacOS X.

In Core Audio, the following definitions apply:

An audio stream is a continuous series of data that represents a sound, such as
a song.

A channel is a discrete track of monophonic audio. A monophonic stream has one
channel; a stereo stream has two channels.

A sample is single numerical value for a single audio channel in an audio
stream.

A frame is a collection of time-coincident samples. For instance, a linear PCM
stereo sound file has two samples per frame, one for the left channel and one
for the right channel.

A packet is a collection of one or more contiguous frames. A packet defines the
smallest meaningful set of frames for a given audio data format, and is the
smallest data unit for which time can be measured. In linear PCM audio, a packet
holds a single frame. In compressed formats, it typically holds more; in some
formats, the number of frames per packet varies.

The sample rate for a stream is the number of frames per second of uncompressed
(or, for compressed formats, the equivalent in decompressed) audio.

Still confused?

In any case, introducing the term 'samples_per_channel' is IMHO plain wrong.

[kwiberg-webrtc, 2016/11/08 13:01:55]

OK.
OK. That's the same definition I was using.
OK, same definition I was using. So in this case, a sample is a single
int16_t.
OK. That's the same definition I was using.
OK. That's the same definition I was using.
Sounds reasonable.
Sounds reasonable.
OK. That's the same definition I was using.
Well, yes... on line 305,

  const size_t sample_frame_size = rec_channels_;

you're computing the number of samples per frame (or, phrased another
way, the size of a frame in samples). "sample_frame_size" is not a
great name for this---I would have picked "samples_per_frame", or not
made a separate variable for this---but I don't find it actively
misleading.

On line 306,

  const size_t packet_size_in_frames = sample_frame_size * num_samples;

you seem to be computing the number of samples per packet by
multiplying the number of samples per frame (sample_frame_size) by the
number of frames per packet (num_samples). If that's the case, there
are two problems here: "packet_size_in_frames" is a bad name for the
number of samples in a packet. Also, this name seems to imply that
what was passed to SetRecordedBuffer was a single packet, but since
this is uncompressed PCM, a packet should by the definition you gave
contain just 1 frame. To me, it looks like you're just computing the
total number of samples the caller passed to us, no matter how many
frames is in a packet, so "total_samples" or "num_samples" or
something would be a better name.
The number of samples per channel is per definition the same as the
number of frames. If you assume the reader knows what a "frame" is,
then calling it "num_frames" would be a good idea. If you don't assume
such knowledge, calling it "samples_per_channel" would seem like a
reasonable alternative.

Do I still seem confused? I'm guessing one of us must be, since we
agree on all the definitions... :-)

   const size_t old_size = rec_buffer_.size();
-  rec_buffer_.SetData(static_cast<const uint8_t*>(audio_buffer), size_in_bytes);
+  rec_buffer_.SetData(static_cast<const uint16_t*>(audio_buffer),
+                      packet_size_in_frames);
   // 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()) {
@@ -316,10 +316,10 @@ int32_t AudioDeviceBuffer::SetRecordedBuffer(const void* audio_buffer,
   int16_t max_abs = 0;
   RTC_DCHECK_LT(rec_stat_count_, 50);
   if (++rec_stat_count_ >= 50) {
-    const size_t size = num_samples * rec_channels_;
     // Returns the largest absolute value in a signed 16-bit vector.
     max_abs = WebRtcSpl_MaxAbsValueW16(
-        reinterpret_cast<const int16_t*>(rec_buffer_.data()), size);
+        reinterpret_cast<const int16_t*>(rec_buffer_.data()),

[kwiberg-webrtc, 2016/11/08 12:19:58]

You shouldn't need this cast anymore, since rec_buffer_.data() returns int16_t*.

[henrika_webrtc, 2016/11/08 12:33:59]

Acknowledged.

+        rec_buffer_.size());
     rec_stat_count_ = 0;
     // Set |only_silence_recorded_| to false as soon as at least one detection
     // of a non-zero audio packet is found. It can only be restored to true
@@ -343,14 +343,14 @@ int32_t AudioDeviceBuffer::DeliverRecordedData() {
     LOG(LS_WARNING) << "Invalid audio transport";
     return 0;
   }
-  const size_t rec_bytes_per_sample = rec_channels_ * sizeof(int16_t);
+  const size_t sample_frame_size = rec_channels_;
+  const size_t sample_frame_size_in_bytes = sample_frame_size * sizeof(int16_t);
   uint32_t new_mic_level(0);
   uint32_t total_delay_ms = play_delay_ms_ + rec_delay_ms_;
-  size_t num_samples = rec_buffer_.size() / rec_bytes_per_sample;
   int32_t res = audio_transport_cb_->RecordedDataIsAvailable(
-      rec_buffer_.data(), num_samples, rec_bytes_per_sample, rec_channels_,
-      rec_sample_rate_, total_delay_ms, clock_drift_, current_mic_level_,
-      typing_status_, new_mic_level);
+      rec_buffer_.data(), rec_buffer_.size(), sample_frame_size_in_bytes,
+      rec_channels_, rec_sample_rate_, total_delay_ms, clock_drift_,
+      current_mic_level_, typing_status_, new_mic_level);
   if (res != -1) {
     new_mic_level_ = new_mic_level;
   } else {
@@ -361,13 +361,14 @@ int32_t AudioDeviceBuffer::DeliverRecordedData() {
 
 int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
   RTC_DCHECK_RUN_ON(&playout_thread_checker_);
-  // The consumer can change the request size on the fly and we therefore
+  // The consumer can change the requested 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);
+  // method. Each sample frame contains |sample_frame_size| * sizeof(int16_t)
+  // bytes.
+  const size_t sample_frame_size = play_channels_;
+  const size_t packet_size_in_frames = sample_frame_size * num_samples;

[kwiberg-webrtc, 2016/11/08 12:19:58]

Same comment as above about the variable names.

[henrika_webrtc, 2016/11/08 12:33:59]

See above

+  if (play_buffer_.size() != packet_size_in_frames) {
+    play_buffer_.SetSize(packet_size_in_frames);
     LOG(LS_INFO) << "Size of playout buffer: " << play_buffer_.size();
   }
 
@@ -382,9 +383,11 @@ int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
   // Retrieve new 16-bit PCM audio data using the audio transport instance.
   int64_t elapsed_time_ms = -1;
   int64_t ntp_time_ms = -1;
+  const size_t sample_frame_size_in_bytes = sample_frame_size * sizeof(int16_t);
   uint32_t res = audio_transport_cb_->NeedMorePlayData(
-      num_samples, play_bytes_per_sample, play_channels_, play_sample_rate_,
-      play_buffer_.data(), num_samples_out, &elapsed_time_ms, &ntp_time_ms);
+      num_samples, sample_frame_size_in_bytes, 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";
   }
@@ -393,10 +396,10 @@ int32_t AudioDeviceBuffer::RequestPlayoutData(size_t num_samples) {
   int16_t max_abs = 0;
   RTC_DCHECK_LT(play_stat_count_, 50);
   if (++play_stat_count_ >= 50) {
-    const size_t size = num_samples * play_channels_;
     // Returns the largest absolute value in a signed 16-bit vector.
     max_abs = WebRtcSpl_MaxAbsValueW16(
-        reinterpret_cast<const int16_t*>(play_buffer_.data()), size);
+        reinterpret_cast<const int16_t*>(play_buffer_.data()),

[kwiberg-webrtc, 2016/11/08 12:19:58]

Remove cast?

[henrika_webrtc, 2016/11/08 12:33:59]

Done.

+        play_buffer_.size());
     play_stat_count_ = 0;
   }
   // Update some stats but do it on the task queue to ensure that the members
@@ -413,8 +416,9 @@ int32_t AudioDeviceBuffer::GetPlayoutData(void* audio_buffer) {
   RTC_DCHECK_RUN_ON(&playout_thread_checker_);
   RTC_DCHECK_GT(play_buffer_.size(), 0u);
   const size_t play_bytes_per_sample = play_channels_ * sizeof(int16_t);
-  memcpy(audio_buffer, play_buffer_.data(), play_buffer_.size());
-  return static_cast<int32_t>(play_buffer_.size() / play_bytes_per_sample);
+  memcpy(audio_buffer, play_buffer_.data(),
+         play_buffer_.size() * play_bytes_per_sample);
+  return static_cast<int32_t>(play_buffer_.size());
 }
 
 void AudioDeviceBuffer::StartPeriodicLogging() {