Fix processing of multiple stream status changes by renderer

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 190 matching lines @@
   }
 
   time_source_->SetMediaTime(time);
 
   if (audio_renderer_)
     audio_renderer_->StartPlaying();
   if (video_renderer_)
     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
     // 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));
@@ skipping 130 matching lines @@
 
   DemuxerStream* audio_stream =
       demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO);
   if (!audio_stream) {
     audio_renderer_.reset();
     task_runner_->PostTask(FROM_HERE, base::Bind(done_cb, PIPELINE_OK));
     return;
   }
 
   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));
   // Note: After the initialization of a renderer, error events from it may
   // happen at any time and all future calls must guard against STATE_ERROR.
   audio_renderer_->Initialize(audio_stream, cdm_context_,
                               audio_renderer_client_.get(), done_cb);
 }
 
 void RendererImpl::OnAudioRendererInitializeDone(PipelineStatus status) {
@@ skipping 28 matching lines @@
 
   DemuxerStream* video_stream =
       demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO);
   if (!video_stream) {
     video_renderer_.reset();
     task_runner_->PostTask(FROM_HERE, base::Bind(done_cb, PIPELINE_OK));
     return;
   }
 
   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));
   video_renderer_->Initialize(
       video_stream, cdm_context_, video_renderer_client_.get(),
       base::Bind(&RendererImpl::GetWallClockTimes, base::Unretained(this)),
       done_cb);
 }
 
 void RendererImpl::OnVideoRendererInitializeDone(PipelineStatus status) {
@@ skipping 114 matching lines @@
       return "HAVE_ENOUGH";
   }
   NOTREACHED();
   return "";
 }
 }
 
 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.
   if (type == DemuxerStream::VIDEO && restarting_video_) {
     if (new_buffering_state == BUFFERING_HAVE_ENOUGH) {
       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
         // still be playing, we only need to unpause the video stream.
         DVLOG(4) << "deferred_video_underflow_cb_.Cancel()";
         deferred_video_underflow_cb_.Cancel();
         video_buffering_state_ = new_buffering_state;
         if (playback_rate_ > 0)
           video_renderer_->OnTimeProgressing();
         return true;
@@ skipping 25 matching lines @@
       DVLOG(4) << "deferred_audio_restart_underflow_cb_.Cancel()";
       deferred_audio_restart_underflow_cb_.Cancel();
     } else if (new_buffering_state == BUFFERING_HAVE_ENOUGH) {
       DVLOG(1) << __func__ << " Got BUFFERING_HAVE_ENOUGH for audio stream,"
                               " resuming playback.";
       deferred_audio_restart_underflow_cb_.Cancel();
       // Now that we have decoded enough audio, pause playback momentarily to
       // 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;
 }
 
 void RendererImpl::OnBufferingStateChange(DemuxerStream::Type type,
                                           BufferingState new_buffering_state) {
   DCHECK((type == DemuxerStream::AUDIO) || (type == DemuxerStream::VIDEO));
   BufferingState* buffering_state = type == DemuxerStream::AUDIO
                                         ? &audio_buffering_state_
@@ skipping 204 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoNaturalSizeChange(size);
 }
 
 void RendererImpl::OnVideoOpacityChange(bool opaque) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoOpacityChange(opaque);
 }
 
 }  // namespace media