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>
@@ skipping 36 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 ISimpleAudioVolume.
+  // TODO(zijiehe): Do we need to control per 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;
 }
 
+bool AudioCapturerWin::IsMuted(float* level) {

[joedow, 2016/03/02 15:25:52]

Couldn't this function return a float (just return 0.0f if the audio is muted)
then you wouldn't need to pass a level pointer and you could rename the function
GetAudioLevel() or similar?  

The current logic handles mute vs. 0 volume but I'm wondering if we actually
want to send audio packets if the volume is 0.0 (i.e. should we treat mute and 0
volume the same?).

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

+1

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

Yes, current logic treat 0 volume exactly the same as mute. And audio packets
will be ignored if volume is 0.0 or audio is muted.

I have updated the function into float GetAudioLevel().

+  DCHECK(level);
+  BOOL mute;
+  HRESULT hr = audio_volume_->GetMute(&mute);
+  if (!FAILED(hr) && mute) {
+    return true;
+  }
+  hr = audio_volume_->GetMasterVolume(level);
+  if (FAILED(hr) || *level > 1 || *level < 0) {

[joedow, 2016/03/02 15:25:52]

If the level is less than zero, should you clamp it to 0 instead of 1?

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

I am OK with both values, we definitely should not receive <0 value from OS.
Updated to 0.

+    *level = 1;

[joedow, 2016/03/02 15:25:52]

Per my previous comment, I think the decision to default to full volume should
be documented.  Can you add a comment on why this is the right behavior.  

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

look at it as the host's volume setting being ignored, not as we are defaulting
to full volume. The client also has volume control that gets applied there.

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

Yes, if anything wrong, current design ignores host volume settings, and returns
(false, 1.0).
I have added comments for the function in .h file.

+  }
+  return *level == 0;
+}
+
 void AudioCapturerWin::DoCapture() {
   DCHECK(AudioCapturer::IsValidSampleRate(sampling_rate_));
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // Fetch all packets from the audio capture endpoint buffer.
   HRESULT hr = S_OK;
   while (true) {
     UINT32 next_packet_size;
     HRESULT hr = audio_capture_client_->GetNextPacketSize(&next_packet_size);
     if (FAILED(hr))
       break;
 
     if (next_packet_size <= 0) {
       return;
     }
 
     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);
+    if (frames > 0 && (flags & AUDCLNT_BUFFERFLAGS_SILENT) == 0) {
+      float level;
+      if (!IsMuted(&level)) {

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

Sorry for not being clear about it on my previous comment. I was suggesting to
move this whole block to a separate method. This would help to avoid 3 levels of
nested if blocks. I.e.

void ProcessSamples(uint8_t* data, size_t num_frames, uint32_t flags) {
  if (!num_frames)
    return;

  if (flags & AUDCLNT_BUFFERFLAGS_SILENT) != 0)
    return;

  if (silence_detector_.IsSilence(dat, num_frames * kChannels))
    return;

  float volume = GetVolume();
  if (volume <= 0)
    return;

  .. apply volume
  .. generate packet and call callback_.
}

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

Done.

+        int16_t* samples = reinterpret_cast<int16_t*>(data);
+        // BYTE to int16_t

[joedow, 2016/03/02 15:25:51]

I don't think this comment adds much value, remove it?

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

Done.

+        size_t sample_count = frames * kChannels * kBytesPerSample
+                              * sizeof(BYTE) / sizeof(int16_t);

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

I still think sizeof(BYTE) is useless here. Something must go terribly wrong for
sizeof(BYTE) to be different than 1. And even if it was different, I don't think
how it would apply here. frames is number of frames. sizeof(int16_t) would still
be 2. I.e. size of the type used for data pointer has no relation to how many
many samples we have.
Also kBytesPerSample / sizeof(int16_t) is useless given that you have the
static_assert() below now. Number of samples equals number of frames times
number of channels. That's it. There is no reason we need to care about sample
size in this expression.

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

Good point, thank you.

+        static_assert(sizeof(samples[0]) == kBytesPerSample,
+                      "expect 16 bits per sample");
+        if (level < 1) {

[joedow, 2016/03/02 15:25:52]

A comment might be good here to describe why we need to manually adjust the
level of each sample.

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

Done.

+          int32_t level_int = static_cast<int32_t>(level * 65536);
+          for (size_t i = 0; i < sample_count; i++) {
+            samples[i] = (static_cast<int32_t>(samples[i]) * level_int) >> 16;
+          }
+        }
+        if (!silence_detector_.IsSilence(samples, sample_count)) {

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

Here almost all data is being dropped (as audio is not played all the time over
CRD session). I think ideally this should be done before we waste time applying
volume.

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

I believe network is more precious than processor, i.e. if we apply volume after
silence detector, we may send silent samples to the client (level is always <=
1). Or do you prefer to do silence check twice?

[Sergey Ulanov, 2016/03/04 21:30:03]

You already have explicit check for volume=0, so samples may become silent after
you apply volume only when volume is close to 0 and all samples are also close
to 0, but above threshold used by silence_detector_. This is extremely rare
scenario and I don't think we need to worry about it. And even if that was
happening frequently I'd still argue that it's better to keep sending data
because we know that there is some application that tries to play back
something.

[Hzj_jie, 2016/03/09 19:47:57]

Not really, if the volume is close to 0, but samples aren't, they may be
detected as silence. Since windows has a specific threshold 2. The most
important thing is, if we check silence detector before attaching volume to the
samples, the behavior is not consistent as what we are doing on linux.

+          scoped_ptr<AudioPacket> packet(new AudioPacket());
+          packet->add_data(data, frames * wave_format_ex_->nBlockAlign);

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

So here the data is copied from data to a new buffer. Maybe do it one with the
loop that updates the volume? I.e. the loop will be copying data from frames to
packet->mutable_data().

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

Same as comment above.

+          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