Allow media track switching.

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 cc1504529eee3f0dbef0eef70fb56a63306f9e05..9c54bbb5bc5949425c589c5f51851e679d258679 100644
--- a/media/renderers/renderer_impl.cc
+++ b/media/renderers/renderer_impl.cc
@@ -80,6 +80,8 @@ RendererImpl::RendererImpl(
       task_runner_(task_runner),
       audio_renderer_(std::move(audio_renderer)),
       video_renderer_(std::move(video_renderer)),
+      current_audio_stream_(nullptr),
+      current_video_stream_(nullptr),
       time_source_(NULL),
       time_ticking_(false),
       playback_rate_(0.0),
@@ -126,6 +128,8 @@ RendererImpl::~RendererImpl() {
     FinishInitialization(PIPELINE_ERROR_ABORT);
   } else if (!flush_cb_.is_null()) {
     base::ResetAndReturn(&flush_cb_).Run();
+  } else if (pending_flush_cb_) {
+    base::ResetAndReturn(&pending_flush_cb_).Run();
   }
 }
 
@@ -194,6 +198,25 @@ void RendererImpl::Flush(const base::Closure& flush_cb) {
     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/10 18:57:30]

Can you just use 

base::Bind(&RendererImpl::Flush, weak_this_, flush_cb)

here so we don't need pending_flush_cb_?

[servolk, 2017/04/10 20:21:22]

We could remove the |pending_flush_cb_| and use |flush_cb_| instead, but then
we'd have to remove the DCHECK at line 189 above, since flush_cb_ would be
non-null when restarting the postponed Flush after a track switch. But that
DCHECK is also the only DCHECK that prevents Flush from being called while a
previous Flush is pending, so I'm not sure if that's a good idea. I think
leaving pending_flush_cb_ to keep a clear separation between the regular and
postponed Flush is actually better, even though it's an extra variable. WDYT?

[servolk, 2017/04/10 20:29:53]

Actually, let me expand a bit more. The main reason why I added the
pending_flush_cb_ is that I wanted to preserve the guarantee that the flush_cb
always gets invoked, even if the RendererImpl is destroyed before the flush
completes. Imagine a situation like:
1. media track switch happens, the RendererImpl starts asynchronously 
processing it, but it might take some time.
2. RendererImpl::Flush is called. The Flush will be postponed due to a pending
audio/video restart due to a track switch in #1. When the Flush is postponed, it
gets placed into the pending_actions_ queue.
3. Now RendererImpl gets destroyed, before the track switch and Flush are
completed. How can we guarantee that the flush_cb gets invoked in this case? We
probably don't want to go and execute pending_actions_ queue, since that's
useless and will only add to destruction delay. We need to know in the
destructor whether there was a pending flush, whose cb we need to invoke, and we
need to be able to invoke the cb without going through the whole
pending_actions_ queue.
I believe the easiest solution to that is to store a flush_cb in a
pending_flush_cb_ as soon as we put the pending flush in the pending_actions_
queue.
If we stored the pending flush callback in the flush_cb_ even for postponed
flushes, then we would run into the problem with the DCHECK on line 189.

[xhwang, 2017/04/11 00:29:41]

I am confused. We are not setting the |flush_cb_| here yet. Why do we need to
remove the DCHECK on l.189?
It seems this is another case we need
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/ZfL_XhGZ7tA/Qrzw9...
 :)

I see your concern, and it makes sense for the current media::Renderer API:
https://cs.chromium.org/chromium/src/media/base/renderer.h?rcl=4a485532d97f66...

But looking at our implementation in PipelineWrapper, I guess it doesn't make
any difference:
https://cs.chromium.org/chromium/src/media/base/pipeline_impl.cc?rcl=4a485532...

So I feel there's room to improve here (in later CLs).

So my last comment for this CL :) Why the FlushInternal() idea Dale mentioned
won't work. I think the ideas is:

void RendererImpl::Flush(const base::Closure& flush_cb) {
  ...

  flush_cb_ = flush_cb;
  state_ = STATE_FLUSHING;
  
  if (restarting_audio_ || restarting_video_) {
    pending_actions_.push_back(
        base::Bind(&RendererImpl::FlushInternal, weak_this_));
    return;
  }

  FlushInternal();
}

void RendererImpl::FlushInternal() {
  // Check for STATE_ERROR etc
  
  DCHECK(flush_cb_);
  DCHECK_EQ(STATE_FLUSHING, state_);

  if (time_ticking_)
    PausePlayback();

  FlushAudioRenderer();
}

[servolk, 2017/04/11 01:14:05]

