Pause EME player whose external surface is stolen by another player

content/browser/media/android/browser_media_player_manager.cc

Index: content/browser/media/android/browser_media_player_manager.cc
diff --git a/content/browser/media/android/browser_media_player_manager.cc b/content/browser/media/android/browser_media_player_manager.cc
index b8016b471f4cf4742db1d5cdf4c633a2752d0bb9..e451b2c5378402e2616eb5979ced03baf9b96e30 100644
--- a/content/browser/media/android/browser_media_player_manager.cc
+++ b/content/browser/media/android/browser_media_player_manager.cc
@@ -349,8 +349,15 @@ void BrowserMediaPlayerManager::AttachExternalVideoSurface(int player_id,
 
 void BrowserMediaPlayerManager::DetachExternalVideoSurface(int player_id) {
   MediaPlayerAndroid* player = GetPlayer(player_id);
-  if (player)
+  if (player) {
+    // If a player looses its external surface because
+    // it enters fullscreen, the video should not be paused.
+    if (player_id != fullscreen_player_id_) {
+      player->Pause(true);
+      Send(new MediaPlayerMsg_DidMediaPlayerPause(RoutingID(), player_id));
+    }
     player->SetVideoSurface(gfx::ScopedJavaSurface());
+  }
 }
 
 void BrowserMediaPlayerManager::OnFrameInfoUpdated() {
@@ -395,8 +402,13 @@ void BrowserMediaPlayerManager::OnRequestExternalSurface(
 void BrowserMediaPlayerManager::OnEnterFullscreen(int player_id) {
   DCHECK_EQ(fullscreen_player_id_, -1);
 #if defined(VIDEO_HOLE)
+  fullscreen_player_id_ = player_id;

[qinmin, 2014/11/10 18:37:24]

this logic is bad, we setting fullscreen_player_id_ because the
DetachExternalVideoSurface() will need that, it is hard for people to understand
this.

I would suggest add a player_id() function to external_video_surface_container_,
and call that function to check if the player_id is equal to the external id, if
so, do nothing. Otherwise, call pause() on that player, so we don't need the
change in DetachExternalVideoSurface().

[Hugo Holgersson, 2014/11/10 22:22:18]

You are right!

I used this logic: the external surface's player should be paused in two cases:
A. When another player enters fullscreen.
B. The "external surface" was manually released.
That was why I wanted to ensure we pause in DetachExternalVideoSurface(). If we
only do pause() here, we would miss case B.

Case B happens more often if we land: https://codereview.chromium.org/709003004/
which enforces only one "external surface" at any time.

+  // Avoid Z-fight: when any player goes
+  // fullscreen release any external surface.
   if (external_video_surface_container_)
-    external_video_surface_container_->ReleaseExternalVideoSurface(player_id);
+    external_video_surface_container_->ReleaseExternalVideoSurface(
+        ExternalVideoSurfaceContainer::kAnyPlayer);
+  fullscreen_player_id_ = -1;
 #endif  // defined(VIDEO_HOLE)
   if (video_view_.get()) {
     fullscreen_player_id_ = player_id;