Fix accidental breakage of seeks resuming from suspend.

http://crrev.com/535822, while not enabled, accidentally broke cases
where a seek should resume the pipeline by completing the seek before
the Resume() call. This was partially documented and partially tested
behavior, but unfortunately neither was sufficient to avoid this issue
being introduced.

I've updated the tests and documentation for this functionality and
reworked how suspended start works; though it is still unlaunched on
current ToT.

This CL also adds some debugging logs to WebMediaPlayerImpl which I
find myself manually readding everytime I need to debug one of these
issues.

BUG=813573, 818554
TEST=manual, fixed existing unittest

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Ifa9c78726abf3e243cfeac8b983d41605c1c7bf2
Reviewed-on: https://chromium-review.googlesource.com/956326
Commit-Queue: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Dan Sanders <sandersd@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542346}

media/blink/webmediaplayer_impl.cc

@@ -2417,6 +2417,8 @@ void WebMediaPlayerImpl::UpdatePlayState() {
 void WebMediaPlayerImpl::SetDelegateState(DelegateState new_state,
                                           bool is_idle) {
   DCHECK(delegate_);
+  DVLOG(2) << __func__ << "(" << static_cast<int>(new_state) << ", " << is_idle
+           << ")";
 
   // Prevent duplicate delegate calls.
   // TODO(sandersd): Move this deduplication into the delegate itself.
@@ -2471,6 +2473,7 @@ void WebMediaPlayerImpl::SetMemoryReportingState(
 
 void WebMediaPlayerImpl::SetSuspendState(bool is_suspended) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
+  DVLOG(2) << __func__ << "(" << is_suspended << ")";
 
   // Do not change the state after an error has occurred.
   // TODO(sandersd): Update PipelineController to remove the need for this.
@@ -2539,6 +2542,15 @@ WebMediaPlayerImpl::UpdatePlayState_ComputePlayState(bool is_remote,
   result.is_suspended = is_remote || must_suspend || idle_suspended ||
                         background_suspended || can_stay_suspended;
 
+  DVLOG(3) << __func__ << ": is_remote=" << is_remote
+           << ", must_suspend=" << must_suspend
+           << ", idle_suspended=" << idle_suspended
+           << ", background_suspended=" << background_suspended
+           << ", can_stay_suspended=" << can_stay_suspended
+           << ", is_stale=" << is_stale
+           << ", have_future_data=" << have_future_data
+           << ", paused_=" << paused_ << ", seeking_=" << seeking_;
+
   // 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

media/filters/pipeline_controller.cc

@@ -49,6 +49,7 @@ void PipelineController::Start(Pipeline::StartType start_type,
 
   // Once the pipeline is started, we want to call the seeked callback but
   // without a time update.
+  pending_startup_ = true;
   pending_seeked_cb_ = true;
   state_ = State::STARTING;
 
@@ -57,7 +58,10 @@ void PipelineController::Start(Pipeline::StartType start_type,
   is_static_ = is_static;
   pipeline_->Start(start_type, demuxer, renderer_factory_cb_.Run(), client,
                    base::Bind(&PipelineController::OnPipelineStatus,
-                              weak_factory_.GetWeakPtr(), State::PLAYING));
+                              weak_factory_.GetWeakPtr(),
+                              start_type == Pipeline::StartType::kNormal
+                                  ? State::PLAYING
+                                  : State::PLAYING_OR_SUSPENDED));
 }
 
 void PipelineController::Seek(base::TimeDelta time, bool time_updated) {
@@ -131,6 +135,12 @@ void PipelineController::OnPipelineStatus(State expected_state,
   State old_state = state_;
   state_ = expected_state;
 
+  // Resolve ambiguity of the current state if we may have suspended in startup.
+  if (state_ == State::PLAYING_OR_SUSPENDED) {
+    waiting_for_seek_ = false;
+    state_ = pipeline_->IsSuspended() ? State::SUSPENDED : State::PLAYING;
+  }
+
   if (state_ == State::PLAYING) {
     // Start(), Seek(), or Resume() completed; we can be sure that
     // |demuxer_| got the seek it was waiting for.
@@ -142,9 +152,6 @@ void PipelineController::OnPipelineStatus(State expected_state,
       DCHECK(!pipeline_->IsSuspended());
       resumed_cb_.Run();
     }
-
-    if (old_state == State::STARTING && pipeline_->IsSuspended())
-      state_ = State::SUSPENDED;
   }
 
   if (state_ == State::SUSPENDED) {
@@ -239,19 +246,21 @@ void PipelineController::Dispatch() {
     return;
   }
 
-  // If |state_| is PLAYING or SUSPENDED and we didn't trigger an operation
-  // above then we are in a stable state. If there is a seeked callback pending,
-  // emit it.
-  if (state_ == State::PLAYING || state_ == State::SUSPENDED) {
-    if (pending_seeked_cb_) {
-      // |seeked_cb_| may be reentrant, so update state first and return
-      // immediately.
-      pending_seeked_cb_ = false;
-      bool was_pending_time_updated = pending_time_updated_;
-      pending_time_updated_ = false;
-      seeked_cb_.Run(was_pending_time_updated);
-      return;
-    }
+  // If |state_| is PLAYING and we didn't trigger an operation above then we
+  // are in a stable state. If there is a seeked callback pending, emit it.
+  //
+  // We also need to emit it if we completed suspended startup.
+  if (pending_seeked_cb_ &&
+      (state_ == State::PLAYING ||
+       (state_ == State::SUSPENDED && pending_startup_))) {
+    // |seeked_cb_| may be reentrant, so update state first and return
+    // immediately.
+    pending_startup_ = false;
+    pending_seeked_cb_ = false;
+    bool was_pending_time_updated = pending_time_updated_;
+    pending_time_updated_ = false;
+    seeked_cb_.Run(was_pending_time_updated);
+    return;
   }
 }
 

media/filters/pipeline_controller.h

@@ -33,6 +33,7 @@ class MEDIA_EXPORT PipelineController {
     STOPPED,
     STARTING,
     PLAYING,
+    PLAYING_OR_SUSPENDED,
     SEEKING,
     SUSPENDING,
     SUSPENDED,
@@ -85,6 +86,11 @@ class MEDIA_EXPORT PipelineController {
   // |seeked_cb| callback will also have |time_updated| set to true; it
   // indicates that the seek was requested by Blink and a time update is
   // expected so that Blink can fire the seeked event.
+  //
+  // Note: This will not resume the pipeline if it is in the suspended state; a
+  // call to Resume() is required. |seeked_cb_| will not be called until the
+  // later Resume() completes. The intention is to avoid unnecessary wake-ups
+  // for suspended players.
   void Seek(base::TimeDelta time, bool time_updated);
 
   // Request that |pipeline_| be suspended. This is a no-op if |pipeline_| has
@@ -190,6 +196,10 @@ class MEDIA_EXPORT PipelineController {
   bool pending_suspend_ = false;
   bool pending_resume_ = false;
 
+  // Set to true during Start(). Indicates that |seeked_cb_| must be fired once
+  // we've completed startup.
+  bool pending_startup_ = false;
+
   base::ThreadChecker thread_checker_;
   base::WeakPtrFactory<PipelineController> weak_factory_;
 

media/filters/pipeline_controller_unittest.cc

@@ -320,11 +320,16 @@ TEST_F(PipelineControllerTest, SeekMergesWithResume) {
   Complete(StartPipeline());
   Complete(SuspendPipeline());
 
+  // Pipeline startup always completes with a seek.
+  EXPECT_TRUE(was_seeked_);
+  was_seeked_ = false;
+
   // Request a seek while suspended.
   // It will be a mock failure if pipeline_.Seek() is called.
   base::TimeDelta seek_time = base::TimeDelta::FromSeconds(5);
   pipeline_controller_.Seek(seek_time, true);
   base::RunLoop().RunUntilIdle();
+  EXPECT_FALSE(was_seeked_);
 
   // Resume and verify the resume time includes the seek.
   Complete(ResumePipeline());