[Video] Don't pause video-only elements if they're already paused...

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 d1f0b448102d3dcb861b90c76ab7e71195487153..fb081b50fae8f5ee7647a5be2c40ce74934f92d5 100644
--- a/media/blink/webmediaplayer_impl.cc
+++ b/media/blink/webmediaplayer_impl.cc
@@ -1121,6 +1121,10 @@ void WebMediaPlayerImpl::OnPipelineSeeked(bool time_updated) {
   // Reset underflow count upon seek; this prevents looping videos and user
   // actions from artificially inflating the underflow count.
   underflow_count_ = 0;
+
+  // Background video optimizations are delayed when shown/hidden if pipeline
+  // is seeking.
+  UpdateBackgroundVideoOptimizationState();
 }
 
 void WebMediaPlayerImpl::OnPipelineSuspended() {
@@ -1160,11 +1164,7 @@ void WebMediaPlayerImpl::OnBeforePipelineResume() {
 void WebMediaPlayerImpl::OnPipelineResumed() {
   is_pipeline_resuming_ = false;
 
-  if (IsHidden()) {
-    DisableVideoTrackIfNeeded();
-  } else {
-    EnableVideoTrackIfNeeded();
-  }
+  UpdateBackgroundVideoOptimizationState();
 }
 
 void WebMediaPlayerImpl::OnDemuxerOpened() {
@@ -1411,13 +1411,8 @@ void WebMediaPlayerImpl::OnFrameHidden() {
     watch_time_reporter_->OnHidden();
 
   if (ShouldPauseVideoWhenHidden()) {

[sandersd (OOO until July 31), 2017/01/20 22:57:00]

Should just call UpdateBackgroundVideoOptimizationState().

if (IsHidden())
  UpdateBackgroundVideoOptimizationState();

is a reasonable structure if we are being pedantic, but if anything it would
make more sense to use IsFrameHidden() there instead of OnHidden(); I don't
think we want undo the optimization on frame close.

[whywhat, 2017/01/20 23:24:45]

Hm, here we already know we're hidden.
We also want to return early in the pause case since UpdatePlayState is already
called.
I guess UpdateBVO could return a bool and we'd return if it's true.

[sandersd (OOO until July 31), 2017/01/20 23:29:42]

Ah I see, I missed the early return when comparing the two blocks of code. It's
not a big deal to run UpdatePlayState() twice, but this duplicate code isn't
going to make refactoring any harder.

I do want to point out that we don't actually know for sure here whether
IsHidden() is true. I don't think it's possible to get OnFrameHidden() after
OnFrameClosed(), but I don't think the API explicitly makes that guarantee.

-    if (!paused_when_hidden_) {
-      // OnPause() will set |paused_when_hidden_| to false and call
-      // UpdatePlayState(), so set the flag to true after and then return.
-      OnPause();
-      paused_when_hidden_ = true;
-      return;
-    }
+    PauseVideoIfNeeded();
+    return;
   } else {
     DisableVideoTrackIfNeeded();
   }
@@ -2182,9 +2177,35 @@ bool WebMediaPlayerImpl::IsBackgroundOptimizationCandidate() const {
          max_keyframe_distance_to_disable_background_video_;
 }
 
+void WebMediaPlayerImpl::UpdateBackgroundVideoOptimizationState() {
+  if (IsHidden()) {
+    if (ShouldPauseVideoWhenHidden())
+      PauseVideoIfNeeded();
+    else
+      DisableVideoTrackIfNeeded();
+  } else {
+    EnableVideoTrackIfNeeded();
+  }
+}
+
+void WebMediaPlayerImpl::PauseVideoIfNeeded() {
+  DCHECK(IsHidden());
+
+  // Don't pause video while the pipeline is stopped, resuming or seeking.
+  // Also if the video is paused already.
+  if (!pipeline_.IsRunning() || is_pipeline_resuming_ || seeking_ || paused_)
+    return;
+
+  // OnPause() will set |paused_when_hidden_| to false and call
+  // UpdatePlayState(), so set the flag to true after and then return.
+  OnPause();
+  paused_when_hidden_ = true;
+}
+
 void WebMediaPlayerImpl::EnableVideoTrackIfNeeded() {
-  // Don't change video track while the pipeline is resuming or seeking.
-  if (is_pipeline_resuming_ || seeking_)
+  // Don't change video track while the pipeline is stopped, resuming or
+  // seeking.
+  if (!pipeline_.IsRunning() || is_pipeline_resuming_ || seeking_)
     return;
 
   if (video_track_disabled_) {