Limit audio buffer size in the audio player used by the chromoting client.

remoting/client/plugin/pepper_audio_player.cc

Index: remoting/client/plugin/pepper_audio_player.cc
diff --git a/remoting/client/plugin/pepper_audio_player.cc b/remoting/client/plugin/pepper_audio_player.cc
index d9c53a5729ee7e70fc311404e3b2e28c8fad7f49..b52a40cf251816ce94e30b2fd96d8ec8e5c56fdd 100644
--- a/remoting/client/plugin/pepper_audio_player.cc
+++ b/remoting/client/plugin/pepper_audio_player.cc
@@ -9,14 +9,18 @@
 #include "base/stl_util.h"
 
 namespace {
-// Constants used to create an audio configuration resource.
-// The sample count we will request from the browser.
-const uint32_t kSampleFrameCount = 4096u;
+
+// The frame size we will request from the browser.
+const int kFrameSizeMs = 40;
+
 // The number of channels in the audio stream (only supporting stereo audio
 // for now).
-const uint32_t kChannels = 2u;
+const int kChannels = 2u;
 const int kSampleSizeBytes = 2;
 
+// If queue grows bigger than 150ms we start dropping packets.
+const int kMaxQueueLatencyMs = 150;
+
 PP_AudioSampleRate ConvertToPepperSampleRate(
     remoting::AudioPacket::SamplingRate sampling_rate) {
   switch (sampling_rate) {
@@ -37,9 +41,10 @@ namespace remoting {
 PepperAudioPlayer::PepperAudioPlayer(pp::Instance* instance)
     : instance_(instance),
       sampling_rate_(AudioPacket::SAMPLING_RATE_INVALID),
-      samples_per_frame_(kSampleFrameCount),
-      bytes_consumed_(0),
-      start_failed_(false) {
+      samples_per_frame_(0),
+      start_failed_(false),
+      queued_samples_(0),
+      bytes_consumed_(0) {
 }
 
 PepperAudioPlayer::~PepperAudioPlayer() {}
@@ -50,16 +55,14 @@ bool PepperAudioPlayer::ResetAudioPlayer(
   PP_AudioSampleRate sample_rate =
       ConvertToPepperSampleRate(sampling_rate);
 
-  // Ask the browser/device for an appropriate sample frame count size.
-  samples_per_frame_ =
-      pp::AudioConfig::RecommendSampleFrameCount(instance_,
-                                                 sample_rate,
-                                                 kSampleFrameCount);
+  // Ask the browser/device for an appropriate frame size.
+  samples_per_frame_ = pp::AudioConfig::RecommendSampleFrameCount(
+      instance_, sample_rate,
+      kFrameSizeMs * sampling_rate / base::Time::kMillisecondsPerSecond);

[Wez, 2012/09/12 20:47:57]

nit: Since you do a similar calculation based on the max latency later on, wrap
this into a trivial static helper function GetFrameSize()?

[Sergey Ulanov, 2012/09/12 21:43:34]

Done. Called the new function MsecToSamples()

 
   // Create an audio configuration resource.
-  pp::AudioConfig audio_config = pp::AudioConfig(instance_,
-                                                 sample_rate,
-                                                 samples_per_frame_);
+  pp::AudioConfig audio_config = pp::AudioConfig(
+      instance_, sample_rate, samples_per_frame_);
 
   // Create an audio resource.
   audio_ = pp::Audio(instance_, audio_config, PepperAudioPlayerCallback, this);
@@ -72,20 +75,12 @@ bool PepperAudioPlayer::ResetAudioPlayer(
 }
 
 void PepperAudioPlayer::ProcessAudioPacket(scoped_ptr<AudioPacket> packet) {
-  // TODO(kxing): Limit the size of the queue so that latency doesn't grow
-  // too large.
-
   CHECK_EQ(1, packet->data_size());
   DCHECK_EQ(AudioPacket::ENCODING_RAW, packet->encoding());
   DCHECK_NE(AudioPacket::SAMPLING_RATE_INVALID, packet->sampling_rate());
   DCHECK_EQ(kSampleSizeBytes, packet->bytes_per_sample());
   DCHECK_EQ(static_cast<int>(kChannels), packet->channels());
-
-  if (packet->data(0).size() % (kChannels * kSampleSizeBytes) != 0) {
-    LOG(WARNING) << "Received corrupted packet.";
-    return;
-  }
-  base::AutoLock auto_lock(lock_);
+  DCHECK_EQ(packet->data(0).size() % (kChannels * kSampleSizeBytes), 0u);

[Wez, 2012/09/12 20:47:57]

Is this check correct; do we not allow splitting of samples across packet
boundaries in any of our Audio capturers?

[Sergey Ulanov, 2012/09/12 21:43:34]

Yes, it's correct. There is no way you can encode/decode packets with partial
samples. I added check for this in the verbatim decoder and the capturers
guarantee that the packets they produce always contain integer number of
samples.

 
   // No-op if the Pepper player won't start.
   if (start_failed_) {
@@ -96,7 +91,11 @@ void PepperAudioPlayer::ProcessAudioPacket(scoped_ptr<AudioPacket> packet) {
   if (sampling_rate_ != packet->sampling_rate()) {
     // Drop all packets currently in the queue, since they are sampled at the
     // wrong rate.
-    STLDeleteElements(&queued_packets_);
+    {
+      base::AutoLock auto_lock(lock_);
+      STLDeleteElements(&queued_packets_);
+      queued_samples_ = 0;
+    }
 
     bool success = ResetAudioPlayer(packet->sampling_rate());
     if (!success) {
@@ -105,6 +104,15 @@ void PepperAudioPlayer::ProcessAudioPacket(scoped_ptr<AudioPacket> packet) {
     }
   }
 
+  base::AutoLock auto_lock(lock_);
+
+  if (queued_samples_ > kMaxQueueLatencyMs * sampling_rate_ /
+      base::Time::kMillisecondsPerSecond) {
+    STLDeleteElements(&queued_packets_);
+    queued_samples_ = 0;
+  }
+
+  queued_samples_ += packet->data(0).size() / (kChannels * kSampleSizeBytes);

[Wez, 2012/09/12 20:47:57]

nit: Since you use kSampleSizeBytes * kChannels in several places, consider
defining it as its own constant e.g. kAllChannelsSampleBytes.

[Sergey Ulanov, 2012/09/12 21:43:34]

Done. I wish there was a term for set of samples for all channels. Media code
calls it a frame, but that would be confusing here, as we use "frame" for
different purposes.

   queued_packets_.push_back(packet.release());
 }
 
@@ -152,6 +160,8 @@ void PepperAudioPlayer::FillWithSamples(void* samples, uint32_t buffer_size) {
     next_sample += bytes_to_copy;
     bytes_consumed_ += bytes_to_copy;
     bytes_extracted += bytes_to_copy;
+    queued_samples_ -= bytes_to_copy / kSampleSizeBytes / kChannels;
+    DCHECK_GE(queued_samples_, 0);
   }
 }