Make PipelineImpl state change tasks consistent.

media/base/pipeline_impl.cc

Index: media/base/pipeline_impl.cc
diff --git a/media/base/pipeline_impl.cc b/media/base/pipeline_impl.cc
index ddbd0cea8e372c56a6be4492630e9bd0883d0b87..fc2b0eb5ba12f8a0030c4e0ca3245259e434ffeb 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -119,11 +119,9 @@ class PipelineImpl::RendererWrapper : public DemuxerHost,
   void CheckPlaybackEnded();
 
   // State transition tasks.
-  void DoSeek(base::TimeDelta seek_timestamp, const PipelineStatusCB& done_cb);
-  void DoStop(const base::Closure& done_cb);
   void SetState(State next_state);
-  State GetNextState() const;
-  void StateTransitionTask(PipelineStatus status);
+  void CompleteSeek(base::TimeDelta seek_time, PipelineStatus status);
+  void CompleteSuspend(PipelineStatus status);
   void InitializeDemuxer(const PipelineStatusCB& done_cb);
   void InitializeRenderer(const PipelineStatusCB& done_cb);
   void DestroyRenderer();
@@ -155,10 +153,6 @@ class PipelineImpl::RendererWrapper : public DemuxerHost,
   // reset the pipeline state, and restore this to PIPELINE_OK.
   PipelineStatus status_;
 
-  // The timestamp to start playback from after starting/seeking/resuming has
-  // completed.
-  base::TimeDelta start_timestamp_;
-
   // Whether we've received the audio/video/text ended events.
   bool renderer_ended_;
   bool text_renderer_ended_;
@@ -195,6 +189,12 @@ PipelineImpl::RendererWrapper::~RendererWrapper() {
   DCHECK(state_ == kCreated || state_ == kStopped);
 }
 
