[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..a7f36f4b6a5784affa17d6be74d16a6df9962244 100644
--- a/remoting/host/audio_capturer_win.cc
+++ b/remoting/host/audio_capturer_win.cc
@@ -16,6 +16,7 @@
 
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/synchronization/lock.h"
 
 namespace {
 const int kChannels = 2;
@@ -40,11 +41,74 @@ const int kMaxExpectedTimerLag = 30;
 
 namespace remoting {
 
+class AudioCapturerWin::MMNotificationClient : public IMMNotificationClient {
+ public:
+  MMNotificationClient(AudioCapturerWin* owner)

[Sergey Ulanov, 2016/07/22 17:38:37]

owner_ is not needed anymore. Remove it?

[Hzj_jie, 2016/07/22 21:03:11]

Yep, sorry.

+      : owner_(owner) {
+    DCHECK(owner_);
+  }
+
+  HRESULT __stdcall OnDefaultDeviceChanged(
+      EDataFlow flow,
+      ERole role,
+      LPCWSTR pwstrDefaultDevice) override {
+    SetDefaultAudioDeviceChanged();
+    return S_OK;
+  }
+
+  HRESULT __stdcall QueryInterface(REFIID iid, void** object) override {
+    if (iid == IID_IUnknown || iid == __uuidof(IMMNotificationClient)) {
+      *object = static_cast<IMMNotificationClient*>(this);
+      return S_OK;
+    }
+    *object = nullptr;
+    return E_NOINTERFACE;
+  }
+
+  // No Ops overrides.
+  HRESULT __stdcall OnDeviceAdded(LPCWSTR pwstrDeviceId) override {
+    return S_OK;
+  }
+  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; }
+
+  bool GetAndResetDefaultAudioDeviceChanged() {
+    base::AutoLock lock(lock_);
+    if (default_audio_device_changed_) {
+      default_audio_device_changed_ = false;
+      return true;
+    }
+    return false;
+  }
+
+  void SetDefaultAudioDeviceChanged() {
+    base::AutoLock lock(lock_);
+    default_audio_device_changed_ = true;
+  }
+
+ private:
+  AudioCapturerWin* const owner_;
+  bool default_audio_device_changed_ = false;  // GUARDED_BY(lock_)

[Sergey Ulanov, 2016/07/22 17:38:37]

replace GUARDED_BY with a comment (e.g. "|lock_| must be locked when accessing
|default_audio_device_changed_|"). We don't have GUARDED_BY macro in chromium.

[Hzj_jie, 2016/07/22 21:03:11]

Done.

+  base::Lock lock_;
+};
+
 AudioCapturerWin::AudioCapturerWin()
     : sampling_rate_(AudioPacket::SAMPLING_RATE_INVALID),
       silence_detector_(kSilenceThreshold),
+      mm_notification_client_(new MMNotificationClient(this)),
       last_capture_error_(S_OK) {
-    thread_checker_.DetachFromThread();
+  thread_checker_.DetachFromThread();
 }
 
 AudioCapturerWin::~AudioCapturerWin() {
@@ -64,22 +128,43 @@ bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) {
 
   callback_ = callback;
 
-  // Initialize the capture timer.
+  if (!ResetAndInitialize()) {
+    return false;
+  }
+
+  // Initialize the capture timer and start capturing.
   capture_timer_.reset(new base::RepeatingTimer());
+  capture_timer_->Start(FROM_HERE, audio_device_period_, this,
+                        &AudioCapturerWin::DoCapture);
+  return true;
+}
 
-  HRESULT hr = S_OK;
+bool AudioCapturerWin::ResetAndInitialize() {
+  wave_format_ex_.Reset(nullptr);
+  mm_device_enumerator_.Release();
+  audio_capture_client_.Release();
+  audio_client_.Release();
+  mm_device_.Release();
+  audio_volume_.Release();
 
-  base::win::ScopedComPtr<IMMDeviceEnumerator> mm_device_enumerator;
-  hr = mm_device_enumerator.CreateInstance(__uuidof(MMDeviceEnumerator));
+  HRESULT hr = S_OK;
+  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(
+      mm_notification_client_.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 IMMNotificationClient. 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;
@@ -209,11 +294,6 @@ bool AudioCapturerWin::Start(const PacketCapturedCallback& callback) {
 
   silence_detector_.Reset(sampling_rate_, kChannels);
 
-  // Start capturing.
-  capture_timer_->Start(FROM_HERE,
-                        audio_device_period_,
-                        this,
-                        &AudioCapturerWin::DoCapture);
   return true;
 }
 
@@ -284,6 +364,13 @@ void AudioCapturerWin::DoCapture() {
   DCHECK(AudioCapturer::IsValidSampleRate(sampling_rate_));
   DCHECK(thread_checker_.CalledOnValidThread());
 
+  if (mm_notification_client_->GetAndResetDefaultAudioDeviceChanged() &&
+      !ResetAndInitialize()) {

[Sergey Ulanov, 2016/07/22 17:38:37]

This expression is hard to read: it's not obvious that you reinitialize when the
flag is set. Please replace with two nested if statements.

[Hzj_jie, 2016/07/22 21:03:11]

Done.

+    // Initialization failed, we should wait for next DoCapture call.

[Sergey Ulanov, 2016/07/22 17:38:37]

When DoCapture() is called again this code won't call ResetAndInitialize() again
and will try to procede in uninitialized state. So I think you need another
check for that case
 if (devices_changed || !initialized_) {
   if (!ResetAndInitialize()) {
      :(
     return;
   }
 }

[Hzj_jie, 2016/07/22 21:03:11]

No, SetDefaultAudioDeviceChanged in line 370 will reset the state if
ResetAndInitialize failed. So we won't need an extra initialized_ variable.

[Sergey Ulanov, 2016/07/22 21:29:27]

I see. It seems error-prone to piggy-back on that flag for reinitialization. Add
a separate flag for this case in AudioCapturerWin?

+    mm_notification_client_->SetDefaultAudioDeviceChanged();
+    return;
+  }
+
   // Fetch all packets from the audio capture endpoint buffer.
   HRESULT hr = S_OK;
   while (true) {