assistant: fix DCHECK in AudioOutputDeviceImpl.

AudioOutputDevice needs to be destroyed on non-UI thread
to avoid blocking.

Bug: b/112782871
Test: Manual test with DCHECK on.
Change-Id: I8e7fd7d9656ff8a9152f10d48d4575055763bd63
Reviewed-on: https://chromium-review.googlesource.com/1197046
Commit-Queue: Muyuan Li <muyuanli@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587807}

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -58,8 +58,7 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
     device::mojom::BatteryMonitorPtr battery_monitor,
     Service* service,
     bool enable_hotword)
-    : platform_api_(connector, std::move(battery_monitor), enable_hotword),
-      enable_hotword_(enable_hotword),
+    : enable_hotword_(enable_hotword),
       action_module_(std::make_unique<action::CrosActionModule>(this)),
       main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
       assistant_settings_manager_(
@@ -70,6 +69,9 @@ AssistantManagerServiceImpl::AssistantManagerServiceImpl(
       background_thread_("background thread"),
       weak_factory_(this) {
   background_thread_.Start();
+  platform_api_ = std::make_unique<PlatformApiImpl>(
+      connector, std::move(battery_monitor), enable_hotword,
+      background_thread_.task_runner());
   connector->BindInterface(ash::mojom::kServiceName,
                            &voice_interaction_controller_);
 
@@ -203,12 +205,12 @@ void AssistantManagerServiceImpl::SendUpdateSettingsUiRequest(
 }
 
 void AssistantManagerServiceImpl::StartVoiceInteraction() {
-  platform_api_.SetMicState(true);
+  platform_api_->SetMicState(true);
   assistant_manager_->StartAssistantInteraction();
 }
 
 void AssistantManagerServiceImpl::StopActiveInteraction() {
-  platform_api_.SetMicState(false);
+  platform_api_->SetMicState(false);
   assistant_manager_->StopAssistantInteraction();
 }
 
@@ -309,7 +311,7 @@ void AssistantManagerServiceImpl::OnConversationTurnFinished(
   if (resolution != Resolution::NORMAL_WITH_FOLLOW_ON &&
       resolution != Resolution::CANCELLED &&
       resolution != Resolution::BARGE_IN) {
-    platform_api_.SetMicState(false);
+    platform_api_->SetMicState(false);
   }
   main_thread_task_runner_->PostTask(
       FROM_HERE,
@@ -560,7 +562,7 @@ void AssistantManagerServiceImpl::OnModifySettingsAction(
 
   if (modify_setting_args.setting_id() == kVolumeLevelDeviceSettingId) {
     assistant_client::VolumeControl& volume_control =
-        this->platform_api_.GetAudioOutputProvider().GetVolumeControl();
+        this->platform_api_->GetAudioOutputProvider().GetVolumeControl();
 
     HandleValueChange(
         modify_setting_args,
@@ -631,7 +633,7 @@ void AssistantManagerServiceImpl::OnVoiceInteractionContextEnabled(
 void AssistantManagerServiceImpl::OnVoiceInteractionHotwordEnabled(
     bool enabled) {
   enable_hotword_ = enabled;
-  platform_api_.OnHotwordEnabled(enabled);
+  platform_api_->OnHotwordEnabled(enabled);
 }
 
 void AssistantManagerServiceImpl::StartAssistantInternal(
@@ -640,7 +642,7 @@ void AssistantManagerServiceImpl::StartAssistantInternal(
   DCHECK(background_thread_.task_runner()->BelongsToCurrentThread());
 
   assistant_manager_.reset(assistant_client::AssistantManager::Create(
-      &platform_api_, CreateLibAssistantConfig(!enable_hotword_)));
+      platform_api_.get(), CreateLibAssistantConfig(!enable_hotword_)));
   assistant_manager_internal_ =
       UnwrapAssistantManagerInternal(assistant_manager_.get());
 

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -196,7 +196,7 @@ class AssistantManagerServiceImpl
       const std::vector<uint8_t>& assistant_screenshot);
 
   State state_ = State::STOPPED;
-  PlatformApiImpl platform_api_;
+  std::unique_ptr<PlatformApiImpl> platform_api_;
   bool enable_hotword_;
   std::unique_ptr<action::CrosActionModule> action_module_;
   scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;

chromeos/services/assistant/platform/audio_output_provider_impl.cc

@@ -80,18 +80,32 @@ void FillAudioFifoWithDataOfBufferFormat(
 
 class AudioOutputImpl : public assistant_client::AudioOutput {
  public:
-  AudioOutputImpl(service_manager::Connector* connector,
-                  scoped_refptr<base::SingleThreadTaskRunner> task_runner,
-                  assistant_client::OutputStreamType type,
-                  assistant_client::OutputStreamFormat format)
+  AudioOutputImpl(
+      service_manager::Connector* connector,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner,
+      scoped_refptr<base::SingleThreadTaskRunner> background_task_runner,
+      assistant_client::OutputStreamType type,
+      assistant_client::OutputStreamFormat format)
       : connector_(connector),
         main_thread_task_runner_(task_runner),
+        background_thread_task_runner_(background_task_runner),
         stream_type_(type),
         format_(format),
-        device_owner_(std::make_unique<AudioDeviceOwner>(task_runner)) {}
+        device_owner_(
+            std::make_unique<AudioDeviceOwner>(task_runner,
+                                               background_task_runner)) {}
 
   ~AudioOutputImpl() override {
-    main_thread_task_runner_->DeleteSoon(FROM_HERE, device_owner_.release());
+    // This ensures that it will be executed after StartOnMainThread.
+    main_thread_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            [](std::unique_ptr<AudioDeviceOwner> device_owner,
+               scoped_refptr<base::SingleThreadTaskRunner> background_runner) {
+              // Ensures |device_owner| is destructed on the correct thread.
+              background_runner->DeleteSoon(FROM_HERE, device_owner.release());
+            },
+            std::move(device_owner_), background_thread_task_runner_));
   }
 
   // assistant_client::AudioOutput overrides:
@@ -105,14 +119,15 @@ class AudioOutputImpl : public assistant_client::AudioOutput {
   }
 
   void Stop() override {
-    main_thread_task_runner_->PostTask(
-        FROM_HERE, base::BindOnce(&AudioDeviceOwner::StopOnMainThread,
+    background_thread_task_runner_->PostTask(
+        FROM_HERE, base::BindOnce(&AudioDeviceOwner::StopOnBackgroundThread,
                                   base::Unretained(device_owner_.get())));
   }
 
  private:
   service_manager::Connector* connector_;
   scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> background_thread_task_runner_;
 
   const assistant_client::OutputStreamType stream_type_;
   assistant_client::OutputStreamFormat format_;
@@ -171,10 +186,12 @@ void VolumeControlImpl::OnMuteStateChanged(bool mute) {
 }
 
 AudioOutputProviderImpl::AudioOutputProviderImpl(
-    service_manager::Connector* connector)
+    service_manager::Connector* connector,
+    scoped_refptr<base::SingleThreadTaskRunner> background_task_runner)
     : volume_control_impl_(connector),
       connector_(connector),
-      main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()) {}
+      main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      background_task_runner_(background_task_runner) {}
 
 AudioOutputProviderImpl::~AudioOutputProviderImpl() = default;
 
@@ -185,8 +202,8 @@ assistant_client::AudioOutput* AudioOutputProviderImpl::CreateAudioOutput(
   // once assistant_client::AudioOutput::Delegate::OnStopped() is called.
   // TODO(muyuanli): Handle encoded stream: OutputStreamEncoding::STREAM_MP3 /
   // OGG.
-  return new AudioOutputImpl(connector_, main_thread_task_runner_, type,
-                             stream_format);
+  return new AudioOutputImpl(connector_, main_thread_task_runner_,
+                             background_task_runner_, type, stream_format);
 }
 
 std::vector<assistant_client::OutputStreamEncoding>
@@ -220,10 +237,14 @@ void AudioOutputProviderImpl::RegisterAudioEmittingStateCallback(
 }
 
 AudioDeviceOwner::AudioDeviceOwner(
-    scoped_refptr<base::SequencedTaskRunner> task_runner)
-    : main_thread_task_runner_(task_runner) {}
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    scoped_refptr<base::SequencedTaskRunner> background_task_runner)
+    : main_thread_task_runner_(task_runner),
+      background_task_runner_(background_task_runner) {}
 
-AudioDeviceOwner::~AudioDeviceOwner() = default;
+AudioDeviceOwner::~AudioDeviceOwner() {
+  DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
+}
 
 void AudioDeviceOwner::StartOnMainThread(
     assistant_client::AudioOutput::Delegate* delegate,
@@ -254,20 +275,31 @@ void AudioDeviceOwner::StartOnMainThread(
 
   // |connector| is null in unittest.
   if (connector) {
-    output_device_ = std::make_unique<audio::OutputDevice>(
-        connector->Clone(), audio_param_, this,
-        media::AudioDeviceDescription::kDefaultDeviceId);
-    output_device_->Play();
+    // |AudioDeviceOwner| is destroyed on background thread. Thus, it's safe to
+    // use base::Unretained.
+    background_task_runner_->PostTask(
+        FROM_HERE,
+        base::BindOnce(&AudioDeviceOwner::StartDeviceOnBackgroundThread,
+                       base::Unretained(this), connector->Clone()));
   }
 }
 
-void AudioDeviceOwner::StopOnMainThread() {
-  DCHECK(main_thread_task_runner_->RunsTasksInCurrentSequence());
+void AudioDeviceOwner::StopOnBackgroundThread() {
+  DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
 
   output_device_.reset();
   delegate_->OnStopped();
 }
 
+void AudioDeviceOwner::StartDeviceOnBackgroundThread(
+    std::unique_ptr<service_manager::Connector> connector) {
+  DCHECK(background_task_runner_->RunsTasksInCurrentSequence());
+  output_device_ = std::make_unique<audio::OutputDevice>(
+      std::move(connector), audio_param_, this,
+      media::AudioDeviceDescription::kDefaultDeviceId);
+  output_device_->Play();
+}
+
 int AudioDeviceOwner::Render(base::TimeDelta delay,
                              base::TimeTicks delay_timestamp,
                              int prior_frames_skipped,
@@ -301,7 +333,7 @@ int AudioDeviceOwner::Render(base::TimeDelta delay,
 
   audio_fifo_->Consume()->CopyTo(dest);
 
-  ScheduleFillLocked(base::TimeTicks::Now());
+  ScheduleFillLocked(base::TimeTicks::Now() - delay);
   return dest->frames();
 }
 

chromeos/services/assistant/platform/audio_output_provider_impl.h

@@ -54,7 +54,9 @@ class VolumeControlImpl : public assistant_client::VolumeControl,
 
 class AudioOutputProviderImpl : public assistant_client::AudioOutputProvider {
  public:
-  explicit AudioOutputProviderImpl(service_manager::Connector* connector);
+  explicit AudioOutputProviderImpl(
+      service_manager::Connector* connector,
+      scoped_refptr<base::SingleThreadTaskRunner> background_task_runner);
   ~AudioOutputProviderImpl() override;
 
   // assistant_client::AudioOutputProvider overrides:
@@ -78,20 +80,23 @@ class AudioOutputProviderImpl : public assistant_client::AudioOutputProvider {
   VolumeControlImpl volume_control_impl_;
   service_manager::Connector* connector_;
   scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
+  scoped_refptr<base::SingleThreadTaskRunner> background_task_runner_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioOutputProviderImpl);
 };
 
 class AudioDeviceOwner : public media::AudioRendererSink::RenderCallback {
  public:
-  AudioDeviceOwner(scoped_refptr<base::SequencedTaskRunner> task_runner);
+  AudioDeviceOwner(
+      scoped_refptr<base::SequencedTaskRunner> task_runner,
+      scoped_refptr<base::SequencedTaskRunner> background_task_runner);
   ~AudioDeviceOwner() override;
 
   void StartOnMainThread(assistant_client::AudioOutput::Delegate* delegate,
                          service_manager::Connector* connector,
                          const assistant_client::OutputStreamFormat& format);
 
-  void StopOnMainThread();
+  void StopOnBackgroundThread();
 
   // media::AudioRenderSink::RenderCallback overrides:
   int Render(base::TimeDelta delay,
@@ -102,6 +107,9 @@ class AudioDeviceOwner : public media::AudioRendererSink::RenderCallback {
   void OnRenderError() override;
 
  private:
+  void StartDeviceOnBackgroundThread(
+      std::unique_ptr<service_manager::Connector> connector);
+
   // Requests assistant to fill buffer with more data.
   void ScheduleFillLocked(const base::TimeTicks& time);
 
@@ -109,6 +117,7 @@ class AudioDeviceOwner : public media::AudioRendererSink::RenderCallback {
   void BufferFillDone(int num_bytes);
 
   scoped_refptr<base::SequencedTaskRunner> main_thread_task_runner_;
+  scoped_refptr<base::SequencedTaskRunner> background_task_runner_;
 
   base::Lock lock_;
   std::unique_ptr<media::AudioBlockFifo> audio_fifo_;  // guarded by lock_.

chromeos/services/assistant/platform_api_impl.cc

@@ -74,9 +74,10 @@ void PlatformApiImpl::DummyAuthProvider::Reset() {}
 PlatformApiImpl::PlatformApiImpl(
     service_manager::Connector* connector,
     device::mojom::BatteryMonitorPtr battery_monitor,
-    bool enable_hotword)
+    bool enable_hotword,
+    scoped_refptr<base::SingleThreadTaskRunner> background_task_runner)
     : audio_input_provider_(connector, enable_hotword),
-      audio_output_provider_(connector),
+      audio_output_provider_(connector, background_task_runner),
       system_provider_(std::move(battery_monitor)) {}
 
 PlatformApiImpl::~PlatformApiImpl() = default;

chromeos/services/assistant/platform_api_impl.h

@@ -30,9 +30,11 @@ namespace assistant {
 // Platform API required by the voice assistant.
 class PlatformApiImpl : public assistant_client::PlatformApi {
  public:
-  PlatformApiImpl(service_manager::Connector* connector,
-                  device::mojom::BatteryMonitorPtr battery_monitor,
-                  bool enable_hotword);
+  PlatformApiImpl(
+      service_manager::Connector* connector,
+      device::mojom::BatteryMonitorPtr battery_monitor,
+      bool enable_hotword,
+      scoped_refptr<base::SingleThreadTaskRunner> background_task_runner);
   ~PlatformApiImpl() override;
 
   // assistant_client::PlatformApi overrides