Libassistant-V2: Migrate fallback media handler

In V1, we call assistant_manager_internal RegisterFallbackMediaHandler.
And later we handle HandleMediaAction() called from Libassistant when
they cannot handle.

There is no V2 functions for RegisterFallbackMediaHandler(), so we
added an new event to handle the media actions fallback in cl/425474947.

This cl migrates the LibassistantMediaHandler to
GrpcServicesObserver<OnMediaActionFallbackEventRequest>

Bug: b:196010315
Test: manual and passed chromeos_unittests
Change-Id: Id635d58c4ffa9a1b73ebffc3d58da3365893d7dd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3552417
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Commit-Queue: Tao Wu <wutao@chromium.org>
Cr-Commit-Position: refs/heads/main@{#985583}

chromeos/services/libassistant/grpc/assistant_client.h

@@ -24,6 +24,7 @@ class OnAssistantDisplayEventRequest;
 class OnConversationStateEventRequest;
 class OnDeviceStateEventRequest;
 class OnDisplayRequestRequest;
+class OnMediaActionFallbackEventRequest;
 class OnSpeakerIdEnrollmentEventRequest;
 class StartSpeakerIdEnrollmentRequest;
 class UpdateAssistantSettingsResponse;
@@ -78,6 +79,8 @@ class AssistantClient {
   // Media:
   using MediaStatus = ::assistant::api::events::DeviceState::MediaStatus;
   using OnDeviceStateEventRequest = ::assistant::api::OnDeviceStateEventRequest;
+  using OnMediaActionFallbackEventRequest =
+      ::assistant::api::OnMediaActionFallbackEventRequest;
 
   // Conversation:
   using OnConversationStateEventRequest =
@@ -137,9 +140,10 @@ class AssistantClient {
   // Sets the current media status of media playing outside of libassistant.
   // Setting external state will stop any internally playing media.
   virtual void SetExternalPlaybackState(const MediaStatus& status_proto) = 0;
-
   virtual void AddDeviceStateEventObserver(
       GrpcServicesObserver<OnDeviceStateEventRequest>* observer) = 0;
+  virtual void AddMediaActionFallbackEventObserver(
+      GrpcServicesObserver<OnMediaActionFallbackEventRequest>* observer) = 0;
 
   // Conversation methods.
   virtual void SendVoicelessInteraction(

chromeos/services/libassistant/grpc/assistant_client_impl.cc

@@ -175,6 +175,11 @@ void AssistantClientImpl::AddDeviceStateEventObserver(
   grpc_services_.AddDeviceStateEventObserver(observer);
 }
 
+void AssistantClientImpl::AddMediaActionFallbackEventObserver(
+    GrpcServicesObserver<OnMediaActionFallbackEventRequest>* observer) {
+  grpc_services_.AddMediaActionFallbackEventObserver(observer);
+}
+
 void AssistantClientImpl::SendVoicelessInteraction(
     const ::assistant::api::Interaction& interaction,
     const std::string& description,

chromeos/services/libassistant/grpc/assistant_client_impl.h

@@ -54,6 +54,9 @@ class AssistantClientImpl : public AssistantClientV1 {
       GrpcServicesObserver<OnAssistantDisplayEventRequest>* observer) override;
   void AddDeviceStateEventObserver(
       GrpcServicesObserver<OnDeviceStateEventRequest>* observer) override;
+  void AddMediaActionFallbackEventObserver(
+      GrpcServicesObserver<OnMediaActionFallbackEventRequest>* observer)
+      override;
   void SendVoicelessInteraction(
       const ::assistant::api::Interaction& interaction,
       const std::string& description,

chromeos/services/libassistant/grpc/assistant_client_v1.cc

@@ -29,6 +29,7 @@
 #include "chromeos/assistant/internal/proto/shared/proto/v2/speaker_id_enrollment_interface.pb.h"
 #include "chromeos/services/assistant/public/cpp/features.h"
 #include "chromeos/services/libassistant/callback_utils.h"
+#include "chromeos/services/libassistant/grpc/assistant_client.h"
 #include "chromeos/services/libassistant/grpc/utils/media_status_utils.h"
 #include "chromeos/services/libassistant/grpc/utils/settings_utils.h"
 #include "chromeos/services/libassistant/grpc/utils/timer_utils.h"
@@ -443,6 +444,17 @@ void AssistantClientV1::AddDeviceStateEventObserver(
   device_state_event_observer_list_.AddObserver(observer);
 }
 
+void AssistantClientV1::AddMediaActionFallbackEventObserver(
+    GrpcServicesObserver<OnMediaActionFallbackEventRequest>* observer) {
+  media_action_fallback_event_observer_list_.AddObserver(observer);
+
+  // Register handler for media actions.
+  auto callback = base::BindRepeating(&AssistantClientV1::HandleMediaAction,
+                                      weak_factory_.GetWeakPtr());
+  assistant_manager_internal()->RegisterFallbackMediaHandler(
+      ToStdFunctionRepeating(BindToCurrentSequenceRepeating(callback)));
+}
+
 void AssistantClientV1::SendVoicelessInteraction(
     const ::assistant::api::Interaction& interaction,
     const std::string& description,
@@ -536,6 +548,19 @@ void AssistantClientV1::AddMediaManagerListener() {
       media_manager_listener_.get());
 }
 
+void AssistantClientV1::HandleMediaAction(
+    const std::string& action_name,
+    const std::string& media_action_args_proto) {
+  OnMediaActionFallbackEventRequest request;
+  auto* media_action = request.mutable_event()->mutable_on_media_action_event();
+  media_action->set_action_name(action_name);
+  media_action->set_action_args(media_action_args_proto);
+
+  for (auto& observer : media_action_fallback_event_observer_list_) {
+    observer.OnGrpcMessage(request);
+  }
+}
+
 void AssistantClientV1::NotifyConversationStateEvent(
     const OnConversationStateEventRequest& request) {
   for (auto& observer : conversation_state_event_observer_list_) {

chromeos/services/libassistant/grpc/assistant_client_v1.h

@@ -57,6 +57,9 @@ class AssistantClientV1 : public AssistantClient {
   void SetExternalPlaybackState(const MediaStatus& status_proto) override;
   void AddDeviceStateEventObserver(
       GrpcServicesObserver<OnDeviceStateEventRequest>* observer) override;
+  void AddMediaActionFallbackEventObserver(
+      GrpcServicesObserver<OnMediaActionFallbackEventRequest>* observer)
+      override;
   void SendVoicelessInteraction(
       const ::assistant::api::Interaction& interaction,
       const std::string& description,
@@ -108,6 +111,8 @@ class AssistantClientV1 : public AssistantClient {
   class AssistantManagerDelegateImpl;
 
   void AddMediaManagerListener();
+  void HandleMediaAction(const std::string& action_name,
+                         const std::string& media_action_args_proto);
 
   void NotifyConversationStateEvent(
       const OnConversationStateEventRequest& request);
@@ -141,6 +146,9 @@ class AssistantClientV1 : public AssistantClient {
   base::ObserverList<GrpcServicesObserver<OnDeviceStateEventRequest>>
       device_state_event_observer_list_;
 
+  base::ObserverList<GrpcServicesObserver<OnMediaActionFallbackEventRequest>>
+      media_action_fallback_event_observer_list_;
+
   base::ObserverList<
       GrpcServicesObserver<::assistant::api::OnAlarmTimerEventRequest>>
       timer_event_observer_list_;

chromeos/services/libassistant/grpc/external_services/event_handler_driver.cc

@@ -16,6 +16,7 @@ constexpr char kAlarmTimerEventName[] = "AlarmTimerEvent";
 constexpr char kAssistantDisplayEventName[] = "AssistantDisplayEvent";
 constexpr char kConversationStateEventName[] = "ConversationStateEvent";
 constexpr char kDeviceStateEventName[] = "DeviceStateEvent";
+constexpr char kMediaActionFallbackEventName[] = "MediaActionFallbackEvent";
 constexpr char kHandlerMethodName[] = "OnEventFromLibas";
 
 template <typename EventSelection>
@@ -75,5 +76,16 @@ CreateRegistrationRequest<::assistant::api::DeviceStateEventHandlerInterface>(
   return request;
 }
 
+template <>
+::assistant::api::RegisterEventHandlerRequest CreateRegistrationRequest<
+    ::assistant::api::MediaActionFallbackEventHandlerInterface>(
+    const std::string& assistant_service_address) {
+  ::assistant::api::RegisterEventHandlerRequest request;
+  PopulateRequest(assistant_service_address, kMediaActionFallbackEventName,
+                  &request,
+                  request.mutable_media_action_fallback_events_to_handle());
+  return request;
+}
+
 }  // namespace libassistant
 }  // namespace chromeos

chromeos/services/libassistant/grpc/external_services/grpc_services_initializer.cc

@@ -98,6 +98,12 @@ void GrpcServicesInitializer::AddDeviceStateEventObserver(
   device_state_event_handler_driver_->AddObserver(observer);
 }
 
+void GrpcServicesInitializer::AddMediaActionFallbackEventObserver(
+    GrpcServicesObserver<::assistant::api::OnMediaActionFallbackEventRequest>*
+        observer) {
+  media_action_fallback_event_handler_driver_->AddObserver(observer);
+}
+
 ActionService* GrpcServicesInitializer::GetActionService() {
   return action_handler_driver_.get();
 }
@@ -140,6 +146,14 @@ void GrpcServicesInitializer::InitDrivers(grpc::ServerBuilder* server_builder) {
       EventHandlerDriver<::assistant::api::DeviceStateEventHandlerInterface>>(
       &server_builder_, libassistant_client_.get(), assistant_service_address_);
   service_drivers_.emplace_back(device_state_event_handler_driver_.get());
+
+  media_action_fallback_event_handler_driver_ =
+      std::make_unique<EventHandlerDriver<
+          ::assistant::api::MediaActionFallbackEventHandlerInterface>>(
+          &server_builder_, libassistant_client_.get(),
+          assistant_service_address_);
+  service_drivers_.emplace_back(
+      media_action_fallback_event_handler_driver_.get());
 }
 
 void GrpcServicesInitializer::InitLibassistGrpcClient() {
@@ -172,6 +186,7 @@ void GrpcServicesInitializer::RegisterEventHandlers() {
   assistant_display_event_handler_driver_->StartRegistration();
   conversation_state_event_handler_driver_->StartRegistration();
   device_state_event_handler_driver_->StartRegistration();
+  media_action_fallback_event_handler_driver_->StartRegistration();
 }
 
 }  // namespace libassistant

chromeos/services/libassistant/grpc/external_services/grpc_services_initializer.h

@@ -24,6 +24,7 @@ class AlarmTimerEventHandlerInterface;
 class AssistantDisplayEventHandlerInterface;
 class ConversationStateEventHandlerInterface;
 class DeviceStateEventHandlerInterface;
+class MediaActionFallbackEventHandlerInterface;
 }  // namespace api
 }  // namespace assistant
 
@@ -63,6 +64,9 @@ class GrpcServicesInitializer : public ServicesInitializerBase {
   void AddDeviceStateEventObserver(
       GrpcServicesObserver<::assistant::api::OnDeviceStateEventRequest>*
           observer);
+  void AddMediaActionFallbackEventObserver(
+      GrpcServicesObserver<::assistant::api::OnMediaActionFallbackEventRequest>*
+          observer);
 
   ActionService* GetActionService();
 
@@ -133,6 +137,10 @@ class GrpcServicesInitializer : public ServicesInitializerBase {
   std::unique_ptr<
       EventHandlerDriver<::assistant::api::DeviceStateEventHandlerInterface>>
       device_state_event_handler_driver_;
+
+  std::unique_ptr<EventHandlerDriver<
+      ::assistant::api::MediaActionFallbackEventHandlerInterface>>
+      media_action_fallback_event_handler_driver_;
 };
 
 }  // namespace libassistant

chromeos/services/libassistant/media_controller.cc

@@ -97,14 +97,15 @@ std::string GetWebUrlFromMediaArgs(const std::string& play_media_args_proto) {
 
 }  // namespace
 
-class MediaController::DeviceStateEventObserver
-    : public GrpcServicesObserver<::assistant::api::OnDeviceStateEventRequest> {
+class MediaController::GrpcEventsObserver
+    : public GrpcServicesObserver<::assistant::api::OnDeviceStateEventRequest>,
+      public GrpcServicesObserver<
+          ::assistant::api::OnMediaActionFallbackEventRequest> {
  public:
-  explicit DeviceStateEventObserver(MediaController* parent)
-      : parent_(parent) {}
-  DeviceStateEventObserver(const DeviceStateEventObserver&) = delete;
-  DeviceStateEventObserver& operator=(const DeviceStateEventObserver&) = delete;
-  ~DeviceStateEventObserver() override = default;
+  explicit GrpcEventsObserver(MediaController* parent) : parent_(parent) {}
+  GrpcEventsObserver(const GrpcEventsObserver&) = delete;
+  GrpcEventsObserver& operator=(const GrpcEventsObserver&) = delete;
+  ~GrpcEventsObserver() override = default;
 
   // GrpcServicesObserver:
   // Invoked when a device state event has been received.
@@ -123,37 +124,20 @@ class MediaController::DeviceStateEventObserver
         ConvertMediaStatusToMojomFromV2(new_state.media_status()));
   }
 
- private:
-  mojom::MediaDelegate& delegate() { return *parent_->delegate_; }
-
-  MediaController* const parent_;
-};
+  // Invoked when a media action fallack event has been received.
+  void OnGrpcMessage(const ::assistant::api::OnMediaActionFallbackEventRequest&
+                         request) override {
+    if (!request.event().has_on_media_action_event())
+      return;
 
-class MediaController::LibassistantMediaHandler {
- public:
-  LibassistantMediaHandler(
-      MediaController* parent,
-      assistant_client::AssistantManagerInternal* assistant_manager_internal)
-      : parent_(parent),
-        mojom_task_runner_(base::SequencedTaskRunnerHandle::Get()) {
-#if !BUILDFLAG(IS_PREBUILT_LIBASSISTANT)
-    // Register handler for media actions.
-    assistant_manager_internal->RegisterFallbackMediaHandler(
-        [this](std::string action_name, std::string media_action_args_proto) {
-          HandleMediaAction(action_name, media_action_args_proto);
-        });
-#endif  // !BUILDFLAG(IS_PREBUILT_LIBASSISTANT)
+    auto media_action_event = request.event().on_media_action_event();
+    HandleMediaAction(media_action_event.action_name(),
+                      media_action_event.action_args());
   }
-  LibassistantMediaHandler(const LibassistantMediaHandler&) = delete;
-  LibassistantMediaHandler& operator=(const LibassistantMediaHandler&) = delete;
-  ~LibassistantMediaHandler() = default;
 
  private:
-  // Called from the Libassistant thread.
   void HandleMediaAction(const std::string& action_name,
                          const std::string& media_action_args_proto) {
-    ENSURE_MOJOM_THREAD(&LibassistantMediaHandler::HandleMediaAction,
-                        action_name, media_action_args_proto);
     if (action_name == kPlayMediaClientOp)
       OnPlayMedia(media_action_args_proto);
     else
@@ -229,13 +213,10 @@ class MediaController::LibassistantMediaHandler {
   mojom::MediaDelegate& delegate() { return *parent_->delegate_; }
 
   MediaController* const parent_;
-  scoped_refptr<base::SequencedTaskRunner> mojom_task_runner_;
-  base::WeakPtrFactory<LibassistantMediaHandler> weak_factory_{this};
 };
 
 MediaController::MediaController()
-    : device_state_event_observer_(
-          std::make_unique<DeviceStateEventObserver>(this)) {}
+    : events_observer_(std::make_unique<GrpcEventsObserver>(this)) {}
 
 MediaController::~MediaController() = default;
 
@@ -271,24 +252,9 @@ void MediaController::SetExternalPlaybackState(mojom::MediaStatePtr state) {
 void MediaController::OnAssistantClientRunning(
     AssistantClient* assistant_client) {
   assistant_client_ = assistant_client;
-
-  handler_ = std::make_unique<LibassistantMediaHandler>(
-      this, assistant_client->assistant_manager_internal());
-
-  // |device_state_event_observer_| outlives |assistant_client_|.
-  assistant_client->AddDeviceStateEventObserver(
-      device_state_event_observer_.get());
-}
-
-void MediaController::OnDestroyingAssistantClient(
-    AssistantClient* assistant_client) {
-  assistant_client_ = nullptr;
-}
-
-void MediaController::OnAssistantClientDestroyed() {
-  // Handler can only be unset after the |AssistantManagerInternal| has been
-  // destroyed, as |AssistantManagerInternal| will call the handler.
-  handler_ = nullptr;
+  // `events_observer_` outlives `assistant_client_`.
+  assistant_client->AddDeviceStateEventObserver(events_observer_.get());
+  assistant_client->AddMediaActionFallbackEventObserver(events_observer_.get());
 }
 
 }  // namespace libassistant

chromeos/services/libassistant/media_controller.h

@@ -32,19 +32,15 @@ class MediaController : public mojom::MediaController,
 
   // AssistantClientObserver implementation:
   void OnAssistantClientRunning(AssistantClient* assistant_client) override;
-  void OnDestroyingAssistantClient(AssistantClient* assistant_client) override;
-  void OnAssistantClientDestroyed() override;
 
  private:
-  class DeviceStateEventObserver;
-  class LibassistantMediaHandler;
+  class GrpcEventsObserver;
 
   AssistantClient* assistant_client_ = nullptr;
 
   mojo::Receiver<mojom::MediaController> receiver_{this};
   mojo::Remote<mojom::MediaDelegate> delegate_;
-  std::unique_ptr<DeviceStateEventObserver> device_state_event_observer_;
-  std::unique_ptr<LibassistantMediaHandler> handler_;
+  std::unique_ptr<GrpcEventsObserver> events_observer_;
 };
 
 }  // namespace libassistant

chromeos/services/libassistant/test_support/fake_assistant_client.cc

@@ -88,6 +88,9 @@ void FakeAssistantClient::StopAssistantInteraction(bool cancel_conversation) {}
 void FakeAssistantClient::AddConversationStateEventObserver(
     GrpcServicesObserver<OnConversationStateEventRequest>* observer) {}
 
+void FakeAssistantClient::AddMediaActionFallbackEventObserver(
+    GrpcServicesObserver<OnMediaActionFallbackEventRequest>* observer) {}
+
 void FakeAssistantClient::SetInternalOptions(const std::string& locale,
                                              bool spoken_feedback_enabled) {}
 

chromeos/services/libassistant/test_support/fake_assistant_client.h

@@ -56,6 +56,9 @@ class FakeAssistantClient : public AssistantClient {
   void SetExternalPlaybackState(const MediaStatus& status_proto) override;
   void AddDeviceStateEventObserver(
       GrpcServicesObserver<OnDeviceStateEventRequest>* observer) override;
+  void AddMediaActionFallbackEventObserver(
+      GrpcServicesObserver<OnMediaActionFallbackEventRequest>* observer)
+      override;
   void SendVoicelessInteraction(
       const ::assistant::api::Interaction& interaction,
       const std::string& description,