Connect Java and native MediaStatusObservers

This CL adds the necessary plumbing to propagate MediaStatus updates
from Java to native code. All the layers between FlingingRenderer and
RemoteMediaPlayerWrapper are associated in a 1:1 capacity. This means
classes don't have to manage collections of observers and can keep
direct references instead.

Bug: 790766
Change-Id: I0e7b2fe061857a07f10149453b86ddb57dba46e5
Reviewed-on: https://chromium-review.googlesource.com/1147678
Commit-Queue: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Tommy Nyquist <nyquist@chromium.org>
Reviewed-by: Derek Cheng <imcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583112}

chrome/android/java/src/org/chromium/chrome/browser/media/remote/RemoteMediaPlayerWrapper.java

@@ -16,6 +16,8 @@ import org.chromium.base.Log;
 import org.chromium.chrome.browser.media.router.CastSessionUtil;
 import org.chromium.chrome.browser.media.router.FlingingController;
 import org.chromium.chrome.browser.media.router.MediaController;
+import org.chromium.chrome.browser.media.router.MediaStatusBridge;
+import org.chromium.chrome.browser.media.router.MediaStatusObserver;
 import org.chromium.chrome.browser.media.ui.MediaNotificationInfo;
 import org.chromium.chrome.browser.media.ui.MediaNotificationManager;
 
@@ -34,6 +36,7 @@ public class RemoteMediaPlayerWrapper implements RemoteMediaPlayer.OnMetadataUpd
     private GoogleApiClient mApiClient;
     private RemoteMediaPlayer mMediaPlayer;
     private MediaNotificationInfo.Builder mNotificationBuilder;
