AudioDeviceBuffer now uses 16-bit buffers

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.
  */
 
 #include <algorithm>
 
 #include "webrtc/modules/audio_device/audio_device_buffer.h"
 
 #include "webrtc/base/arraysize.h"
 #include "webrtc/base/bind.h"
 #include "webrtc/base/checks.h"
 #include "webrtc/base/logging.h"
 #include "webrtc/base/format_macros.h"
 #include "webrtc/base/timeutils.h"
 #include "webrtc/common_audio/signal_processing/include/signal_processing_library.h"
 #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";
 
 // Time between two sucessive calls to LogStats().
 static const size_t kTimerIntervalInSeconds = 10;
 static const size_t kTimerIntervalInMilliseconds =
     kTimerIntervalInSeconds * rtc::kNumMillisecsPerSec;
 // Min time required to qualify an audio session as a "call". If playout or
 // recording has been active for less than this time we will not store any
@@ skipping 260 matching lines @@
 
 int32_t AudioDeviceBuffer::StopOutputFileRecording() {
   LOG(LS_WARNING) << "Not implemented";
   return 0;
 }
 
 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()) {
     LOG(LS_INFO) << "Size of recording buffer: " << rec_buffer_.size();
   }
   // Derive a new level value twice per second and check if it is non-zero.
   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
     // again by restarting the call.
     if (max_abs > 0) {
       only_silence_recorded_ = false;
     }
   }
   // 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 be
   // zero in most calls and then have no effect of the stats. It is only updated
   // approximately two times per second and can then change the stats.
   task_queue_.PostTask(
       [this, max_abs, num_samples] { UpdateRecStats(max_abs, num_samples); });
   return 0;
 }
 
 int32_t AudioDeviceBuffer::DeliverRecordedData() {
   RTC_DCHECK_RUN_ON(&recording_thread_checker_);
   if (!audio_transport_cb_) {
     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 {
     LOG(LS_ERROR) << "RecordedDataIsAvailable() failed";
   }
   return 0;
 }
 
 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();
   }
 
   size_t num_samples_out(0);
   // 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;
+  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";
   }
 
   // Derive a new level value twice per second.
   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
   // are modified and read on the same thread. Note that |max_abs| will be
   // zero in most calls and then have no effect of the stats. It is only updated
   // approximately two times per second and can then change the stats.
   task_queue_.PostTask([this, max_abs, num_samples_out] {
     UpdatePlayStats(max_abs, num_samples_out);
   });
   return static_cast<int32_t>(num_samples_out);
 }
 
 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() {
   task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::LogStats, this,
                                  AudioDeviceBuffer::LOG_START));
 }
 
 void AudioDeviceBuffer::StopPeriodicLogging() {
   task_queue_.PostTask(rtc::Bind(&AudioDeviceBuffer::LogStats, this,
                                  AudioDeviceBuffer::LOG_STOP));
@@ skipping 88 matching lines @@
 void AudioDeviceBuffer::UpdatePlayStats(int16_t max_abs, size_t num_samples) {
   RTC_DCHECK_RUN_ON(&task_queue_);
   ++play_callbacks_;
   play_samples_ += num_samples;
   if (max_abs > max_play_level_) {
     max_play_level_ = max_abs;
   }
 }
 
 }  // namespace webrtc