Don't resume paused media. Don't resume playing media after timeout.

media/blink/webmediaplayer_impl.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 "media/blink/webmediaplayer_impl.h"
 
 #include <algorithm>
 #include <cmath>
 #include <limits>
 #include <string>
@@ skipping 1095 matching lines @@
   opaque_ = opaque;
   // Modify content opaqueness of cc::Layer directly so that
   // SetContentsOpaqueIsFixed is ignored.
   if (video_weblayer_)
     video_weblayer_->layer()->SetContentsOpaque(opaque_);
 }
 
 void WebMediaPlayerImpl::OnHidden() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   UpdatePlayState();
+
+  // If the user doesn't come back to the suspended media in some time (chosen
+  // by dice roll) then pause the media so the user isn't surprised later.
+  if (pipeline_controller_.IsSuspended() && !paused_) {

[sandersd (OOO until July 31), 2016/06/08 23:16:27]

We shouldn't schedule this while casting either. Probably the timer callback
should be through a helper that checks the conditions, so that we don't need to
think about whether the conditions can change.

I'm also not sure if we want to be setting a |paused_time_| before
HAVE_METADATA.

It looks like playbackStateChanged() reads the paused state even before
HAVE_FUTURE_DATA, so it looks like any call (even without your changes) results
in the media becoming paused. Perhaps a larger bug?

[DaleCurtis, 2016/06/10 00:02:11]

Done. I don't understand your concerns about HAVE_METADATA though, we must have
reached that state to be in the playing state, so we should be fine to call
pause (which could have happened from JS anyways).

+    background_pause_timer_.Start(FROM_HERE, base::TimeDelta::FromSeconds(5),
+                                  this, &WebMediaPlayerImpl::OnPause);
+  }
 }
 
 void WebMediaPlayerImpl::OnShown() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   must_suspend_ = false;
