[Chromoting] Audio playback won't work after switching default playback device

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 f6ea113160b6662e1e3f31ff5625f3755d6e7497..248b336aad3ce0188631103b8b646681826fd32c 100644
--- a/remoting/host/audio_capturer_win.cc
+++ b/remoting/host/audio_capturer_win.cc
@@ -40,46 +40,106 @@ const int kMaxExpectedTimerLag = 30;
 
 namespace remoting {
 
+class AudioCapturerWin::MMNotificationClient : public IMMNotificationClient {

[Sergey Ulanov, 2016/07/18 23:09:24]

Do we need this as a separate class? Maybe just implement IMMNotificationClient
interface in AudioCapturerWin?

[Hzj_jie, 2016/07/19 02:36:39]

Then AudioCapturerWin will export a set of unrelated functions. And the most
important thing is, there are AddRef and Release function, which are too common
and easily to conflict with our code base.

+ public:
+  MMNotificationClient(AudioCapturerWin* owner);
+
+  HRESULT __stdcall OnDefaultDeviceChanged(EDataFlow flow,
+                                           ERole role,
+                                           LPCWSTR pwstrDefaultDevice) override;
+
+  HRESULT __stdcall QueryInterface(REFIID iid, void** object) override;
+
+  // No Ops overrides.
+  HRESULT __stdcall OnDeviceAdded(LPCWSTR pwstrDeviceId) override {
+      return S_OK; }

[Sergey Ulanov, 2016/07/18 23:09:24]

here and below: formatting

[Hzj_jie, 2016/07/19 02:36:39]

Done.

+  HRESULT __stdcall OnDeviceRemoved(LPCWSTR pwstrDeviceId) override {
+      return S_OK; }
+  HRESULT __stdcall OnDeviceStateChanged(LPCWSTR pwstrDeviceId,
+                                         DWORD dwNewState) override {
+      return S_OK; }
+  HRESULT __stdcall OnPropertyValueChanged(LPCWSTR pwstrDeviceId,
+                                           const PROPERTYKEY key) override {
+      return S_OK; }
+  ULONG __stdcall AddRef() override { return 1; }
+  ULONG __stdcall Release() override { return 1; }
+
+ private:
+  AudioCapturerWin* owner_;
+};
+
+AudioCapturerWin::MMNotificationClient::MMNotificationClient(
+    AudioCapturerWin* owner) : owner_(owner) {
+  DCHECK(owner_);
+}
+
+HRESULT AudioCapturerWin::MMNotificationClient::OnDefaultDeviceChanged(

[Sergey Ulanov, 2016/07/18 23:09:24]

Which thread is this called on?

[Hzj_jie, 2016/07/19 02:36:40]

Totally no idea, no document about it on MSDN.

[Sergey Ulanov, 2016/07/20 17:28:34]

According to comments in audio/win/audio_device_listener_win.h it's called from
system's media thread. I.e. it's not the audio thread that's used for
AudioCapturer and so you need to add synchronization.

[Hzj_jie, 2016/07/20 20:43:14]

Assignment of a bool value is atomic, so the worst case is we miss the true
value in several DoCapture calls after default_audio_device_changed_ has been
set to true. Considering we execute DoCapture function 33 times per second, this
does not have significant impact.

Note, default_audio_device_changed_ will only be set to false once we got a true
value before, so we won't loss a single false-to-true flip.

[Sergey Ulanov, 2016/07/21 18:54:01]

No types are atomic in C++ by default. You cannot assume that this will always
be the behavior you get. Also TSan will detect this as an issue.
When you have two threads writing the same variable without any synchronization
you cannot make any assumption about how it will work in C++. It's undefined
behavior, which means the compiler is allowed generate code that would crash,
hang, or make itself a sandwich when it's executed.
Please add a lock which is locked whenever the flag is accessed.

[Hzj_jie, 2016/07/21 23:38:50]

Done.

Emmm, I believe the way I am using here is definitely troubleless.

+    EDataFlow flow, ERole role, LPCWSTR pwstrDefaultDevice) {

[Sergey Ulanov, 2016/07/18 23:09:24]

One argument per line please
git cl format

[Hzj_jie, 2016/07/19 02:36:40]

Done.

+  owner_->default_audio_device_changed_ = true;
+  return S_OK;
+}
+
+HRESULT AudioCapturerWin::MMNotificationClient::QueryInterface(
+    REFIID iid, void** object) {
+  if (iid == IID_IUnknown || iid == __uuidof(IMMNotificationClient)) {
+    *object = static_cast<IMMNotificationClient*>(this);
+    return S_OK;
+  }
+  *object = nullptr;
+  return E_NOINTERFACE;
+}
+
 AudioCapturerWin::AudioCapturerWin()
     : sampling_rate_(AudioPacket::SAMPLING_RATE_INVALID),
       silence_detector_(kSilenceThreshold),
+      default_audio_device_changed_(false),
+      notificator_(new MMNotificationClient(this)),

[Sergey Ulanov, 2016/07/18 23:09:24]

I don't think that |notificator_| is the right name for this variable. Call it
mm_notification_client_

[Hzj_jie, 2016/07/19 02:36:40]

Done.

       last_capture_error_(S_OK) {
     thread_checker_.DetachFromThread();
 }
 
 AudioCapturerWin::~AudioCapturerWin() {
   DCHECK(thread_checker_.CalledOnValidThread());
+
+  HRESULT hr = mm_device_enumerator_->UnregisterEndpointNotificationCallback(
+      notificator_.get());
+  if (FAILED(hr)) {
+    LOG(ERROR) << "Failed to unregister notificator. Error " << hr;
+  }
+
   if (audio_client_) {
     audio_client_->Stop();
   }
 }
 
-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());
+bool AudioCapturerWin::Initialize() {

[Sergey Ulanov, 2016/07/18 23:09:24]

Call this ResetAndInitialize() as it doesn't just initialize. 

[Hzj_jie, 2016/07/19 02:36:40]

Done.

+  default_audio_device_changed_ = false;
+  wave_format_ex_.Reset(nullptr);
+  mm_device_enumerator_.Release();
+  audio_capture_client_.Release();
+  audio_client_.Release();
+  mm_device_.Release();
+  audio_volume_.Release();
 
   HRESULT hr = S_OK;
-
-  base::win::ScopedComPtr<IMMDeviceEnumerator> mm_device_enumerator;
-  hr = mm_device_enumerator.CreateInstance(__uuidof(MMDeviceEnumerator));
+  hr = mm_device_enumerator_.CreateInstance(__uuidof(MMDeviceEnumerator));
   if (FAILED(hr)) {
     LOG(ERROR) << "Failed to create IMMDeviceEnumerator. Error " << hr;
     return false;
   }
 
+  hr = mm_device_enumerator_->RegisterEndpointNotificationCallback(

[Sergey Ulanov, 2016/07/18 23:09:24]

Do we also need to unregister the callback when calling
mm_device_enumerator_.Release() above or when the capturer is deleted?

[Hzj_jie, 2016/07/19 02:36:40]

Done.

+      notificator_.get());
+  if (FAILED(hr)) {
+    // We cannot predict which kind of error the API may return, but this is
+    // not a fatal error.
+    LOG(ERROR) << "Failed to register notificator. Error " << hr;
+  }
+
   // Get the audio endpoint.
-  hr = mm_device_enumerator->GetDefaultAudioEndpoint(eRender,
-                                                     eConsole,
-                                                     mm_device_.Receive());
+  hr = mm_device_enumerator_->GetDefaultAudioEndpoint(eRender,
+                                                      eConsole,
+                                                      mm_device_.Receive());
   if (FAILED(hr)) {
     LOG(ERROR) << "Failed to get IMMDevice. Error " << hr;
     return false;
@@ -200,7 +260,9 @@ bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) {
 
   // Initialize IAudioEndpointVolume.
   // TODO(zijiehe): Do we need to control per process volume?
-  hr = mm_device_->Activate(__uuidof(IAudioEndpointVolume), CLSCTX_ALL, nullptr,
+  hr = mm_device_->Activate(__uuidof(IAudioEndpointVolume),
+                            CLSCTX_ALL,
+                            nullptr,
                             audio_volume_.ReceiveVoid());
   if (FAILED(hr)) {
     LOG(ERROR) << "Failed to get an IAudioEndpointVolume. Error " << hr;
@@ -209,6 +271,26 @@ bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) {
 
   silence_detector_.Reset(sampling_rate_, kChannels);
 
+  return true;
+}
+
+bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) {

[Sergey Ulanov, 2016/07/18 23:09:24]

Move this before initialize, so the order of methods matches the order in the
header.

[Hzj_jie, 2016/07/19 02:36:39]

Done.

+  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());

[Sergey Ulanov, 2016/07/18 23:09:24]

nit: move this below, next to the capture_timer_->Start() call.

[Hzj_jie, 2016/07/19 02:36:40]

Done.

+
+  if (!Initialize()) {
+    return false;
+  }
+
   // Start capturing.
   capture_timer_->Start(FROM_HERE,
                         audio_device_period_,
@@ -284,6 +366,11 @@ void AudioCapturerWin::DoCapture() {
   DCHECK(AudioCapturer::IsValidSampleRate(sampling_rate_));
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  if (default_audio_device_changed_ && !Initialize()) {
+    // Initialization failed, we should wait for next DoCapture call.
+    return;
+  }
+
   // Fetch all packets from the audio capture endpoint buffer.
   HRESULT hr = S_OK;
   while (true) {