Introduce a FLUSHED state in media::RendererImpl

media/renderers/renderer_impl.cc

Index: media/renderers/renderer_impl.cc
diff --git a/media/renderers/renderer_impl.cc b/media/renderers/renderer_impl.cc
index 1d38452b5a1138cfc26a276530a130720199e552..e1a79aa28322db3099e4fed71b4ecabfda8ab545 100644
--- a/media/renderers/renderer_impl.cc
+++ b/media/renderers/renderer_impl.cc
@@ -188,6 +188,11 @@ void RendererImpl::Flush(const base::Closure& flush_cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(flush_cb_.is_null());
 
+  if (state_ == STATE_FLUSHED) {
+    flush_cb.Run();

[xhwang, 2017/04/06 05:20:28]

Directly run the callback could cause reentrancy issues if the caller isn't
carefully implemetned, e.g.

Step1();
Flush(base::Bind(&RendererHost::Bar, this));
Step2();

That's why traditionally we require all callbacks to be returned asynchronously
(which is why we have media::BindToCurrentLoop). Recently we are more lenient
with this as long as the caller is implemented correctly (to avoid unnecessary
posts). However, in this case the caller isn't super clear since it's the
SerialRunner:

https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=875e8893...

With that does it make sense to post the callback?

[servolk, 2017/04/06 17:11:47]

Yes, I agree, PostTask should be safer here. Fixed.

+    return;
+  }
+
   if (state_ != STATE_PLAYING) {
     DCHECK_EQ(state_, STATE_ERROR);
     return;
@@ -225,13 +230,14 @@ void RendererImpl::StartPlayingFrom(base::TimeDelta time) {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
-  if (state_ != STATE_PLAYING) {
+  if (state_ != STATE_FLUSHED) {
     DCHECK_EQ(state_, STATE_ERROR);
     return;
   }
 
   time_source_->SetMediaTime(time);
 
+  state_ = STATE_PLAYING;
   if (audio_renderer_)
     audio_renderer_->StartPlaying();
   if (video_renderer_)
@@ -243,7 +249,7 @@ void RendererImpl::SetPlaybackRate(double playback_rate) {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // Playback rate changes are only carried out while playing.
-  if (state_ != STATE_PLAYING)
+  if (state_ != STATE_PLAYING && state_ != STATE_FLUSHED)
     return;
 
   time_source_->SetPlaybackRate(playback_rate);
@@ -449,7 +455,7 @@ void RendererImpl::OnVideoRendererInitializeDone(PipelineStatus status) {
     time_source_ = wall_clock_time_source_.get();
   }
 
-  state_ = STATE_PLAYING;
+  state_ = STATE_FLUSHED;
   DCHECK(time_source_);
   DCHECK(audio_renderer_ || video_renderer_);
 
@@ -521,7 +527,7 @@ void RendererImpl::OnVideoRendererFlushDone() {
 
   DCHECK_EQ(video_buffering_state_, BUFFERING_HAVE_NOTHING);
   video_ended_ = false;
-  state_ = STATE_PLAYING;
+  state_ = STATE_FLUSHED;
   base::ResetAndReturn(&flush_cb_).Run();
 
   if (!pending_actions_.empty()) {
@@ -539,7 +545,9 @@ void RendererImpl::OnStreamStatusChanged(DemuxerStream* stream,
   bool video = (stream->type() == DemuxerStream::VIDEO);
   DVLOG(1) << __func__ << (video ? " video" : " audio") << " stream=" << stream
            << " enabled=" << enabled << " time=" << time.InSecondsF();
-  if ((state_ != STATE_PLAYING && state_ != STATE_FLUSHING) ||
+
+  if ((state_ != STATE_PLAYING && state_ != STATE_FLUSHING &&
+       state_ != STATE_FLUSHED) ||
       (audio_ended_ && video_ended_))
     return;
   if (restarting_audio_ || restarting_video_ || flush_cb_) {
@@ -634,7 +642,7 @@ void RendererImpl::RestartAudioRenderer(DemuxerStream* stream,
                                         base::TimeDelta time) {
   DVLOG(2) << __func__ << " stream=" << stream << " time=" << time.InSecondsF();
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(state_ == STATE_PLAYING || state_ == STATE_FLUSHING);
+  DCHECK(state_ == STATE_PLAYING || state_ == STATE_FLUSHED);
   DCHECK(time_source_);
   DCHECK(audio_renderer_);
   DCHECK_EQ(stream, current_audio_stream_);
@@ -648,7 +656,7 @@ void RendererImpl::RestartVideoRenderer(DemuxerStream* stream,
   DVLOG(2) << __func__ << " stream=" << stream << " time=" << time.InSecondsF();
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(video_renderer_);
-  DCHECK(state_ == STATE_PLAYING || state_ == STATE_FLUSHING);
+  DCHECK(state_ == STATE_PLAYING || state_ == STATE_FLUSHED);
   DCHECK_EQ(stream, current_video_stream_);
 
   video_ended_ = false;
@@ -835,6 +843,7 @@ void RendererImpl::PausePlayback() {
       break;
 
     case STATE_FLUSHING:
+    case STATE_FLUSHED:
       // It's OK to pause playback when flushing.
       break;