Create MediaPlayer and set up MediaPlayerObserver simultaneously

Creating a MediaPlayer will now automatically initialize an initial
MediaPlayerObserver instance so that the remote end can receive
notifications since the very first moment the player is created.

This replaces an earlier workaround made for OnMediaMetadataChanged()
specifically.

The new browser test CanvasCaptureControlledByMediaSession verifies the
desired behavior.

Bug: 1191332
Change-Id: I849ef9f9ef10beedb69906f6aeea284f6f7846b5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2781733
Commit-Queue: Wojciech Dzierżanowski <wdzierzanowski@opera.com>
Reviewed-by: Tommy Steimel <steimel@chromium.org>
Reviewed-by: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: François Beaufort <beaufort.francois@gmail.com>
Cr-Commit-Position: refs/heads/master@{#868035}

content/browser/media/media_web_contents_observer.cc

@@ -292,9 +292,12 @@ void MediaWebContentsObserver::MediaPlayerHostImpl::BindMediaPlayerHostReceiver(
 
 void MediaWebContentsObserver::MediaPlayerHostImpl::OnMediaPlayerAdded(
     mojo::PendingAssociatedRemote<media::mojom::MediaPlayer> media_player,
+    mojo::PendingAssociatedReceiver<media::mojom::MediaPlayerObserver>
+        media_player_observer,
     int32_t player_id) {
   media_web_contents_observer_->OnMediaPlayerAdded(
-      std::move(media_player), MediaPlayerId(frame_routing_id_, player_id));
+      std::move(media_player), std::move(media_player_observer),
+      MediaPlayerId(frame_routing_id_, player_id));
 }
 
 MediaWebContentsObserver::MediaPlayerObserverHostImpl::
@@ -307,21 +310,17 @@ MediaWebContentsObserver::MediaPlayerObserverHostImpl::
 MediaWebContentsObserver::MediaPlayerObserverHostImpl::
     ~MediaPlayerObserverHostImpl() = default;
 
-mojo::PendingAssociatedRemote<media::mojom::MediaPlayerObserver>
-MediaWebContentsObserver::MediaPlayerObserverHostImpl::
-    BindMediaPlayerObserverReceiverAndPassRemote() {
-  media_player_observer_receiver_.reset();
-  mojo::PendingAssociatedRemote<media::mojom::MediaPlayerObserver>
-      pending_remote =
-          media_player_observer_receiver_.BindNewEndpointAndPassRemote();
+void MediaWebContentsObserver::MediaPlayerObserverHostImpl::
+    BindMediaPlayerObserverReceiver(
+        mojo::PendingAssociatedReceiver<media::mojom::MediaPlayerObserver>
+            media_player_observer) {
+  media_player_observer_receiver_.Bind(std::move(media_player_observer));
 
   // |media_web_contents_observer_| outlives MediaPlayerHostImpl, so it's safe
   // to use base::Unretained().
   media_player_observer_receiver_.set_disconnect_handler(base::BindOnce(
       &MediaWebContentsObserver::OnMediaPlayerObserverDisconnected,
       base::Unretained(media_web_contents_observer_), media_player_id_));
-
-  return pending_remote;
 }
 
 void MediaWebContentsObserver::MediaPlayerObserverHostImpl::
@@ -570,6 +569,8 @@ void MediaWebContentsObserver::BindMediaPlayerHost(
 
 void MediaWebContentsObserver::OnMediaPlayerAdded(
     mojo::PendingAssociatedRemote<media::mojom::MediaPlayer> player_remote,
+    mojo::PendingAssociatedReceiver<media::mojom::MediaPlayerObserver>
+        media_player_observer,
     MediaPlayerId player_id) {
   auto* const rfh = RenderFrameHost::FromID(player_id.frame_routing_id);
   DCHECK(rfh);
@@ -602,9 +603,8 @@ void MediaWebContentsObserver::OnMediaPlayerAdded(
     media_player_observer_hosts_[player_id] =
         std::make_unique<MediaPlayerObserverHostImpl>(player_id, this);
   }
-  media_player_remotes_[player_id]->AddMediaPlayerObserver(
-      media_player_observer_hosts_[player_id]
-          ->BindMediaPlayerObserverReceiverAndPassRemote());
+  media_player_observer_hosts_[player_id]->BindMediaPlayerObserverReceiver(
+      std::move(media_player_observer));
 }
 
 void MediaWebContentsObserver::SuspendAllMediaPlayers() {

content/browser/media/media_web_contents_observer.h

@@ -125,13 +125,6 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
       mojo::PendingAssociatedReceiver<media::mojom::MediaPlayerHost>
           player_receiver);
 
-  // Communicates with the MediaSessionControllerManager to find or create (if
-  // needed) a MediaSessionController identified by |player_id|, in order to
-  // bind its mojo remote for media::mojom::MediaPlayer.
-  void OnMediaPlayerAdded(
-      mojo::PendingAssociatedRemote<media::mojom::MediaPlayer> player_remote,
-      MediaPlayerId player_id);
-
   // Called by the WebContents when a tab has been closed but may still be
   // available for "undo" -- indicates that all media players (even audio only
   // players typically allowed background audio) bound to this WebContents must
@@ -145,8 +138,6 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
 
  private:
   class PlayerInfo;
-  friend class PlayerInfo;
-
   using PlayerInfoMap =
       base::flat_map<MediaPlayerId, std::unique_ptr<PlayerInfo>>;
 
@@ -168,6 +159,8 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
     // media::mojom::MediaPlayerHost implementation.
     void OnMediaPlayerAdded(
         mojo::PendingAssociatedRemote<media::mojom::MediaPlayer> media_player,
+        mojo::PendingAssociatedReceiver<media::mojom::MediaPlayerObserver>
+            media_player_observer,
         int32_t player_id) override;
 
    private:
@@ -186,8 +179,9 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
     ~MediaPlayerObserverHostImpl() override;
 
     // Used to bind the receiver via the BrowserInterfaceBroker.
-    mojo::PendingAssociatedRemote<media::mojom::MediaPlayerObserver>
-    BindMediaPlayerObserverReceiverAndPassRemote();
+    void BindMediaPlayerObserverReceiver(
+        mojo::PendingAssociatedReceiver<media::mojom::MediaPlayerObserver>
+            media_player_observer);
 
     // media::mojom::MediaPlayerObserver implementation.
     void OnMediaPlaying() override;
@@ -225,6 +219,15 @@ class CONTENT_EXPORT MediaWebContentsObserver : public WebContentsObserver {
       base::flat_map<MediaPlayerId,
                      mojo::AssociatedRemote<media::mojom::MediaPlayer>>;
 
+  // Communicates with the MediaSessionControllerManager to find or create (if
+  // needed) a MediaSessionController identified by |player_id|, in order to
+  // bind its mojo remote for media::mojom::MediaPlayer.
+  void OnMediaPlayerAdded(
+      mojo::PendingAssociatedRemote<media::mojom::MediaPlayer> player_remote,
+      mojo::PendingAssociatedReceiver<media::mojom::MediaPlayerObserver>
+          media_player_observer,
+      MediaPlayerId player_id);
+
   // Returns the PlayerInfo associated with |id|, or nullptr if no such
   // PlayerInfo exists.
   PlayerInfo* GetPlayerInfo(const MediaPlayerId& id) const;

content/browser/media/session/media_session_controller_unittest.cc

@@ -15,6 +15,7 @@
 #include "media/audio/audio_device_description.h"
 #include "media/mojo/mojom/media_player.mojom.h"
 #include "mojo/public/cpp/bindings/associated_receiver.h"
+#include "mojo/public/cpp/bindings/associated_remote.h"
 #include "mojo/public/cpp/bindings/pending_associated_remote.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -56,21 +57,34 @@ class FakeAudioFocusDelegate : public content::AudioFocusDelegate {
 // MediaSessionController instance owned by the test with a valid mojo remote,
 // that will be bound to the mojo receiver provided by this class instead of the
 // real one used in production which would be owned by HTMLMediaElement instead.
-class MockMediaPlayerReceiverForTesting : public media::mojom::MediaPlayer {
+class TestMediaPlayer : public media::mojom::MediaPlayer {
  public:
   enum class PauseRequestType {
     kNone,
     kTriggeredByUser,
     kNotTriggeredByUser,
   };
 
-  explicit MockMediaPlayerReceiverForTesting(
-      MediaWebContentsObserver* media_web_contents_observer,
-      const MediaPlayerId& player_id) {
+  TestMediaPlayer(MediaWebContentsObserver* media_web_contents_observer,
+                  const MediaPlayerId& player_id) {
     // Bind the remote to the receiver, so that we can intercept incoming
-    // messages sent via the different methods that use the remote.
-    media_web_contents_observer->OnMediaPlayerAdded(
-        receiver_.BindNewEndpointAndPassDedicatedRemote(), player_id);
+    // MediaPlayer messages sent via the different methods that use the remote.
+    // MediaPlayerObserver messages will not be used.
+    mojo::AssociatedRemote<media::mojom::MediaPlayerHost> player_host;
+    media_web_contents_observer->BindMediaPlayerHost(
+        player_id.frame_routing_id,
+        player_host.BindNewEndpointAndPassDedicatedReceiver());
+
+    mojo::PendingAssociatedRemote<media::mojom::MediaPlayer> player;
+    receiver_.Bind(player.InitWithNewEndpointAndPassReceiver());
+
+    mojo::PendingAssociatedRemote<media::mojom::MediaPlayerObserver>
+        dummy_player_observer;
+    player_host->OnMediaPlayerAdded(
+        std::move(player),
+        dummy_player_observer.InitWithNewEndpointAndPassReceiver(),
+        player_id.delegate_id);
+    player_host.FlushForTesting();
   }
 
   // Needs to be called from tests after invoking a method related playback
@@ -92,10 +106,6 @@ class MockMediaPlayerReceiverForTesting : public media::mojom::MediaPlayer {
   }
 
   // media::mojom::MediaPlayer implementation.
-  void AddMediaPlayerObserver(
-      mojo::PendingAssociatedRemote<media::mojom::MediaPlayerObserver>)
-      override {}
-
   void RequestPlay() override {
     received_play_ = true;
     run_loop_->Quit();
@@ -183,7 +193,7 @@ class MediaSessionControllerTest : public RenderViewHostImplTestHarness {
     id_ =
         MediaPlayerId(contents()->GetMainFrame()->GetGlobalFrameRoutingId(), 0);
     controller_ = CreateController();
-    media_player_receiver_ = CreateMediaPlayerReceiver(controller_.get());
+    media_player_ = CreateMediaPlayer(controller_.get());
 
     auto delegate = std::make_unique<FakeAudioFocusDelegate>();
     audio_focus_delegate_ = delegate.get();
@@ -194,7 +204,7 @@ class MediaSessionControllerTest : public RenderViewHostImplTestHarness {
     // Destruct the controller prior to any other teardown to avoid out of order
     // destruction relative to the MediaSession instance.
     controller_.reset();
-    media_player_receiver_.reset();
+    media_player_.reset();
 
     RenderViewHostImplTestHarness::TearDown();
   }
@@ -204,13 +214,12 @@ class MediaSessionControllerTest : public RenderViewHostImplTestHarness {
     return std::make_unique<MediaSessionController>(id_, contents());
   }
 
-  std::unique_ptr<MockMediaPlayerReceiverForTesting> CreateMediaPlayerReceiver(
+  std::unique_ptr<TestMediaPlayer> CreateMediaPlayer(
       MediaSessionController* controller) {
     MediaWebContentsObserver* media_web_contents_observer =
         contents()->media_web_contents_observer();
     DCHECK(media_web_contents_observer);
-    return std::make_unique<MockMediaPlayerReceiverForTesting>(
-        media_web_contents_observer, id_);
+    return std::make_unique<TestMediaPlayer>(media_web_contents_observer, id_);
   }
 
   MediaSessionImpl* media_session() {
@@ -223,72 +232,68 @@ class MediaSessionControllerTest : public RenderViewHostImplTestHarness {
 
   void Suspend() {
     controller_->OnSuspend(controller_->get_player_id_for_testing());
-    media_player_receiver_->WaitUntilReceivedMessage();
+    media_player_->WaitUntilReceivedMessage();
   }
 
   void Resume() {
     controller_->OnResume(controller_->get_player_id_for_testing());
-    media_player_receiver_->WaitUntilReceivedMessage();
+    media_player_->WaitUntilReceivedMessage();
   }
 
   void SeekForward(base::TimeDelta seek_time) {
     controller_->OnSeekForward(controller_->get_player_id_for_testing(),
                                seek_time);
-    media_player_receiver_->WaitUntilReceivedMessage();
+    media_player_->WaitUntilReceivedMessage();
   }
 
   void SeekBackward(base::TimeDelta seek_time) {
     controller_->OnSeekBackward(controller_->get_player_id_for_testing(),
                                 seek_time);
-    media_player_receiver_->WaitUntilReceivedMessage();
+    media_player_->WaitUntilReceivedMessage();
   }
 
   void SeekTo(base::TimeDelta seek_time) {
     controller_->OnSeekTo(controller_->get_player_id_for_testing(), seek_time);
-    media_player_receiver_->WaitUntilReceivedMessage();
+    media_player_->WaitUntilReceivedMessage();
   }
 
   void SetVolumeMultiplier(double multiplier) {
     controller_->OnSetVolumeMultiplier(controller_->get_player_id_for_testing(),
                                        multiplier);
-    media_player_receiver_->WaitUntilVolumeChanged();
+    media_player_->WaitUntilVolumeChanged();
   }
 
   // Helpers to check the results of using the basic controls.
-  bool ReceivedMessagePlay() { return media_player_receiver_->received_play(); }
+  bool ReceivedMessagePlay() { return media_player_->received_play(); }
 
   bool ReceivedMessagePause(bool triggered_by_user) {
-    MockMediaPlayerReceiverForTesting::PauseRequestType expected_pause_request =
-        triggered_by_user ? MockMediaPlayerReceiverForTesting::
-                                PauseRequestType::kTriggeredByUser
-                          : MockMediaPlayerReceiverForTesting::
-                                PauseRequestType::kNotTriggeredByUser;
-    return media_player_receiver_->received_pause() == expected_pause_request;
+    TestMediaPlayer::PauseRequestType expected_pause_request =
+        triggered_by_user
+            ? TestMediaPlayer::PauseRequestType::kTriggeredByUser
+            : TestMediaPlayer::PauseRequestType::kNotTriggeredByUser;
+    return media_player_->received_pause() == expected_pause_request;
   }
 
   bool ReceivedMessageSeekForward(base::TimeDelta expected_seek_time) {
-    return expected_seek_time ==
-           media_player_receiver_->received_seek_forward_time();
+    return expected_seek_time == media_player_->received_seek_forward_time();
   }
 
   bool ReceivedMessageSeekBackward(base::TimeDelta expected_seek_time) {
-    return expected_seek_time ==
-           media_player_receiver_->received_seek_backward_time();
+    return expected_seek_time == media_player_->received_seek_backward_time();
   }
 
   bool ReceivedMessageSeekTo(base::TimeDelta expected_seek_time) {
-    return expected_seek_time ==
-           media_player_receiver_->received_seek_to_time();
+    return expected_seek_time == media_player_->received_seek_to_time();
   }
 
   bool ReceivedMessageVolume(double expected_volume_multiplier) {
     return expected_volume_multiplier ==
-           media_player_receiver_->received_volume_multiplier();
+           media_player_->received_volume_multiplier();
   }
 
   MediaPlayerId id_ = MediaPlayerId::CreateMediaPlayerIdForTests();
   std::unique_ptr<MediaSessionController> controller_;
-  std::unique_ptr<MockMediaPlayerReceiverForTesting> media_player_receiver_;
+  std::unique_ptr<TestMediaPlayer> media_player_;
   FakeAudioFocusDelegate* audio_focus_delegate_ = nullptr;
 };
 
@@ -344,7 +349,7 @@ TEST_F(MediaSessionControllerTest, VolumeMultiplier) {
   EXPECT_TRUE(media_session()->IsControllable());
 
   // Upon creation of the MediaSession the default multiplier will be sent.
-  media_player_receiver_->WaitUntilVolumeChanged();
+  media_player_->WaitUntilVolumeChanged();
   EXPECT_TRUE(ReceivedMessageVolume(1.0));
 
   // Verify a different volume multiplier is sent.

content/browser/picture_in_picture/picture_in_picture_content_browsertest.cc

@@ -382,6 +382,19 @@ IN_PROC_BROWSER_TEST_F(MediaSessionPictureInPictureContentBrowserTest,
   WaitForPlaybackState(OverlayWindow::PlaybackState::kPaused);
 }
 
+IN_PROC_BROWSER_TEST_F(MediaSessionPictureInPictureContentBrowserTest,
+                       CanvasCaptureControlledByMediaSession) {
+  ASSERT_TRUE(embedded_test_server()->Start());
+  const GURL test_page_url = embedded_test_server()->GetURL(
+      "example.com", "/media/picture_in_picture/canvas-in-pip.html");
+  ASSERT_TRUE(NavigateToURL(shell(), test_page_url));
+  ASSERT_EQ(true, EvalJs(shell(), "start();"));
+  WaitForPlaybackState(OverlayWindow::PlaybackState::kPlaying);
+
+  window_controller()->TogglePlayPause();
+  WaitForPlaybackState(OverlayWindow::PlaybackState::kPaused);
+}
+
 class AutoPictureInPictureContentBrowserTest
     : public PictureInPictureContentBrowserTest {
  public:

content/browser/picture_in_picture/picture_in_picture_service_impl_unittest.cc

@@ -118,9 +118,6 @@ class PictureInPictureMediaPlayerReceiver : public media::mojom::MediaPlayer {
   }
 
   // media::mojom::MediaPlayer implementation.
-  void AddMediaPlayerObserver(
-      mojo::PendingAssociatedRemote<media::mojom::MediaPlayerObserver>)
-      override {}
   void RequestPlay() override {}
   void RequestPause(bool triggered_by_user) override {}
   void RequestSeekForward(base::TimeDelta seek_time) override {}

content/test/data/media/picture_in_picture/canvas-in-pip.html

@@ -0,0 +1,17 @@
+<!DOCTYPE html>
+<html>
+  <body />
+
+  <script>
+    const video = document.createElement('video');
+
+    async function start() {
+      const canvas = document.createElement('canvas');
+      canvas.getContext('2d').fillRect(0, 0, canvas.width, canvas.height);
+      video.srcObject = canvas.captureStream();
+      await video.play();
+      await video.requestPictureInPicture();
+      return true;
+    }
+  </script>
+</html>

media/mojo/mojom/media_player.mojom

@@ -11,12 +11,6 @@ import "ui/gfx/geometry/mojom/geometry.mojom";
 
 // Implemented by HTMLMediaElement in the renderer process.
 interface MediaPlayer {
-  // Sends |observer| to the renderer process so that it can be established a
-  // communication channel with the implementor of MediaPlayerObserver, and
-  // allows multiple observers for more observers like PictureInPictureService.
-  AddMediaPlayerObserver(
-      pending_associated_remote<MediaPlayerObserver> observer);
-
   // Requests the media player to start or resume media playback.
   RequestPlay();
 
@@ -113,6 +107,9 @@ interface MediaPlayerHost {
   // document owning the HTMLMediaElement that a new MediaPlayer is available,
   // passing a pending remote (i.e. |player_remote|) that will be used in the
   // browser process to establish a channel with the HTMLMediaElement.
+  // |observer| starts observing the MediaPlayer as soon as the player is
+  // created.
   OnMediaPlayerAdded(pending_associated_remote<MediaPlayer> player_remote,
+                     pending_associated_receiver<MediaPlayerObserver> observer,
                      int32 player_id);
 };