+// Note that the usage of base::Unretained() with the renderers is considered
+// safe as they are owned by |pending_callbacks_| and share the same lifetime.
+//
+// That being said, deleting the renderers while keeping |pending_callbacks_|
+// running on the media thread would result in crashes.
+
 void PipelineImpl::RendererWrapper::Start(
     Demuxer* demuxer,
     std::unique_ptr<Renderer> renderer,
@@ -204,6 +204,8 @@ void PipelineImpl::RendererWrapper::Start(
   DCHECK_EQ(kCreated, state_) << "Received start in unexpected state: "
                               << state_;
 
+  SetState(kStarting);
+
   DCHECK(!demuxer_);
   DCHECK(!shared_state_.renderer);
   DCHECK(!text_renderer_);
@@ -222,7 +224,24 @@ void PipelineImpl::RendererWrapper::Start(
   }
   weak_pipeline_ = weak_pipeline;
 
-  StateTransitionTask(PIPELINE_OK);
+  // Queue asynchronous actions required to start.
+  DCHECK(!pending_callbacks_);
+  SerialRunner::Queue fns;
+
+  // Initialize demuxer.
+  fns.Push(base::Bind(&RendererWrapper::InitializeDemuxer, weak_this_));
+
+  // Once the demuxer is initialized successfully, media metadata must be
+  // available - report the metadata to client.
+  fns.Push(base::Bind(&RendererWrapper::ReportMetadata, weak_this_));
+
+  // Initialize renderer.
+  fns.Push(base::Bind(&RendererWrapper::InitializeRenderer, weak_this_));
+
+  // Run tasks.
+  pending_callbacks_ =
+      SerialRunner::Run(fns, base::Bind(&RendererWrapper::CompleteSeek,
+                                        weak_this_, base::TimeDelta()));
 }
 
 void PipelineImpl::RendererWrapper::Stop(const base::Closure& stop_cb) {
@@ -242,7 +261,23 @@ void PipelineImpl::RendererWrapper::Stop(const base::Closure& stop_cb) {
   // tasks.
   pending_callbacks_.reset();
 
-  DoStop(stop_cb);
+  DestroyRenderer();

[alokp, 2016/06/24 06:15:06]

DoStop() moved here.

+  text_renderer_.reset();
+
+  if (demuxer_) {
+    demuxer_->Stop();
+    demuxer_ = NULL;
+  }
+
+  SetState(kStopped);
+
+  // Post the stop callback to enqueue it after the tasks that may have been
+  // posted by Demuxer and Renderer during stopping. Note that in theory the
+  // tasks posted by Demuxer/Renderer may post even more tasks that will get
+  // enqueued after |stop_cb|. This may be problematic because Demuxer may
+  // get destroyed as soon as |stop_cb| is run. In practice this is not a
+  // problem, but ideally Demuxer should be destroyed on the media thread.
+  media_task_runner_->PostTask(FROM_HERE, stop_cb);
 }
 
 void PipelineImpl::RendererWrapper::Seek(base::TimeDelta time) {
@@ -256,16 +291,40 @@ void PipelineImpl::RendererWrapper::Seek(base::TimeDelta time) {
     return;
   }
 
-  const base::TimeDelta seek_timestamp =
-      std::max(time, demuxer_->GetStartTime());
+  base::TimeDelta seek_timestamp = std::max(time, demuxer_->GetStartTime());
 
   SetState(kSeeking);
   renderer_ended_ = false;
   text_renderer_ended_ = false;
-  start_timestamp_ = seek_timestamp;
 
-  DoSeek(seek_timestamp,
-         base::Bind(&RendererWrapper::StateTransitionTask, weak_this_));
+  // Queue asynchronous actions required to start.

[alokp, 2016/06/24 06:15:07]

DoSeek() moved here.

+  DCHECK(!pending_callbacks_);
+  SerialRunner::Queue bound_fns;
+
+  // Pause.
+  if (text_renderer_) {
+    bound_fns.Push(base::Bind(&TextRenderer::Pause,
+                              base::Unretained(text_renderer_.get())));
+  }
+
+  // Flush.
+  DCHECK(shared_state_.renderer);
+  bound_fns.Push(base::Bind(&Renderer::Flush,
+                            base::Unretained(shared_state_.renderer.get())));
+
+  if (text_renderer_) {
+    bound_fns.Push(base::Bind(&TextRenderer::Flush,
+                              base::Unretained(text_renderer_.get())));
+  }
+
+  // Seek demuxer.
+  bound_fns.Push(
+      base::Bind(&Demuxer::Seek, base::Unretained(demuxer_), seek_timestamp));
+
+  // Run tasks.
+  pending_callbacks_ = SerialRunner::Run(
+      bound_fns,
+      base::Bind(&RendererWrapper::CompleteSeek, weak_this_, seek_timestamp));
 }
 
 void PipelineImpl::RendererWrapper::Suspend() {
@@ -292,9 +351,7 @@ void PipelineImpl::RendererWrapper::Suspend() {
     DCHECK(shared_state_.suspend_timestamp != kNoTimestamp());
   }
 
-  // Queue the asynchronous actions required to stop playback. (Matches setup in
-  // DoSeek().)
-  // TODO(sandersd): Share implementation with DoSeek().
+  // Queue the asynchronous actions required to stop playback.
   SerialRunner::Queue fns;
 
   if (text_renderer_) {
@@ -311,7 +368,7 @@ void PipelineImpl::RendererWrapper::Suspend() {
   }
 
   pending_callbacks_ = SerialRunner::Run(
-      fns, base::Bind(&RendererWrapper::StateTransitionTask, weak_this_));
+      fns, base::Bind(&RendererWrapper::CompleteSuspend, weak_this_));
 }
 
 void PipelineImpl::RendererWrapper::Resume(std::unique_ptr<Renderer> renderer,
@@ -335,25 +392,22 @@ void PipelineImpl::RendererWrapper::Resume(std::unique_ptr<Renderer> renderer,
     shared_state_.renderer = std::move(renderer);
   }
 
-  // Set up for a seek. (Matches setup in SeekTask().)
-  // TODO(sandersd): Share implementation with SeekTask().
   renderer_ended_ = false;
   text_renderer_ended_ = false;
-  start_timestamp_ = std::max(timestamp, demuxer_->GetStartTime());
+  base::TimeDelta start_timestamp =
+      std::max(timestamp, demuxer_->GetStartTime());
 
-  // Queue the asynchronous actions required to start playback. Unlike DoSeek(),
-  // we need to initialize the renderer ourselves (we don't want to enter state
-  // kInitDemuxer, and even if we did the current code would seek to the start
-  // instead of |timestamp|).
+  // Queue the asynchronous actions required to start playback.
   SerialRunner::Queue fns;
 
   fns.Push(
-      base::Bind(&Demuxer::Seek, base::Unretained(demuxer_), start_timestamp_));
+      base::Bind(&Demuxer::Seek, base::Unretained(demuxer_), start_timestamp));
 
   fns.Push(base::Bind(&RendererWrapper::InitializeRenderer, weak_this_));
 
   pending_callbacks_ = SerialRunner::Run(
-      fns, base::Bind(&RendererWrapper::StateTransitionTask, weak_this_));
+      fns,
+      base::Bind(&RendererWrapper::CompleteSeek, weak_this_, start_timestamp));
 }
 
 void PipelineImpl::RendererWrapper::SetPlaybackRate(double playback_rate) {
@@ -619,66 +673,6 @@ void PipelineImpl::RendererWrapper::CheckPlaybackEnded() {
       FROM_HERE, base::Bind(&PipelineImpl::OnEnded, weak_pipeline_));
 }
 
