[Video] MediaSession API event handlers can resume background video.

media/blink/webmediaplayer_impl.cc

Index: media/blink/webmediaplayer_impl.cc
diff --git a/media/blink/webmediaplayer_impl.cc b/media/blink/webmediaplayer_impl.cc
index bea9a92a34c679b3dc60456d9d76b9d03d7d1536..befdcd4953e2d6aa3c4677382080ceb491c130e1 100644
--- a/media/blink/webmediaplayer_impl.cc
+++ b/media/blink/webmediaplayer_impl.cc
@@ -121,10 +121,6 @@ bool IsBackgroundedSuspendEnabled() {
 #endif
 }
 
-bool IsResumeBackgroundVideosEnabled() {
-  return base::FeatureList::IsEnabled(kResumeBackgroundVideo);
-}
-
 bool IsBackgroundVideoTrackOptimizationEnabled() {
   return base::FeatureList::IsEnabled(kBackgroundVideoTrackOptimization);
 }
@@ -424,6 +420,10 @@ void WebMediaPlayerImpl::play() {
     return;
   }
 #endif
+
+  if (IsHidden() && ShouldPauseVideoWhenHidden())
+    return;
+
   // TODO(sandersd): Do we want to reset the idle timer here?
   delegate_->SetIdle(delegate_id_, false);
   paused_ = false;
