assistant: disable hotword without preferred input

Bug: b:141751193
Test: locally build and unittests
Change-Id: I1eb5ccfecb83723e54f67997283885c7f85c0fc1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1845795Reviewed-by: Tao Wu <wutao@chromium.org>
Commit-Queue: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704719}

chromeos/services/assistant/platform/audio_input_impl.cc

@@ -193,15 +193,13 @@ void AudioInputImpl::HotwordStateManager::RecreateAudioInputStream() {
 AudioInputImpl::AudioInputImpl(mojom::Client* client,
                                PowerManagerClient* power_manager_client,
                                CrasAudioHandler* cras_audio_handler,
-                               const std::string& device_id,
-                               const std::string& hotword_device_id)
+                               const std::string& device_id)
     : client_(client),
       power_manager_client_(power_manager_client),
       power_manager_client_observer_(this),
       cras_audio_handler_(cras_audio_handler),
       task_runner_(base::SequencedTaskRunnerHandle::Get()),
-      device_id_(device_id),
-      hotword_device_id_(hotword_device_id),
+      preferred_device_id_(device_id),
       weak_factory_(this) {
   DETACH_FROM_SEQUENCE(observer_sequence_checker_);
 
@@ -287,15 +285,7 @@ assistant_client::BufferFormat AudioInputImpl::GetFormat() const {
 void AudioInputImpl::AddObserver(
     assistant_client::AudioInput::Observer* observer) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(observer_sequence_checker_);
-  VLOG(1) << device_id_ << " add observer";
-
-  // Feed the observer one frame of empty data to work around crbug/942268
-  std::vector<int16_t> buffer(g_current_format.num_channels);
-  int64_t time = features::IsAudioEraserEnabled()
-                     ? base::TimeTicks::Now().since_origin().InMicroseconds()
-                     : 0;
-  AudioInputBufferImpl input_buffer(buffer.data(), /*frame_count=*/1);
-  observer->OnAudioBufferAvailable(input_buffer, time);
+  VLOG(1) << " add observer";
 
   bool have_first_observer = false;
   {
@@ -372,18 +362,20 @@ void AudioInputImpl::OnConversationTurnFinished() {
 void AudioInputImpl::OnHotwordEnabled(bool enable) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
-  if (default_on_ == enable)
+  if (hotword_enabled_ == enable)
     return;
 
-  default_on_ = enable;
+  hotword_enabled_ = enable;
   UpdateRecordingState();
 }
 
 void AudioInputImpl::SetDeviceId(const std::string& device_id) {
-  if (device_id_ == device_id)
+  if (preferred_device_id_ == device_id)
     return;
 
-  device_id_ = device_id;
+  preferred_device_id_ = device_id;
+
+  UpdateRecordingState();
   if (source_)
     state_manager_->RecreateAudioInputStream();
 }
@@ -449,29 +441,32 @@ void AudioInputImpl::RecreateAudioInputStream(bool use_dsp) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
   StopRecording();
 
+  device_id_ = preferred_device_id_.empty()
+                   ? media::AudioDeviceDescription::kDefaultDeviceId
+                   : preferred_device_id_;
+
   // AUDIO_PCM_LINEAR and AUDIO_PCM_LOW_LATENCY are the same on CRAS.
   auto param = media::AudioParameters(
       media::AudioParameters::AUDIO_PCM_LOW_LATENCY,
       GetChannelLayout(g_current_format), g_current_format.sample_rate,
       g_current_format.sample_rate / 10 /* buffer size for 100 ms */);
 
-  std::string* device_id = &device_id_;
   if (use_dsp && !hotword_device_id_.empty()) {
     param.set_effects(media::AudioParameters::PlatformEffectsMask::HOTWORD);
-    device_id = &hotword_device_id_;
+    device_id_ = hotword_device_id_;
   }
 
   mojo::PendingRemote<audio::mojom::StreamFactory> stream_factory;
   client_->RequestAudioStreamFactory(
       stream_factory.InitWithNewPipeAndPassReceiver());
-  source_ = audio::CreateInputDevice(std::move(stream_factory), *device_id);
+  source_ = audio::CreateInputDevice(std::move(stream_factory), device_id_);
 
   source_->Initialize(param, this);
   source_->Start();
   VLOG(1) << device_id_ << " start recording";
 }
 
-bool AudioInputImpl::IsHotwordAvailable() {
+bool AudioInputImpl::IsHotwordAvailable() const {
   return features::IsDspHotwordEnabled() && !hotword_device_id_.empty();
 }
 
@@ -479,6 +474,10 @@ bool AudioInputImpl::IsRecordingForTesting() const {
   return !!source_;
 }
 
+bool AudioInputImpl::IsUsingHotwordDeviceForTesting() const {
+  return device_id_ == hotword_device_id_ && IsHotwordAvailable();
+}
+
 void AudioInputImpl::StartRecording() {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
   DCHECK(!source_);
@@ -491,6 +490,7 @@ void AudioInputImpl::StopRecording() {
     VLOG(1) << device_id_ << " stop recording";
     source_->Stop();
     source_.reset();
+    device_id_ = std::string();
     VLOG(1) << device_id_
             << " ending captured frames: " << captured_frames_count_;
   }
@@ -508,21 +508,19 @@ void AudioInputImpl::OnSwitchStatesReceived(
 void AudioInputImpl::UpdateRecordingState() {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
-  bool should_start;
+  bool has_observers = false;
   {
     base::AutoLock lock(lock_);
-
-    switch (lid_state_) {
-      case chromeos::PowerManagerClient::LidState::OPEN:
-      case chromeos::PowerManagerClient::LidState::NOT_PRESENT:
-        should_start = (default_on_ || mic_open_) && observers_.size() > 0;
-        break;
-      case chromeos::PowerManagerClient::LidState::CLOSED:
-        should_start = false;
-        break;
-    }
+    has_observers = observers_.size() > 0;
   }
 
+  bool is_lid_closed =
+      lid_state_ == chromeos::PowerManagerClient::LidState::CLOSED;
+  bool should_enable_hotword =
+      hotword_enabled_ && (!preferred_device_id_.empty());
+  bool should_start =
+      !is_lid_closed && (should_enable_hotword || mic_open_) && has_observers;
+
   if (!source_ && should_start)
     StartRecording();
   else if (source_ && !should_start)

chromeos/services/assistant/platform/audio_input_impl.h

@@ -35,8 +35,7 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AudioInputImpl
   AudioInputImpl(mojom::Client* client,
                  PowerManagerClient* power_manager_client,
                  CrasAudioHandler* cras_audio_handler,
-                 const std::string& device_id,
-                 const std::string& hotword_device_id);
+                 const std::string& device_id);
   ~AudioInputImpl() override;
 
   class HotwordStateManager {
@@ -92,10 +91,12 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AudioInputImpl
 
   void RecreateAudioInputStream(bool use_dsp);
 
-  bool IsHotwordAvailable();
+  bool IsHotwordAvailable() const;
 
   // Returns the recording state used in unittests.
   bool IsRecordingForTesting() const;
+  // Returns if the hotword device is used for recording now.
+  bool IsUsingHotwordDeviceForTesting() const;
 
  private:
   void StartRecording();
@@ -108,12 +109,12 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AudioInputImpl
 
   scoped_refptr<media::AudioCapturerSource> source_;
 
-  // Should audio input always recording actively.
-  bool default_on_ = true;
-
   // User explicitly requested to open microphone.
   bool mic_open_ = false;
 
+  // Whether hotword is currently enabled.
+  bool hotword_enabled_ = true;
+
   // Guards observers_;
   base::Lock lock_;
   std::vector<assistant_client::AudioInput::Observer*> observers_;
@@ -142,10 +143,12 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AudioInputImpl
 
   std::unique_ptr<HotwordStateManager> state_manager_;
 
-  // Audio input device which will be used for capture.
-  std::string device_id_;
+  // Preferred audio input device which will be used for capture.
+  std::string preferred_device_id_;
   // Hotword input device used for hardware based hotword detection.
   std::string hotword_device_id_;
+  // Device currently being used for recording.
+  std::string device_id_;
 
   chromeos::PowerManagerClient::LidState lid_state_ =
       chromeos::PowerManagerClient::LidState::NOT_PRESENT;

chromeos/services/assistant/platform/audio_input_impl_unittest.cc

@@ -54,7 +54,7 @@ class AudioInputImplTest : public testing::Test,
 
     audio_input_impl_ = std::make_unique<AudioInputImpl>(
         &fake_assistant_client_, FakePowerManagerClient::Get(),
-        CrasAudioHandler::Get(), "fake-device-id", "fake-hotword-device-id");
+        CrasAudioHandler::Get(), "fake-device-id");
 
     audio_input_impl_->AddObserver(this);
   }
@@ -62,6 +62,7 @@ class AudioInputImplTest : public testing::Test,
   ~AudioInputImplTest() override {
     audio_input_impl_->RemoveObserver(this);
     audio_input_impl_.reset();
+    CrasAudioHandler::Shutdown();
     chromeos::PowerManagerClient::Shutdown();
   }
 
@@ -69,6 +70,12 @@ class AudioInputImplTest : public testing::Test,
     return audio_input_impl_->IsRecordingForTesting();
   }
 
+  bool IsUsingHotwordDevice() const {
+    return audio_input_impl_->IsUsingHotwordDeviceForTesting();
+  }
+
+  AudioInputImpl* audio_input_impl() { return audio_input_impl_.get(); }
+
   // assistant_client::AudioInput::Observer overrides:
   void OnAudioBufferAvailable(const assistant_client::AudioBuffer& buffer,
                               int64_t timestamp) override {}
@@ -104,5 +111,79 @@ TEST_F(AudioInputImplTest, StopRecordingWhenLidClosed) {
   EXPECT_TRUE(GetRecordingStatus());
 }
 
+TEST_F(AudioInputImplTest, StopRecordingWithNoPreferredDevice) {
+  // Start as recording.
+  ReportLidEvent(chromeos::PowerManagerClient::LidState::OPEN);
+  EXPECT_TRUE(GetRecordingStatus());
+
+  // Preferred input device is lost.
+  audio_input_impl()->SetDeviceId(std::string());
+  EXPECT_FALSE(GetRecordingStatus());
+
+  // Preferred input device is set again.
+  audio_input_impl()->SetDeviceId("fake-device_id");
+  EXPECT_TRUE(GetRecordingStatus());
+}
+
+TEST_F(AudioInputImplTest, StopRecordingWhenDisableHotword) {
+  // Start as recording.
+  ReportLidEvent(chromeos::PowerManagerClient::LidState::OPEN);
+  EXPECT_TRUE(GetRecordingStatus());
+
+  // Hotword disabled should stop recording.
+  audio_input_impl()->OnHotwordEnabled(false);
+  EXPECT_FALSE(GetRecordingStatus());
+
+  // Hotword enabled again should start recording.
+  audio_input_impl()->OnHotwordEnabled(true);
+  EXPECT_TRUE(GetRecordingStatus());
+}
+
+TEST_F(AudioInputImplTest, StartRecordingWhenDisableHotwordAndForceOpenMic) {
+  // Start as recording.
+  ReportLidEvent(chromeos::PowerManagerClient::LidState::OPEN);
+  EXPECT_TRUE(GetRecordingStatus());
+
+  // Hotword disabled should stop recording.
+  audio_input_impl()->OnHotwordEnabled(false);
+  EXPECT_FALSE(GetRecordingStatus());
+
+  // Force open mic should start recording.
+  audio_input_impl()->SetMicState(true);
+  EXPECT_TRUE(GetRecordingStatus());
+
+  // Stop force open mic should stop recording.
+  audio_input_impl()->SetMicState(false);
+  EXPECT_FALSE(GetRecordingStatus());
+}
+
+TEST_F(AudioInputImplTest, SettingHotwordDeviceDoesNotAffectRecordingState) {
+  // Start as recording.
+  ReportLidEvent(chromeos::PowerManagerClient::LidState::OPEN);
+  EXPECT_TRUE(GetRecordingStatus());
+
+  // Hotword device does not change recording state.
+  audio_input_impl()->SetHotwordDeviceId(std::string());
+  EXPECT_TRUE(GetRecordingStatus());
+
+  audio_input_impl()->SetHotwordDeviceId("fake-hotword-device");
+  EXPECT_TRUE(GetRecordingStatus());
+}
+
+TEST_F(AudioInputImplTest, SettingHotwordDeviceUsesHotwordDeviceForRecording) {
+  // Start as recording.
+  ReportLidEvent(chromeos::PowerManagerClient::LidState::OPEN);
+  EXPECT_TRUE(GetRecordingStatus());
+
+  // Hotword device does not change recording state.
+  audio_input_impl()->SetHotwordDeviceId(std::string());
+  EXPECT_TRUE(GetRecordingStatus());
+  EXPECT_FALSE(IsUsingHotwordDevice());
+
+  audio_input_impl()->SetHotwordDeviceId("fake-hotword-device");
+  EXPECT_TRUE(GetRecordingStatus());
+  EXPECT_TRUE(IsUsingHotwordDevice());
+}
+
 }  // namespace assistant
 }  // namespace chromeos

chromeos/services/assistant/platform/audio_input_provider_impl.cc

@@ -12,14 +12,11 @@ namespace assistant {
 AudioInputProviderImpl::AudioInputProviderImpl(
     mojom::Client* client,
     PowerManagerClient* power_manager_client,
-    CrasAudioHandler* cras_audio_handler,
-    const std::string& input_device_id,
-    const std::string& hotword_device_id)
+    CrasAudioHandler* cras_audio_handler)
     : audio_input_(client,
                    power_manager_client,
                    cras_audio_handler,
-                   input_device_id,
-                   hotword_device_id) {}
+                   /*input_device_id=*/std::string()) {}
 
 AudioInputProviderImpl::~AudioInputProviderImpl() = default;
 