-// Note that the usage of base::Unretained() with the renderers is considered
-// safe as they are owned by |pending_callbacks_| and share the same lifetime.
-//
-// That being said, deleting the renderers while keeping |pending_callbacks_|
-// running on the media thread would result in crashes.
-void PipelineImpl::RendererWrapper::DoSeek(base::TimeDelta seek_timestamp,
-                                           const PipelineStatusCB& done_cb) {
-  DCHECK(media_task_runner_->BelongsToCurrentThread());
-  DCHECK(!pending_callbacks_.get());
-  DCHECK_EQ(state_, kSeeking);
-  SerialRunner::Queue bound_fns;
-
-  // Pause.
-  if (text_renderer_) {
-    bound_fns.Push(base::Bind(&TextRenderer::Pause,
-                              base::Unretained(text_renderer_.get())));
-  }
-
-  // Flush.
-  DCHECK(shared_state_.renderer);
-  bound_fns.Push(base::Bind(&Renderer::Flush,
-                            base::Unretained(shared_state_.renderer.get())));
-
-  if (text_renderer_) {
-    bound_fns.Push(base::Bind(&TextRenderer::Flush,
-                              base::Unretained(text_renderer_.get())));
-  }
-
-  // Seek demuxer.
-  bound_fns.Push(
-      base::Bind(&Demuxer::Seek, base::Unretained(demuxer_), seek_timestamp));
-
-  pending_callbacks_ = SerialRunner::Run(bound_fns, done_cb);
-}
-
-void PipelineImpl::RendererWrapper::DoStop(const base::Closure& done_cb) {
-  DVLOG(2) << __FUNCTION__;
-  DCHECK(media_task_runner_->BelongsToCurrentThread());
-  DCHECK_EQ(state_, kStopping);
-  DCHECK(!pending_callbacks_.get());
-
-  DestroyRenderer();
-  text_renderer_.reset();
-
-  if (demuxer_) {
-    demuxer_->Stop();
-    demuxer_ = NULL;
-  }
-
-  SetState(kStopped);
-
-  // Post the stop callback to enqueue it after the tasks that may have been
-  // posted by Demuxer and Renderer during stopping. Note that in theory the
-  // tasks posted by Demuxer/Renderer may post even more tasks that will get
-  // enqueued after |done_cb|. This may be problematic because Demuxer may
-  // get destroyed as soon as |done_cb| is run. In practice this is not a
-  // problem, but ideally Demuxer should be destroyed on the media thread.
-  media_task_runner_->PostTask(FROM_HERE, done_cb);
-}
-
 void PipelineImpl::RendererWrapper::SetState(State next_state) {
   DCHECK(media_task_runner_->BelongsToCurrentThread());
   DVLOG(1) << PipelineImpl::GetStateString(state_) << " -> "
@@ -688,119 +682,60 @@ void PipelineImpl::RendererWrapper::SetState(State next_state) {
   media_log_->AddEvent(media_log_->CreatePipelineStateChangedEvent(next_state));
 }
 
-PipelineImpl::State PipelineImpl::RendererWrapper::GetNextState() const {
+void PipelineImpl::RendererWrapper::CompleteSeek(base::TimeDelta seek_time,
+                                                 PipelineStatus status) {
   DCHECK(media_task_runner_->BelongsToCurrentThread());
-  DCHECK_EQ(status_, PIPELINE_OK)
-      << "State transitions don't happen when there's an error: " << status_;
-
-  switch (state_) {
-    case kCreated:
-      return kInitDemuxer;
+  DCHECK(state_ == kStarting || state_ == kSeeking || state_ == kResuming);
 
-    case kInitDemuxer:
-      return kInitRenderer;
+  DCHECK(pending_callbacks_);
+  pending_callbacks_.reset();
 
-    case kInitRenderer:
-    case kSeeking:
-      return kPlaying;
+  if (status != PIPELINE_OK) {
+    OnPipelineError(status);
+    return;
+  }
 
-    case kSuspending:
-      return kSuspended;
+  shared_state_.renderer->StartPlayingFrom(

[alokp, 2016/06/24 06:15:07]

copied from PipelineImpl::RendererWrapper::StateTransitionTask; switch statement
kPlaying.

+      std::max(seek_time, demuxer_->GetStartTime()));
+  {
+    base::AutoLock auto_lock(shared_state_lock_);
+    shared_state_.suspend_timestamp = kNoTimestamp();
+  }
 
-    case kSuspended:
-      return kResuming;
+  if (text_renderer_)
+    text_renderer_->StartPlaying();
 
-    case kResuming:
-      return kPlaying;
+  shared_state_.renderer->SetPlaybackRate(playback_rate_);
+  shared_state_.renderer->SetVolume(volume_);
 
-    case kPlaying:
-    case kStopping:
-    case kStopped:
-      break;
-  }
-  NOTREACHED() << "State has no transition: " << state_;
-  return state_;
+  SetState(kPlaying);
+  main_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&PipelineImpl::OnSeekDone, weak_pipeline_));
 }