@@ -1437,17 +1437,7 @@ void WebMediaPlayerImpl::OnFrameHidden() {
   if (watch_time_reporter_)
     watch_time_reporter_->OnHidden();
 
-  // OnFrameHidden() can be called when frame is closed, then IsHidden() will
-  // return false, so check explicitly.
-  if (IsHidden()) {
-    if (ShouldPauseVideoWhenHidden()) {
-      PauseVideoIfNeeded();
-      return;
-    } else {
-      DisableVideoTrackIfNeeded();
-    }
-  }
-
+  UpdateBackgroundVideoOptimizationState();
   UpdatePlayState();
 
   // Schedule suspended playing media to be paused if the user doesn't come back
@@ -1484,8 +1474,7 @@ void WebMediaPlayerImpl::OnFrameShown() {
 
   if (paused_when_hidden_) {
     paused_when_hidden_ = false;
-    OnPlay();  // Calls UpdatePlayState() so return afterwards.

[DaleCurtis, 2017/02/16 02:47:38]

Comment is still true, so we end up updating play state twice here?

[whywhat, 2017/02/16 13:19:31]

Hm, yes, otherwise on Android we may not reenable the video track.
Same as in OnFrameHidden above (UpdateBgVideoOptimizationState may call
OnPause()) but then we do want to schedule the pause timer.
UPS is supposed to be reenterable IIRC and I'm relying on that here instead of
extracting the common bits somehow so that UPS is called only once and only when
needed.

Let me check if I can simply:

EnableVideoTrackIfNeeded();
if (paused_when_hidden_) {
  paused_when_hidden_ = false;
  OnPlay();
} else {
  UpdatePlayState();
}

-    return;
+    OnPlay();
   }
 
   EnableVideoTrackIfNeeded();
@@ -1954,25 +1943,14 @@ WebMediaPlayerImpl::UpdatePlayState_ComputePlayState(bool is_remote,
   // errors.
   bool has_error = IsNetworkStateError(network_state_);
 
-  // After HaveMetadata, we know which tracks are present and the duration.
-  bool have_metadata = ready_state_ >= WebMediaPlayer::ReadyStateHaveMetadata;
-
   // After HaveFutureData, Blink will call play() if the state is not paused;
   // prior to this point |paused_| is not accurate.
   bool have_future_data =
       highest_ready_state_ >= WebMediaPlayer::ReadyStateHaveFutureData;
 
-  // Background suspend is not enabled for audio-only players unless paused,
-  // though in the case of audio-only the session should be kept.
-  // Videos are not suspended if the user resumed the playback via the remote
-  // controls earlier and it's still playing.
-  bool is_backgrounded_video = is_backgrounded && have_metadata && hasVideo();
-  bool can_play_backgrounded = is_backgrounded_video && !is_remote &&
-                               hasAudio() && IsResumeBackgroundVideosEnabled();
-  bool is_background_playing = delegate_->IsBackgroundVideoPlaybackUnlocked();
-  bool background_suspended = can_auto_suspend && is_backgrounded_video &&
-                              !(can_play_backgrounded && is_background_playing);
-  bool background_pause_suspended =
+  // Background suspend is only enabled for paused players.
+  // In the case of players with audio the session should be kept.
+  bool background_suspended =
       can_auto_suspend && is_backgrounded && paused_ && have_future_data;
 
   // Idle suspension is allowed prior to have future data since there exist
@@ -1992,8 +1970,7 @@ WebMediaPlayerImpl::UpdatePlayState_ComputePlayState(bool is_remote,
 
   // Combined suspend state.
   result.is_suspended = is_remote || must_suspend || idle_suspended ||
-                        background_suspended || background_pause_suspended ||
-                        can_stay_suspended;
+                        background_suspended || can_stay_suspended;
 
   // We do not treat |playback_rate_| == 0 as paused. For the media session,
   // being paused implies displaying a play button, which is incorrect in this
@@ -2015,20 +1992,15 @@ WebMediaPlayerImpl::UpdatePlayState_ComputePlayState(bool is_remote,
   // TODO(sandersd): If Blink told us the paused state sooner, we could create
   // the media session sooner.
   bool can_play = !has_error && !is_remote && have_future_data;
-  bool has_session_playing = can_play && !must_suspend && !background_suspended;
-
-  // |has_session_suspended| means the player is suspended from the media
-  // element point of view but paused and can be resumed from the delegate point
-  // of view. Therefore it behaves like |paused_| for the delegate.
-  bool has_session_suspended = can_play && !must_suspend &&
-                               background_suspended && can_play_backgrounded;
-
-  bool has_session = has_session_playing || has_session_suspended;
+  bool has_session =
+      can_play && !must_suspend && hasAudio() &&
+      (!hasVideo() || !is_backgrounded || !IsBackgroundedSuspendEnabled() ||
+       base::FeatureList::IsEnabled(kResumeBackgroundVideo));
 
   if (!has_session) {
     result.delegate_state = DelegateState::GONE;
     result.is_idle = delegate_->IsIdle(delegate_id_);
-  } else if (paused_ || has_session_suspended) {
+  } else if (paused_) {
     // TODO(sandersd): Is it possible to have a suspended session, be ended,
     // and not be paused? If so we should be in a PLAYING state.
     result.delegate_state =
@@ -2106,8 +2078,9 @@ void WebMediaPlayerImpl::FinishMemoryUsageReport(int64_t demuxer_memory_usage) {
 }
 
 void WebMediaPlayerImpl::ScheduleIdlePauseTimer() {
-  // Only schedule the pause timer if we're playing and are suspended.
-  if (paused_ || !pipeline_controller_.IsSuspended())
+  // Only schedule the pause timer if we're not paused or paused but going to
+  // resume when foregrounded, and are suspended.
+  if ((paused_ && !paused_when_hidden_) || !pipeline_controller_.IsSuspended())
     return;
 
 #if defined(OS_ANDROID)
@@ -2155,14 +2128,25 @@ void WebMediaPlayerImpl::ActivateViewportIntersectionMonitoring(bool activate) {
 }
 
 bool WebMediaPlayerImpl::ShouldPauseVideoWhenHidden() const {
-#if !defined(OS_ANDROID)
-  // On desktop, this behavior is behind the feature flag.
-  if (!IsBackgroundVideoTrackOptimizationEnabled())
-    return false;
+  if (IsBackgroundedSuspendEnabled()) {
+    if (!hasVideo())
+      return false;
+
+#if defined(OS_ANDROID)
+    if (isRemote())
+      return false;
 #endif
 
-  // Pause video-only players that match the criteria for being optimized.
-  return !hasAudio() && IsBackgroundOptimizationCandidate();
+    if (hasAudio() && delegate_->IsBackgroundVideoPlaybackUnlocked())
+      return false;
+
+    return true;
+  }
+
+  // Otherwise only pause if the optimization is on and it's a video-only
+  // optimization candidate.
+  return IsBackgroundVideoTrackOptimizationEnabled() && !hasAudio() &&
+         IsBackgroundOptimizationCandidate();
 }
 
 bool WebMediaPlayerImpl::ShouldDisableVideoWhenHidden() const {
@@ -2293,7 +2277,6 @@ void WebMediaPlayerImpl::ReportTimeFromForegroundToFirstFrame(
                         time_to_first_frame);
   }
 }
-
 void WebMediaPlayerImpl::SwitchRenderer(bool disable_pipeline_auto_suspend) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   disable_pipeline_auto_suspend_ = disable_pipeline_auto_suspend;