assistant: Check for lid state when DSP is enabled.

This change adds logic to check for the device lid state by observing
the lid state switch when DSP is enabled, and stop DSP from recording
when the lid is closed. In this way, the Assistant cannot be waked up
by voice queries when the lid is closed.

Bug: b/139313417
Test: run unittest in this CL.
Change-Id: I1765db4f16e43ac7a5de71acde9dff253be8f363
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1758991
Commit-Queue: Meilin Wang <meilinw@chromium.org>
Reviewed-by: Tao Wu <wutao@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695778}

chromeos/services/assistant/BUILD.gn

@@ -163,6 +163,7 @@ source_set("tests") {
 
   if (enable_cros_libassistant) {
     sources += [
+      "platform/audio_input_impl_unittest.cc",
       "platform/audio_output_provider_impl_unittest.cc",
       "platform/network_provider_impl_unittest.cc",
       "platform/power_manager_provider_impl_unittest.cc",
@@ -173,6 +174,7 @@ source_set("tests") {
       "//chromeos/dbus",
       "//chromeos/services/network_config/public/mojom",
       "//libassistant/shared/public",
+      "//services/audio/public/cpp:test_support",
       "//services/device/public/cpp:test_support",
     ]
   }

chromeos/services/assistant/platform/audio_input_impl.cc

@@ -4,6 +4,8 @@
 
 #include "chromeos/services/assistant/platform/audio_input_impl.h"
 
+#include <utility>
+
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_functions.h"
@@ -52,12 +54,14 @@ media::ChannelLayout GetChannelLayout(
 
 class DspHotwordStateManager : public AudioInputImpl::HotwordStateManager {
  public:
-  DspHotwordStateManager(scoped_refptr<base::SequencedTaskRunner> task_runner,
-                         AudioInputImpl* input)
+  DspHotwordStateManager(AudioInputImpl* input,
+                         scoped_refptr<base::SequencedTaskRunner> task_runner,
+                         chromeos::PowerManagerClient* power_manager_client)
       : AudioInputImpl::HotwordStateManager(input),
         task_runner_(task_runner),
-        weak_factory_(this) {
+        power_manager_client_(power_manager_client) {
     DCHECK(task_runner_->RunsTasksInCurrentSequence());
+    DCHECK(power_manager_client_);
   }
 
   // HotwordStateManager overrides:
@@ -77,7 +81,7 @@ class DspHotwordStateManager : public AudioInputImpl::HotwordStateManager {
     // recognized hotword and started a conversation. We intentionally
     // avoid using |NotifyUserActivity| because it is not suitable for
     // this case according to the Platform team.
-    chromeos::PowerManagerClient::Get()->NotifyWakeNotification();
+    power_manager_client_->NotifyWakeNotification();
   }
 
   // Runs on main thread.
@@ -144,10 +148,11 @@ class DspHotwordStateManager : public AudioInputImpl::HotwordStateManager {
     base::UmaHistogramEnumeration("Assistant.DspHotwordDetection", status);
   }
 
-  StreamState stream_state_ = StreamState::HOTWORD;
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
+  chromeos::PowerManagerClient* power_manager_client_;
+  StreamState stream_state_ = StreamState::HOTWORD;
   base::OneShotTimer second_phase_timer_;
-  base::WeakPtrFactory<DspHotwordStateManager> weak_factory_;
+  base::WeakPtrFactory<DspHotwordStateManager> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(DspHotwordStateManager);
 };
@@ -185,16 +190,25 @@ void AudioInputImpl::HotwordStateManager::RecreateAudioInputStream() {
   input_->RecreateAudioInputStream(/*use_dsp=*/false);
 }
 
-AudioInputImpl::AudioInputImpl(mojom::Client* client,
-                               const std::string& device_id,
-                               const std::string& hotword_device_id)
+AudioInputImpl::AudioInputImpl(
+    mojom::Client* client,
+    chromeos::PowerManagerClient* power_manager_client,
+    const std::string& device_id,
+    const std::string& hotword_device_id)
     : client_(client),
+      power_manager_client_(power_manager_client),
+      power_manager_client_observer_(this),
       task_runner_(base::SequencedTaskRunnerHandle::Get()),
       device_id_(device_id),
       hotword_device_id_(hotword_device_id),
       weak_factory_(this) {
   DETACH_FROM_SEQUENCE(observer_sequence_checker_);
 
+  DCHECK(power_manager_client);
+  power_manager_client_observer_.Add(power_manager_client);
+  power_manager_client->GetSwitchStates(base::BindOnce(
+      &AudioInputImpl::OnSwitchStatesReceived, weak_factory_.GetWeakPtr()));
+
   RecreateStateManager();
   if (features::IsStereoAudioInputEnabled())
     g_current_format = kFormatStereo;
@@ -209,8 +223,8 @@ AudioInputImpl::~AudioInputImpl() {
 
 void AudioInputImpl::RecreateStateManager() {
   if (IsHotwordAvailable()) {
-    state_manager_ =
-        std::make_unique<DspHotwordStateManager>(task_runner_, this);
+    state_manager_ = std::make_unique<DspHotwordStateManager>(
+        this, task_runner_, power_manager_client_);
   } else {
     state_manager_ = std::make_unique<HotwordStateManager>(this);
   }
@@ -322,6 +336,19 @@ void AudioInputImpl::RemoveObserver(
   }
 }
 
+void AudioInputImpl::LidEventReceived(
+    chromeos::PowerManagerClient::LidState state,
+    const base::TimeTicks& timestamp) {
+  // Lid switch event still gets fired during system suspend, which enables
+  // us to stop DSP recording correctly when user closes lid after the device
+  // goes to sleep.
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+  if (state != lid_state_) {
+    lid_state_ = state;
+    UpdateRecordingState();
+  }
+}
+
 void AudioInputImpl::SetMicState(bool mic_open) {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
   if (mic_open_ == mic_open)
@@ -447,6 +474,10 @@ bool AudioInputImpl::IsHotwordAvailable() {
   return features::IsDspHotwordEnabled() && !hotword_device_id_.empty();
 }
 
+bool AudioInputImpl::IsRecordingForTesting() const {
+  return !!source_;
+}
+
 void AudioInputImpl::StartRecording() {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
   DCHECK(!source_);
@@ -464,13 +495,31 @@ void AudioInputImpl::StopRecording() {
   }
 }
 
+void AudioInputImpl::OnSwitchStatesReceived(
+    base::Optional<chromeos::PowerManagerClient::SwitchStates> switch_states) {
+  DCHECK(task_runner_->RunsTasksInCurrentSequence());
+  if (switch_states.has_value()) {
+    lid_state_ = switch_states->lid_state;
+    UpdateRecordingState();
+  }
+}
+
 void AudioInputImpl::UpdateRecordingState() {
   DCHECK(task_runner_->RunsTasksInCurrentSequence());
 
   bool should_start;
   {
     base::AutoLock lock(lock_);
-    should_start = (default_on_ || mic_open_) && observers_.size() > 0;
+
+    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;
+    }
   }
 
   if (!source_ && should_start)

chromeos/services/assistant/platform/audio_input_impl.h

@@ -9,11 +9,14 @@
 #include <string>
 #include <vector>
 
+#include "base/component_export.h"
 #include "base/macros.h"
 #include "base/observer_list.h"
+#include "base/scoped_observer.h"
 #include "base/sequence_checker.h"
 #include "base/synchronization/lock.h"
 #include "base/time/time.h"
+#include "chromeos/dbus/power/power_manager_client.h"
 #include "chromeos/services/assistant/public/mojom/assistant.mojom.h"
 #include "libassistant/shared/public/platform_audio_input.h"
 #include "media/base/audio_capturer_source.h"
@@ -22,10 +25,13 @@
 namespace chromeos {
 namespace assistant {
 
-class AudioInputImpl : public assistant_client::AudioInput,
-                       public media::AudioCapturerSource::CaptureCallback {
+class COMPONENT_EXPORT(ASSISTANT_SERVICE) AudioInputImpl
+    : public assistant_client::AudioInput,
+      public media::AudioCapturerSource::CaptureCallback,
+      public chromeos::PowerManagerClient::Observer {
  public:
   AudioInputImpl(mojom::Client* client,
+                 chromeos::PowerManagerClient* power_manager_client,
                  const std::string& device_id,
                  const std::string& hotword_device_id);
   ~AudioInputImpl() override;
@@ -64,6 +70,10 @@ class AudioInputImpl : public assistant_client::AudioInput,
   void RemoveObserver(
       assistant_client::AudioInput::Observer* observer) override;
 
+  // chromeos::PowerManagerClient::Observer overrides:
+  void LidEventReceived(chromeos::PowerManagerClient::LidState state,
+                        const base::TimeTicks& timestamp) override;
+
   // Called when the mic state associated with the interaction is changed.
   void SetMicState(bool mic_open);
   void OnConversationTurnStarted();
@@ -81,11 +91,18 @@ class AudioInputImpl : public assistant_client::AudioInput,
 
   bool IsHotwordAvailable();
 
+  // Returns the recording state used in unittests.
+  bool IsRecordingForTesting() const;
+
  private:
   void StartRecording();
   void StopRecording();
   void UpdateRecordingState();
 
+  // Updates lid state from received switch states.
+  void OnSwitchStatesReceived(
+      base::Optional<chromeos::PowerManagerClient::SwitchStates> switch_states);
+
   scoped_refptr<media::AudioCapturerSource> source_;
 
   // Should audio input always recording actively.
@@ -111,6 +128,11 @@ class AudioInputImpl : public assistant_client::AudioInput,
 
   mojom::Client* const client_;
 
+  chromeos::PowerManagerClient* power_manager_client_;
+  ScopedObserver<chromeos::PowerManagerClient,
+                 chromeos::PowerManagerClient::Observer>
+      power_manager_client_observer_;
+
   scoped_refptr<base::SequencedTaskRunner> task_runner_;
 
   std::unique_ptr<HotwordStateManager> state_manager_;
@@ -120,6 +142,9 @@ class AudioInputImpl : public assistant_client::AudioInput,
   // Hotword input device used for hardware based hotword detection.
   std::string hotword_device_id_;
 
+  chromeos::PowerManagerClient::LidState lid_state_ =
+      chromeos::PowerManagerClient::LidState::NOT_PRESENT;
+
   base::WeakPtrFactory<AudioInputImpl> weak_factory_;
   DISALLOW_COPY_AND_ASSIGN(AudioInputImpl);
 };

chromeos/services/assistant/platform/audio_input_impl_unittest.cc

+// Copyright 2019 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_input_impl.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/macros.h"
+#include "base/test/scoped_feature_list.h"
+#include "base/test/task_environment.h"
+#include "chromeos/dbus/power/fake_power_manager_client.h"
+#include "chromeos/services/assistant/fake_client.h"
+#include "chromeos/services/assistant/public/features.h"
+#include "services/audio/public/cpp/fake_stream_factory.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace assistant {
+
+namespace {
+
+class FakeAssistantClient : public FakeClient {
+ public:
+  FakeAssistantClient() = default;
+  ~FakeAssistantClient() override = default;
+
+  // FakeClient overrides:
+  void RequestAudioStreamFactory(
+      mojo::PendingReceiver<audio::mojom::StreamFactory> receiver) override {
+    if (!fake_stream_factory_.receiver_.is_bound())
+      fake_stream_factory_.receiver_.Bind(std::move(receiver));
+  }
+
+ private:
+  audio::FakeStreamFactory fake_stream_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(FakeAssistantClient);
+};
+
+}  // namespace
+
+class AudioInputImplTest : public testing::Test,
+                           public assistant_client::AudioInput::Observer {
+ public:
+  AudioInputImplTest() {
+    // Enable DSP feature flag.
+    scoped_feature_list_.InitAndEnableFeature(features::kEnableDspHotword);
+
+    chromeos::PowerManagerClient::InitializeFake();
+    audio_input_impl_ = std::make_unique<AudioInputImpl>(
+        &fake_assistant_client_, FakePowerManagerClient::Get(),
+        "fake-device-id", "fake-hotword-device-id");
+
+    audio_input_impl_->AddObserver(this);
+  }
+
+  ~AudioInputImplTest() override {
+    audio_input_impl_->RemoveObserver(this);
+    audio_input_impl_.reset();
+    chromeos::PowerManagerClient::Shutdown();
+  }
+
+  bool GetRecordingStatus() const {
+    return audio_input_impl_->IsRecordingForTesting();
+  }
+
+  // assistant_client::AudioInput::Observer overrides:
+  void OnAudioBufferAvailable(const assistant_client::AudioBuffer& buffer,
+                              int64_t timestamp) override {}
+  void OnAudioError(assistant_client::AudioInput::Error error) override {}
+  void OnAudioStopped() override {}
+
+ protected:
+  void ReportLidEvent(chromeos::PowerManagerClient::LidState state) {
+    FakePowerManagerClient::Get()->SetLidState(state,
+                                               base::TimeTicks::UnixEpoch());
+  }
+
+ private:
+  base::test::TaskEnvironment task_environment_;
+  base::test::ScopedFeatureList scoped_feature_list_;
+  FakeAssistantClient fake_assistant_client_;
+  std::unique_ptr<AudioInputImpl> audio_input_impl_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioInputImplTest);
+};
+
+TEST_F(AudioInputImplTest, StopRecordingWhenLidClosed) {
+  // Trigger a lid open event.
+  ReportLidEvent(chromeos::PowerManagerClient::LidState::OPEN);
+  EXPECT_TRUE(GetRecordingStatus());
+
+  // Trigger a lid closed event.
+  ReportLidEvent(chromeos::PowerManagerClient::LidState::CLOSED);
+  EXPECT_FALSE(GetRecordingStatus());
+
+  // Trigger a lid open event again.
+  ReportLidEvent(chromeos::PowerManagerClient::LidState::OPEN);
+  EXPECT_TRUE(GetRecordingStatus());
+}
+
+}  // namespace assistant
+}  // namespace chromeos

chromeos/services/assistant/platform/audio_input_provider_impl.cc

@@ -11,9 +11,13 @@ namespace assistant {
 
 AudioInputProviderImpl::AudioInputProviderImpl(
     mojom::Client* client,
+    chromeos::PowerManagerClient* power_manager_client,
     const std::string& input_device_id,
     const std::string& hotword_device_id)
-    : audio_input_(client, input_device_id, hotword_device_id) {}
+    : audio_input_(client,
+                   power_manager_client,
+                   input_device_id,
+                   hotword_device_id) {}
 
 AudioInputProviderImpl::~AudioInputProviderImpl() = default;
 

chromeos/services/assistant/platform/audio_input_provider_impl.h

@@ -20,6 +20,7 @@ namespace assistant {
 class AudioInputProviderImpl : public assistant_client::AudioInputProvider {
  public:
   AudioInputProviderImpl(mojom::Client* client,
+                         chromeos::PowerManagerClient* power_manager_client,
                          const std::string& input_device_id,
                          const std::string& hotword_device_id);
   ~AudioInputProviderImpl() override;

chromeos/services/assistant/platform/audio_output_provider_impl.cc

@@ -141,11 +141,13 @@ class AudioOutputImpl : public assistant_client::AudioOutput {
 
 AudioOutputProviderImpl::AudioOutputProviderImpl(
     mojom::Client* client,
+    chromeos::PowerManagerClient* power_manager_client,
     AssistantMediaSession* media_session,
     scoped_refptr<base::SequencedTaskRunner> background_task_runner,
     const std::string& device_id)
     : client_(client),
       loop_back_input_(client,
+                       power_manager_client,
                        media::AudioDeviceDescription::kLoopbackInputDeviceId,
                        /*hotword_device_id=*/std::string()),
       volume_control_impl_(client, media_session),

chromeos/services/assistant/platform/audio_output_provider_impl.h

@@ -32,6 +32,7 @@ class AudioOutputProviderImpl : public assistant_client::AudioOutputProvider {
  public:
   AudioOutputProviderImpl(
       mojom::Client* client,
+      chromeos::PowerManagerClient* power_manager_client,
       AssistantMediaSession* media_session,
       scoped_refptr<base::SequencedTaskRunner> background_task_runner,
       const std::string& device_id);

chromeos/services/assistant/platform_api_impl.cc

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/system/sys_info.h"
+#include "chromeos/dbus/power/power_manager_client.h"
 #include "chromeos/services/assistant/media_session/assistant_media_session.h"
 #include "chromeos/services/assistant/platform/power_manager_provider_impl.h"
 #include "chromeos/services/assistant/public/features.h"
@@ -84,9 +85,11 @@ PlatformApiImpl::PlatformApiImpl(
     scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
     std::string pref_locale)
     : audio_input_provider_(client,
+                            chromeos::PowerManagerClient::Get(),
                             media::AudioDeviceDescription::kDefaultDeviceId,
                             /*hotword_device_id=*/std::string()),
       audio_output_provider_(client,
+                             chromeos::PowerManagerClient::Get(),
                              media_session,
                              background_task_runner,
                              media::AudioDeviceDescription::kDefaultDeviceId),