Add volume control for windows host

remoting/host/audio_capturer_win.cc

Index: remoting/host/audio_capturer_win.cc
diff --git a/remoting/host/audio_capturer_win.cc b/remoting/host/audio_capturer_win.cc
index 8c901137945dd58a475a9faca8dfcf36b92ed237..14448fe585b368cccf09f6b5a02c54d097a7209c 100644
--- a/remoting/host/audio_capturer_win.cc
+++ b/remoting/host/audio_capturer_win.cc
@@ -18,6 +18,7 @@
 
 namespace {
 const int kChannels = 2;
+// Following logic expects kBytesPerSample always be 2.

[Sergey Ulanov, 2016/03/01 21:48:17]

I don't think you need this comment here. See my suggestion below.

[Hzj_jie, 2016/03/02 08:16:33]

Done.

 const int kBytesPerSample = 2;
 const int kBitsPerSample = kBytesPerSample * 8;
 // Conversion factor from 100ns to 1ms.
@@ -54,6 +55,7 @@ bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) {
   DCHECK(!audio_capture_client_.get());
   DCHECK(!audio_client_.get());
   DCHECK(!mm_device_.get());
+  DCHECK(!audio_volume_.get());
   DCHECK(static_cast<PWAVEFORMATEX>(wave_format_ex_) == nullptr);
   DCHECK(thread_checker_.CalledOnValidThread());
 
@@ -193,6 +195,15 @@ bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) {
     return false;
   }
 
+  // Initialize ISampleAudioVolume. TODO(zijiehe): Do we need to control per

[joedow, 2016/03/01 22:38:40]

s/ISampleAudioVolume/ISimpleAudioVolume.

The TODO should be on the next line.

[Hzj_jie, 2016/03/02 08:16:33]

Done.

+  // process volume?
+  hr = audio_client_->GetService(__uuidof(ISimpleAudioVolume),
+                                 audio_volume_.ReceiveVoid());
+  if (FAILED(hr)) {
+    LOG(ERROR) << "Failed to get an ISimpleAudioVolume. Error " << hr;
+    return false;
+  }
+
   silence_detector_.Reset(sampling_rate_, kChannels);
 
   // Start capturing.
