Clean up WebMediaPlayerAndroid needs_establish_peer_

content/renderer/media/android/webmediaplayer_android.cc

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/media/android/webmediaplayer_android.h"
 
 #include <limits>
 
 #include "base/android/build_info.h"
 #include "base/bind.h"
@@ skipping 116 matching lines @@
       pending_seek_(false),
       seeking_(false),
       did_loading_progress_(false),
       player_manager_(player_manager),
       cdm_manager_(cdm_manager),
       network_state_(WebMediaPlayer::NetworkStateEmpty),
       ready_state_(WebMediaPlayer::ReadyStateHaveNothing),
       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),
       needs_external_surface_(false),
       video_frame_provider_client_(NULL),
       pending_playback_(false),
       player_type_(MEDIA_PLAYER_TYPE_URL),
       current_time_(0),
       is_remote_(false),
       media_log_(media_log),
       web_cdm_(NULL),
       allow_stored_credentials_(false),
       is_local_resource_(false),
       weak_factory_(this) {
   DCHECK(player_manager_);
   DCHECK(cdm_manager_);
 
   DCHECK(main_thread_checker_.CalledOnValidThread());
 
   player_id_ = player_manager_->RegisterMediaPlayer(this);
 
 #if defined(VIDEO_HOLE)
   force_use_overlay_embedded_video_ = CommandLine::ForCurrentProcess()->
       HasSwitch(switches::kForceUseOverlayEmbeddedVideo);
   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)
   TryCreateStreamTextureProxyIfNeeded();
 }
 
 WebMediaPlayerAndroid::~WebMediaPlayerAndroid() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   SetVideoFrameProviderClient(NULL);
   client_->setWebLayer(NULL);
@@ skipping 120 matching lines @@
 }
 
 void WebMediaPlayerAndroid::play() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
 
   // For HLS streams, some devices cannot detect the video size unless a surface
   // texture is bind to it. See http://crbug.com/400145.
 #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_);
   UpdatePlayingState(true);
   UpdateNetworkState(WebMediaPlayer::NetworkStateLoading);
 }
 
 void WebMediaPlayerAndroid::pause() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   Pause(true);
@@ skipping 485 matching lines @@
   has_size_info_ = true;
   if (natural_size_.width == width && natural_size_.height == height)
     return;
 
 #if defined(VIDEO_HOLE)
   // Use H/W surface for encrypted video.
   // TODO(qinmin): Change this so that only EME needs the H/W surface
   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();
 
   // For hidden video element (with style "display:none"), ensure the texture
   // size is set.
   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_;
   }
@@ skipping 17 matching lines @@
   if (is_local_resource_ && current_time_ <= duration())
     buffered_[0].end = current_time_;
 }
 
 void WebMediaPlayerAndroid::OnConnectedToRemoteDevice(
     const std::string& remote_playback_message) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   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_)
     player_manager_->RequestExternalSurface(player_id_, last_computed_rect_);
 #endif  // defined(VIDEO_HOLE)
 
   player_manager_->DidExitFullscreen();
   client_->repaint();
 }
 
@@ skipping 41 matching lines @@
   client_->networkStateChanged();
 }
 
 void WebMediaPlayerAndroid::UpdateReadyState(
     WebMediaPlayer::ReadyState state) {
   ready_state_ = state;
   client_->readyStateChanged();
 }
 
 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();
 
 #if defined(VIDEO_HOLE)
   last_computed_rect_ = gfx::RectF();
 #endif  // defined(VIDEO_HOLE)
 }
 
 void WebMediaPlayerAndroid::ReleaseMediaResources() {
   switch (network_state_) {
     // Pause the media player and inform WebKit if the player is in a good
     // shape.
     case WebMediaPlayer::NetworkStateIdle:
     case WebMediaPlayer::NetworkStateLoading:
     case WebMediaPlayer::NetworkStateLoaded:
       Pause(false);
       client_->playbackStateChanged();
       break;
     // If a WebMediaPlayer instance has entered into one of these states,
     // the internal network state in HTMLMediaElement could be set to empty.
     // And calling playbackStateChanged() could get this object deleted.
     case WebMediaPlayer::NetworkStateEmpty:
     case WebMediaPlayer::NetworkStateFormatError:
     case WebMediaPlayer::NetworkStateNetworkError:
     case WebMediaPlayer::NetworkStateDecodeError:
       break;
   }
   player_manager_->ReleaseResources(player_id_);
-  if (!needs_external_surface_)
-    SetNeedsEstablishPeer(true);
+  peer_established_ = false;  // Established when this plays.
 }
 
 void WebMediaPlayerAndroid::OnDestruct() {
   NOTREACHED() << "WebMediaPlayer should be destroyed before any "
                   "RenderFrameObserver::OnDestruct() gets called when "
                   "the RenderFrame goes away.";
 }
 
 void WebMediaPlayerAndroid::InitializePlayer(
     const GURL& url,
@@ skipping 223 matching lines @@
   DCHECK(main_thread_checker_.CalledOnValidThread());
   // Already created.
   if (stream_texture_proxy_)
     return;
 
   // No factory to create proxy.
   if (!stream_texture_factory_)
     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 (video_frame_provider_client_)
+      stream_texture_proxy_->SetClient(video_frame_provider_client_);
   }
-
-  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_;
-  }
-
-  needs_establish_peer_ = false;
 }
 
 void WebMediaPlayerAndroid::DoCreateStreamTexture() {
   DCHECK(main_thread_checker_.CalledOnValidThread());
   DCHECK(!stream_id_);
   DCHECK(!texture_id_);
   stream_id_ = stream_texture_factory_->CreateStreamTexture(
       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) {
   player_manager_->SetPoster(player_id_, 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)
     delegate_->DidPlay(this);
   else
     delegate_->DidPause(this);
 }
 
 #if defined(VIDEO_HOLE)
 bool WebMediaPlayerAndroid::UpdateBoundaryRectangle() {
@@ skipping 449 matching lines @@
     decryptor_ready_cb.Run(web_cdm_->GetDecryptor(), base::Bind(DoNothing));
     return;
   }
 
   decryptor_ready_cb_ = decryptor_ready_cb;
 }
 
 void WebMediaPlayerAndroid::enterFullscreen() {
   if (player_manager_->CanEnterFullscreen(frame_)) {
     player_manager_->EnterFullscreen(player_id_, frame_);
-    SetNeedsEstablishPeer(false);
   }
 }
 
 bool WebMediaPlayerAndroid::canEnterFullscreen() const {
   return player_manager_->CanEnterFullscreen(frame_);
 }
 
 bool WebMediaPlayerAndroid::IsHLSStream() const {
   std::string mime;
   GURL url = redirected_url_.is_empty() ? url_ : redirected_url_;
   if (!net::GetMimeTypeFromFile(base::FilePath(url.path()), &mime))
     return false;
   return !mime.compare("application/x-mpegurl");
 }
 
 }  // namespace content