Add volume control for windows host

remoting/host/audio_capturer_win.cc

 // 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 <avrt.h>
 #include <mmreg.h>
 #include <mmsystem.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <windows.h>
 
 #include <algorithm>
 #include <utility>
 
 #include "base/logging.h"
 
 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.
 const int k100nsPerMillisecond = 10000;
 
 // Tolerance for catching packets of silence. If all samples have absolute
 // value less than this threshold, the packet will be counted as a packet of
 // silence. A value of 2 was chosen, because Windows can give samples of 1 and
 // -1, even when no audio is playing.
 const int kSilenceThreshold = 2;
@@ skipping 16 matching lines @@
 }
 
 AudioCapturerWin::~AudioCapturerWin() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
 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());
 
   callback_ = callback;
 
   // Initialize the capture timer.
   capture_timer_.reset(new base::RepeatingTimer());
 
   HRESULT hr = S_OK;
 
@@ skipping 119 matching lines @@
     return false;
   }
 
   // Start the IAudioClient.
   hr = audio_client_->Start();
   if (FAILED(hr)) {
     LOG(ERROR) << "Failed to start IAudioClient. Error " << hr;
     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.
   capture_timer_->Start(FROM_HERE,
                         audio_device_period_,
                         this,
                         &AudioCapturerWin::DoCapture);
   return true;
 }
 
@@ skipping 14 matching lines @@
     }
 
     BYTE* data;
     UINT32 frames;
     DWORD flags;
     hr = audio_capture_client_->GetBuffer(&data, &frames, &flags, nullptr,
                                           nullptr);
     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);
+    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));
+          callback_.Run(std::move(packet));
+        }
+      }
     }
 
     hr = audio_capture_client_->ReleaseBuffer(frames);
     if (FAILED(hr))
       break;
   }
 
   // There is nothing to capture if the audio endpoint device has been unplugged
   // or disabled.
   if (hr == AUDCLNT_E_DEVICE_INVALIDATED)
     return;
 
   // Avoid reporting the same error multiple times.
   if (FAILED(hr) && hr != last_capture_error_) {
     last_capture_error_ = hr;
     LOG(ERROR) << "Failed to capture an audio packet: 0x"
                << std::hex << hr << std::dec << ".";
   }
 }
 
 bool AudioCapturer::IsSupported() {
   return true;
 }
 
 scoped_ptr<AudioCapturer> AudioCapturer::Create() {
   return make_scoped_ptr(new AudioCapturerWin());
 }
 
 }  // namespace remoting