Allow media track switching.

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 62 matching lines @@
 };
 
 RendererImpl::RendererImpl(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
     std::unique_ptr<AudioRenderer> audio_renderer,
     std::unique_ptr<VideoRenderer> video_renderer)
     : state_(STATE_UNINITIALIZED),
       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),
       audio_buffering_state_(BUFFERING_HAVE_NOTHING),
       video_buffering_state_(BUFFERING_HAVE_NOTHING),
       audio_ended_(false),
       video_ended_(false),
       cdm_context_(nullptr),
       underflow_disabled_for_testing_(false),
       clockless_video_playback_enabled_for_testing_(false),
@@ skipping 92 matching lines @@
   DCHECK(flush_cb_.is_null());
 
   if (state_ != STATE_PLAYING) {
     DCHECK_EQ(state_, STATE_ERROR);
     return;
   }
 
   flush_cb_ = flush_cb;
   state_ = STATE_FLUSHING;
 
+  // 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_)
+    return;
+
   if (time_ticking_)
     PausePlayback();
 
   FlushAudioRenderer();
 }
 
 void RendererImpl::StartPlayingFrom(base::TimeDelta time) {
   DVLOG(1) << __func__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (state_ != STATE_PLAYING) {
     DCHECK_EQ(state_, STATE_ERROR);
     return;
   }
 
   time_source_->SetMediaTime(time);
 
   if (audio_renderer_)
     audio_renderer_->StartPlaying();
   if (video_renderer_)
     video_renderer_->StartPlayingFrom(time);
 }
 
 void RendererImpl::OnStreamStatusChanged(DemuxerStream* stream,

[xhwang, 2017/03/31 23:38:11]

nit: method definition order should match declaration order. But please only do
this after you got all approvals.

[servolk, 2017/03/31 23:53:55]

Will do

[servolk, 2017/04/03 21:32:33]

I've rearranged the methods to match declaration order in the latest patchset,
since I have to restart CQ for an comment change anyway :)

                                          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_))

[DaleCurtis, 2017/03/31 21:22:08]

Does this prevent the stream status change from occurring while in the middle of
a Flush() if not, tests are needed.

[servolk, 2017/03/31 23:53:55]

Fixed in the latest patchset (by postponing flush handling via the same action
queue as postponed track status changes).

     return;

[xhwang, 2017/03/31 23:38:11]

nit: Add a comment why it's safe to drop this notification when we are in the
FLUSHING state.

Actually, thinking about this case:

1. We are in the middle of reinit, say, restarting_audio_ is true. And we have 3
pending stream status notifications.
2. Flush() is called, so we enter STATE_FLUSHING state, and return on l.204.
3. Then at OnStreamRestartCompleted(), since
pending_stream_status_notifications_ is not empty, we'll process the next
pending stream status notification. Then since the state_ isn't PLAYING, we'll
return here.

Is this possible? I think I am missing something but would like to double check
with you.

Overall, I feel RendererImpl is getting pretty complicated with the stream
switching logic. I wish we can separate that logic out into some helper class
but I don't have any concrete idea at this point :(

[servolk, 2017/03/31 23:53:55]

Actually was only safe until we allowed track switching. And now that we have
track switching we can no longer drop these notifications in the flushing state,
instead we'll need to postpone them below (changed in patchset 22).

   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));
+        base::Bind((stream == current_video_stream_)

[DaleCurtis, 2017/03/31 21:22:08]

Unnecessary parens.

[servolk, 2017/03/31 23:53:55]

Done.

+                       ? &RendererImpl::RestartVideoRenderer
+                       : &RendererImpl::ReinitializeVideoRenderer,
+                   weak_this_, stream, 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));
+        base::Bind((stream == current_audio_stream_)

[DaleCurtis, 2017/03/31 21:22:08]

Ditto.

[servolk, 2017/03/31 23:53:55]

Done.

+                       ? &RendererImpl::RestartAudioRenderer
+                       : &RendererImpl::ReinitializeAudioRenderer,
+                   weak_this_, stream, time));
   }
 }
 