-void PipelineImpl::RendererWrapper::StateTransitionTask(PipelineStatus status) {
-  DCHECK(media_task_runner_->BelongsToCurrentThread());
 
-  // No-op any state transitions if we're stopping or already encountered error.
-  if (state_ == kStopping || state_ == kStopped || status_ != PIPELINE_OK)
-    return;
-
-  // Report error from the previous operation.
-  if (status != PIPELINE_OK) {
-    OnPipelineError(status);
-    return;
-  }
-
-  // Guard against accidentally clearing |pending_callbacks_| for states that
-  // use it as well as states that should not be using it.
-  DCHECK_EQ(pending_callbacks_.get() != NULL,
-            state_ == kSeeking || state_ == kSuspending || state_ == kResuming);
+void PipelineImpl::RendererWrapper::CompleteSuspend(PipelineStatus status) {
+  DCHECK(media_task_runner_->BelongsToCurrentThread());
+  DCHECK_EQ(kSuspending, state_);
 
+  DCHECK(pending_callbacks_);
   pending_callbacks_.reset();
 
-  PipelineStatusCB done_cb =
-      base::Bind(&RendererWrapper::StateTransitionTask, weak_this_);
-
-  // Switch states, performing any entrance actions for the new state as well.
-  SetState(GetNextState());
-  switch (state_) {
-    case kInitDemuxer:
-      return InitializeDemuxer(done_cb);
-
-    case kInitRenderer:
-      // When the state_ transfers to kInitRenderer, it means the demuxer has
-      // finished parsing the init info. It should call ReportMetadata in case
-      // meeting 'decode' error when passing media segment but WebMediaPlayer's
-      // ready_state_ is still ReadyStateHaveNothing. In that case, it will
-      // treat it as NetworkStateFormatError not NetworkStateDecodeError.
-      ReportMetadata();
-      start_timestamp_ = demuxer_->GetStartTime();
-
-      return InitializeRenderer(done_cb);
-
-    case kPlaying:
-      DCHECK(start_timestamp_ >= base::TimeDelta());
-      shared_state_.renderer->StartPlayingFrom(start_timestamp_);
-      {
-        base::AutoLock auto_lock(shared_state_lock_);
-        shared_state_.suspend_timestamp = kNoTimestamp();
-      }
-
-      if (text_renderer_)
-        text_renderer_->StartPlaying();
-
-      main_task_runner_->PostTask(
-          FROM_HERE, base::Bind(&PipelineImpl::OnSeekDone, weak_pipeline_,
-                                start_timestamp_));
-
-      shared_state_.renderer->SetPlaybackRate(playback_rate_);
-      shared_state_.renderer->SetVolume(volume_);
-      return;
-
-    case kSuspended:
-      DestroyRenderer();
-      {
-        base::AutoLock auto_lock(shared_state_lock_);
-        shared_state_.statistics.audio_memory_usage = 0;
-        shared_state_.statistics.video_memory_usage = 0;
-      }
-      main_task_runner_->PostTask(
-          FROM_HERE, base::Bind(&PipelineImpl::OnSuspendDone, weak_pipeline_,
-                                shared_state_.suspend_timestamp));
-      return;
-
-    case kStopping:
-    case kStopped:
-    case kCreated:
-    case kSeeking:
-    case kSuspending:
-    case kResuming:
-      NOTREACHED() << "State has no transition: " << state_;
-      return;
+  // In case we are suspending or suspended, the error may be recoverable,
+  // so don't propagate it now, instead let the subsequent seek during resume
+  // propagate it if it's unrecoverable.
+  LOG_IF(WARNING, status != PIPELINE_OK)
+      << "Encountered pipeline error while suspending: " << status;
+
+  DestroyRenderer();

[alokp, 2016/06/24 06:15:07]

copied from PipelineImpl::RendererWrapper::StateTransitionTask; switch statement
kSuspended.

+  {
+    base::AutoLock auto_lock(shared_state_lock_);
+    shared_state_.statistics.audio_memory_usage = 0;
+    shared_state_.statistics.video_memory_usage = 0;
   }
+
+  SetState(kSuspended);
+  main_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&PipelineImpl::OnSuspendDone, weak_pipeline_));
 }
 
 void PipelineImpl::RendererWrapper::InitializeDemuxer(
@@ -1105,8 +1040,7 @@ void PipelineImpl::SetCdm(CdmContext* cdm_context,
 const char* PipelineImpl::GetStateString(State state) {
   switch (state) {
     RETURN_STRING(kCreated);
-    RETURN_STRING(kInitDemuxer);
-    RETURN_STRING(kInitRenderer);
+    RETURN_STRING(kStarting);
     RETURN_STRING(kSeeking);
     RETURN_STRING(kPlaying);
     RETURN_STRING(kStopping);
@@ -1218,8 +1152,8 @@ void PipelineImpl::OnVideoOpacityChange(bool opaque) {
   client_->OnVideoOpacityChange(opaque);
 }
 
-void PipelineImpl::OnSeekDone(base::TimeDelta start_time) {
-  DVLOG(3) << __FUNCTION__ << "(" << start_time.InMicroseconds() << ")";
+void PipelineImpl::OnSeekDone() {
+  DVLOG(3) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(IsRunning());
 
@@ -1227,8 +1161,8 @@ void PipelineImpl::OnSeekDone(base::TimeDelta start_time) {
   base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
 }
 
-void PipelineImpl::OnSuspendDone(base::TimeDelta suspend_time) {
-  DVLOG(3) << __FUNCTION__ << "(" << suspend_time.InMicroseconds() << ")";
+void PipelineImpl::OnSuspendDone() {
+  DVLOG(3) << __FUNCTION__;
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(IsRunning());