[Fuchsia][WebEngine] Use AudioConsumer only when session_id is set.

Previously WebEngine was using AudioConsumer for all audio streams. Now
it will be used only when media_session_id is set.

Bug: 1271250, b/199318444
Change-Id: Iecb2239dac4cf3718b5a858f6ee5b6704daf3119
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3329001
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Sergey Ulanov <sergeyu@chromium.org>
Reviewed-by: David Dorwin <ddorwin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#952136}

fuchsia/engine/browser/frame_impl.h

@@ -84,7 +84,9 @@ class FrameImpl : public fuchsia::web::Frame,
   FrameImpl(const FrameImpl&) = delete;
   FrameImpl& operator=(const FrameImpl&) = delete;
 
-  uint64_t media_session_id() const { return media_session_id_; }
+  absl::optional<uint64_t> media_session_id() const {
+    return media_session_id_;
+  }
 
   FramePermissionController* permission_controller() {
     return &permission_controller_;
@@ -360,7 +362,7 @@ class FrameImpl : public fuchsia::web::Frame,
 
   // Session ID to use for fuchsia.media.AudioConsumer. Set with
   // SetMediaSessionId().
-  uint64_t media_session_id_ = 0;
+  absl::optional<uint64_t> media_session_id_;
 
   // Stored settings for web contents in the current Frame.
   fuchsia::web::ContentAreaSettings content_area_settings_;

fuchsia/engine/browser/fuchsia_media_resource_provider_impl.cc

@@ -32,6 +32,13 @@ FuchsiaMediaResourceProviderImpl::FuchsiaMediaResourceProviderImpl(
 
 FuchsiaMediaResourceProviderImpl::~FuchsiaMediaResourceProviderImpl() = default;
 
+void FuchsiaMediaResourceProviderImpl::ShouldUseAudioConsumer(
+    ShouldUseAudioConsumerCallback callback) {
+  auto* frame_impl = FrameImpl::FromRenderFrameHost(render_frame_host());
+  DCHECK(frame_impl);
+  std::move(callback).Run(frame_impl->media_session_id().has_value());
+}
+
 void FuchsiaMediaResourceProviderImpl::CreateAudioConsumer(
     fidl::InterfaceRequest<fuchsia::media::AudioConsumer> request) {
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
@@ -47,7 +54,14 @@ void FuchsiaMediaResourceProviderImpl::CreateAudioConsumer(
                      ->Connect<fuchsia::media::SessionAudioConsumerFactory>();
   auto* frame_impl = FrameImpl::FromRenderFrameHost(render_frame_host());
   DCHECK(frame_impl);
-  factory->CreateAudioConsumer(frame_impl->media_session_id(),
+
+  if (!frame_impl->media_session_id().has_value()) {
+    LOG(WARNING) << "Renderer tried creating AudioConsumer for a Frame without "
+                    "media_session_id().";
+    return;
+  }
+
+  factory->CreateAudioConsumer(frame_impl->media_session_id().value(),
                                std::move(request));
 }
 

fuchsia/engine/browser/fuchsia_media_resource_provider_impl.h

@@ -41,6 +41,7 @@ class FuchsiaMediaResourceProviderImpl final
           receiver);
 
   // media::mojom::FuchsiaMediaResourceProvider:
+  void ShouldUseAudioConsumer(ShouldUseAudioConsumerCallback callback) override;
   void CreateAudioConsumer(
       fidl::InterfaceRequest<fuchsia::media::AudioConsumer> request) override;
   void CreateAudioCapturer(

fuchsia/engine/renderer/web_engine_audio_device_factory.cc

@@ -62,10 +62,6 @@ WebEngineAudioDeviceFactory::CreateAudioRendererSink(
       // Return nullptr for WebRTC streams. This will cause the caller to
       // fallback to AudioOutputDevice, which outputs through
       // AudioOutputStreamFuchsia.
-      //
-      // TODO(crbug.com/1066203): Make sure FuchsiaAudioOutputDevice doesn't
-      // increase latency (or degrade quality otherwise) and then switch to
-      // using FuchsiaAudioOutputDevice for WebRTC.
       return nullptr;
 
     // kNone is used in AudioDeviceFactory::GetOutputDeviceInfo() to get
@@ -90,6 +86,13 @@ WebEngineAudioDeviceFactory::CreateAudioRendererSink(
   render_frame->GetBrowserInterfaceBroker()->GetInterface(
       media_resource_provider.BindNewPipeAndPassReceiver());
 
+  // If AudioConsumer is not enabled then fallback to AudioOutputDevice.
+  bool use_audio_consumer = false;
+  if (!media_resource_provider->ShouldUseAudioConsumer(&use_audio_consumer) ||
+      !use_audio_consumer) {
+    return nullptr;
+  }
+
   // AudioConsumer can be used only to output to the default device.
   if (!params.device_id.empty())
     return nullptr;

fuchsia/engine/renderer/web_engine_content_renderer_client.cc

@@ -25,6 +25,7 @@
 #include "services/network/public/cpp/features.h"
 #include "services/service_manager/public/cpp/binder_registry.h"
 #include "third_party/blink/public/common/associated_interfaces/associated_interface_registry.h"
+#include "third_party/blink/public/common/browser_interface_broker_proxy.h"
 #include "third_party/blink/public/web/web_view.h"
 #include "third_party/widevine/cdm/widevine_cdm_common.h"
 
@@ -271,9 +272,22 @@ WebEngineContentRendererClient::GetBaseRendererFactory(
     media::DecoderFactory* decoder_factory,
     base::RepeatingCallback<media::GpuVideoAcceleratorFactories*()>
         get_gpu_factories_cb) {
+  auto* interface_broker = render_frame->GetBrowserInterfaceBroker();
+
+  mojo::Remote<media::mojom::FuchsiaMediaResourceProvider>
+      media_resource_provider;
+  interface_broker->GetInterface(
+      media_resource_provider.BindNewPipeAndPassReceiver());
+
+  bool use_audio_consumer = false;
+  if (!media_resource_provider->ShouldUseAudioConsumer(&use_audio_consumer) ||
+      !use_audio_consumer) {
+    return nullptr;
+  }
+
   return std::make_unique<WebEngineMediaRendererFactory>(
       media_log, decoder_factory, std::move(get_gpu_factories_cb),
-      render_frame->GetBrowserInterfaceBroker());
+      std::move(media_resource_provider));
 }
 
 bool WebEngineContentRendererClient::RunClosureWhenInForeground(

fuchsia/engine/renderer/web_engine_media_renderer_factory.cc

@@ -11,24 +11,21 @@
 #include "base/bind.h"
 #include "fuchsia/engine/renderer/web_engine_audio_renderer.h"
 #include "media/base/decoder_factory.h"
-#include "media/fuchsia/mojom/fuchsia_media_resource_provider.mojom.h"
 #include "media/renderers/renderer_impl.h"
 #include "media/renderers/video_renderer_impl.h"
 #include "media/video/gpu_memory_buffer_video_frame_pool.h"
 #include "media/video/gpu_video_accelerator_factories.h"
-#include "mojo/public/cpp/bindings/pending_remote.h"
-#include "mojo/public/cpp/bindings/remote.h"
-#include "third_party/blink/public/common/browser_interface_broker_proxy.h"
 
 WebEngineMediaRendererFactory::WebEngineMediaRendererFactory(
     media::MediaLog* media_log,
     media::DecoderFactory* decoder_factory,
     GetGpuFactoriesCB get_gpu_factories_cb,
-    blink::BrowserInterfaceBrokerProxy* interface_broker)
+    mojo::Remote<media::mojom::FuchsiaMediaResourceProvider>
+        media_resource_provider)
     : media_log_(media_log),
       decoder_factory_(decoder_factory),
       get_gpu_factories_cb_(std::move(get_gpu_factories_cb)),
-      interface_broker_(interface_broker) {
+      media_resource_provider_(std::move(media_resource_provider)) {
   DCHECK(decoder_factory_);
 }
 
@@ -54,16 +51,8 @@ std::unique_ptr<media::Renderer> WebEngineMediaRendererFactory::CreateRenderer(
     media::VideoRendererSink* video_renderer_sink,
     media::RequestOverlayInfoCB request_overlay_info_cb,
     const gfx::ColorSpace& target_color_space) {
-  mojo::PendingRemote<media::mojom::FuchsiaMediaResourceProvider>
-      media_resource_provider;
-  interface_broker_->GetInterface(
-      media_resource_provider.InitWithNewPipeAndPassReceiver());
-
-  mojo::Remote<media::mojom::FuchsiaMediaResourceProvider>
-      remote_media_resource_provider;
-  remote_media_resource_provider.Bind(std::move(media_resource_provider));
   fidl::InterfaceHandle<fuchsia::media::AudioConsumer> audio_consumer_handle;
-  remote_media_resource_provider->CreateAudioConsumer(
+  media_resource_provider_->CreateAudioConsumer(
       audio_consumer_handle.NewRequest());
   auto audio_renderer = std::make_unique<WebEngineAudioRenderer>(
       media_log_, std::move(audio_consumer_handle));

fuchsia/engine/renderer/web_engine_media_renderer_factory.h

@@ -10,10 +10,8 @@
 
 #include "base/callback.h"
 #include "media/base/renderer_factory.h"
-
-namespace blink {
-class BrowserInterfaceBrokerProxy;
-}  // namespace blink
+#include "media/fuchsia/mojom/fuchsia_media_resource_provider.mojom.h"
+#include "mojo/public/cpp/bindings/remote.h"
 
 namespace media {
 class AudioRendererSink;
@@ -35,7 +33,8 @@ class WebEngineMediaRendererFactory final : public media::RendererFactory {
       media::MediaLog* media_log,
       media::DecoderFactory* decoder_factory,
       GetGpuFactoriesCB get_gpu_factories_cb,
-      blink::BrowserInterfaceBrokerProxy* interface_broker);
+      mojo::Remote<media::mojom::FuchsiaMediaResourceProvider>
+          media_resource_provider);
   ~WebEngineMediaRendererFactory() override;
 
   // RendererFactory interface.
@@ -63,7 +62,8 @@ class WebEngineMediaRendererFactory final : public media::RendererFactory {
   // Creates factories for supporting video accelerators. May be null.
   GetGpuFactoriesCB get_gpu_factories_cb_;
 
-  blink::BrowserInterfaceBrokerProxy* const interface_broker_;
+  mojo::Remote<media::mojom::FuchsiaMediaResourceProvider>
+      media_resource_provider_;
 };
 
 #endif  // FUCHSIA_ENGINE_RENDERER_WEB_ENGINE_MEDIA_RENDERER_FACTORY_H_

fuchsia/engine/web_engine_integration_test.cc

@@ -347,7 +347,7 @@ TEST_F(WebEngineIntegrationTest, ContentDirectoryProvider) {
 TEST_F(WebEngineIntegrationMediaTest, PlayAudio) {
   CreateContextAndFrame(ContextParamsWithAudioAndTestData());
 
-  static uint16_t kTestMediaSessionId = 43;
+  static const uint16_t kTestMediaSessionId = 43;
   frame_->SetMediaSessionId(kTestMediaSessionId);
 
   ASSERT_NO_FATAL_FAILURE(LoadUrlAndExpectResponse(
@@ -388,6 +388,9 @@ TEST_F(WebEngineIntegrationMediaTest, PlayAudio_NoFlag) {
           [&is_requested](auto request) { is_requested = true; }));
   ZX_CHECK(status == ZX_OK, status) << "AddPublicService";
 
+  static const uint16_t kTestMediaSessionId = 1;
+  frame_->SetMediaSessionId(kTestMediaSessionId);
+
   ASSERT_NO_FATAL_FAILURE(LoadUrlAndExpectResponse(
       "fuchsia-dir://testdata/play_audio.html",
       cr_fuchsia::CreateLoadUrlParamsWithUserActivation()));
@@ -399,6 +402,9 @@ TEST_F(WebEngineIntegrationMediaTest, PlayAudio_NoFlag) {
 TEST_F(WebEngineIntegrationMediaTest, PlayVideo) {
   CreateContextAndFrame(ContextParamsWithAudioAndTestData());
 
+  static const uint16_t kTestMediaSessionId = 1;
+  frame_->SetMediaSessionId(kTestMediaSessionId);
+
   ASSERT_NO_FATAL_FAILURE(LoadUrlAndExpectResponse(
       kAutoplayVp9OpusToEndUrl,
       cr_fuchsia::CreateLoadUrlParamsWithUserActivation()));
@@ -609,6 +615,9 @@ TEST_F(MAYBE_VulkanWebEngineIntegrationTest,
       fuchsia::web::ContextFeatureFlags::AUDIO);
   CreateContextAndFrame(std::move(create_params));
 
+  static const uint16_t kTestMediaSessionId = 1;
+  frame_->SetMediaSessionId(kTestMediaSessionId);
+
   ASSERT_NO_FATAL_FAILURE(LoadUrlAndExpectResponse(
       kAutoplayVp9OpusToEndUrl,
       cr_fuchsia::CreateLoadUrlParamsWithUserActivation()));
@@ -630,6 +639,9 @@ TEST_F(WebEngineIntegrationMediaTest, HardwareVideoDecoderFlag_NotProvided) {
       ContextParamsWithAudioAndTestData();
   CreateContextAndFrame(std::move(create_params));
 
+  static const uint16_t kTestMediaSessionId = 1;
+  frame_->SetMediaSessionId(kTestMediaSessionId);
+
   ASSERT_NO_FATAL_FAILURE(LoadUrlAndExpectResponse(
       kAutoplayVp9OpusToEndUrl,
       cr_fuchsia::CreateLoadUrlParamsWithUserActivation()));

media/fuchsia/mojom/fuchsia_media_resource_provider.mojom

@@ -17,6 +17,12 @@ struct AudioCapturerRequest {
 // Interface used by the render to create media resources. Instances are
 // document-scoped.
 interface FuchsiaMediaResourceProvider {
+  // Returns true if the frame should use AudioConsumer to render audio
+  // streams. The call needs to be synchronous because `media::Renderer` needs
+  // to be constructed synchronously.
+  [Sync]
+  ShouldUseAudioConsumer() => (bool result);
+
   // Creates a fuchsia.media.AudioConsumer for the current frame.
   CreateAudioConsumer(AudioConsumerRequest request);