chromeos/services/assistant/platform/audio_input_provider_impl.h

@@ -24,9 +24,7 @@ class AudioInputProviderImpl : public assistant_client::AudioInputProvider {
  public:
   AudioInputProviderImpl(mojom::Client* client,
                          PowerManagerClient* power_manager_client,
-                         CrasAudioHandler* cras_audio_handler,
-                         const std::string& input_device_id,
-                         const std::string& hotword_device_id);
+                         CrasAudioHandler* cras_audio_handler);
   ~AudioInputProviderImpl() override;
 
   // assistant_client::AudioInputProvider overrides:

chromeos/services/assistant/platform/audio_output_provider_impl.cc

@@ -150,8 +150,7 @@ AudioOutputProviderImpl::AudioOutputProviderImpl(
       loop_back_input_(client,
                        power_manager_client,
                        cras_audio_handler,
-                       media::AudioDeviceDescription::kLoopbackInputDeviceId,
-                       /*hotword_device_id=*/std::string()),
+                       media::AudioDeviceDescription::kLoopbackInputDeviceId),
       volume_control_impl_(client, media_session),
       main_task_runner_(base::SequencedTaskRunnerHandle::Get()),
       background_task_runner_(background_task_runner),

chromeos/services/assistant/platform_api_impl.cc

@@ -86,11 +86,7 @@ PlatformApiImpl::PlatformApiImpl(
     scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner,
     scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
     std::string pref_locale)
-    : audio_input_provider_(client,
-                            power_manager_client,
-                            cras_audio_handler,
-                            media::AudioDeviceDescription::kDefaultDeviceId,
-                            /*hotword_device_id=*/std::string()),
+    : audio_input_provider_(client, power_manager_client, cras_audio_handler),
       audio_output_provider_(client,
                              power_manager_client,
                              cras_audio_handler,
@@ -171,12 +167,16 @@ void PlatformApiImpl::OnAudioNodesChanged() {
         break;
     }
   }
-  if (input_device)
-    audio_input_provider_.SetDeviceId(base::NumberToString(input_device->id));
+
+  audio_input_provider_.SetDeviceId(
+      input_device ? base::NumberToString(input_device->id) : std::string());
+
   if (hotword_device) {
     audio_input_provider_.SetHotwordDeviceId(
         base::NumberToString(hotword_device->id));
     audio_input_provider_.SetDspHotwordLocale(pref_locale_);
+  } else {
+    audio_input_provider_.SetHotwordDeviceId(std::string());
   }
 }