Implement disabling and enabling media tracks

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 6de238c68ce39ea96f3a74813e535b21bb177605..61fb9dfd70f99b1383eda566b5bae7bc240906c8 100644
--- a/media/renderers/renderer_impl.cc
+++ b/media/renderers/renderer_impl.cc
@@ -30,6 +30,8 @@ namespace media {
 // See |video_underflow_threshold_|.
 static const int kDefaultVideoUnderflowThresholdMs = 3000;
 
+static const int kAudioRestartUnderflowThresholdMs = 2000;
+
 class RendererImpl::RendererClientInternal : public RendererClient {
  public:
   RendererClientInternal(DemuxerStream::Type type, RendererImpl* renderer)
@@ -129,6 +131,17 @@ void RendererImpl::Initialize(DemuxerStreamProvider* demuxer_stream_provider,
   demuxer_stream_provider_ = demuxer_stream_provider;
   init_cb_ = init_cb;
 
+  DemuxerStream* audio_stream =
+      demuxer_stream_provider->GetStream(DemuxerStream::AUDIO);
+  if (audio_stream)
+    audio_stream->SetStreamRestartedCB(
+        base::Bind(&RendererImpl::RestartStreamPlayback, weak_this_));
+  DemuxerStream* video_stream =
+      demuxer_stream_provider->GetStream(DemuxerStream::VIDEO);
+  if (video_stream)
+    video_stream->SetStreamRestartedCB(
+        base::Bind(&RendererImpl::RestartStreamPlayback, weak_this_));
+
   if (HasEncryptedStream() && !cdm_context_) {
     state_ = STATE_INIT_PENDING_CDM;
     return;
@@ -201,6 +214,57 @@ void RendererImpl::StartPlayingFrom(base::TimeDelta time) {
     video_renderer_->StartPlayingFrom(time);
 }
 
+void RendererImpl::RestartStreamPlayback(DemuxerStream* stream,
+                                         base::TimeDelta time) {
+  DVLOG(1) << __FUNCTION__ << " stream=" << stream
+           << " time=" << time.InSecondsF();
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  if (stream->type() == DemuxerStream::VIDEO) {
+    DCHECK(video_renderer_);
+    if (restarting_video_)
+      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_)
+      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));
+  }
+}
+
+void RendererImpl::RestartVideoRenderer(base::TimeDelta time) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DVLOG(2) << __FUNCTION__;
+  video_ended_ = false;
+  if (state_ == STATE_PLAYING) {
+    DCHECK(video_renderer_);
+    video_renderer_->StartPlayingFrom(time);
+  }
+}
+
+void RendererImpl::RestartAudioRenderer(base::TimeDelta time) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DVLOG(2) << __FUNCTION__;
+  audio_ended_ = false;
+  if (state_ == STATE_PLAYING) {
+    DCHECK(time_source_);
+    DCHECK(audio_renderer_);
+    audio_renderer_->StartPlaying();

[chcunningham, 2016/06/29 22:49:13]

I think you still need to call time_source_->SetMediaTime(time) right before you
call StartPlaying. 

As you pointed out this will destroy the audio_clock and make a new one while
the video renderer is still playing, but I believe this is actually safe to do
because both AudioRendererImpl::SetMediaTime and ARI::GetWallClockTimes (which
video renderer calls for AV sync) are guarded by the same lock. I've tried
making this call here (patched your cl) and do not observe any dcheck/check
errors.

I wasn't able to develop a theory about how PausePlayback is improving AV
sync... the fact that it doesn't change the state of the audio_clock means that
any wrongness in the audio_clock should be preserved accross the pause. So I
patched your CL and tried to investigate ... In my testing I was not able to
introduce any noticeable drift with OR without your PausePlayback call. I
disabled the audio track in a 1 second interval for 5 straight minutes...
couldn't see anything significant. Still, I am confident that it is
theoretically possible to introduce drift if we do not reset the clock (see my
previous theory about staleness of currentTime). I could probably get this occur
with an hours-long video, but you're hoping to land soon. 

[servolk, 2016/06/29 23:40:13]

Ok, maybe that was a stray unrelated dcheck then, and I didn't get it on the
first try either (but still fairly easily, on like a second or third attempt).
I'll give it another try.

[servolk, 2016/06/30 00:06:11]

No, unfortunately that wasn't a stray dcheck. I don't get it every time, but I
consistently get it at least once out of 5-10 times when disabling/reenabling
audio with an explicit SetMediaTime.
Re PausePlayback I believe it improves a/v sync because here we are at a point
where we have just Flushed audio renderer. So now video renderer is still
playing and rendering, but audio renderer needs some time to decode enough
frames to reach the HAVE_ENOUGH buffering state. So I think regardless of
whether we call SetMediaTime here, we should still pause and unpause playback in
OnBufferingStateChange when we get a notification that the audio renderer
reached HAVE_ENOUGH status. At that point we'll know that both audio and video
streams are in HAVE_ENOUGH state and there should be enough data for smooth
playback. Only then can we be sure that they'll be in sync.
Re calling SetMediaTime. I'll try to investigate further. But TBH it's not clear
to me why do we need to call it here in the first place?
Remember that time_source_ keeps ticking regardless of whether audio stream has
ended or not. So when restarting audio stream we want time source to keep
ticking uninterrupted to allow smooth video playback to continue until audio
renderer decodes enough to get HAVE_ENOUGH state.

+  }
+}
+
 void RendererImpl::SetPlaybackRate(double playback_rate) {
   DVLOG(1) << __FUNCTION__ << "(" << playback_rate << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ -488,6 +552,20 @@ void RendererImpl::OnStatisticsUpdate(const PipelineStatistics& stats) {
   client_->OnStatisticsUpdate(stats);
 }
 
+namespace {
+
+const char* BufferingStateStr(BufferingState state) {
+  switch (state) {
+    case BUFFERING_HAVE_NOTHING:
+      return "HAVE_NOTHING";
+    case BUFFERING_HAVE_ENOUGH:
+      return "HAVE_ENOUGH";
+  }
+  NOTREACHED();
+  return "";
+}
+}
+
 void RendererImpl::OnBufferingStateChange(DemuxerStream::Type type,

[chcunningham, 2016/06/29 22:49:13]

This method was once very simple, but it now has lots of distinct paths for
audio and video. Can you break this into the sub methods along those lines? It
should be possible to still have in this method or some other submethod the
common ending where we pause/play and callback to the client_

[servolk, 2016/06/29 23:40:13]

Done.

                                           BufferingState new_buffering_state) {
   DCHECK((type == DemuxerStream::AUDIO) || (type == DemuxerStream::VIDEO));
@@ -495,13 +573,66 @@ void RendererImpl::OnBufferingStateChange(DemuxerStream::Type type,
                                         ? &audio_buffering_state_
                                         : &video_buffering_state_;
 
-  DVLOG(1) << __FUNCTION__ << "(" << *buffering_state << ", "
-           << new_buffering_state << ") "
-           << (type == DemuxerStream::AUDIO ? "audio" : "video");
+  DVLOG(1) << __FUNCTION__
+           << (type == DemuxerStream::AUDIO ? " audio " : " video ")
+           << BufferingStateStr(*buffering_state) << " -> "
+           << BufferingStateStr(new_buffering_state);
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   bool was_waiting_for_enough_data = WaitingForEnoughData();
 
+  // 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) << __FUNCTION__ << " Got BUFFERING_HAVE_ENOUGH for video stream,"
+                                  " resuming playback.";
+      restarting_video_ = false;
+      if (state_ == STATE_PLAYING && !deferred_underflow_cb_.IsCancelled()) {
+        // If deferred_underflow_cb_ wasn't triggered, then audio should still
+        // be playing, we only need to unpause the video stream.
+        deferred_underflow_cb_.Cancel();
+        video_buffering_state_ = new_buffering_state;
+        if (playback_rate_ > 0)
+          video_renderer_->OnTimeStateChanged(true);
+        return;
+      }
+    }
+    // Fall through to the regular video underflow handling path.
+  }
+
+  if (type == DemuxerStream::AUDIO && restarting_audio_) {
+    if (new_buffering_state == BUFFERING_HAVE_NOTHING) {
+      if (deferred_underflow_cb_.IsCancelled() &&

[chcunningham, 2016/06/29 22:49:13]

can you rename this to make it clear that this one is for video (now that we
have one for audio too)

[servolk, 2016/06/29 23:40:13]

Done.

+          deferred_audio_restart_underflow_cb_.IsCancelled()) {
+        DVLOG(1) << __FUNCTION__ << " Deferring BUFFERING_HAVE_NOTHING for "
+                                    "audio stream which is being restarted.";
+        deferred_audio_restart_underflow_cb_.Reset(
+            base::Bind(&RendererImpl::OnBufferingStateChange, weak_this_, type,
+                       new_buffering_state));
+        task_runner_->PostDelayedTask(
+            FROM_HERE, deferred_audio_restart_underflow_cb_.callback(),
+            base::TimeDelta::FromMilliseconds(
+                kAudioRestartUnderflowThresholdMs));
+        return;
+      }
+      // Cancel the deferred callback and report an underflow immediately.
+      DVLOG(4) << "deferred_audio_restart_underflow_cb_.Cancel()";
+      deferred_audio_restart_underflow_cb_.Cancel();
+    } else if (new_buffering_state == BUFFERING_HAVE_ENOUGH) {
+      DVLOG(1) << __FUNCTION__ << " 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();

[chcunningham, 2016/06/29 22:49:13]

As mentioned above, I think this should go. I don't think it helps sync. 

[servolk, 2016/06/29 23:40:13]

Acknowledged.

+      restarting_audio_ = false;
+      // Ensure that StartPlayback gets invoked below.
+      was_waiting_for_enough_data = true;
+    }
+  }
+
   // When audio is present and has enough data, defer video underflow callbacks
   // for some time to avoid unnecessary glitches in audio; see
   // http://crbug.com/144683#c53.
@@ -511,6 +642,7 @@ void RendererImpl::OnBufferingStateChange(DemuxerStream::Type type,
         audio_buffering_state_ == BUFFERING_HAVE_ENOUGH &&
         new_buffering_state == BUFFERING_HAVE_NOTHING &&
         deferred_underflow_cb_.IsCancelled()) {
+      DVLOG(2) << __FUNCTION__ << " Deferring HAVE_NOTHING for video stream.";
       deferred_underflow_cb_.Reset(
           base::Bind(&RendererImpl::OnBufferingStateChange,
                      weak_factory_.GetWeakPtr(), type, new_buffering_state));
@@ -520,6 +652,7 @@ void RendererImpl::OnBufferingStateChange(DemuxerStream::Type type,
       return;
     }
 
+    DVLOG(4) << "deferred_underflow_cb_.Cancel()";
     deferred_underflow_cb_.Cancel();
   } else if (!deferred_underflow_cb_.IsCancelled() &&
              type == DemuxerStream::AUDIO &&
@@ -570,11 +703,12 @@ bool RendererImpl::WaitingForEnoughData() const {
 void RendererImpl::PausePlayback() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(time_ticking_);
   switch (state_) {
     case STATE_PLAYING:
-      DCHECK(PlaybackHasEnded() || WaitingForEnoughData())
-          << "Playback should only pause due to ending or underflowing";
+      DCHECK(PlaybackHasEnded() || WaitingForEnoughData() || restarting_audio_)
+          << "Playback should only pause due to ending or underflowing or"
+             " when restarting audio stream";
+
       break;
 
     case STATE_FLUSHING:
@@ -592,8 +726,10 @@ void RendererImpl::PausePlayback() {
       break;
   }
 
-  time_ticking_ = false;
-  time_source_->StopTicking();
+  if (time_ticking_) {
+    time_ticking_ = false;
+    time_source_->StopTicking();
+  }
   if (playback_rate_ > 0 && video_renderer_)
     video_renderer_->OnTimeStateChanged(false);
 }
@@ -612,7 +748,8 @@ void RendererImpl::StartPlayback() {
 }
 
 void RendererImpl::OnRendererEnded(DemuxerStream::Type type) {
-  DVLOG(1) << __FUNCTION__;
+  DVLOG(1) << __FUNCTION__
+           << (type == DemuxerStream::AUDIO ? " audio" : " video");
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK((type == DemuxerStream::AUDIO) || (type == DemuxerStream::VIDEO));
 
@@ -625,13 +762,14 @@ void RendererImpl::OnRendererEnded(DemuxerStream::Type type) {
   } else {
     DCHECK(!video_ended_);
     video_ended_ = true;
+    DCHECK(video_renderer_);
+    video_renderer_->OnTimeStateChanged(false);
   }
 
   RunEndedCallbackIfNeeded();
 }
 
 bool RendererImpl::PlaybackHasEnded() const {
-  DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (audio_renderer_ && !audio_ended_)