[Chromoting] Implement down mixing in AudioPump

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 <objbase.h>
 #include <stdint.h>
 #include <stdlib.h>
 #include <windows.h>
 
 #include <algorithm>
 #include <utility>
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/synchronization/lock.h"
 #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.
 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;
 
 // Lower bound for timer intervals, in milliseconds.
 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 {
 
 AudioCapturerWin::AudioCapturerWin()
     : sampling_rate_(AudioPacket::SAMPLING_RATE_INVALID),
       volume_filter_(kSilenceThreshold),
       last_capture_error_(S_OK) {
   thread_checker_.DetachFromThread();
 }
@@ skipping 104 matching lines @@
       // Intentional fall-through.
     case WAVE_FORMAT_PCM:
       if (!AudioCapturer::IsValidSampleRate(wave_format_ex_->nSamplesPerSec)) {
         LOG(ERROR) << "Host sampling rate is neither 44.1 kHz nor 48 kHz.";
         return false;
       }
       sampling_rate_ = static_cast<AudioPacket::SamplingRate>(
           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 =
           reinterpret_cast<WAVEFORMATEXTENSIBLE*>(
           static_cast<WAVEFORMATEX*>(wave_format_ex_));
       if (IsEqualGUID(KSDATAFORMAT_SUBTYPE_IEEE_FLOAT,
                       wave_format_extensible->SubFormat)) {
         if (!AudioCapturer::IsValidSampleRate(
                 wave_format_extensible->Format.nSamplesPerSec)) {
           LOG(ERROR) << "Host sampling rate is neither 44.1 kHz nor 48 kHz.";
           return false;
         }
         sampling_rate_ = static_cast<AudioPacket::SamplingRate>(
             wave_format_extensible->Format.nSamplesPerSec);
 
         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;
       }
       break;
     }
     default:
       LOG(ERROR) << "Failed to force 16-bit PCM";
       return false;
   }
@@ skipping 20 matching lines @@
   }
 
   // Start the IAudioClient.
   hr = audio_client_->Start();
   if (FAILED(hr)) {
     LOG(ERROR) << "Failed to start IAudioClient. Error " << hr;
     return false;
   }
 
   volume_filter_.ActivateBy(mm_device_.Get());
-  volume_filter_.Initialize(sampling_rate_, kChannels);
+  volume_filter_.Initialize(sampling_rate_, wave_format_ex_->nChannels);
 
   return true;
 }
 
 bool AudioCapturerWin::is_initialized() const {
   // All Com components should be initialized / deinitialized together.
   return !!audio_client_;
 }
 
 void AudioCapturerWin::DoCapture() {
@@ skipping 26 matching lines @@
                                           nullptr);
     if (FAILED(hr))
       break;
 
     if (volume_filter_.Apply(reinterpret_cast<int16_t*>(data), frames)) {
       std::unique_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);
+      packet->set_channels(static_cast<AudioPacket::Channels>(
+          wave_format_ex_->nChannels));
 
       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
@@ skipping 11 matching lines @@
 
 bool AudioCapturer::IsSupported() {
   return true;
 }
 
 std::unique_ptr<AudioCapturer> AudioCapturer::Create() {
   return base::WrapUnique(new AudioCapturerWin());
 }
 
 }  // namespace remoting