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..bd46b34e8f72c981c166ff7f56c22fb738aed784 100644
--- a/media/renderers/renderer_impl.cc
+++ b/media/renderers/renderer_impl.cc
@@ -129,6 +129,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 +212,51 @@ 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_);
+    DCHECK(video_ended_);
+    video_ended_ = false;
+    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_);
+    DCHECK(audio_ended_);
+    audio_ended_ = false;
+    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.
+    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());
+  if (state_ == STATE_PLAYING) {
+    DCHECK(video_renderer_);
+    video_renderer_->StartPlayingFrom(time);
+  }
+}
+
+void RendererImpl::RestartAudioRenderer(base::TimeDelta time) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  if (state_ == STATE_PLAYING) {
+    DCHECK(time_source_);
+    DCHECK(audio_renderer_);
+    audio_renderer_->StartPlaying();
+  }
+}
+
 void RendererImpl::SetPlaybackRate(double playback_rate) {
   DVLOG(1) << __FUNCTION__ << "(" << playback_rate << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ -502,6 +558,45 @@ void RendererImpl::OnBufferingStateChange(DemuxerStream::Type type,
 
   bool was_waiting_for_enough_data = WaitingForEnoughData();
 
+  // When restarting playback we want to ignore the BUFFERING_HAVE_NOTHING for
+  // the stream being restarted, to allow continuing uninterrupted playback on
+  // the other stream.
+  if (type == DemuxerStream::VIDEO && restarting_video_) {

[chcunningham, 2016/06/24 23:32:55]

For both audio and video, my previous comments still apply. You need to consider
the case where the stream never reaches HAVE_ENOUGH because of buffer underflow.
As a first idea, you could try something like the deferred underflow callback we
have for video underflow - basically, treat this as a potential underflow, but
cancel it if HAVE_ENOUGH comes in quickly thereafter. If the deferred callback
fails to become cancelled, it should re-enter this method, setting
restarting_{video/audio}_ to false, and ultimately pause playback via "//
Renderer underflowed" below. For video, it make sense to reuse the existing
deferred callback. For audio, a new one is needed. 

[servolk, 2016/06/25 00:36:33]

Well, I have looked into this some more after your comment, but then I've
noticed that we never actually report the HAVE_NOTHING to the RendererClient
(see the comment at lines 641-642 below). So even if we do use the deferred
underflow callback here and it gets triggered, then what happens next? We never
report that condition up the media stack. We will just sit in the paused state
until more video data becomes available, and at that point we'll resume
playback. How is this better than simply continuing to play audio until more
video becomes available? I think by not underflowing/pausing due to the resuming
stream we would get better use experience.

[chcunningham, 2016/06/27 20:31:52]

This is true, but its also a bug that I'm soon fixing. See
http://crbug.com/144683. The current situation is not to spec and is not
interoperable. I don't see why a track being disabled/enabled absolves it of
responsibility to correctly report its readyState. 
Its better because doing so matches the spec and helps interop. 

Furthermore, I can imagine scenarios where after enabling the video track where
not enough is buffered and I continue hearing audio play through some content
for which video is hugely important to the overall value. In such a case, it
would be better for playback to have paused while the video is buffered. 

[servolk, 2016/06/28 00:15:58]

Ok, sure, if you are fixing the delivery of HAVE_NOTHING notifications to
renderer clients, then yes, we can implement the deferral logic here.
PS In fact, after I've tried to do this, I've realized that the deferral logic
here (when we are restarting video playback) would be exactly the same as in the
regular continuing playback case, so we can just to allow fall through to the
code below, on lines 603-618, to allow the deferred cb to be created and posted.
The only change that we need is when we get HAVE_ENOUGH for the video stream
which is being restarted. Then we need to check if the deferred cb has been
triggered (in which case playback has been fully paused, i.e. both audio and
video were paused) or not (in which case we need to resume playback of only
video renderer).

+    if (new_buffering_state == BUFFERING_HAVE_NOTHING) {
+      DVLOG(1) << __FUNCTION__ << " Ignoring BUFFERING_HAVE_NOTHING for video "
+                                  "stream which is being restarted.";
+    } else if (new_buffering_state == BUFFERING_HAVE_ENOUGH) {
+      DVLOG(1) << __FUNCTION__ << " Got BUFFERING_HAVE_ENOUGH for video stream,"
+                                  " resuming playback.";
+      restarting_video_ = false;
+      if (playback_rate_ > 0)
+        video_renderer_->OnTimeStateChanged(true);

[chcunningham, 2016/06/24 23:32:55]

Re-posting discussion from earlier - ping xhwang or wolenetz. I think this may
have significant power implications. It wasn't really a big deal before this CL
(an EOS from video was almost always likely followed by a EOS from audio and
pipepline stop), but now that video could potentially be disabled for a long
time while audio runs, its important to stop the video sink's timers from
firing. 


me 2016/06/03 01:38:49
======================
Not really related to this line, but it triggered a thought...

I think we should make video renderer call OnTimeStateChanged(false) on itself
when it observes an EOS. Or we could do it in the renderer_impl when video
renderer signals its playback has ended. As it stands, I think the video sink's
timer will continue firing in spite of the track being disabled.


servolk 2016/06/03 02:53:15
===========================
Yes, it sounds reasonable to me. I'm not sure why that hasn't been done yet,
perhaps there are reasons why we need to keep ticking. I'll let Xiaohan or Matt
comment on this.

[servolk, 2016/06/25 00:36:33]

Acknowledged. Should we open a separate bug for this? I agree that this might
have implications beyond the scenario of enabled/disabled tracks. It think it
should be safe to stop ticking when eos is reached.

[chcunningham, 2016/06/27 20:31:52]

Since xhwang is AFK and wolenetz is just CC, I reached out to Dale to see if
this was deliberately not being done or if there were any expected side effects.
He says this additional call to OnTimeStateChanged(false) (or done internally in
response to rendering EOS) is probably fine for us to do. 
Sorry, this is the opposite of the meaning I intended. I think beyond
enabled/disabled tracks the implications are very narrow if any at all. I think
your introducing this feature *introduces* significant reasons to stop the sink
on EOS. Can you do it in this CL?

[servolk, 2016/06/28 00:15:58]

Done.

+    }
+    return;
+  }
+
+  if (type == DemuxerStream::AUDIO && restarting_audio_) {
+    if (new_buffering_state == BUFFERING_HAVE_NOTHING) {
+      DVLOG(1) << __FUNCTION__ << " Ignoring BUFFERING_HAVE_NOTHING for audio "
+                                  "stream which is being restarted.";
+    } else if (new_buffering_state == BUFFERING_HAVE_ENOUGH) {
+      DVLOG(1) << __FUNCTION__ << " Got BUFFERING_HAVE_ENOUGH for audio stream,"
+                                  " resuming playback.";
+
+      restarting_audio_ = false;
+
+      // Now that we have decoded enough audio, pause and unpause playback
+      // momentarily to ensure video renderer is synchronised with audio.
+      double curr_playback_rate = playback_rate_;
+      SetPlaybackRate(0);

[chcunningham, 2016/06/24 23:32:55]

My previous comment is not addressed. Reposting

Why does this help sync? I think you're already paused because you stopped
ticking prior to flush. I think you just need to call
time_source_->SetMediaTime() in OnAudioRendererFlushDone_ForRestart, and then
here you just need to StartTicking. 

[servolk, 2016/06/25 00:36:33]

Prior to this point only the audio renderer is paused. The video renderer is
still rendering and ticking! So SetPlaybackRate(0) pauses both renderers and
then SetPlaybackRate(curr_playback_rate) restarts both renderers at the same
time. I have noticed that without this (if I simply resumed ticking on the
time_source_) a/v sync was slightly off. To be honest I didn't get to the bottom
of why the sync was off, I just figured that if we did pause and resume here,
that would go through all the necessary steps that we usually take to ensure a/v
sync after restarting playback from a fully paused state (where both streams
were paused). I tried this and it worked :)

[chcunningham, 2016/06/27 20:31:52]

Can you instead try to audio_renderer_->SetMediaTime before
audio_renderer_->StartPlaying in RestartAudioRenderer above? Then here, I think
you'll be able to remove these SetPlaybackRate() calls and just call
time_source_->StartTicking().

I *think* whats happening here is the value of currentTime has grown a little
stale in the time it takes to seek and get packets out of the demuxers. This
means that we rendered some silence at time currentTime and by the time we
actually start rendering the audio intended for currentTime its really rendering
at currentTime + delta. 

I'm not certain why pausing the streams works, but my suggestion *should* also
work in a less magical fashion. LMK if you find its not working or has some side
effect.

[servolk, 2016/06/28 00:15:58]

We need to be very careful here. audio_renderer_->SetMediaTime recreates the
audio clock (see
https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.cc?r...).
I'm not sure if it's safe to do that while video is still rendering (and
potentially might try to access that clock any time).
I'll need to ponder this some more and play with different implementations to
see what can be done here.

[servolk, 2016/06/28 02:05:43]

Btw, just FYI: When I tried to set audio_renderer_->SetMediaTime in
RestartAudioRenderer, I got a dcheck when restarting an audio stream:

[1:3:0627/185900:FATAL:video_renderer_algorithm.cc(317)] Check failed:
frame_queue_[last_frame_index_].drop_count <=
frame_queue_[last_frame_index_].render_count (2 vs. 1)
#0 0x7fd0700a161e base::debug::StackTrace::StackTrace()
#1 0x7fd07010575f logging::LogMessage::~LogMessage()
#2 0x7fd066ad3b9b media::VideoRendererAlgorithm::OnLastFrameDropped()
#3 0x7fd066b38d84 media::VideoRendererImpl::OnFrameDropped()
#4 0x7fd05a89e648 media::VideoFrameCompositor::CallRender()
#5 0x7fd05a89e340 media::VideoFrameCompositor::UpdateCurrentFrame()
#6 0x7fd0676050f9 cc::VideoFrameProviderClientImpl::OnBeginFrame()
#7 0x7fd067813872 cc::LayerTreeHostImpl::WillBeginImplFrame()
#8 0x7fd0678a371b cc::ProxyImpl::WillBeginImplFrame()
#9 0x7fd06774dc94 cc::Scheduler::BeginImplFrame()
#10 0x7fd06774cd09 cc::Scheduler::BeginImplFrameWithDeadline()
#11 0x7fd06774c32c cc::Scheduler::OnBeginFrameDerivedImpl()
#12 0x7fd067737f3a cc::BeginFrameObserverBase::OnBeginFrame()
#13 0x7fd06ab4cd78 content::CompositorExternalBeginFrameSource::OnBeginFrame()

I also tried to change this to be more similar to how we handle the video stream
restart (i.e. allow the HAVE_NOTHING to fall through to the regular audio
underflow code path below), but that's also problematic, since regular underflow
calls time_source_->StopTicking() and that DCHECKs, since StopTicking expects
the audio renderer to be in the kPlaying state, and not in kFlushed state.
Getting this right is very tricky, and so far this has been the only way I could
make it work reliably. I'll try some more tomorrow, but I'm running out of
ideas. How else can we resume playback here without stopping/flushing the video
renderer?

[servolk, 2016/06/28 18:51:44]

I think we can make this better by allowing PausePlayback to work correctly even
if ticking of the time_source_ has been stopped already. Then we can just pause
the video playback here via PausePlayback and since we have just got HAVE_ENOUGH
for audio here, both audio and video playback will be resumed immediately below
on the line 648. PTAL at patchset 47 and see if you like this better.
Next I'll see if it would make sense to introduce a deferred audio overflow cb,
similar to what we do for video. The video underflow handling logic is already
pretty complicated (take a look at 603-608 below), adding another deferred
underflow for audio might make it even more complex, since the number of
possible states will increase.

[servolk, 2016/06/28 23:03:40]

Ok, and patchset #48 implements deferred reporting for audio underflows when
audio stream is being restarted. So I believe functionally this CL should be
complete. Please take another look at it. In the meantime I'll see into adding
some tests for this functionality.

+      SetPlaybackRate(curr_playback_rate);
+
+      time_ticking_ = true;
+      time_source_->StartTicking();
+    }
+    return;
+  }
+
   // 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.
@@ -612,7 +707,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));