Introduce a FLUSHED state in media::RendererImpl

media/renderers/renderer_impl.cc

 // Copyright 2014 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/renderers/renderer_impl.h"
 
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
@@ skipping 170 matching lines @@
   pending_cdm_attached_cb_ = cdm_attached_cb;
 
   InitializeAudioRenderer();
 }
 
 void RendererImpl::Flush(const base::Closure& flush_cb) {
   DVLOG(1) << __func__;
   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;
   }
 
   // If we are currently handling a media stream status change, then postpone
   // Flush until after that's done (because stream status changes also flush
   // audio_renderer_/video_renderer_ and they need to be restarted before they
   // can be flushed again). OnStreamRestartCompleted will resume Flush
   // processing after audio/video restart has completed and there are no other
   // pending stream status changes.
   if (restarting_audio_ || restarting_video_) {
     DCHECK(!pending_flush_cb_);
     pending_flush_cb_ = flush_cb;
     pending_actions_.push_back(
         base::Bind(&RendererImpl::Flush, weak_this_, pending_flush_cb_));

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

oh, this is based on your other CL? I thought you want to land this first.?

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

I have initially create it based on my other CL, to ensure that it solves the
issues discovered there, but now I've rebased the latest patchset to the clean
Chromium master branch.

     return;
   }
 
   if (pending_flush_cb_) {
     DCHECK(flush_cb.Equals(pending_flush_cb_));
     base::ResetAndReturn(&pending_flush_cb_);
   }
 
   flush_cb_ = flush_cb;
   state_ = STATE_FLUSHING;
 
   if (time_ticking_)
     PausePlayback();
 
   FlushAudioRenderer();
 }
 
 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_)
     video_renderer_->StartPlayingFrom(time);
 }
 
 void RendererImpl::SetPlaybackRate(double playback_rate) {
   DVLOG(1) << __func__ << "(" << 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);
 
   const double old_rate = playback_rate_;
   playback_rate_ = playback_rate;
   if (!time_ticking_ || !video_renderer_)
     return;
 
   if (old_rate == 0 && playback_rate > 0)
@@ skipping 185 matching lines @@
   media_resource_->SetStreamStatusChangeCB(
       base::Bind(&RendererImpl::OnStreamStatusChanged, weak_this_));
 
   if (audio_renderer_) {
     time_source_ = audio_renderer_->GetTimeSource();
   } else if (!time_source_) {
     wall_clock_time_source_.reset(new WallClockTimeSource());
     time_source_ = wall_clock_time_source_.get();
   }
 
-  state_ = STATE_PLAYING;
+  state_ = STATE_FLUSHED;
   DCHECK(time_source_);
   DCHECK(audio_renderer_ || video_renderer_);
 
   FinishInitialization(PIPELINE_OK);
 }
 
 void RendererImpl::FlushAudioRenderer() {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_FLUSHING);
@@ skipping 51 matching lines @@
   if (state_ == STATE_ERROR) {
     DCHECK(flush_cb_.is_null());
     return;
   }
 
   DCHECK_EQ(state_, STATE_FLUSHING);
   DCHECK(!flush_cb_.is_null());
 
   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()) {
     base::Closure closure = pending_actions_.front();
     pending_actions_.pop_front();
     closure.Run();
   }
 }
 
 void RendererImpl::OnStreamStatusChanged(DemuxerStream* stream,
                                          bool enabled,
                                          base::TimeDelta time) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(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_) {
     DVLOG(3) << __func__ << ": postponed stream " << stream
              << " status change handling.";
     pending_actions_.push_back(base::Bind(&RendererImpl::OnStreamStatusChanged,
                                           weak_this_, stream, enabled, time));
     return;
   }
   if (stream->type() == DemuxerStream::VIDEO) {
@@ skipping 74 matching lines @@
     OnError(status);
     return;
   }
   RestartVideoRenderer(stream, time);
 }
 
 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_);
 
   audio_ended_ = false;
   audio_renderer_->StartPlaying();
 }
 
 void RendererImpl::RestartVideoRenderer(DemuxerStream* stream,
                                         base::TimeDelta time) {
   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;
   video_renderer_->StartPlayingFrom(time);
 }
 
 void RendererImpl::OnStatisticsUpdate(const PipelineStatistics& stats) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnStatisticsUpdate(stats);
 }
@@ skipping 166 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
   switch (state_) {
     case STATE_PLAYING:
       DCHECK(PlaybackHasEnded() || WaitingForEnoughData() || restarting_audio_)
           << "Playback should only pause due to ending or underflowing or"
              " when restarting audio stream";
 
       break;
 
     case STATE_FLUSHING:
+    case STATE_FLUSHED:
       // It's OK to pause playback when flushing.
       break;
 
     case STATE_UNINITIALIZED:
     case STATE_INIT_PENDING_CDM:
     case STATE_INITIALIZING:
       NOTREACHED() << "Invalid state: " << state_;
       break;
 
     case STATE_ERROR:
@@ skipping 103 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoNaturalSizeChange(size);
 }
 
 void RendererImpl::OnVideoOpacityChange(bool opaque) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoOpacityChange(opaque);
 }
 
 }  // namespace media