Fix processing of multiple stream status changes by renderer

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 210e5cc574f2437b4368c498f56b02facf5c1e61..a9367d85c27a668fc5e5419f1366671fdc3a87e8 100644
--- a/media/renderers/renderer_impl.cc
+++ b/media/renderers/renderer_impl.cc
@@ -208,28 +208,40 @@ void RendererImpl::StartPlayingFrom(base::TimeDelta time) {
     video_renderer_->StartPlayingFrom(time);
 }
 
-void RendererImpl::RestartStreamPlayback(DemuxerStream* stream,
+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=" << stream->enabled() << " time=" << time.InSecondsF();
+           << " enabled=" << enabled << " time=" << time.InSecondsF();
   if ((state_ != STATE_PLAYING) || (audio_ended_ && video_ended_))
     return;
   if (stream->type() == DemuxerStream::VIDEO) {
     DCHECK(video_renderer_);
-    if (restarting_video_)
+    if (restarting_video_) {
+      DVLOG(3) << __func__ << ": postponed stream " << stream
+               << " status change handling.";
+      postponed_video_status_notifications_.push(
+          base::Bind(&RendererImpl::OnStreamStatusChanged, weak_this_, stream,
+                     enabled, time));
       return;
+    }
     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_);
-    if (restarting_audio_)
+    if (restarting_audio_) {
+      DVLOG(3) << __func__ << ": postponed stream " << stream
+               << " status change handling.";
+      postponed_audio_status_notifications_.push(
+          base::Bind(&RendererImpl::OnStreamStatusChanged, weak_this_, stream,
+                     enabled, time));
       return;
+    }
     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
@@ -380,7 +392,7 @@ void RendererImpl::InitializeAudioRenderer() {
   }
 
   audio_stream->SetStreamStatusChangeCB(base::Bind(
-      &RendererImpl::RestartStreamPlayback, weak_this_, audio_stream));
+      &RendererImpl::OnStreamStatusChanged, weak_this_, audio_stream));
 
   audio_renderer_client_.reset(
       new RendererClientInternal(DemuxerStream::AUDIO, this));
@@ -429,7 +441,7 @@ void RendererImpl::InitializeVideoRenderer() {
   }
 
   video_stream->SetStreamStatusChangeCB(base::Bind(
-      &RendererImpl::RestartStreamPlayback, weak_this_, video_stream));
+      &RendererImpl::OnStreamStatusChanged, weak_this_, video_stream));
 
   video_renderer_client_.reset(
       new RendererClientInternal(DemuxerStream::VIDEO, this));
@@ -564,6 +576,7 @@ const char* BufferingStateStr(BufferingState state) {
 bool RendererImpl::HandleRestartedStreamBufferingChanges(
     DemuxerStream::Type type,
     BufferingState new_buffering_state) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
   // When restarting playback we want to defer the BUFFERING_HAVE_NOTHING for
   // the stream being restarted, to allow continuing uninterrupted playback on
   // the other stream.
@@ -572,6 +585,11 @@ bool RendererImpl::HandleRestartedStreamBufferingChanges(
       DVLOG(1) << __func__ << " Got BUFFERING_HAVE_ENOUGH for video stream,"
                               " resuming playback.";
       restarting_video_ = false;
+      if (!postponed_video_status_notifications_.empty()) {
+        task_runner_->PostTask(FROM_HERE,
+                               postponed_video_status_notifications_.front());

[xhwang, 2017/01/04 22:04:16]

There's a chance where after this task (say SSC1) is posted but before that task
is processed, OnStreamStatusChanged() is called again (say SSC2). Then SSC2 will
be processed before SSC1.

It might make sense to keep |restarting_video_| to be true and call
OnStreamStatusChanged() directly here with the pending status. Or we could
always push status changes in the queue then process them in order. Or anything
else that works better :)

[servolk, 2017/01/05 02:38:45]

Yes, good catch.
We need to set restarting_video_=false before calling  OnStreamStatusChanged,
since otherwise it will just postpone handling the status change. But what we
can do here to prevent the problem you described is instead of setting
restarting_video_ = false immediately, we can do it in a posted task and then
call the pending status notification callback (if any) from the same posted
task. This will ensure that the rest of the logic in the
HandleRestartedStreamBufferingChanges and OnBufferingStateChange gets executed
(thus putting us back into steady ongoing playback state) before we process the
next pending notification.

+        postponed_video_status_notifications_.pop();

[xhwang, 2017/01/04 22:04:16]

Does it make sense to return here and only proceed when we don't have any
pending status changes?

Actually, if we have multiple pending status changes, does it make sense to only
process the last one, since all intermediate ones will be overwritten by the
last one anyways I guess?

[servolk, 2017/01/05 02:38:45]

I think it's better to do the opposite here, instead of returning right away, we
should let the rest of the code in here and in OnBufferingStateChange to finish
the work for putting the renderer back into playing state, when it's safe to
start processing the next notification.

Re multiple notifications: yes, we can probably try to handle only the last
notification, but it needs to be done on a per-stream basis. I.e. if we have
multiple audio or video streams we will still need to postpone handling an SSC
notification for a different stream, we won't be able to get away with just a
single pending SSC callback per each stream type. And once you start considering
the case of multiple streams, it gets tricky. If we store pending callback in a
map keyed by the demuxer stream, then how can we preserve the correct ordering
of notifications between different streams of the same type. I guess I'll need
to think more about this tomorrow, but for now having a queue seems a safer
option.

+      }
       if (state_ == STATE_PLAYING &&
           !deferred_video_underflow_cb_.IsCancelled()) {
         // If deferred_video_underflow_cb_ wasn't triggered, then audio should
@@ -617,6 +635,11 @@ bool RendererImpl::HandleRestartedStreamBufferingChanges(
       // ensure video renderer is synchronised with audio.
       PausePlayback();
       restarting_audio_ = false;
+      if (!postponed_audio_status_notifications_.empty()) {
+        task_runner_->PostTask(FROM_HERE,
+                               postponed_audio_status_notifications_.front());
+        postponed_audio_status_notifications_.pop();
+      }
     }
   }
   return false;