@@ -227,17 +238,37 @@ void AudioCapturerWin::DoCapture() {
     if (FAILED(hr))
       break;
 
-    if ((flags & AUDCLNT_BUFFERFLAGS_SILENT) == 0 &&
-        !silence_detector_.IsSilence(reinterpret_cast<const int16_t*>(data),
-                                     frames * kChannels)) {
-      scoped_ptr<AudioPacket> packet(new AudioPacket());
-      packet->add_data(data, frames * wave_format_ex_->nBlockAlign);
-      packet->set_encoding(AudioPacket::ENCODING_RAW);
-      packet->set_sampling_rate(sampling_rate_);
-      packet->set_bytes_per_sample(AudioPacket::BYTES_PER_SAMPLE_2);
-      packet->set_channels(AudioPacket::CHANNELS_STEREO);
-
-      callback_.Run(std::move(packet));
+    BOOL mute;
+    if (FAILED(audio_volume_->GetMute(&mute)) || !mute) {

[Sergey Ulanov, 2016/03/01 21:48:16]

Suggest moving this code to a separate function to make it more readable.

[joedow, 2016/03/01 22:38:40]

Agree with Sergey, also GetMute looks like it would fail for the same reasons as
GetMasterVolume.  If calling these APIs fails, do we want to default to 'full
volume'?  Or do we receive failed HRs for common scenarios such as headphones
being unplugged?

[Hzj_jie, 2016/03/02 08:16:32]

I just want to make sure this change won't break existing logic even something
bad happened.

+      float level;
+      if (FAILED(audio_volume_->GetMasterVolume(&level) ||
+          level > 1 || level < -1)) {

[Sergey Ulanov, 2016/03/01 21:48:16]

is the level allowed to be negative?

[Hzj_jie, 2016/03/02 08:16:33]

No, the value should be [0.0, 1.0], but this check is to make sure an int16_t *
level won't exceed int16_t.

+        level = 1;

[joedow, 2016/03/01 22:38:40]

Should we default to 1 if the call above fails?  The hresults returned generally
point to audio hardware errors or the audio service not running so I wonder if
'full volume' makes more sense here.  

[Hzj_jie, 2016/03/02 08:16:33]

The reason is same as GetMute call, i.e. make sure the behavior won't change
even something bad happened.

Currently the logic has been moved to IsMuted function, we can discuss if the
principle is correct, and make necessary changes there.

+      }
+      if (level > 0 && (flags & AUDCLNT_BUFFERFLAGS_SILENT) == 0) {
+        // NOTE: The samples may not be 16 bits align, if any constants changed.
+        // i.e. samples[x] may be part of one sample of one channel, or may be
+        // the combination of one sample of two channels.

[Sergey Ulanov, 2016/03/01 21:48:16]

I don't understand this comment. We always get data in 16-bit format, so
samples[x] is always a whole sample from one of the channels. If the point is
that this code assumes 16-bit encoding then maybe replace this comment with a
compile assert, e.g.:
  COMPILE_ASSERT(kBytesPerSample==sizeof(samples[0]),
expect_16_bits_per_sample).

[Hzj_jie, 2016/03/02 08:16:33]

Yes, 'We always get data in 16-bit format' is the assumption, i.e.
kBytesPerSample must be 2, it's not changeable. This assumption impacts both
AudioSilenceDetector and the logic here.
I agree a compile assert should be better.

+        int16_t* samples = reinterpret_cast<int16_t*>(data);
+        // BYTE to int16_t
+        size_t sample_count = frames * kChannels * kBytesPerSample
+                              * sizeof(BYTE) / sizeof(int16_t);

[Sergey Ulanov, 2016/03/01 21:48:16]

nit: don't need sizeof(BYTE). kBytesPerSample already returns size in bytes.

[Hzj_jie, 2016/03/02 08:16:33]

Though less possibility, what if sizeof(BYTE) != 1 :)

+        if (!silence_detector_.IsSilence(samples, sample_count)) {
+          scoped_ptr<AudioPacket> packet(new AudioPacket());
+          if (level < 1) {
+            for (size_t i = 0; i < sample_count; i++) {
+              samples[i] *= level;

[Sergey Ulanov, 2016/03/01 21:48:16]

This would be many times faster if you avoid floating point arithmetic in the
loop. E.g.

     int32_t level_int = static_cast<int>(level * 65536);
     for (size_t i = 0; i < sample_count; i++) {
        samples[i] = (static_cast<int32_t>(samples[i]) * level_int) >> 16;
     }

[Hzj_jie, 2016/03/02 08:16:32]

Done. But the performance impact would not be many times.

[Sergey Ulanov, 2016/03/02 20:09:39]

FWIW I see 2.5x-3x difference on ia64 and on ia32 with GCC:

$ cat apply_volume.cc 
#include <array>
#include <cstdint>
#include <iostream>
#include <time.h>
int main() {
  const int kNumSamples = 100*1000;
  const int kIterations = 100;
  std::array<int16_t, kNumSamples> samples;
  float volume = 0.3;
  clock_t started;

  started = clock();
  for (size_t j = 0; j < kIterations; ++j) {
    for (size_t i = 0; i < kNumSamples; ++i) {
      samples[i] *= volume;
    }
  }
  std::cerr << static_cast<double>(clock() - started) / CLOCKS_PER_SEC <<
std::endl;

  started = clock();
  int32_t level_int = static_cast<int>(volume * 65536);
  for (size_t j = 0; j < kIterations; ++j) {
    for (size_t i = 0; i < kNumSamples; ++i) {
      samples[i] = (static_cast<int32_t>(samples[i]) * level_int) >> 16;
    }
  }
  std::cerr << static_cast<double>(clock() - started) / CLOCKS_PER_SEC
            << std::endl;

  int sum = 0;
  for (size_t i = 0; i < kNumSamples; ++i) {
    sum += samples[i];
  }
  std::cerr << sum << std::endl;
}
$ g++  apply_volume.cc -std=c++11 -g -O2  
$ ./a.out                                
0.037469
0.01429
0
$ g++  apply_volume.cc -std=c++11 -g -O2 -m32
$ ./a.out                                    
0.034593
0.012743
0
 
It's not that floating point multiplication is slow (it may be even faster than
int). It's int->float->int conversion that kills it.

[Hzj_jie, 2016/03/03 09:33:38]

I tried with only multiplication (which is on-par). Thank you for the detail
analysis.

+            }
+          }
+          packet->add_data(data, frames * wave_format_ex_->nBlockAlign);
+          packet->set_encoding(AudioPacket::ENCODING_RAW);
+          packet->set_sampling_rate(sampling_rate_);
+          packet->set_bytes_per_sample(AudioPacket::BYTES_PER_SAMPLE_2);
+          packet->set_channels(AudioPacket::CHANNELS_STEREO);
+
+          callback_.Run(std::move(packet));
+        }
+      }
     }
 
     hr = audio_capture_client_->ReleaseBuffer(frames);