[Chromoting] Implement down mixing in AudioPump

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 2610e6e2b692eed2523b0dfe2047d3222b379996..6c851518bb286948ced67cf847d72590a7e52057 100644
--- a/remoting/host/audio_capturer_win.cc
+++ b/remoting/host/audio_capturer_win.cc
@@ -21,7 +21,6 @@
 #include "remoting/host/win/default_audio_device_change_detector.h"
 
 namespace {
-const int kChannels = 2;
 const int kBytesPerSample = 2;
 const int kBitsPerSample = kBytesPerSample * 8;
 // Conversion factor from 100ns to 1ms.
@@ -39,6 +38,20 @@ const int kMinTimerInterval = 30;
 // Upper bound for the timer precision error, in milliseconds.
 // Timers are supposed to be accurate to 20ms, so we use 30ms to be safe.
 const int kMaxExpectedTimerLag = 30;
+
+int ChooseChannels(int channel_count) {

[joedow, 2017/05/26 16:01:50]

This function doesn't choose a channel, it converts an unsupported channel
number into stereo (supported values are passed through as-is).

Is that safe to do? i.e. if the channel count was 10 (9.1 audio) you would
convert it to 2 which doesn't seem right.

[Hzj_jie, 2017/05/26 20:08:03]

Before this change, we always select stereo. But this change add supports to
mono / 5.1 / 6.1 / 7.1.
We can surely add more channel choices, but AFAICT, Windows supports only 1 / 2
/ 6 / 8. While 6.1 is for testing purpose only.

[Sergey Ulanov, 2017/05/26 22:24:45]

If I understand correctly this function defines channel configuration that the
request from a system. But if the channel count is not one of the supported set,
does it still make sense to request STEREO? Most likely it will fail anyway - we
saw that Windows doesn't downmix captured streams, so I'm not sure it's worth
trying initialize the stream anyway.

I'm not sure these are the only configurations that windows supports. Is it
documented anywhere? At very least should support 2.1 and Quad configurations.
E.g. this page shows two different 4 channel configs
https://msdn.microsoft.com/en-us/library/windows/desktop/dd390971(v=vs.85).aspx

[Hzj_jie, 2017/05/28 20:58:58]

IMO, it's still worthy to have a shot: it won't waste too many resources as only
one more function call is needed but it ensures no regression will ever be
triggered.
No, I found no full lists. But searching on web leads me to the wiki page
https://en.wikipedia.org/wiki/Sound_card and ebay or amazon: we do have 2.1, 4
channels and 4.1 channels. But 7.1 / 8 channels is the top level for PC.

[Sergey Ulanov, 2017/06/05 18:03:19]

I still don't think this is necessary. The problem is not any resources being
wasted, but that this logic makes this code more complicated, while we don't
know if it will ever make any difference.

[Hzj_jie, 2017/06/06 03:21:42]

Then I would prefer to rewrite the wave_format_ex_->wFormatTag switch in
Initialize() function: fast-return here indeed makes the logic more complex.

[chcunningham, 2017/06/09 19:25:28]

This is fine for this CL, but we have seen more than 8 channels for people with
elaborate setups (fancy mixers). Down the road you can introduce a notion of
"discrete" layouts (basically a big shrug) and map that to media's notion for
down mixing to stereo.

[Hzj_jie, 2017/06/09 21:33:47]

IMO, a long-term solution is to migrate to AudioInputStream, so we won't need to
care about these details any more :)

+  using remoting::AudioPacket;
+  // The condition should match AudioPacket::Channels enum in audio.proto.
+  if (channel_count == AudioPacket::CHANNELS_MONO ||
+      channel_count == AudioPacket::CHANNELS_STEREO ||
+      channel_count == AudioPacket::CHANNELS_5_1 ||
+      channel_count == AudioPacket::CHANNELS_6_1 ||
+      channel_count == AudioPacket::CHANNELS_7_1) {
+    return channel_count;
+  }
+  return AudioPacket::CHANNELS_STEREO;
+}
+
 }  // namespace
 
 namespace remoting {
@@ -163,11 +176,12 @@ bool AudioCapturerWin::Initialize() {
           wave_format_ex_->nSamplesPerSec);
 
       wave_format_ex_->wFormatTag = WAVE_FORMAT_PCM;
-      wave_format_ex_->nChannels = kChannels;
+      wave_format_ex_->nChannels = ChooseChannels(wave_format_ex_->nChannels);
       wave_format_ex_->wBitsPerSample = kBitsPerSample;
-      wave_format_ex_->nBlockAlign = kChannels * kBytesPerSample;
+      wave_format_ex_->nBlockAlign =
+          wave_format_ex_->nChannels * kBytesPerSample;
       wave_format_ex_->nAvgBytesPerSec =
-          sampling_rate_ * kChannels * kBytesPerSample;
+          sampling_rate_ * wave_format_ex_->nChannels * kBytesPerSample;
       break;
     case WAVE_FORMAT_EXTENSIBLE: {
       PWAVEFORMATEXTENSIBLE wave_format_extensible =
@@ -186,13 +200,15 @@ bool AudioCapturerWin::Initialize() {
         wave_format_extensible->SubFormat = KSDATAFORMAT_SUBTYPE_PCM;
         wave_format_extensible->Samples.wValidBitsPerSample = kBitsPerSample;
 
-        wave_format_extensible->Format.nChannels = kChannels;
+        wave_format_extensible->Format.nChannels =
+            ChooseChannels(wave_format_extensible->Format.nChannels);

[Sergey Ulanov, 2017/05/26 22:24:45]

If you change number of channels here then you probably want to change
configuration in wave_format_extensible->dwChannelMask as well. Could it be the
reason windows doesn't downmix the stream currently?

[Hzj_jie, 2017/05/28 20:58:59]

According to MSDN,
https://social.msdn.microsoft.com/Forums/vstudio/en-US/3a5d0748-fb7f-4de9-80f...,
WASAPI (IAudioClient) does not do any format conversion. I have also tried to
set the dwChannelMask, but it won't make any difference,
IAudioClient::Initialize() function still returns 2004287480.
But the answer on MSDN also mentioned, "you have to stream in the mix format, or
at least something fairly close to it." So always trying stereo is not a bad
idea for unknown channels.

P.S. Most higher-level APIs will do format conversion for you:
waveOut / waveIn
Media Foundation
MediaCapture
etc.

We can also try waveOut.

[Sergey Ulanov, 2017/05/30 19:18:19]

My point was that, if the API will never downmix the stream, then it makes no
sense to set nChannels to 2 here.
If changing nChannels is useful, then you also want to change
wave_format_extensible->dwChannelMask.

[Hzj_jie, 2017/05/31 00:12:08]

Got you. I will update both nChannels and dwChannelMask.

[Sergey Ulanov, 2017/05/31 00:44:43]

Is this useful given that we know that windows will not downmix the stream
anyway? I think it's better to just return error here when nChannels is outside
of the supported range instead of trying to initialize in stereo.

[Hzj_jie, 2017/05/31 02:49:28]

Trying to initialize in stereo does not really waste too much resources, and
because of the hidden source character of Windows, we cannot predict what will
happen if we simply drop the initialization attempt here. I tend to avoid
changes to existing logic path when handling edge cases.

[Sergey Ulanov, 2017/06/05 18:03:19]

I don't think it can cause any problems. It's reasonable for an application to
drop the object without calling Initialize() after it has called GetMixFormat()
and determined that it can't support the format.
In this change you are changing how multi-channel layouts are handled. It seems
strange to change it only for 1-8 channel layouts, but keep the old behavior for
layouts with more than 8 channels, even though we know it won't work properly.
Also in general we want to optimize for readability and simplicity of the code
overall, not for simplicity of each individual CL.

[Hzj_jie, 2017/06/06 03:21:42]

Done.

         wave_format_extensible->Format.nSamplesPerSec = sampling_rate_;
         wave_format_extensible->Format.wBitsPerSample = kBitsPerSample;
         wave_format_extensible->Format.nBlockAlign =
-            kChannels * kBytesPerSample;
+            wave_format_extensible->Format.nChannels * kBytesPerSample;
         wave_format_extensible->Format.nAvgBytesPerSec =
-            sampling_rate_ * kChannels * kBytesPerSample;
+            sampling_rate_ * wave_format_extensible->Format.nChannels *
+            kBytesPerSample;
       } else {
         LOG(ERROR) << "Failed to force 16-bit samples";
         return false;
@@ -233,7 +249,7 @@ bool AudioCapturerWin::Initialize() {
   }
 
   volume_filter_.ActivateBy(mm_device_.Get());
-  volume_filter_.Initialize(sampling_rate_, kChannels);
+  volume_filter_.Initialize(sampling_rate_, wave_format_ex_->nChannels);
 
   return true;
 }
@@ -280,7 +296,8 @@ void AudioCapturerWin::DoCapture() {
       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);
+      packet->set_channels(static_cast<AudioPacket::Channels>(
+          wave_format_ex_->nChannels));
 
       callback_.Run(std::move(packet));
     }