Well, yes, if we do it like that (by setting state_ to STATE_FLUSHING
immediately, instead of after the FlushInternal actually starts) then we can
keep the DCHECK. We'll just need to make some small adjustments to the
RestartAudio/VideoRenderer logic (since now those might get invoked when we are
in the STATE_FLUSHING state).
Done in the latest patchset, PTAL.

+    return;
+  }
+
+  if (pending_flush_cb_) {
+    DCHECK(flush_cb.Equals(pending_flush_cb_));
+    base::ResetAndReturn(&pending_flush_cb_);
+  }
+
   flush_cb_ = flush_cb;
   state_ = STATE_FLUSHING;
 
@@ -221,64 +244,6 @@ void RendererImpl::StartPlayingFrom(base::TimeDelta time) {
     video_renderer_->StartPlayingFrom(time);
 }
 
-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) || (audio_ended_ && video_ended_))
-    return;
-  if (restarting_audio_ || restarting_video_) {
-    DVLOG(3) << __func__ << ": postponed stream " << stream
-             << " status change handling.";
-    pending_stream_status_notifications_.push_back(
-        base::Bind(&RendererImpl::OnStreamStatusChanged, weak_this_, stream,
-                   enabled, time));
-    return;
-  }
-  if (stream->type() == DemuxerStream::VIDEO) {
-    DCHECK(video_renderer_);
-    restarting_video_ = true;
-    video_renderer_->Flush(
-        base::Bind(&RendererImpl::RestartVideoRenderer, weak_this_, time));
-  } else if (stream->type() == DemuxerStream::AUDIO) {
-    DCHECK(audio_renderer_);
-    DCHECK(time_source_);
-    restarting_audio_ = true;
-    // Stop ticking (transition into paused state) in audio renderer before
-    // calling Flush, since after Flush we are going to restart playback by
-    // calling audio renderer StartPlaying which would fail in playing state.
-    if (time_ticking_) {
-      time_ticking_ = false;
-      time_source_->StopTicking();
-    }
-    audio_renderer_->Flush(
-        base::Bind(&RendererImpl::RestartAudioRenderer, weak_this_, time));
-  }
-}
-
-void RendererImpl::RestartVideoRenderer(base::TimeDelta time) {
-  DVLOG(3) << __func__;
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(video_renderer_);
-  DCHECK_EQ(state_, STATE_PLAYING);
-  video_ended_ = false;
-  video_renderer_->StartPlayingFrom(time);
-}
-
-void RendererImpl::RestartAudioRenderer(base::TimeDelta time) {
-  DVLOG(3) << __func__;
-  DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK_EQ(state_, STATE_PLAYING);
-  DCHECK(time_source_);
-  DCHECK(audio_renderer_);
-  audio_ended_ = false;
-  audio_renderer_->StartPlaying();
-}
-
 void RendererImpl::SetPlaybackRate(double playback_rate) {
   DVLOG(1) << __func__ << "(" << playback_rate << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ -400,6 +365,8 @@ void RendererImpl::InitializeAudioRenderer() {
     return;
   }
 
+  current_audio_stream_ = audio_stream;
+
   audio_renderer_client_.reset(
       new RendererClientInternal(DemuxerStream::AUDIO, this));
   // Note: After the initialization of a renderer, error events from it may
@@ -448,6 +415,8 @@ void RendererImpl::InitializeVideoRenderer() {
     return;
   }
 
+  current_video_stream_ = video_stream;
+
   video_renderer_client_.reset(
       new RendererClientInternal(DemuxerStream::VIDEO, this));
   video_renderer_->Initialize(
@@ -560,6 +529,156 @@ void RendererImpl::OnVideoRendererFlushDone() {
   video_ended_ = false;
   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 &&
+       state_ != STATE_FLUSHED) ||
+      (audio_ended_ && video_ended_))
+    return;
+
+  if (restarting_audio_ || restarting_video_ || state_ == STATE_FLUSHING) {
+    DVLOG(3) << __func__ << ": postponed stream " << stream
+             << " status change handling.";
+    pending_actions_.push_back(base::Bind(&RendererImpl::OnStreamStatusChanged,
+                                          weak_this_, stream, enabled, time));
+    return;
+  }
+
+  DCHECK(state_ == STATE_PLAYING || state_ == STATE_FLUSHED);
+  if (stream->type() == DemuxerStream::VIDEO) {
+    DCHECK(video_renderer_);
+    restarting_video_ = true;
+    base::Closure handle_track_status_cb =
+        base::Bind(stream == current_video_stream_
+                       ? &RendererImpl::RestartVideoRenderer
+                       : &RendererImpl::ReinitializeVideoRenderer,
+                   weak_this_, stream, time);
+    if (state_ == STATE_FLUSHED)
+      handle_track_status_cb.Run();
+    else
+      video_renderer_->Flush(handle_track_status_cb);
+  } else if (stream->type() == DemuxerStream::AUDIO) {
+    DCHECK(audio_renderer_);
+    DCHECK(time_source_);
+    restarting_audio_ = true;
+    base::Closure handle_track_status_cb =
+        base::Bind(stream == current_audio_stream_
+                       ? &RendererImpl::RestartAudioRenderer
+                       : &RendererImpl::ReinitializeAudioRenderer,
+                   weak_this_, stream, time);
+    if (state_ == STATE_FLUSHED) {
+      handle_track_status_cb.Run();
+      return;
+    }
+    // Stop ticking (transition into paused state) in audio renderer before
+    // calling Flush, since after Flush we are going to restart playback by
+    // calling audio renderer StartPlaying which would fail in playing state.
+    if (time_ticking_) {
+      time_ticking_ = false;
+      time_source_->StopTicking();
+    }
+    audio_renderer_->Flush(handle_track_status_cb);
+  }
+}
+
+void RendererImpl::ReinitializeAudioRenderer(DemuxerStream* stream,
+                                             base::TimeDelta time) {
+  DVLOG(2) << __func__ << " stream=" << stream << " time=" << time.InSecondsF();
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK_NE(stream, current_audio_stream_);
+
+  current_audio_stream_ = stream;
+  audio_renderer_->Initialize(
+      stream, cdm_context_, audio_renderer_client_.get(),
+      base::Bind(&RendererImpl::OnAudioRendererReinitialized, weak_this_,
+                 stream, time));
+}
+
+void RendererImpl::OnAudioRendererReinitialized(DemuxerStream* stream,
+                                                base::TimeDelta time,
+                                                PipelineStatus status) {
+  DVLOG(2) << __func__ << ": status=" << status;
+  DCHECK_EQ(stream, current_audio_stream_);
+
+  if (status != PIPELINE_OK) {
+    OnError(status);
+    return;
+  }
+  RestartAudioRenderer(stream, time);
+}
+
+void RendererImpl::ReinitializeVideoRenderer(DemuxerStream* stream,
+                                             base::TimeDelta time) {
+  DVLOG(2) << __func__ << " stream=" << stream << " time=" << time.InSecondsF();
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK_NE(stream, current_video_stream_);
+
+  current_video_stream_ = stream;
+  video_renderer_->OnTimeStopped();
+  video_renderer_->Initialize(
+      stream, cdm_context_, video_renderer_client_.get(),
+      base::Bind(&RendererImpl::GetWallClockTimes, base::Unretained(this)),
+      base::Bind(&RendererImpl::OnVideoRendererReinitialized, weak_this_,
+                 stream, time));
+}
+
+void RendererImpl::OnVideoRendererReinitialized(DemuxerStream* stream,
+                                                base::TimeDelta time,
+                                                PipelineStatus status) {
+  DVLOG(2) << __func__ << ": status=" << status;
+  DCHECK_EQ(stream, current_video_stream_);
+
+  if (status != PIPELINE_OK) {
+    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_FLUSHED);
+  DCHECK(time_source_);
+  DCHECK(audio_renderer_);
+  DCHECK_EQ(stream, current_audio_stream_);
+
+  audio_ended_ = false;
+  if (state_ == STATE_PLAYING)
+    audio_renderer_->StartPlaying();

[xhwang, 2017/04/10 18:57:30]

nit: add a comment when stream restart will be finished in this case

[servolk, 2017/04/10 20:21:22]

Done.

+  else
+    OnStreamRestartCompleted();
+}
+
+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_FLUSHED);
+  DCHECK_EQ(stream, current_video_stream_);
+
+  video_ended_ = false;
+  if (state_ == STATE_PLAYING)
+    video_renderer_->StartPlayingFrom(time);
+  else
+    OnStreamRestartCompleted();
 }
 
 void RendererImpl::OnStatisticsUpdate(const PipelineStatistics& stats) {
@@ -634,12 +753,16 @@ bool RendererImpl::HandleRestartedStreamBufferingChanges(
 }
 
 void RendererImpl::OnStreamRestartCompleted() {
+  DVLOG(3) << __func__ << " restarting_audio_=" << restarting_audio_
+           << " restarting_video_=" << restarting_video_;
+  DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(restarting_audio_ || restarting_video_);
   restarting_audio_ = false;
   restarting_video_ = false;
-  if (!pending_stream_status_notifications_.empty()) {
-    pending_stream_status_notifications_.front().Run();
-    pending_stream_status_notifications_.pop_front();
+  if (!pending_actions_.empty()) {
+    base::Closure closure = pending_actions_.front();
+    pending_actions_.pop_front();
+    closure.Run();
   }
 }