+    private MediaStatusObserver mMediaStatusObserver;
 
     public RemoteMediaPlayerWrapper(GoogleApiClient apiClient,
             MediaNotificationInfo.Builder notificationBuilder, CastDevice castDevice) {
@@ -63,6 +66,10 @@ public class RemoteMediaPlayerWrapper implements RemoteMediaPlayer.OnMetadataUpd
         MediaStatus mediaStatus = mMediaPlayer.getMediaStatus();
         if (mediaStatus == null) return;
 
+        if (mMediaStatusObserver != null) {
+            mMediaStatusObserver.onMediaStatusUpdate(new MediaStatusBridge(mediaStatus));
+        }
+
         int playerState = mediaStatus.getPlayerState();
         if (playerState == MediaStatus.PLAYER_STATE_PAUSED
                 || playerState == MediaStatus.PLAYER_STATE_PLAYING) {
@@ -220,4 +227,16 @@ public class RemoteMediaPlayerWrapper implements RemoteMediaPlayer.OnMetadataUpd
     public long getApproximateCurrentTime() {
         return mMediaPlayer.getApproximateStreamPosition();
     }
+
+    @Override
+    public void setMediaStatusObserver(MediaStatusObserver observer) {
+        assert mMediaStatusObserver == null;
+        mMediaStatusObserver = observer;
+    }
+
+    @Override
+    public void clearMediaStatusObserver() {
+        assert mMediaStatusObserver != null;
+        mMediaStatusObserver = null;
+    }
 }

chrome/android/java/src/org/chromium/chrome/browser/media/router/FlingingController.java

@@ -8,7 +8,6 @@ package org.chromium.chrome.browser.media.router;
  * Interface that groups all the necessary hooks to control media being flung to a Cast device,
  * as part of RemotePlayback.
  * This interface should be the same as media/base/flinging_controller.h.
- * TODO(tguilbert): add MediaStatusObserver.
  */
 public interface FlingingController {
     /**
@@ -16,6 +15,12 @@ public interface FlingingController {
      */
     public MediaController getMediaController();
 
+    /**
+     * Subscribe or unsubscribe to changes in the MediaStatus.
+     */
+    public void setMediaStatusObserver(MediaStatusObserver observer);
+    public void clearMediaStatusObserver();
+
     /**
      * Gets the current media time. Implementers may sacrifice precision in order to avoid a
      * round-trip query to Cast devices (see gms.cast.RemoteMediaPlayer's

chrome/android/java/src/org/chromium/chrome/browser/media/router/FlingingControllerBridge.java

@@ -13,8 +13,9 @@ import org.chromium.base.annotations.JNINamespace;
  * corresponding native code.
  */
 @JNINamespace("media_router")
-public class FlingingControllerBridge {
+public class FlingingControllerBridge implements MediaStatusObserver {
     private final FlingingController mFlingingController;
+    private long mNativeFlingingControllerBridge;
 
     public FlingingControllerBridge(FlingingController flingingController) {
         mFlingingController = flingingController;
@@ -44,4 +45,27 @@ public class FlingingControllerBridge {
     public void seek(long positionInMs) {
         mFlingingController.getMediaController().seek(positionInMs);
     }
+
+    // MediaStatusObserver implementation.
+    @Override
+    public void onMediaStatusUpdate(MediaStatusBridge status) {
+        if (mNativeFlingingControllerBridge != 0) {
+            nativeOnMediaStatusUpdated(mNativeFlingingControllerBridge, status);
+        }
+    }
+
+    @CalledByNative
+    public void addNativeFlingingController(long nativeFlingingControllerBridge) {
+        mNativeFlingingControllerBridge = nativeFlingingControllerBridge;
+        mFlingingController.setMediaStatusObserver(this);
+    }
+
+    @CalledByNative
+    public void clearNativeFlingingController() {
+        mFlingingController.clearMediaStatusObserver();
+        mNativeFlingingControllerBridge = 0;
+    }
+
+    private native void nativeOnMediaStatusUpdated(
+            long nativeFlingingControllerBridge, MediaStatusBridge status);
 }

chrome/browser/media/android/remote/flinging_controller_bridge.cc

@@ -58,11 +58,33 @@ media::MediaController* FlingingControllerBridge::GetMediaController() {
 
 void FlingingControllerBridge::AddMediaStatusObserver(
     media::MediaStatusObserver* observer) {
-  NOTREACHED();
+  DCHECK(!observer_);
+  observer_ = observer;
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  DCHECK(env);
+
+  Java_FlingingControllerBridge_addNativeFlingingController(
+      env, j_flinging_controller_bridge_, reinterpret_cast<intptr_t>(this));
 }
+
 void FlingingControllerBridge::RemoveMediaStatusObserver(
     media::MediaStatusObserver* observer) {
-  NOTREACHED();
+  DCHECK_EQ(observer_, observer);
+  observer_ = nullptr;
+
+  JNIEnv* env = base::android::AttachCurrentThread();
+  DCHECK(env);
+
+  Java_FlingingControllerBridge_clearNativeFlingingController(
+      env, j_flinging_controller_bridge_);
+}
+
+void FlingingControllerBridge::OnMediaStatusUpdated(
+    JNIEnv* env,
+    const base::android::JavaParamRef<jobject>& j_bridge,
+    const base::android::JavaParamRef<jobject>& j_status) {
+  // TODO(tguilbert): convert j_status to media::MediaStatus.
 }
 
 base::TimeDelta FlingingControllerBridge::GetApproximateCurrentTime() {

chrome/browser/media/android/remote/flinging_controller_bridge.h

@@ -33,10 +33,21 @@ class FlingingControllerBridge : public media::FlingingController,
   void SetVolume(float volume) override;
   void Seek(base::TimeDelta time) override;
 
+  // Called by the Java side on status updates.
+  void OnMediaStatusUpdated(
+      JNIEnv* env,
+      const base::android::JavaParamRef<jobject>& j_bridge,
+      const base::android::JavaParamRef<jobject>& j_status);
+
  private:
   // Java MediaControllerBridge instance.
   base::android::ScopedJavaGlobalRef<jobject> j_flinging_controller_bridge_;
 
+  // Observer to be notified of media status changes from the Java side.
+  // NOTE: We don't manage a collection of observers because FlingingRenderer is
+  // the only observer that subscribes to |this|, with a 1:1 relationship.
+  media::MediaStatusObserver* observer_ = nullptr;
+
   DISALLOW_COPY_AND_ASSIGN(FlingingControllerBridge);
 };
 

content/browser/media/flinging_renderer.cc

@@ -16,9 +16,13 @@ namespace content {
 
 FlingingRenderer::FlingingRenderer(
     std::unique_ptr<media::FlingingController> controller)
-    : controller_(std::move(controller)) {}
+    : controller_(std::move(controller)) {
+  controller_->AddMediaStatusObserver(this);
+}
 
-FlingingRenderer::~FlingingRenderer() = default;
+FlingingRenderer::~FlingingRenderer() {
+  controller_->RemoveMediaStatusObserver(this);
+};
 
 // static
 std::unique_ptr<FlingingRenderer> FlingingRenderer::Create(
@@ -98,4 +102,8 @@ base::TimeDelta FlingingRenderer::GetMediaTime() {
   return base::TimeDelta();
 }
 
+void FlingingRenderer::OnMediaStatusUpdated(const media::MediaStatus& status) {
+  // TODO(tguilbert): propagate important changes to RendererClient.
+}
+
 }  // namespace content

content/browser/media/flinging_renderer.h

@@ -9,6 +9,7 @@
 #include "content/common/content_export.h"
 #include "media/base/flinging_controller.h"
 #include "media/base/media_resource.h"
+#include "media/base/media_status_observer.h"
 #include "media/base/renderer.h"
 #include "media/base/renderer_client.h"
 #include "url/gurl.h"
@@ -23,7 +24,8 @@ class RenderFrameHost;
 // playback commands. In this case, the media we are controlling should be an
 // already existing RemotingCastSession, which should have been initiated by a
 // blink::RemotePlayback object, using the PresentationService.
-class CONTENT_EXPORT FlingingRenderer : public media::Renderer {
+class CONTENT_EXPORT FlingingRenderer : public media::Renderer,
+                                        media::MediaStatusObserver {
  public:
   // Helper method to create a FlingingRenderer from an already existing
   // presentation ID.
@@ -47,6 +49,9 @@ class CONTENT_EXPORT FlingingRenderer : public media::Renderer {
   void SetVolume(float volume) override;
   base::TimeDelta GetMediaTime() override;
 
+  // media::MediaStatusObserver implementation.
+  void OnMediaStatusUpdated(const media::MediaStatus& status) override;
+
  private:
   friend class FlingingRendererTest;
 

content/browser/media/flinging_renderer_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "content/browser/media/flinging_renderer.h"
 
+#include "base/memory/ptr_util.h"
 #include "base/time/time.h"
 #include "base/version.h"
 #include "media/base/media_controller.h"
@@ -44,14 +45,18 @@ class FlingingRendererTest : public testing::Test {
   FlingingRendererTest()
       : media_controller_(new StrictMock<MockMediaController>()),
         flinging_controller_(
-            new StrictMock<MockFlingingController>(media_controller_.get())),
-        renderer_(
-            std::unique_ptr<media::FlingingController>(flinging_controller_)) {}
+            new StrictMock<MockFlingingController>(media_controller_.get())) {
+    EXPECT_CALL(*flinging_controller_, AddMediaStatusObserver(_));
+    EXPECT_CALL(*flinging_controller_, RemoveMediaStatusObserver(_));
+
+    renderer_ = base::WrapUnique(new FlingingRenderer(
+        std::unique_ptr<media::FlingingController>(flinging_controller_)));
+  }
 
  protected:
   std::unique_ptr<MockMediaController> media_controller_;
   StrictMock<MockFlingingController>* flinging_controller_;
-  FlingingRenderer renderer_;
+  std::unique_ptr<FlingingRenderer> renderer_;
 };
 
 TEST_F(FlingingRendererTest, StartPlayingFromTime) {
@@ -59,28 +64,28 @@ TEST_F(FlingingRendererTest, StartPlayingFromTime) {
   EXPECT_CALL(*media_controller_, Play());
   EXPECT_CALL(*media_controller_, Seek(seek_time));
 
-  renderer_.StartPlayingFrom(seek_time);
+  renderer_->StartPlayingFrom(seek_time);
 }
 
 TEST_F(FlingingRendererTest, StartPlayingFromBeginning) {
   EXPECT_CALL(*media_controller_, Play());
   EXPECT_CALL(*media_controller_, Seek(base::TimeDelta()));
 
-  renderer_.StartPlayingFrom(base::TimeDelta());
+  renderer_->StartPlayingFrom(base::TimeDelta());
 }
 
 TEST_F(FlingingRendererTest, SetPlaybackRate) {
   double playback_rate = 1.0;
   EXPECT_CALL(*media_controller_, Play());
 
-  renderer_.SetPlaybackRate(playback_rate);
+  renderer_->SetPlaybackRate(playback_rate);
 }
 
 TEST_F(FlingingRendererTest, SetPlaybackRateToZero) {
   double playback_rate = 0.0;
   EXPECT_CALL(*media_controller_, Pause());
 
-  renderer_.SetPlaybackRate(playback_rate);
+  renderer_->SetPlaybackRate(playback_rate);
 }
 
 // Setting the volume to a positive value should not change the mute state.
@@ -89,7 +94,7 @@ TEST_F(FlingingRendererTest, SetVolume) {
   EXPECT_CALL(*media_controller_, SetVolume(volume));
   EXPECT_CALL(*media_controller_, SetMute(_)).Times(0);
 
-  renderer_.SetVolume(volume);
+  renderer_->SetVolume(volume);
 }
 
 // Setting the volume to 0 should not set the mute state.
@@ -98,7 +103,7 @@ TEST_F(FlingingRendererTest, SetVolumeToZero) {
   EXPECT_CALL(*media_controller_, SetVolume(volume));
   EXPECT_CALL(*media_controller_, SetMute(_)).Times(0);
 
-  renderer_.SetVolume(volume);
+  renderer_->SetVolume(volume);
 }
 
 }  // namespace content

media/base/media_status.h

@@ -6,6 +6,7 @@
 #define MEDIA_BASE_MEDIA_STATUS_H_
 
 #include "base/time/time.h"
+#include "media/base/media_export.h"
 
 namespace media {
 
@@ -13,7 +14,7 @@ namespace media {
 // interface. This is a copy of the media_router.mojom.MediaStatus interface,
 // without the cast specific portions.
 // TODO(https://crbug.com/820277): Deduplicate media_router::MediaStatus.
-struct MediaStatus {
+struct MEDIA_EXPORT MediaStatus {
  public:
   enum class PlayState { PLAYING, PAUSED, BUFFERING };
 

media/base/media_status_observer.h

@@ -9,9 +9,9 @@
 
 namespace media {
 
-// Describes the current state of media being controlled via the MediaController
-// interface. This is a copy of the media_router.mojom.MediaStatus interface,
-// without the cast specific portions.
+// Describes the current state of media being controlled via the
+// FlingingController interface. This is a copy of
+// media_router.mojom.MediaStatus interface, without the cast specific portions.
 // TODO(https://crbug.com/820277): Deduplicate media_router::MediaStatus.
 class MediaStatusObserver {
  public: