Reland "Consolidate interactions with |CrasAudioHandler|."

This is a reland of ccba8bc4

The problem was that the fixes for the review comments had caused us
to call AudioDevices::SetAudioDevices() before the constructor of
|AudioDevices| had finished initializing all variables.
Unfortunately this only happened on real hardware, and I had not
retested these review comment fixes on hardware.

Original change's description:
> Consolidate interactions with |CrasAudioHandler|.
>
> Previously 2 places in the code were interacting with |CrasAudioHandler|:
>    - |PlatformApiImpl| subscribes to device changes, and passes this
>      information to |AudioInputHost|.
>    - |AudioInputHost| posts the new hotword model to |CrasAudioHandler|.
>
> This made it hard/impossible to split |AudioInputHost| from
> |PlatformApiImpl|, which is needed to handle audio input in the
> Libassistant mojom service.
>
> To solve this I moved all interactions with |CrasAudioHandler| inside a
> new class |AudioDevices|, of which an instance is owned by
> |AudioInputHost|.
>
> This also allowed me to add a bunch of new unittests.
>
> deployed on real hardware.
>
> Bug: b/171748795
> Test: chromeos_unittests --gtest_filter="Assistant*:Audio*" and manually
> Change-Id: I8ec9cec6672236f6365345b292efbe828518be69
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2610953
> Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
> Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
> Auto-Submit: Jeroen Dhollander <jeroendh@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#841155}

Bug: b/171748795
Change-Id: I9b8b970fbd1011ce911bf99961159805a4c0d2a3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2617568
Commit-Queue: Jeroen Dhollander <jeroendh@chromium.org>
Auto-Submit: Jeroen Dhollander <jeroendh@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841677}

chromeos/services/assistant/BUILD.gn

