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 e8e786731a7d00f938395a5a22dd928fc48f67d5..38a835b17af3248a968cb8650ca0b88ea908389c 100644
--- a/media/base/pipeline_impl.cc
+++ b/media/base/pipeline_impl.cc
@@ -128,11 +128,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();
@@ -164,10 +162,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_;
@@ -204,6 +198,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,
@@ -213,6 +213,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_);
@@ -231,7 +233,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) {
@@ -251,7 +270,23 @@ void PipelineImpl::RendererWrapper::Stop(const base::Closure& stop_cb) {
   // tasks.
   pending_callbacks_.reset();
 
-  DoStop(stop_cb);
+  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 |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) {
@@ -265,16 +300,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.
+  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() {
@@ -301,9 +360,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_) {
@@ -320,7 +377,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,
@@ -344,25 +401,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) {
@@ -532,7 +586,7 @@ void PipelineImpl::RendererWrapper::OnEnabledAudioTracksChanged(
 
   base::TimeDelta currTime = (state_ == kPlaying)
                                  ? shared_state_.renderer->GetMediaTime()
-                                 : start_timestamp_;
+                                 : demuxer_->GetStartTime();

[alokp, 2016/07/29 18:30:52]

I cannot call RendererWrapper::GetMediaTime here because this function can get
called even before Renderer has been initialized. And calling
Renderer::GetMediaTime before Renderer::Initialize is not legal.

Sergey advised me to use Demuxer::GetStartTime instead. Sergey: Can you check if
this is what you meant?

[servolk, 2016/07/29 18:37:36]

Yes, I believe this should be fine.
Indeed OnEnabledAudioTracksChanged and OnSelectedVideoTrackChanged can be
invoked before renderer is initialized, as part of HTMLMediaElement
initialization, since HTML ME selects the first audio track and the first video
track by default just before the playback is started. In those cases the
renderer might be not yet initialized and thus renderer->GetMediaTime would
return an incorrect time (kNoTimestamp or base::TimeDelta). Using the
demuxer_->GetStartTime here should be fine, since the demuxer should be able to
tell us the correct start time after media tracks are initialized (and HTML ME
wouldn't be able to select media tracks before they are populated by the
demuxer).

   demuxer_->OnEnabledAudioTracksChanged(enabledTrackIds, currTime);
 }
 
@@ -551,7 +605,7 @@ void PipelineImpl::RendererWrapper::OnSelectedVideoTrackChanged(
 
   base::TimeDelta currTime = (state_ == kPlaying)
                                  ? shared_state_.renderer->GetMediaTime()
-                                 : start_timestamp_;
+                                 : demuxer_->GetStartTime();
   demuxer_->OnSelectedVideoTrackChanged(selectedTrackId, currTime);
 }
 
@@ -684,66 +738,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) << __func__;
-  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_) << " -> "
@@ -753,119 +747,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(
+      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();
+  {
+    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(
@@ -1170,8 +1105,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);
@@ -1283,8 +1217,8 @@ void PipelineImpl::OnVideoOpacityChange(bool opaque) {
   client_->OnVideoOpacityChange(opaque);
 }
 
-void PipelineImpl::OnSeekDone(base::TimeDelta start_time) {
-  DVLOG(3) << __func__ << "(" << start_time.InMicroseconds() << ")";
+void PipelineImpl::OnSeekDone() {
+  DVLOG(3) << __func__;
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(IsRunning());
 
@@ -1292,8 +1226,8 @@ void PipelineImpl::OnSeekDone(base::TimeDelta start_time) {
   base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
 }
 
-void PipelineImpl::OnSuspendDone(base::TimeDelta suspend_time) {
-  DVLOG(3) << __func__ << "(" << suspend_time.InMicroseconds() << ")";
+void PipelineImpl::OnSuspendDone() {
+  DVLOG(3) << __func__;
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(IsRunning());