+  background_pause_timer_.Stop();
   UpdatePlayState();
 }
 
 void WebMediaPlayerImpl::OnSuspendRequested(bool must_suspend) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
   if (must_suspend) {
     must_suspend_ = true;
   } else {
     // TODO(sandersd): Remove this when idleness is separate from play state.
@@ skipping 270 matching lines @@
   event.Wait();
   return video_frame;
 }
 
 void WebMediaPlayerImpl::UpdatePlayState() {
 #if defined(OS_ANDROID)  // WMPI_CAST
   bool is_remote = isRemote();
 #else
   bool is_remote = false;
 #endif
+  bool is_suspended = pipeline_controller_.IsSuspended();
   bool is_backgrounded =
       IsBackgroundedSuspendEnabled() && delegate_ && delegate_->IsHidden();
-  PlayState state =
-      UpdatePlayState_ComputePlayState(is_remote, is_backgrounded);
+  PlayState state = UpdatePlayState_ComputePlayState(is_remote, is_suspended,
+                                                     is_backgrounded);
   SetDelegateState(state.delegate_state);
   SetMemoryReportingState(state.is_memory_reporting_enabled);
   SetSuspendState(state.is_suspended || pending_suspend_resume_cycle_);
 }
 
 void WebMediaPlayerImpl::SetDelegateState(DelegateState new_state) {
   if (!delegate_ || delegate_state_ == new_state)
     return;
 
   delegate_state_ = new_state;
@@ skipping 70 matching lines @@
 
   if (is_suspended) {
     pipeline_controller_.Suspend();
   } else {
     pipeline_controller_.Resume();
   }
 }
 
 WebMediaPlayerImpl::PlayState
 WebMediaPlayerImpl::UpdatePlayState_ComputePlayState(bool is_remote,
+                                                     bool is_suspended,
                                                      bool is_backgrounded) {
   PlayState result;
 
   // This includes both data source (before pipeline startup) and pipeline
   // 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.
   bool have_future_data =
       highest_ready_state_ >= WebMediaPlayer::ReadyStateHaveFutureData;
 
-  // Background suspend is not enabled for audio-only players.
+  // Background suspend is not enabled for audio-only players unless paused,
+  // though in the case of audio-only the session should be kept.
   bool background_suspended = is_backgrounded && have_metadata && hasVideo();
+  bool background_audio_suspended = is_backgrounded && have_metadata && paused_;

[sandersd (OOO until July 31), 2016/06/08 23:16:27]

Should be |have_future_data| as a prerequisite for inspecting |paused_|.

To confirm: this will result in the delegate state becoming GONE for
backgrounded paused audio, so the system-level play control will disappear upon
backgrounding. Is that the intended behavior?

[DaleCurtis, 2016/06/10 00:02:11]

That's not what will happen since background_audio_suspended is not considered
in the has_session calculation, just the is_suspended one. I added tests
explicitly for this case.

 
   // Idle suspend is enabled once there is future data. We don't want to idle
   // suspend before that because play() may never be triggered to leave the idle
   // state. There could be other theoretical problems if the page is waiting for
   // other events before actually calling play(), but at least we don't break
   // Blink.
   //
   // TODO(sandersd): Make the delegate suspend idle players immediately when
   // hidden.
   // TODO(sandersd): If Blink told us the paused state sooner, we could
   // idle suspend sooner.
   bool idle_suspended = is_idle_ && have_future_data;
 
   // Combined suspend state.
-  result.is_suspended =
-      is_remote || must_suspend_ || idle_suspended || background_suspended;
+  result.is_suspended = is_remote || must_suspend_ || idle_suspended ||
+                        background_suspended || background_audio_suspended;
+
+  // If we're already suspended and the target state is unsuspended, check to
+  // see if we really must resume right now or can just wait for user action.
+  if (is_suspended && !result.is_suspended && paused_)

[sandersd (OOO until July 31), 2016/06/08 23:16:27]

Better to move all the conditions up so that the assignment is just
|result.is_suspended = true|. Perhaps there is a computation that can be pulled
out and shared below?

[DaleCurtis, 2016/06/10 00:02:11]

I prefer the current phrasing for readability, but will change if you feel
strongly.

[sandersd (OOO until July 31), 2016/06/10 01:15:09]

I feel strongly because the this version doesn't make it trivially obvious that
this will never unset |result.is_suspended|.

[DaleCurtis, 2016/06/10 18:20:31]

Done.

+    result.is_suspended = !seeking_ && have_future_data;
 
   // 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
   // case. For memory usage reporting, we just use the same definition (but we
   // don't have to).
   //
   // Similarly, we don't consider |ended_| to be paused. Blink will immediately
   // call pause() or seek(), so |ended_| should not affect the computation.
   // Despite that, |ended_| does result in a separate paused state, to simplfy
   // the contract for SetDelegateState().
@@ skipping 17 matching lines @@
       result.delegate_state = DelegateState::PAUSED_BUT_NOT_IDLE;
     } else if (ended_) {
       result.delegate_state = DelegateState::ENDED;
     } else {
       result.delegate_state = DelegateState::PAUSED;
     }
   } else {
     result.delegate_state = DelegateState::PLAYING;
   }
 
-  // It's not  critical if some cases where memory usage can change are missed,
+  // It's not critical if some cases where memory usage can change are missed,
   // since media memory changes are usually gradual.
   result.is_memory_reporting_enabled =
       can_play && !result.is_suspended && !paused_;
 
   return result;
 }
 
 void WebMediaPlayerImpl::ReportMemoryUsage() {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
 
@@ skipping 28 matching lines @@
            << ", Video: " << stats.video_memory_usage << ", DataSource: "
            << (data_source_ ? data_source_->GetMemoryUsage() : 0)
            << ", Demuxer: " << demuxer_memory_usage;
 
   const int64_t delta = current_memory_usage - last_reported_memory_usage_;
   last_reported_memory_usage_ = current_memory_usage;
   adjust_allocated_memory_cb_.Run(delta);
 }
 
 }  // namespace media