-void RendererImpl::RestartVideoRenderer(base::TimeDelta time) {
-  DVLOG(3) << __func__;
+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::RestartVideoRenderer(DemuxerStream* stream,
+                                        base::TimeDelta time) {
+  DVLOG(2) << __func__ << " stream=" << stream << " time=" << time.InSecondsF();
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(video_renderer_);
-  DCHECK_EQ(state_, STATE_PLAYING);
+  DCHECK(state_ == STATE_PLAYING || state_ == STATE_FLUSHING);
+  DCHECK_EQ(stream, current_video_stream_);
+
   video_ended_ = false;
   video_renderer_->StartPlayingFrom(time);

[DaleCurtis, 2017/03/31 21:22:08]

This doesn't seem to toggle OnTimeProgressing?

[servolk, 2017/03/31 23:53:55]

Yes, this simply tells video renderer to start reading from the input stream and
decoding video frames. When enough video frames are decoded, we'll get
OnBufferingStateChange(HAVE_ENOUGH) for the video stream, which will go into
HandleRestartedStreamBufferingChanges and, if necessary, will toggle
OnTimeProgressing at
https://cs.chromium.org/chromium/src/media/renderers/renderer_impl.cc?rcl=6f6....

 }
 
-void RendererImpl::RestartAudioRenderer(base::TimeDelta time) {
-  DVLOG(3) << __func__;
+void RendererImpl::ReinitializeAudioRenderer(DemuxerStream* stream,
+                                             base::TimeDelta time) {
+  DVLOG(2) << __func__ << " stream=" << stream << " time=" << time.InSecondsF();
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK_EQ(state_, STATE_PLAYING);
+  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::RestartAudioRenderer(DemuxerStream* stream,
+                                        base::TimeDelta time) {
+  DVLOG(2) << __func__ << " stream=" << stream << " time=" << time.InSecondsF();
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(state_ == STATE_PLAYING || state_ == STATE_FLUSHING);
   DCHECK(time_source_);
   DCHECK(audio_renderer_);
+  DCHECK_EQ(stream, current_audio_stream_);
+
   audio_ended_ = false;
   audio_renderer_->StartPlaying();

[xhwang, 2017/03/31 23:38:11]

Not for this CL and I think we discussed this before. Can you remind me why we
want to start playing here if we have pending stream status change or if we know
we are pending flushing? Having to wait until HAVING_ENOUGH to process next
stream status change seems unnecessary.

[servolk, 2017/03/31 23:53:55]

Because a. We don't know if the pending stream status change is for the same
stream or for a different one and b. If a RendererImpl::Flush is pending, then
it'll try to flush AudioRenderer again. Currently AudioRenderer::Flush can be
called only from a kPlaying state due to DCHECK at
https://cs.chromium.org/chromium/src/media/renderers/audio_renderer_impl.cc?r....

 }
 
 void RendererImpl::SetPlaybackRate(double playback_rate) {
   DVLOG(1) << __func__ << "(" << playback_rate << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // Playback rate changes are only carried out while playing.
   if (state_ != STATE_PLAYING)
     return;
 
@@ skipping 103 matching lines @@
   // TODO(servolk): Implement proper support for multiple streams. But for now
   // pick the first enabled stream to preserve the existing behavior.
   DemuxerStream* audio_stream =
       media_resource_->GetFirstStream(DemuxerStream::AUDIO);
   if (!audio_stream) {
     audio_renderer_.reset();
     task_runner_->PostTask(FROM_HERE, base::Bind(done_cb, PIPELINE_OK));
     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
   // 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) {
   DVLOG(1) << __func__ << ": " << status;
@@ skipping 28 matching lines @@
   // TODO(servolk): Implement proper support for multiple streams. But for now
   // pick the first enabled stream to preserve the existing behavior.
   DemuxerStream* video_stream =
       media_resource_->GetFirstStream(DemuxerStream::VIDEO);
   if (!video_stream) {
     video_renderer_.reset();
     task_runner_->PostTask(FROM_HERE, base::Bind(done_cb, PIPELINE_OK));
     return;
   }
 
+  current_video_stream_ = 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) {
   DVLOG(1) << __func__ << ": " << status;
@@ skipping 166 matching lines @@
       PausePlayback();
       task_runner_->PostTask(
           FROM_HERE,
           base::Bind(&RendererImpl::OnStreamRestartCompleted, weak_this_));
     }
   }
   return false;
 }
 
 void RendererImpl::OnStreamRestartCompleted() {
+  DVLOG(3) << __func__ << " a=" << restarting_audio_
+           << " v=" << restarting_video_;

[xhwang, 2017/03/31 23:38:11]

nit: it's gonna be hard to understand what a=1 means. Just use the full name
like "restarting_audio"?

[servolk, 2017/03/31 23:53:55]

Done.

   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();

[xhwang, 2017/03/31 23:38:11]

nit: The Run() part calls OnStreamStatusChanged() which touches
pending_stream_status_notifications_. We are fine since here restarting_audio_
and restarting_video_ are both false. But it's a bit scary...

Can we top, pop_front and then Run(), just to be on the safer side?

[servolk, 2017/03/31 23:53:55]

Done.

+  } else if (flush_cb_) {
+    DCHECK_EQ(state_, STATE_FLUSHING);
+    DVLOG(3) << __func__ << " completing pending Flush";
+    // Resume pending Flush processing.
+    if (time_ticking_)
+      PausePlayback();
+
+    FlushAudioRenderer();
   }
 }
 
 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_
                                         : &video_buffering_state_;
 
@@ skipping 202 matching lines @@
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoNaturalSizeChange(size);
 }
 
 void RendererImpl::OnVideoOpacityChange(bool opaque) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   client_->OnVideoOpacityChange(opaque);
 }
 
 }  // namespace media