Clean up WebMediaPlayerAndroid needs_establish_peer_

content/renderer/media/android/webmediaplayer_android.cc

Index: content/renderer/media/android/webmediaplayer_android.cc
diff --git a/content/renderer/media/android/webmediaplayer_android.cc b/content/renderer/media/android/webmediaplayer_android.cc
index 67aa9b097a3ed7e1755f7f42697bd01a9ab5a5a3..50a135c8b5b4382f740ef420b256af4fbda1cb2b 100644
--- a/content/renderer/media/android/webmediaplayer_android.cc
+++ b/content/renderer/media/android/webmediaplayer_android.cc
@@ -134,7 +134,8 @@ WebMediaPlayerAndroid::WebMediaPlayerAndroid(
       texture_id_(0),
       stream_id_(0),
       is_playing_(false),
-      needs_establish_peer_(true),
+      peer_established_(false),
+      in_fullscreen_(false),
       stream_texture_proxy_initialized_(false),
       has_size_info_(false),
       stream_texture_factory_(factory),
@@ -162,7 +163,7 @@ WebMediaPlayerAndroid::WebMediaPlayerAndroid(
   if (force_use_overlay_embedded_video_ ||
     player_manager_->ShouldUseVideoOverlayForEmbeddedEncryptedVideo()) {
     // Defer stream texture creation until we are sure it's necessary.
-    needs_establish_peer_ = false;
+    needs_external_surface_ = true;

[qinmin, 2014/09/09 06:39:18]

why set this to true? if the video is not EME, no external surface is needed.
As mentioned by the comments above, it is not determined whether a stream
texture is needed or not.

[boliu, 2014/09/09 21:17:04]

Done.

     current_frame_ = VideoFrame::CreateBlackFrame(gfx::Size(1, 1));
   }
 #endif  // defined(VIDEO_HOLE)
@@ -303,18 +304,11 @@ void WebMediaPlayerAndroid::play() {
 #if defined(VIDEO_HOLE)
   if ((hasVideo() || IsHLSStream()) && needs_external_surface_ &&
       !player_manager_->IsInFullscreen(frame_)) {
-    DCHECK(!needs_establish_peer_);
     player_manager_->RequestExternalSurface(player_id_, last_computed_rect_);
   }
 #endif  // defined(VIDEO_HOLE)
 
   TryCreateStreamTextureProxyIfNeeded();
-  // There is no need to establish the surface texture peer for fullscreen
-  // video.
-  if ((hasVideo() || IsHLSStream()) && needs_establish_peer_ &&
-      !player_manager_->IsInFullscreen(frame_)) {
-    EstablishSurfaceTexturePeer();
-  }
 
   if (paused())
     player_manager_->Start(player_id_);
@@ -820,25 +814,19 @@ void WebMediaPlayerAndroid::OnVideoSizeChanged(int width, int height) {
   if (force_use_overlay_embedded_video_ ||
       (media_source_delegate_ && media_source_delegate_->IsVideoEncrypted() &&
        player_manager_->ShouldUseVideoOverlayForEmbeddedEncryptedVideo())) {
-    needs_external_surface_ = true;
     if (!paused() && !player_manager_->IsInFullscreen(frame_))
       player_manager_->RequestExternalSurface(player_id_, last_computed_rect_);
-  } else if (stream_texture_proxy_ && !stream_id_) {
-    // Do deferred stream texture creation finally.
-    DoCreateStreamTexture();
-    SetNeedsEstablishPeer(true);
   }
 #endif  // defined(VIDEO_HOLE)
   natural_size_.width = width;
   natural_size_.height = height;
 
   // When play() gets called, |natural_size_| may still be empty and
-  // EstablishSurfaceTexturePeer() will not get called. As a result, the video
-  // may play without a surface texture. When we finally get the valid video
-  // size here, we should call EstablishSurfaceTexturePeer() if it has not been
-  // previously called.
-  if (!paused() && needs_establish_peer_)
-    EstablishSurfaceTexturePeer();
+  // EstablishSurfaceTexturePeerIfNeeded() will not get called. As a result, the

[qinmin, 2014/09/09 06:39:18]

when play is called, isn't updatePlayingState() always call into
EstablishSurfaceTexturePeerIfNeeded()?

[boliu, 2014/09/09 21:17:04]

Done.

+  // video may play without a surface texture. When we finally get the valid
+  // video size here, we should call EstablishSurfaceTexturePeerIfNeeded() if
+  // it has not been previously called.
+  EstablishSurfaceTexturePeerIfNeeded();
 
   ReallocateVideoFrame();
 
@@ -876,32 +864,28 @@ void WebMediaPlayerAndroid::OnConnectedToRemoteDevice(
   DCHECK(!media_source_delegate_);
   DrawRemotePlaybackText(remote_playback_message);
   is_remote_ = true;
-  SetNeedsEstablishPeer(false);
+  peer_established_ = false;
 }
 
 void WebMediaPlayerAndroid::OnDisconnectedFromRemoteDevice() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   DCHECK(!media_source_delegate_);
-  SetNeedsEstablishPeer(true);
-  if (!paused())
-    EstablishSurfaceTexturePeer();
   is_remote_ = false;
+  EstablishSurfaceTexturePeerIfNeeded();
   ReallocateVideoFrame();
 }
 
 void WebMediaPlayerAndroid::OnDidEnterFullscreen() {
   if (!player_manager_->IsInFullscreen(frame_))
     player_manager_->DidEnterFullscreen(frame_);
+  in_fullscreen_ = true;
+  peer_established_ = false;

[qinmin, 2014/09/09 06:39:18]

Sorry, I was confused with this call and the
FullscreenController::DidEnterFullscreen() earlier.

your change will cause racing issue. If EnterFullscreen() is called and we don't
set peer_established_, then later on a play() call will find that it should
establish the surface texture peer. So now the fullscreen surface will compete
with the surface texture in the browser process to bind to the media player.

[boliu, 2014/09/09 21:17:04]

Done.

 }
 
 void WebMediaPlayerAndroid::OnDidExitFullscreen() {
   // |needs_external_surface_| is always false on non-TV devices.

[qinmin, 2014/09/09 06:39:18]

why removing the if statement? And why not removing the comment?

[boliu, 2014/09/09 21:17:04]

Done.

-  if (!needs_external_surface_)
-    SetNeedsEstablishPeer(true);
-  // We had the fullscreen surface connected to Android MediaPlayer,
-  // so reconnect our surface texture for embedded playback.
-  if (!paused() && needs_establish_peer_)
-    EstablishSurfaceTexturePeer();
+  in_fullscreen_ = false;
+  EstablishSurfaceTexturePeerIfNeeded();
 
 #if defined(VIDEO_HOLE)
   if (!paused() && needs_external_surface_)
@@ -963,9 +947,7 @@ void WebMediaPlayerAndroid::UpdateReadyState(
 }
 
 void WebMediaPlayerAndroid::OnPlayerReleased() {
-  // |needs_external_surface_| is always false on non-TV devices.
-  if (!needs_external_surface_)
-    needs_establish_peer_ = true;
+  peer_established_ = false;  // Established when this plays.
 
   if (is_playing_)
     OnMediaPlayerPause();
@@ -995,8 +977,7 @@ void WebMediaPlayerAndroid::ReleaseMediaResources() {
       break;
   }
   player_manager_->ReleaseResources(player_id_);
-  if (!needs_external_surface_)
-    SetNeedsEstablishPeer(true);
+  peer_established_ = false;  // Established when this plays.
 }
 
 void WebMediaPlayerAndroid::OnDestruct() {
@@ -1240,31 +1221,13 @@ void WebMediaPlayerAndroid::TryCreateStreamTextureProxyIfNeeded() {
     return;
 
   stream_texture_proxy_.reset(stream_texture_factory_->CreateProxy());
-  if (needs_establish_peer_ && stream_texture_proxy_) {
+  if (stream_texture_proxy_) {

[qinmin, 2014/09/09 06:39:18]

why we remove the if condition? if we use external surface, we don't need to
create stream texture for now

[boliu, 2014/09/09 21:17:04]

Done.

     DoCreateStreamTexture();
     ReallocateVideoFrame();
-  }
-
-  if (stream_texture_proxy_ && video_frame_provider_client_)
-    stream_texture_proxy_->SetClient(video_frame_provider_client_);
-}
-
-void WebMediaPlayerAndroid::EstablishSurfaceTexturePeer() {
-  DCHECK(main_thread_checker_.CalledOnValidThread());
-  if (!stream_texture_proxy_)
-    return;
-
-  if (stream_texture_factory_.get() && stream_id_)
-    stream_texture_factory_->EstablishPeer(stream_id_, player_id_);
 
-  // Set the deferred size because the size was changed in remote mode.
-  if (!is_remote_ && cached_stream_texture_size_ != natural_size_) {
-    stream_texture_factory_->SetStreamTextureSize(
-        stream_id_, gfx::Size(natural_size_.width, natural_size_.height));
-    cached_stream_texture_size_ = natural_size_;
+    if (video_frame_provider_client_)
+      stream_texture_proxy_->SetClient(video_frame_provider_client_);
   }
-
-  needs_establish_peer_ = false;
 }
 
 void WebMediaPlayerAndroid::DoCreateStreamTexture() {
@@ -1275,8 +1238,21 @@ void WebMediaPlayerAndroid::DoCreateStreamTexture() {
       kGLTextureExternalOES, &texture_id_, &texture_mailbox_);
 }
 
-void WebMediaPlayerAndroid::SetNeedsEstablishPeer(bool needs_establish_peer) {
-  needs_establish_peer_ = needs_establish_peer;
+void WebMediaPlayerAndroid::EstablishSurfaceTexturePeerIfNeeded() {
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  if (peer_established_ || in_fullscreen_ || needs_external_surface_ ||
+      is_remote_ || !is_playing_ || !stream_texture_proxy_ ||
+      (!hasVideo() && !IsHLSStream())) {
+    return;
+  }
+
+  stream_texture_factory_->EstablishPeer(stream_id_, player_id_);
+  if (cached_stream_texture_size_ != natural_size_) {
+    stream_texture_factory_->SetStreamTextureSize(
+        stream_id_, gfx::Size(natural_size_.width, natural_size_.height));
+    cached_stream_texture_size_ = natural_size_;
+  }
+  peer_established_ = true;
 }
 
 void WebMediaPlayerAndroid::setPoster(const blink::WebURL& poster) {
@@ -1285,6 +1261,7 @@ void WebMediaPlayerAndroid::setPoster(const blink::WebURL& poster) {
 
 void WebMediaPlayerAndroid::UpdatePlayingState(bool is_playing) {
   is_playing_ = is_playing;
+  EstablishSurfaceTexturePeerIfNeeded();

[qinmin, 2014/09/09 06:39:18]

UpdatePlayingState() can be called by 4 different functions, and only play()
actually requires the surface texture to be established. So why not simply put
it in play()?

[boliu, 2014/09/09 21:17:04]

Because contract is any states change should lead to calling
EstablishSurfaceTexturePeerIfNeeded. So I also put it in enterFullScreen.

   if (!delegate_)
     return;
   if (is_playing)
@@ -1754,7 +1731,6 @@ void WebMediaPlayerAndroid::SetDecryptorReadyCB(
 void WebMediaPlayerAndroid::enterFullscreen() {
   if (player_manager_->CanEnterFullscreen(frame_)) {
     player_manager_->EnterFullscreen(player_id_, frame_);
-    SetNeedsEstablishPeer(false);
   }
 }