@@ -26,6 +26,8 @@ component("lib") {
     "fake_assistant_manager_service_impl.h",
     "fake_assistant_settings_impl.cc",
     "fake_assistant_settings_impl.h",
+    "platform/audio_devices.cc",
+    "platform/audio_devices.h",
     "service.cc",
     "service.h",
     "service_context.h",
@@ -174,6 +176,7 @@ source_set("tests") {
   ]
 
   sources = [
+    "platform/audio_devices_unittest.cc",
     "service_unittest.cc",
     "test_support/fully_initialized_assistant_state.cc",
     "test_support/fully_initialized_assistant_state.h",

chromeos/services/assistant/platform/audio_devices.cc

+// Copyright 2020 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 "chromeos/services/assistant/platform/audio_devices.h"
+
+#include "base/metrics/histogram_functions.h"
+#include "base/optional.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_split.h"
+#include "base/strings/string_util.h"
+#include "base/system/sys_info.h"
+#include "chromeos/audio/audio_device.h"
+#include "chromeos/audio/cras_audio_handler.h"
+#include "chromeos/services/assistant/public/cpp/features.h"
+
+namespace chromeos {
+namespace assistant {
+
+namespace {
+
+constexpr const char kDefaultLocale[] = "en_us";
+
+// Hotword model is expected to have <language>_<region> format with lower
+// case, while the locale in pref is stored as <language>-<region> with region
+// code in capital letters. So we need to convert the pref locale to the
+// correct format.
+// Examples:
+//     "fr"     ->  "fr_fr"
+//     "nl-BE"  ->  "nl_be"
+base::Optional<std::string> ToHotwordModel(std::string pref_locale) {
+  std::vector<std::string> code_strings = base::SplitString(
+      pref_locale, "-", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
+
+  if (code_strings.size() == 0) {
+    // Note: I am not sure this happens during real operations, but it
+    // definitely happens during the ChromeOS performance tests.
+    return base::nullopt;
+  }
+
+  DCHECK_LT(code_strings.size(), 3u);
+
+  // For locales with language code "en", use "en_all" hotword model.
+  if (code_strings[0] == "en")
+    return "en_all";
+
+  // If the language code and country code happen to be the same, e.g.
+  // France (FR) and French (fr), the locale will be stored as "fr" instead
+  // of "fr-FR" in the profile on Chrome OS.
+  if (code_strings.size() == 1)
+    return code_strings[0] + "_" + code_strings[0];
+
+  return code_strings[0] + "_" + base::ToLowerASCII(code_strings[1]);
+}
+
+const chromeos::AudioDevice* GetHighestPriorityDevice(
+    const chromeos::AudioDevice* left,
+    const chromeos::AudioDevice* right) {
+  if (!left)
+    return right;
+  if (!right)
+    return left;
+  return left->priority < right->priority ? right : left;
+}
+
+base::Optional<uint64_t> IdToOptional(const AudioDevice* device) {
+  if (!device)
+    return base::nullopt;
+  return device->id;
+}
+
+base::Optional<uint64_t> GetHotwordDeviceId(
+    const chromeos::AudioDeviceList& devices) {
+  const chromeos::AudioDevice* result = nullptr;
+
+  for (const chromeos::AudioDevice& device : devices) {
+    if (!device.is_input)
+      continue;
+
+    switch (device.type) {
+      case chromeos::AUDIO_TYPE_HOTWORD:
+        result = GetHighestPriorityDevice(result, &device);
+        break;
+      default:
+        // ignore other devices
+        break;
+    }
+  }
+
+  return IdToOptional(result);
+}
+
+base::Optional<uint64_t> GetPreferredDeviceId(
+    const chromeos::AudioDeviceList& devices) {
+  const chromeos::AudioDevice* result = nullptr;
+
+  for (const chromeos::AudioDevice& device : devices) {
+    if (!device.is_input)
+      continue;
+
+    switch (device.type) {
+      case chromeos::AUDIO_TYPE_USB:
+      case chromeos::AUDIO_TYPE_HEADPHONE:
+      case chromeos::AUDIO_TYPE_INTERNAL_MIC:
+      case chromeos::AUDIO_TYPE_FRONT_MIC:
+        result = GetHighestPriorityDevice(result, &device);
+        break;
+      default:
+        // ignore other devices
+        break;
+    }
+  }
+
+  return IdToOptional(result);
+}
+
+base::Optional<std::string> ToString(base::Optional<uint64_t> int_value) {
+  if (!int_value)
+    return base::nullopt;
+  return base::NumberToString(int_value.value());
+}
+
+}  // namespace
+
+// Observer that will report all changes to the audio devices.
+// It will unsubscribe from |CrasAudioHandler| in its destructor.
+class AudioDevices::ScopedCrasAudioHandlerObserver
+    : private CrasAudioHandler::AudioObserver {
+ public:
+  ScopedCrasAudioHandlerObserver(CrasAudioHandler* cras_audio_handler,
+                                 AudioDevices* parent)
+      : parent_(parent), cras_audio_handler_(cras_audio_handler) {}
+  ScopedCrasAudioHandlerObserver(const ScopedCrasAudioHandlerObserver&) =
+      delete;
+  ScopedCrasAudioHandlerObserver& operator=(
+      const ScopedCrasAudioHandlerObserver&) = delete;
+  ~ScopedCrasAudioHandlerObserver() override = default;
+
+  // Start the observer, which means it will
+  //    - Subscribe for changes
+  //    - Fetch the current state.
+  void StartObserving() {
+    scoped_observer_.Observe(cras_audio_handler_);
+    FetchAudioNodes();
+  }
+
+ private:
+  // CrasAudioHandler::AudioObserver implementation:
+  void OnAudioNodesChanged() override { FetchAudioNodes(); }
+
+  void FetchAudioNodes() {
+    if (!base::SysInfo::IsRunningOnChromeOS())
+      return;
+
+    chromeos::AudioDeviceList audio_devices;
+    cras_audio_handler_->GetAudioDevices(&audio_devices);
+    parent_->SetAudioDevices(audio_devices);
+  }
+
+  AudioDevices* const parent_;
+  // Owned by |AssistantManagerServiceImpl|.
+  CrasAudioHandler* const cras_audio_handler_;
+  base::ScopedObservation<CrasAudioHandler,
+                          CrasAudioHandler::AudioObserver,
+                          &CrasAudioHandler::AddAudioObserver,
+                          &CrasAudioHandler::RemoveAudioObserver>
+      scoped_observer_{this};
+};
+
+// Sends the new hotword model to |cras_audio_handler|. If that fails this class
+// will attempt to set the hotword model to |kDefaultLocale|.
+class AudioDevices::HotwordModelUpdater {
+ public:
+  HotwordModelUpdater(CrasAudioHandler* cras_audio_handler,
+                      uint64_t hotword_device,
+                      const std::string& locale)
+      : cras_audio_handler_(cras_audio_handler),
+        hotword_device_(hotword_device),
+        locale_(locale) {
+    SendUpdate();
+  }
+
+  HotwordModelUpdater(const HotwordModelUpdater&) = delete;
+  HotwordModelUpdater& operator=(const HotwordModelUpdater&) = delete;
+  ~HotwordModelUpdater() = default;
+
+ private:
+  void SendUpdate() {
+    std::string hotword_model =
+        ToHotwordModel(locale_).value_or(kDefaultLocale);
+
+    VLOG(2) << "Changing audio hotword model of device " << hotword_device_
+            << " to '" << hotword_model << "'";
+
+    cras_audio_handler_->SetHotwordModel(
+        hotword_device_, hotword_model,
+        base::BindOnce(&HotwordModelUpdater::SetDspHotwordLocaleCallback,
+                       weak_factory_.GetWeakPtr(), hotword_model));
+  }
+
+  void SetDspHotwordLocaleCallback(std::string pref_locale, bool success) {
+    base::UmaHistogramBoolean("Assistant.SetDspHotwordLocale", success);
+    if (success) {
+      VLOG(2) << "Successfully changed audio hotword model";
+      return;
+    }
+
+    LOG(ERROR) << "Set " << pref_locale
+               << " hotword model failed, fallback to default locale.";
+    // Reset the locale to the default value if we failed to sync it to the
+    // locale stored in user's pref.
+    cras_audio_handler_->SetHotwordModel(
+        hotword_device_, /* hotword_model */ kDefaultLocale,
+        base::BindOnce([](bool success) {
+          if (!success)
+            LOG(ERROR) << "Reset to default hotword model failed.";
+        }));
+  }
+
+  CrasAudioHandler* const cras_audio_handler_;
+  uint64_t hotword_device_;
+  std::string locale_;
+
+  base::WeakPtrFactory<HotwordModelUpdater> weak_factory_{this};
+};
+
+AudioDevices::AudioDevices(CrasAudioHandler* cras_audio_handler,
+                           const std::string& locale)
+    : cras_audio_handler_(cras_audio_handler),
+      locale_(locale),
+      scoped_cras_audio_handler_observer_(
+          std::make_unique<ScopedCrasAudioHandlerObserver>(cras_audio_handler,
+                                                           this)) {
+  // Note we can only start the observer here, at the end of the constructor,
+  // to ensure this class is properly initialized when we receive the current
+  // list of audio devices.
+  scoped_cras_audio_handler_observer_->StartObserving();
+}
+
+AudioDevices::~AudioDevices() = default;
+
+void AudioDevices::AddAndFireObserver(Observer* observer) {
+  DCHECK(observer);
+  observers_.AddObserver(observer);
+
+  observer->SetHotwordDeviceId(ToString(hotword_device_id_));
+  observer->SetDeviceId(ToString(device_id_));
+}
+
+void AudioDevices::RemoveObserver(Observer* observer) {
+  observers_.RemoveObserver(observer);
+}
+
+void AudioDevices::SetLocale(const std::string& locale) {
+  locale_ = locale;
+  UpdateHotwordModel();
+}
+
+void AudioDevices::SetAudioDevicesForTest(
+    const chromeos::AudioDeviceList& audio_devices) {
+  SetAudioDevices(audio_devices);
+}
+
+void AudioDevices::SetAudioDevices(const chromeos::AudioDeviceList& devices) {
+  UpdateHotwordDeviceId(devices);
+  UpdateDeviceId(devices);
+  UpdateHotwordModel();
+}
+
+void AudioDevices::UpdateHotwordDeviceId(
+    const chromeos::AudioDeviceList& devices) {
+  hotword_device_id_ = GetHotwordDeviceId(devices);
+
+  VLOG(2) << "Changed audio hotword input device to "
+          << ToString(hotword_device_id_).value_or("<none>");
+
+  for (auto& observer : observers_)
+    observer.SetHotwordDeviceId(ToString(hotword_device_id_));
+}
+
+void AudioDevices::UpdateDeviceId(const chromeos::AudioDeviceList& devices) {
+  device_id_ = GetPreferredDeviceId(devices);
+
+  VLOG(2) << "Changed audio input device to "
+          << ToString(device_id_).value_or("<none>");
+
+  for (auto& observer : observers_)
+    observer.SetDeviceId(ToString(device_id_));
+}
+
+void AudioDevices::UpdateHotwordModel() {
+  if (!hotword_device_id_)
+    return;
+
+  if (!features::IsDspHotwordEnabled())
+    return;
+
+  hotword_model_updater_ = std::make_unique<HotwordModelUpdater>(
+      cras_audio_handler_, hotword_device_id_.value(), locale_);
+}
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/platform/audio_devices.h

+// Copyright 2020 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.
+
+#ifndef CHROMEOS_SERVICES_ASSISTANT_PLATFORM_AUDIO_DEVICES_H_
+#define CHROMEOS_SERVICES_ASSISTANT_PLATFORM_AUDIO_DEVICES_H_
+
+#include <cstdint>
+#include "base/component_export.h"
+#include "base/observer_list.h"
+#include "base/optional.h"
+#include "base/scoped_observation.h"
+#include "chromeos/audio/audio_device.h"
+
+namespace chromeos {
+class CrasAudioHandler;
+}  // namespace chromeos
+
+namespace chromeos {
+namespace assistant {
+
+// This class will monitor the available audio devices (through
+// |CrasAudioHandler|), and select the devices to use for audio input (both
+// regular input and hotword detection).
+// When the selected devices change, this class will:
+//     - Inform the observers.
+//     - Find the hotword model to use, and send it to
+//       CrasAudioHandler::SetHotwordModel().
+class COMPONENT_EXPORT(ASSISTANT_SERVICE) AudioDevices {
+ public:
+  class Observer : public base::CheckedObserver {
+   public:
+    ~Observer() override = default;
+
+    // Set the input device to use for audio capture.
+    virtual void SetDeviceId(const base::Optional<std::string>& device_id) = 0;
+    // Set the input device to use for hardware based hotword detection.
+    virtual void SetHotwordDeviceId(
+        const base::Optional<std::string>& device_id) = 0;
+  };
+
+  AudioDevices(CrasAudioHandler* cras_audio_handler, const std::string& locale);
+  AudioDevices(const AudioDevices&) = delete;
+  AudioDevices& operator=(const AudioDevices&) = delete;
+  ~AudioDevices();
+
+  void AddAndFireObserver(Observer*);
+  void RemoveObserver(Observer*);
+
+  void SetLocale(const std::string& locale);
+
+  // Used during unittests to simulate an update to the list of available audio
+  // devices.
+  void SetAudioDevicesForTest(const chromeos::AudioDeviceList& audio_devices);
+
+  using ScopedObservation =
+      base::ScopedObservation<AudioDevices,
+                              Observer,
+                              &AudioDevices::AddAndFireObserver,
+                              &AudioDevices::RemoveObserver>;
+
+ private:
+  class ScopedCrasAudioHandlerObserver;
+  class HotwordModelUpdater;
+
+  void SetAudioDevices(const chromeos::AudioDeviceList& audio_devices);
+  void UpdateHotwordDeviceId(const chromeos::AudioDeviceList& devices);
+  void UpdateDeviceId(const chromeos::AudioDeviceList& devices);
+  void UpdateHotwordModel();
+
+  // Handles the asynchronous nature of sending a new hotword model to
+  // |cras_audio_handler_|.
+  std::unique_ptr<HotwordModelUpdater> hotword_model_updater_;
+
+  base::ObserverList<Observer> observers_;
+
+  // Owned by |AssistantManagerServiceImpl|.
+  CrasAudioHandler* const cras_audio_handler_;
+
+  std::string locale_;
+  base::Optional<uint64_t> hotword_device_id_;
+  base::Optional<uint64_t> device_id_;
+
+  // Observes changes to the available audio devices, and sends the list of
+  // devices to SetAudioDevices().
+  std::unique_ptr<ScopedCrasAudioHandlerObserver>
+      scoped_cras_audio_handler_observer_;
+};
+
+}  // namespace assistant
+}  // namespace chromeos
+
+#endif  // CHROMEOS_SERVICES_ASSISTANT_PLATFORM_AUDIO_DEVICES_H_

chromeos/services/assistant/platform/audio_input_host.cc

@@ -5,11 +5,8 @@
 #include "chromeos/services/assistant/platform/audio_input_host.h"
 
 #include "base/check.h"
-#include "base/metrics/histogram_functions.h"
 #include "base/optional.h"
-#include "base/strings/string_util.h"
-#include "base/strings/stringprintf.h"
-#include "chromeos/audio/cras_audio_handler.h"
+#include "chromeos/services/assistant/platform/audio_devices.h"
 #include "chromeos/services/assistant/platform/audio_input_impl.h"
 #include "chromeos/services/assistant/public/cpp/features.h"
 
@@ -18,8 +15,6 @@ namespace assistant {
 
 namespace {
 
-constexpr const char kDefaultLocale[] = "en_us";
-
 AudioInputImpl::LidState ConvertLidState(
     chromeos::PowerManagerClient::LidState state) {
   switch (state) {
@@ -33,52 +28,21 @@ AudioInputImpl::LidState ConvertLidState(
   }
 }
 
-// Hotword model is expected to have <language>_<region> format with lower
-// case, while the locale in pref is stored as <language>-<region> with region
-// code in capital letters. So we need to convert the pref locale to the
-// correct format.
-// Examples:
-//     "fr"     ->  "fr_fr"
-//     "nl-BE"  ->  "nl_be"
-base::Optional<std::string> ToHotwordModel(std::string pref_locale) {
-  std::vector<std::string> code_strings = base::SplitString(
-      pref_locale, "-", base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
-
-  if (code_strings.size() == 0) {
-    // Note: I am not sure this happens during real operations, but it
-    // definitely happens during the ChromeOS performance tests.
-    return base::nullopt;
-  }
-
-  DCHECK_LT(code_strings.size(), 3u);
-
-  // For locales with language code "en", use "en_all" hotword model.
-  if (code_strings[0] == "en")
-    return "en_all";
-
-  // If the language code and country code happen to be the same, e.g.
-  // France (FR) and French (fr), the locale will be stored as "fr" instead
-  // of "fr-FR" in the profile on Chrome OS.
-  if (code_strings.size() == 1)
-    return code_strings[0] + "_" + code_strings[0];
-
-  return code_strings[0] + "_" + base::ToLowerASCII(code_strings[1]);
-}
-
 }  // namespace
 
 chromeos::assistant::AudioInputHost::AudioInputHost(
     AudioInputImpl* audio_input,
     CrasAudioHandler* cras_audio_handler,
-    chromeos::PowerManagerClient* power_manager_client)
+    chromeos::PowerManagerClient* power_manager_client,
+    const std::string& locale)
     : audio_input_(audio_input),
-      cras_audio_handler_(cras_audio_handler),
       power_manager_client_(power_manager_client),
-      power_manager_client_observer_(this) {
+      power_manager_client_observer_(this),
+      audio_devices_(cras_audio_handler, locale) {
   DCHECK(audio_input_);
-  DCHECK(cras_audio_handler_);
   DCHECK(power_manager_client_);
 
+  audio_devices_observation_.Observe(&audio_devices_);
   power_manager_client_observer_.Observe(power_manager_client);
   power_manager_client->GetSwitchStates(base::BindOnce(
       &AudioInputHost::OnInitialLidStateReceived, weak_factory_.GetWeakPtr()));
@@ -90,8 +54,8 @@ void AudioInputHost::SetMicState(bool mic_open) {
   audio_input_->SetMicState(mic_open);
 }
 
-void AudioInputHost::SetDeviceId(const std::string& device_id) {
-  audio_input_->SetDeviceId(device_id);
+void AudioInputHost::SetDeviceId(const base::Optional<std::string>& device_id) {
+  audio_input_->SetDeviceId(device_id.value_or(""));
 }
 
 void AudioInputHost::OnConversationTurnStarted() {
@@ -111,48 +75,9 @@ void AudioInputHost::OnHotwordEnabled(bool enable) {
   audio_input_->OnHotwordEnabled(enable);
 }
 
-void AudioInputHost::SetHotwordDeviceId(const std::string& device_id) {
-  hotword_device_id_ = device_id;
-  audio_input_->SetHotwordDeviceId(device_id);
-}
-
-void AudioInputHost::SetDspHotwordLocale(std::string pref_locale) {
-  if (!features::IsDspHotwordEnabled())
-    return;
-
-  std::string hotword_model =
-      ToHotwordModel(pref_locale).value_or(kDefaultLocale);
-
-  cras_audio_handler_->SetHotwordModel(
-      GetDspNodeId(), hotword_model,
-      base::BindOnce(&AudioInputHost::SetDspHotwordLocaleCallback,
-                     weak_factory_.GetWeakPtr(), hotword_model));
-}
-
-void AudioInputHost::SetDspHotwordLocaleCallback(std::string pref_locale,
-                                                 bool success) {
-  base::UmaHistogramBoolean("Assistant.SetDspHotwordLocale", success);
-  if (success)
-    return;
-
-  LOG(ERROR) << "Set " << pref_locale
-             << " hotword model failed, fallback to default locale.";
-  // Reset the locale to the default value if we failed to sync it to the locale
-  // stored in user's pref.
-  cras_audio_handler_->SetHotwordModel(
-      GetDspNodeId(), /* hotword_model */ kDefaultLocale,
-      base::BindOnce([](bool success) {
-        if (!success)
-          LOG(ERROR) << "Reset to default hotword model failed.";
-      }));
-}
-
-uint64_t AudioInputHost::GetDspNodeId() const {
-  DCHECK(!hotword_device_id_.empty());
-  uint64_t result;
-  bool success = base::StringToUint64(hotword_device_id_, &result);
-  DCHECK(success) << "Invalid hotword device id '" << hotword_device_id_ << "'";
-  return result;
+void AudioInputHost::SetHotwordDeviceId(
+    const base::Optional<std::string>& device_id) {
+  audio_input_->SetHotwordDeviceId(device_id.value_or(""));
 }
 
 void AudioInputHost::LidEventReceived(

chromeos/services/assistant/platform/audio_input_host.h

@@ -11,10 +11,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/scoped_observation.h"
 #include "chromeos/dbus/power/power_manager_client.h"
-
-namespace chromeos {
-class CrasAudioHandler;
-}  // namespace chromeos
+#include "chromeos/services/assistant/platform/audio_devices.h"
 
 namespace chromeos {
 namespace assistant {
@@ -27,11 +24,15 @@ class AudioInputImpl;
 // This will allow us to move it to the Libassistant mojom service (at which
 // point this class will talk to the Libassistant mojom service).
 class COMPONENT_EXPORT(ASSISTANT_SERVICE) AudioInputHost
-    : public chromeos::PowerManagerClient::Observer {
+    : private chromeos::PowerManagerClient::Observer,
+      private AudioDevices::Observer
+
+{
  public:
   AudioInputHost(AudioInputImpl* audio_input,
                  CrasAudioHandler* cras_audio_handler,
-                 chromeos::PowerManagerClient* power_manager_client);
+                 chromeos::PowerManagerClient* power_manager_client,
+                 const std::string& locale);
   AudioInputHost(AudioInputHost&) = delete;
   AudioInputHost& operator=(AudioInputHost&) = delete;
   ~AudioInputHost() override;
@@ -39,26 +40,18 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AudioInputHost
   // Called when the mic state associated with the interaction is changed.
   void SetMicState(bool mic_open);
 
-  // Setting the input device to use for audio capture.
-  void SetDeviceId(const std::string& device_id);
-
   // Called when hotword enabled status changed.
   void OnHotwordEnabled(bool enable);
 
-  // Setting the hotword input device with hardware based hotword detection.
-  void SetHotwordDeviceId(const std::string& device_id);
-
-  // Setting the hotword locale for the input device with DSP support.
-  void SetDspHotwordLocale(std::string pref_locale);
-
   void OnConversationTurnStarted();
   void OnConversationTurnFinished();
 
- private:
-  void SetDspHotwordLocaleCallback(std::string pref_locale, bool success);
-
-  uint64_t GetDspNodeId() const;
+  // AudioDevices::Observer implementation:
+  void SetDeviceId(const base::Optional<std::string>& device_id) override;
+  void SetHotwordDeviceId(
+      const base::Optional<std::string>& device_id) override;
 
+ private:
   // chromeos::PowerManagerClient::Observer overrides:
   void LidEventReceived(chromeos::PowerManagerClient::LidState state,
                         base::TimeTicks timestamp) override;
@@ -68,14 +61,15 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AudioInputHost
 
   // Owned by |PlatformApiImpl| which also owns |this|.
   AudioInputImpl* const audio_input_;
-  CrasAudioHandler* const cras_audio_handler_;
   chromeos::PowerManagerClient* const power_manager_client_;
   base::ScopedObservation<chromeos::PowerManagerClient,
                           chromeos::PowerManagerClient::Observer>
       power_manager_client_observer_;
 
-  // Hotword input device used for hardware based hotword detection.
-  std::string hotword_device_id_;
+  // Observes available audio devices and will set device-id/hotword-device-id
+  // accordingly.
+  AudioDevices audio_devices_;
+  AudioDevices::ScopedObservation audio_devices_observation_{this};
 
   base::WeakPtrFactory<AudioInputHost> weak_factory_{this};
 };

chromeos/services/assistant/platform/audio_input_impl.cc

@@ -283,6 +283,7 @@ void AudioInputImpl::RemoveObserver(
   DCHECK_CALLED_ON_VALID_SEQUENCE(observer_sequence_checker_);
   if (open_audio_stream_)
     VLOG(1) << open_audio_stream_->device_id() << " remove observer";
+
   bool have_no_observer = false;
   {
     base::AutoLock lock(lock_);

chromeos/services/assistant/platform/audio_input_impl_unittest.cc

@@ -11,7 +11,6 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "chromeos/audio/cras_audio_handler.h"
-#include "chromeos/dbus/audio/fake_cras_audio_client.h"
 #include "chromeos/dbus/power/fake_power_manager_client.h"
 #include "chromeos/services/assistant/platform/audio_input_host.h"
 #include "chromeos/services/assistant/platform/audio_stream_factory_delegate.h"
@@ -48,22 +47,14 @@ class ScopedFakeAssistantClient : public ScopedAssistantClient {
   DISALLOW_COPY_AND_ASSIGN(ScopedFakeAssistantClient);
 };
 
-// Mock for |CrosAudioClient|. This inherits from |FakeCrasAudioClient| so we
-// only have to mock the methods we're interested in.
-// It will automatically be installed as the global singleton in its
-// constructor, and removed in the destructor.
-class ScopedCrasAudioClientMock : public FakeCrasAudioClient {
+class ScopedCrasAudioHandler {
  public:
-  ScopedCrasAudioClientMock() = default;
-  ScopedCrasAudioClientMock(ScopedCrasAudioClientMock&) = delete;
-  ScopedCrasAudioClientMock& operator=(ScopedCrasAudioClientMock&) = delete;
-  ~ScopedCrasAudioClientMock() override = default;
-
-  MOCK_METHOD(void,
-              SetHotwordModel,
-              (uint64_t node_id,
-               const std::string& hotword_model,
-               VoidDBusMethodCallback callback));
+  ScopedCrasAudioHandler() { CrasAudioHandler::InitializeForTesting(); }
+  ScopedCrasAudioHandler(const ScopedCrasAudioHandler&) = delete;
+  ScopedCrasAudioHandler& operator=(const ScopedCrasAudioHandler&) = delete;
+  ~ScopedCrasAudioHandler() { CrasAudioHandler::Shutdown(); }
+
+  CrasAudioHandler* Get() { return CrasAudioHandler::Get(); }
 };
 
 }  // namespace
@@ -76,7 +67,6 @@ class AudioInputImplTest : public testing::Test,
     scoped_feature_list_.InitAndEnableFeature(features::kEnableDspHotword);
 
     PowerManagerClient::InitializeFake();
-    CrasAudioHandler::InitializeForTesting();
 
     CreateNewAudioInputImpl();
   }
@@ -87,7 +77,6 @@ class AudioInputImplTest : public testing::Test,
     // |audio_input_host_| uses the fake power manager client, so must be
     // destroyed before the power manager client.
     audio_input_host_.reset();
-    CrasAudioHandler::Shutdown();
     chromeos::PowerManagerClient::Shutdown();
   }
 
@@ -113,8 +102,9 @@ class AudioInputImplTest : public testing::Test,
         &audio_stream_factory_delegate_, "fake-device-id");
 
     audio_input_host_ = std::make_unique<AudioInputHost>(
-        audio_input_impl_.get(), CrasAudioHandler::Get(),
-        FakePowerManagerClient::Get());
+        audio_input_impl_.get(), cras_audio_handler_.Get(),
+        FakePowerManagerClient::Get(), "initial-locale");
+    audio_input_host_->SetDeviceId("initial-device-id");
 
     audio_input_impl_->AddObserver(this);
 
@@ -132,10 +122,6 @@ class AudioInputImplTest : public testing::Test,
 
   AudioInputHost& audio_input_host() { return *audio_input_host_; }
 
-  ScopedCrasAudioClientMock& cras_audio_client_mock() {
-    return cras_audio_client_mock_;
-  }
-
   // assistant_client::AudioInput::Observer overrides:
   void OnAudioBufferAvailable(const assistant_client::AudioBuffer& buffer,
                               int64_t timestamp) override {}
@@ -164,7 +150,7 @@ class AudioInputImplTest : public testing::Test,
   base::test::ScopedFeatureList scoped_feature_list_;
   ScopedFakeAssistantClient fake_assistant_client_;
   DefaultAudioStreamFactoryDelegate audio_stream_factory_delegate_;
-  ::testing::NiceMock<ScopedCrasAudioClientMock> cras_audio_client_mock_;
+  ScopedCrasAudioHandler cras_audio_handler_;
   std::unique_ptr<AudioInputImpl> audio_input_impl_;
   std::unique_ptr<AudioInputHost> audio_input_host_;
 
@@ -290,81 +276,5 @@ TEST_F(AudioInputImplTest,
   EXPECT_FALSE(IsUsingDeadStreamDetection());
 }
 
-TEST_F(AudioInputImplTest, ShouldSendHotwordLocaleToCrasAudioClient) {
-  StopAudioRecording();
-
-  audio_input_host().SetHotwordDeviceId("111");
-
-  EXPECT_CALL(cras_audio_client_mock(), SetHotwordModel);
-  audio_input_host().SetDspHotwordLocale("bla");
-}
-
-TEST_F(AudioInputImplTest,
-       ShouldFormatHotwordLocaleAndSendItToCrasAudioClient) {
-  StopAudioRecording();
-  audio_input_host().SetHotwordDeviceId("111");
-
-  // Normal case
-  EXPECT_CALL(cras_audio_client_mock(), SetHotwordModel(111, "nl_be", _));
-  audio_input_host().SetDspHotwordLocale("nl-BE");
-
-  // Handle the case where country code and language code are the same
-  EXPECT_CALL(cras_audio_client_mock(), SetHotwordModel(111, "fr_fr", _));
-  audio_input_host().SetDspHotwordLocale("fr");
-
-  // use "en_all" for all english locales
-  EXPECT_CALL(cras_audio_client_mock(), SetHotwordModel(111, "en_all", _));
-  audio_input_host().SetDspHotwordLocale("en-US");
-}
-
-TEST_F(AudioInputImplTest, ShouldUseDefaultLocaleIfUserPrefIsRejected) {
-  const std::string default_locale = "en_us";
-  StopAudioRecording();
-  audio_input_host().SetHotwordDeviceId("222");
-
-  EXPECT_CALL(cras_audio_client_mock(),
-              SetHotwordModel(222, "rejected_locale", _))
-      .WillOnce([](uint64_t node_id, const std::string&,
-                   VoidDBusMethodCallback callback) {
-        // Report failure to change the locale
-        std::move(callback).Run(/*success=*/false);
-      });
-
-  EXPECT_CALL(cras_audio_client_mock(),
-              SetHotwordModel(222, default_locale, _));
-
-  audio_input_host().SetDspHotwordLocale("rejected-LOCALE");
-}
-
-TEST_F(AudioInputImplTest, ShouldUseDefaultLocaleIfUserPrefIsEmpty) {
-  const std::string default_locale = "en_us";
-  StopAudioRecording();
-  audio_input_host().SetHotwordDeviceId("222");
-
-  EXPECT_CALL(cras_audio_client_mock(),
-              SetHotwordModel(222, default_locale, _));
-
-  audio_input_host().SetDspHotwordLocale("");
-}
-
-TEST_F(AudioInputImplTest, ShouldDoNothingIfUserPrefIsAccepted) {
-  const std::string default_locale = "en_us";
-  StopAudioRecording();
-  audio_input_host().SetHotwordDeviceId("222");
-
-  EXPECT_CALL(cras_audio_client_mock(),
-              SetHotwordModel(222, "accepted_locale", _))
-      .WillOnce([](uint64_t node_id, const std::string&,
-                   VoidDBusMethodCallback callback) {
-        // Accept the change to the locale.
-        std::move(callback).Run(/*success=*/true);
-      });
-
-  // Do not expect a second call if change of locale is accepted
-  EXPECT_CALL(cras_audio_client_mock(), SetHotwordModel(222, default_locale, _))
-      .Times(0);
-
-  audio_input_host().SetDspHotwordLocale("accepted-LOCALE");
-}
 }  // namespace assistant
 }  // namespace chromeos

chromeos/services/assistant/platform_api_impl.cc

@@ -8,9 +8,8 @@
 #include <utility>
 #include <vector>
 
-#include "base/system/sys_info.h"
-#include "chromeos/audio/cras_audio_handler.h"
 #include "chromeos/services/assistant/media_session/assistant_media_session.h"
+#include "chromeos/services/assistant/platform/audio_devices.h"
 #include "chromeos/services/assistant/platform/power_manager_provider_impl.h"
 #include "chromeos/services/assistant/public/cpp/features.h"
 #include "chromeos/services/assistant/utils.h"
@@ -83,16 +82,15 @@ PlatformApiImpl::PlatformApiImpl(
     mojo::PendingRemote<device::mojom::BatteryMonitor> battery_monitor,
     scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
-    std::string pref_locale)
+    const std::string& pref_locale)
     : audio_input_provider_(),
       audio_output_provider_(media_session,
                              background_task_runner,
                              media::AudioDeviceDescription::kDefaultDeviceId),
       audio_input_host_(&audio_input_provider_.GetAudioInput(),
                         cras_audio_handler,
-                        power_manager_client),
-      pref_locale_(pref_locale),
-      cras_audio_handler_(cras_audio_handler) {
+                        power_manager_client,
+                        pref_locale) {
   // Only enable native power features if they are supported by the UI.
   std::unique_ptr<PowerManagerProviderImpl> provider;
   if (features::IsPowerManagerEnabled()) {
@@ -101,14 +99,9 @@ PlatformApiImpl::PlatformApiImpl(
   }
   system_provider_ = std::make_unique<SystemProviderImpl>(
       std::move(provider), std::move(battery_monitor));
-
-  cras_audio_handler_->AddAudioObserver(this);
-  OnAudioNodesChanged();
 }
 
-PlatformApiImpl::~PlatformApiImpl() {
-  cras_audio_handler_->RemoveAudioObserver(this);
-}
+PlatformApiImpl::~PlatformApiImpl() = default;
 
 AudioInputProviderImpl& PlatformApiImpl::GetAudioInputProvider() {
   return audio_input_provider_;
@@ -134,50 +127,6 @@ SystemProvider& PlatformApiImpl::GetSystemProvider() {
   return *system_provider_;
 }
 
-void PlatformApiImpl::OnAudioNodesChanged() {
-  if (!base::SysInfo::IsRunningOnChromeOS())
-    return;
-
-  chromeos::AudioDeviceList devices;
-  cras_audio_handler_->GetAudioDevices(&devices);
-
-  const chromeos::AudioDevice* input_device = nullptr;
-  const chromeos::AudioDevice* hotword_device = nullptr;
-
-  for (const chromeos::AudioDevice& device : devices) {
-    if (!device.is_input)
-      continue;
-
-    switch (device.type) {
-      case chromeos::AUDIO_TYPE_USB:
-      case chromeos::AUDIO_TYPE_HEADPHONE:
-      case chromeos::AUDIO_TYPE_INTERNAL_MIC:
-      case chromeos::AUDIO_TYPE_FRONT_MIC:
-        if (!input_device || input_device->priority < device.priority)
-          input_device = &device;
-        break;
-      case chromeos::AUDIO_TYPE_HOTWORD:
-        if (!hotword_device || hotword_device->priority < device.priority)
-          hotword_device = &device;
-        break;
-      default:
-        // ignore other devices
-        break;
-    }
-  }
-
-  audio_input_host_.SetDeviceId(
-      input_device ? base::NumberToString(input_device->id) : std::string());
-
-  if (hotword_device) {
-    audio_input_host_.SetHotwordDeviceId(
-        base::NumberToString(hotword_device->id));
-    audio_input_host_.SetDspHotwordLocale(pref_locale_);
-  } else {
-    audio_input_host_.SetHotwordDeviceId(std::string());
-  }
-}
-
 void PlatformApiImpl::SetMicState(bool mic_open) {
   audio_input_host_.SetMicState(mic_open);
 }

chromeos/services/assistant/platform_api_impl.h

@@ -10,7 +10,6 @@
 #include <utility>
 #include <vector>
 
-#include "chromeos/audio/cras_audio_handler.h"
 #include "chromeos/services/assistant/platform/audio_input_host.h"
 #include "chromeos/services/assistant/platform/audio_input_provider_impl.h"
 #include "chromeos/services/assistant/platform/audio_output_provider_impl.h"
@@ -24,6 +23,7 @@
 #include "services/device/public/mojom/battery_monitor.mojom.h"
 
 namespace chromeos {
+class CrasAudioHandler;
 class PowerManagerClient;
 
 namespace assistant {
@@ -31,8 +31,7 @@ namespace assistant {
 class AssistantMediaSession;
 
 // Platform API required by the voice assistant.
-class PlatformApiImpl : public CrosPlatformApi,
-                        CrasAudioHandler::AudioObserver {
+class PlatformApiImpl : public CrosPlatformApi {
  public:
   PlatformApiImpl(
       AssistantMediaSession* media_session,
@@ -41,7 +40,7 @@ class PlatformApiImpl : public CrosPlatformApi,
       mojo::PendingRemote<device::mojom::BatteryMonitor> battery_monitor,
       scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner,
       scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
-      std::string pref_locale);
+      const std::string& pref_locale);
   ~PlatformApiImpl() override;
 
   // assistant_client::PlatformApi overrides
@@ -52,9 +51,6 @@ class PlatformApiImpl : public CrosPlatformApi,
   assistant_client::NetworkProvider& GetNetworkProvider() override;
   assistant_client::SystemProvider& GetSystemProvider() override;
 
-  // chromeos::CrasAudioHandler::AudioObserver overrides
-  void OnAudioNodesChanged() override;
-
   // Called when the mic state associated with the interaction is changed.
   void SetMicState(bool mic_open) override;
 
@@ -104,9 +100,6 @@ class PlatformApiImpl : public CrosPlatformApi,
   NetworkProviderImpl network_provider_;
   AudioInputHost audio_input_host_;
   std::unique_ptr<SystemProviderImpl> system_provider_;
-  std::string pref_locale_;
-
-  CrasAudioHandler* const cras_audio_handler_;
 
   DISALLOW_COPY_AND_ASSIGN(PlatformApiImpl);
 };