Better silence detection in audio capturers

Previously audio capturers were dropping silent packets too aggressively
and that would disrupt playback on the client side when there are short
silence pauses in the stream.

BUG=164308

Review URL: https://codereview.chromium.org/11694002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@174886 0039d316-1c4b-4281-b951-d872f2087c98

10 files changed, 200 insertions, 75 deletions

diff --git a/remoting/host/audio_capturer_linux.cc b/remoting/host/audio_capturer_linux.cc
index d8cb548..59e30e4 100644
--- a/remoting/host/audio_capturer_linux.cc
+++ b/remoting/host/audio_capturer_linux.cc
@@ -36,7 +36,8 @@ void AudioCapturerLinux::InitializePipeReader(
 
 AudioCapturerLinux::AudioCapturerLinux(
     scoped_refptr<AudioPipeReader> pipe_reader)
-    : pipe_reader_(pipe_reader) {
+    : pipe_reader_(pipe_reader),
+      silence_detector_(0) {
 }
 
 AudioCapturerLinux::~AudioCapturerLinux() {
@@ -44,6 +45,7 @@ AudioCapturerLinux::~AudioCapturerLinux() {
 
 bool AudioCapturerLinux::Start(const PacketCapturedCallback& callback) {
   callback_ = callback;
+  silence_detector_.Reset(kSamplingRate, AudioPacket::CHANNELS_STEREO);
   pipe_reader_->AddObserver(this);
   return true;
 }
@@ -61,6 +63,12 @@ void AudioCapturerLinux::OnDataRead(
     scoped_refptr<base::RefCountedString> data) {
   DCHECK(!callback_.is_null());
 
+  if (silence_detector_.IsSilence(
+          reinterpret_cast<const int16*>(data->data().data()),
+          data->data().size() / sizeof(int16))) {
+    return;
+  }
+
   scoped_ptr<AudioPacket> packet(new AudioPacket());
   packet->add_data(data->data());
   packet->set_encoding(AudioPacket::ENCODING_RAW);
diff --git a/remoting/host/audio_capturer_linux.h b/remoting/host/audio_capturer_linux.h
index 1a9453c..b52f1ff 100644
--- a/remoting/host/audio_capturer_linux.h
+++ b/remoting/host/audio_capturer_linux.h
@@ -7,6 +7,7 @@
 
 #include "base/memory/ref_counted.h"
 #include "remoting/host/audio_capturer.h"
+#include "remoting/host/audio_silence_detector.h"
 #include "remoting/host/linux/audio_pipe_reader.h"
 
 class FilePath;
@@ -41,6 +42,8 @@ class AudioCapturerLinux : public AudioCapturer,
   scoped_refptr<AudioPipeReader> pipe_reader_;
   PacketCapturedCallback callback_;
 
+  AudioSilenceDetector silence_detector_;
+
   DISALLOW_COPY_AND_ASSIGN(AudioCapturerLinux);
 };
 
diff --git a/remoting/host/audio_capturer_win.cc b/remoting/host/audio_capturer_win.cc
index 977bdaf..cca276b 100644
--- a/remoting/host/audio_capturer_win.cc
+++ b/remoting/host/audio_capturer_win.cc
@@ -16,8 +16,8 @@
 
 namespace {
 const int kChannels = 2;
-const int kBitsPerSample = 16;
-const int kBitsPerByte = 8;
+const int kBytesPerSample = 2;
+const int kBitsPerSample = kBytesPerSample * 8;
 // Conversion factor from 100ns to 1ms.
 const int k100nsPerMillisecond = 10000;
 
@@ -38,7 +38,8 @@ const int kMaxExpectedTimerLag = 30;
 namespace remoting {
 
 AudioCapturerWin::AudioCapturerWin()
-    : sampling_rate_(AudioPacket::SAMPLING_RATE_INVALID) {
+    : sampling_rate_(AudioPacket::SAMPLING_RATE_INVALID),
+      silence_detector_(kSilenceThreshold){
       thread_checker_.DetachFromThread();
 }
 
@@ -120,9 +121,9 @@ bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) {
       wave_format_ex_->wFormatTag = WAVE_FORMAT_PCM;
       wave_format_ex_->nChannels = kChannels;
       wave_format_ex_->wBitsPerSample = kBitsPerSample;
-      wave_format_ex_->nBlockAlign = kChannels * kBitsPerSample / kBitsPerByte;
+      wave_format_ex_->nBlockAlign = kChannels * kBytesPerSample;
       wave_format_ex_->nAvgBytesPerSec =
-          sampling_rate_ * kChannels * kBitsPerSample / kBitsPerByte;
+          sampling_rate_ * kChannels * kBytesPerSample;
       break;
     case WAVE_FORMAT_EXTENSIBLE: {
       PWAVEFORMATEXTENSIBLE wave_format_extensible =
@@ -145,9 +146,9 @@ bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) {
         wave_format_extensible->Format.nSamplesPerSec = sampling_rate_;
         wave_format_extensible->Format.wBitsPerSample = kBitsPerSample;
         wave_format_extensible->Format.nBlockAlign =
-            kChannels * kBitsPerSample / kBitsPerByte;
+            kChannels * kBytesPerSample;
         wave_format_extensible->Format.nAvgBytesPerSec =
-            sampling_rate_ * kChannels * kBitsPerSample / kBitsPerByte;
+            sampling_rate_ * kChannels * kBytesPerSample;
       } else {
         LOG(ERROR) << "Failed to force 16-bit samples";
         return false;
@@ -188,6 +189,8 @@ bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) {
     return false;
   }
 
+  silence_detector_.Reset(sampling_rate_, kChannels);
+
   // Start capturing.
   capture_timer_->Start(FROM_HERE,
                         audio_device_period_,
@@ -235,16 +238,14 @@ void AudioCapturerWin::DoCapture() {
     BYTE* data;
     UINT32 frames;
     DWORD flags;
-    hr = audio_capture_client_->GetBuffer(
-        &data, &frames, &flags, NULL, NULL);
+    hr = audio_capture_client_->GetBuffer(&data, &frames, &flags, NULL, NULL);
     if (FAILED(hr)) {
       LOG(ERROR) << "Failed to GetBuffer. Error " << hr;
       return;
     }
 
-    if (!IsPacketOfSilence(
-            reinterpret_cast<const int16*>(data),
-            frames * kChannels)) {
+    if (!silence_detector_.IsSilence(
+            reinterpret_cast<const int16*>(data), frames * kChannels)) {
       scoped_ptr<AudioPacket> packet =
           scoped_ptr<AudioPacket>(new AudioPacket());
       packet->add_data(data, frames * wave_format_ex_->nBlockAlign);
@@ -264,19 +265,6 @@ void AudioCapturerWin::DoCapture() {
   }
 }
 
-// Detects whether there is audio playing in a packet of samples.
-// Windows can give nonzero samples, even when there is no audio playing, so
-// extremely low amplitude samples are counted as silence.
-// static
-bool AudioCapturerWin::IsPacketOfSilence(
-    const int16* samples, int number_of_samples) {
-  for (int i = 0; i < number_of_samples; i++) {
-    if (abs(samples[i]) > kSilenceThreshold)
-      return false;
-  }
-  return true;
-}
-
 bool AudioCapturer::IsSupported() {
   return true;
 }
diff --git a/remoting/host/audio_capturer_win.h b/remoting/host/audio_capturer_win.h
index ad3ddea..771cf20 100644
--- a/remoting/host/audio_capturer_win.h
+++ b/remoting/host/audio_capturer_win.h
@@ -14,6 +14,7 @@
 #include "base/win/scoped_co_mem.h"
 #include "base/win/scoped_comptr.h"
 #include "remoting/host/audio_capturer.h"
+#include "remoting/host/audio_silence_detector.h"
 #include "remoting/proto/audio.pb.h"
 
 namespace remoting {
@@ -28,8 +29,6 @@ class AudioCapturerWin : public AudioCapturer {
   virtual void Stop() OVERRIDE;
   virtual bool IsStarted() OVERRIDE;
 
-  static bool IsPacketOfSilence(const int16* samples, int number_of_samples);
-
  private:
   // Receives all packets from the audio capture endpoint buffer and pushes them
   // to the network.
@@ -42,6 +41,8 @@ class AudioCapturerWin : public AudioCapturer {
   scoped_ptr<base::RepeatingTimer<AudioCapturerWin> > capture_timer_;
   base::TimeDelta audio_device_period_;
 
+  AudioSilenceDetector silence_detector_;
+
   base::win::ScopedCoMem<WAVEFORMATEX> wave_format_ex_;
   base::win::ScopedComPtr<IAudioCaptureClient> audio_capture_client_;
   base::win::ScopedComPtr<IAudioClient> audio_client_;
diff --git a/remoting/host/audio_capturer_win_unittest.cc b/remoting/host/audio_capturer_win_unittest.cc
deleted file mode 100644
index 5b71ae4..0000000
--- a/remoting/host/audio_capturer_win_unittest.cc
+++ /dev/null
@@ -1,29 +0,0 @@
-// Copyright (c) 2012 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "remoting/host/audio_capturer_win.h"
-
-#include "base/basictypes.h"
-#include "testing/gtest/include/gtest/gtest.h"
-
-namespace remoting {
-
-TEST(AudioCapturerWinTest, SilenceTest) {
-  int16 samples1[] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
-  int16 samples2[] = {65, 73, 83, 89, 92, -1, 5, 9, 123, 0};
-  int16 samples3[] = {0, 0, 0, 0, 1, 0, 0, -1, 0, 0};
-  // MSVC++ doesn't allow allocating arrays of size 0.
-  int16 samples4[] = {0};
-
-  ASSERT_TRUE(
-      AudioCapturerWin::IsPacketOfSilence(samples1, arraysize(samples1)));
-  ASSERT_FALSE(
-      AudioCapturerWin::IsPacketOfSilence(samples2, arraysize(samples2)));
-  ASSERT_TRUE(
-      AudioCapturerWin::IsPacketOfSilence(samples3, arraysize(samples3)));
-  ASSERT_TRUE(
-      AudioCapturerWin::IsPacketOfSilence(samples4, 0));
-}
-
-}  // namespace remoting
diff --git a/remoting/host/audio_silence_detector.cc b/remoting/host/audio_silence_detector.cc
new file mode 100644
index 0000000..17d625f
--- /dev/null
+++ b/remoting/host/audio_silence_detector.cc
@@ -0,0 +1,59 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/audio_silence_detector.h"
+
+#include <stdlib.h>
+
+namespace remoting {
+
+namespace {
+
+// Silence period threshold in seconds. Silence intervals shorter than this
+// value are still encoded and sent to the client, so that we don't disrupt
+// playback by dropping them.
+int kSilencePeriodThresholdSeconds = 1;
+
+}  // namespace
+
+AudioSilenceDetector::AudioSilenceDetector(int threshold)
+    : threshold_(threshold),
+      silence_length_max_(0),
+      silence_length_(0) {
+  DCHECK_GE(threshold_, 0);
+}
+
+AudioSilenceDetector::~AudioSilenceDetector() {
+}
+
+void AudioSilenceDetector::Reset(int sampling_rate, int channels) {
+  DCHECK_GT(sampling_rate, 0);
+  silence_length_ = 0;
+  silence_length_max_ =
+      sampling_rate * channels * kSilencePeriodThresholdSeconds;
+}
+
+bool AudioSilenceDetector::IsSilence(const int16* samples,
+                                     size_t samples_count) {
+  bool silent_packet = true;
+  // Potentially this loop can be optimized (e.g. using SSE or adding special
+  // case for threshold_==0), but it's not worth worrying about because the
+  // amount of data it processes is relaively small.
+  for (size_t i = 0; i < samples_count; ++i) {
+    if (abs(samples[i]) > threshold_) {
+      silent_packet = false;
+      break;
+    }
+  }
+
+  if (!silent_packet) {
+    silence_length_ = 0;
+    return false;
+  }
+
+  silence_length_ += samples_count;
+  return silence_length_ > silence_length_max_;
+}
+
+}  // namespace remoting
diff --git a/remoting/host/audio_silence_detector.h b/remoting/host/audio_silence_detector.h
new file mode 100644
index 0000000..4bc686e
--- /dev/null
+++ b/remoting/host/audio_silence_detector.h
@@ -0,0 +1,42 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef REMOTING_HOST_AUDIO_SILENCE_DETECTOR_H_
+#define REMOTING_HOST_AUDIO_SILENCE_DETECTOR_H_
+
+#include "base/basictypes.h"
+#include "base/logging.h"
+
+namespace remoting {
+
+// Helper used in audio capturers to detect and drop silent audio packets.
+class AudioSilenceDetector {
+ public:
+  // |threshold| is used to specify maximum absolute sample value that should
+  // still be considered as silence.
+  AudioSilenceDetector(int threshold);
+  ~AudioSilenceDetector();
+
+  void Reset(int sampling_rate, int channels);
+
+  // Must be called for each new chunk of data. Return true the given packet
+  // is silence should be dropped.
+  bool IsSilence(const int16* samples, size_t samples_count);
+
+ private:
+  // Maximum absolute sample value that should still be considered as silence.
+  int threshold_;
+
+  // Silence period threshold in samples. Silence intervals shorter than this
+  // value are still encoded and sent to the client, so that we don't disrupt
+  // playback by dropping them.
+  int silence_length_max_;
+
+  // Lengths of the current silence period in samples.
+  int silence_length_;
+};
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_AUDIO_SILENCE_DETECTOR_H_
\ No newline at end of file
diff --git a/remoting/host/audio_silence_detector_unittest.cc b/remoting/host/audio_silence_detector_unittest.cc
new file mode 100644
index 0000000..93a8309
--- /dev/null
+++ b/remoting/host/audio_silence_detector_unittest.cc
@@ -0,0 +1,68 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "remoting/host/audio_silence_detector.h"
+
+#include "base/basictypes.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace remoting {
+
+namespace {
+
+const int kSamplingRate = 1000;
+
+void TestSilenceDetector(AudioSilenceDetector* target,
+                         const int16* samples, int samples_count,
+                         bool silence_expected) {
+  target->Reset(kSamplingRate, 1);
+  bool silence_started = false;
+  int threshold_length = 0;
+  for (int i = 0; i < 3 * kSamplingRate / samples_count; ++i) {
+    bool result = target->IsSilence(samples, samples_count);
+    if (silence_started) {
+      ASSERT_TRUE(result);
+    } else if (result) {
+      silence_started = true;
+      threshold_length = i * samples_count;
+    }
+  }
+
+  // Check that the silence was detected if it was expected.
+  EXPECT_EQ(silence_expected, silence_started);
+
+  if (silence_expected) {
+    // Check that silence threshold is between 0.5 and 2 seconds.
+    EXPECT_GE(threshold_length, kSamplingRate / 2);
+    EXPECT_LE(threshold_length, kSamplingRate * 2);
+  }
+}
+
+}  // namespace
+
+TEST(AudioSilenceDetectorTest, Silence) {
+  const int16 kSamples[] = {0, 0, 0, 0, 0, 0, 0, 0, 0, 0};
+
+  AudioSilenceDetector target(0);
+  TestSilenceDetector(&target, kSamples, arraysize(kSamples), true);
+}
+
+TEST(AudioSilenceDetectorTest, Sound) {
+  const int16 kSamples[] = {65, 73, 83, 89, 92, -1, 5, 9, 123, 0};
+
+  AudioSilenceDetector target(0);
+  TestSilenceDetector(&target, kSamples, arraysize(kSamples), false);
+}
+
+TEST(AudioSilenceDetectorTest, Threshold) {
+  const int16 kSamples[] = {0, 0, 0, 0, 1, 0, 0, -1, 0, 0};
+
+  AudioSilenceDetector target1(0);
+  TestSilenceDetector(&target1, kSamples, arraysize(kSamples), false);
+
+  AudioSilenceDetector target2(1);
+  TestSilenceDetector(&target2, kSamples, arraysize(kSamples), true);
+}
+
+}  // namespace remoting
diff --git a/remoting/host/linux/audio_pipe_reader.cc b/remoting/host/linux/audio_pipe_reader.cc
index 82d832e..00599fd 100644
--- a/remoting/host/linux/audio_pipe_reader.cc
+++ b/remoting/host/linux/audio_pipe_reader.cc
@@ -42,20 +42,6 @@ const int kPipeBufferSizeBytes = kPipeBufferSizeMs * kSampleBytesPerSecond /
 #define F_SETPIPE_SZ 1031
 #endif  // defined(F_SETPIPE_SZ)
 
-const int IsPacketOfSilence(const std::string& data) {
-  const int64* int_buf = reinterpret_cast<const int64*>(data.data());
-  for (size_t i = 0; i < data.size() / sizeof(int64); i++) {
-    if (int_buf[i] != 0)
-      return false;
-  }
-  for (size_t i = data.size() - data.size() % sizeof(int64);
-       i < data.size(); i++) {
-    if (data.data()[i] != 0)
-      return false;
-  }
-  return true;
-}
-
 }  // namespace
 
 // static
@@ -176,9 +162,6 @@ void AudioPipeReader::DoCapture() {
     last_capture_position_ = stream_position_bytes - kPipeBufferSizeBytes;
   DCHECK_LE(last_capture_position_, stream_position_bytes);
 
-  if (IsPacketOfSilence(data))
-    return;
-
   // Dispatch asynchronous notification to the stream observers.
   scoped_refptr<base::RefCountedString> data_ref =
       base::RefCountedString::TakeString(&data);
diff --git a/remoting/remoting.gyp b/remoting/remoting.gyp
index 9b315d1..f8df09e 100644
--- a/remoting/remoting.gyp
+++ b/remoting/remoting.gyp
@@ -337,6 +337,8 @@
             'host/audio_capturer_win.h',
             'host/audio_scheduler.cc',
             'host/audio_scheduler.h',
+            'host/audio_silence_detector.cc',
+            'host/audio_silence_detector.h',
             'host/capture_scheduler.cc',
             'host/capture_scheduler.h',
             'host/chromoting_host.cc',
@@ -2217,7 +2219,7 @@
         'codec/video_decoder_vp8_unittest.cc',
         'codec/video_encoder_verbatim_unittest.cc',
         'codec/video_encoder_vp8_unittest.cc',
-        'host/audio_capturer_win_unittest.cc',
+        'host/audio_silence_detector_unittest.cc',
         'host/branding.cc',
         'host/branding.h',
         'host/chromoting_host_context_unittest.cc',