Modify MojoRendererService to host a RendererImpl.

media/mojo/services/mojo_renderer_service.cc

Index: media/mojo/services/mojo_renderer_service.cc
diff --git a/media/mojo/services/mojo_renderer_service.cc b/media/mojo/services/mojo_renderer_service.cc
index f1a1ac8791d8b9b99ebfb920d56424758b003164..45afd1079e5ac2b842437845d7d4429fcd87c6fb 100644
--- a/media/mojo/services/mojo_renderer_service.cc
+++ b/media/mojo/services/mojo_renderer_service.cc
@@ -15,9 +15,11 @@
 #include "media/base/audio_renderer_sink.h"
 #include "media/base/decryptor.h"
 #include "media/base/media_log.h"
+#include "media/base/video_renderer.h"
 #include "media/filters/audio_renderer_impl.h"
 #include "media/filters/ffmpeg_audio_decoder.h"
 #include "media/filters/opus_audio_decoder.h"
+#include "media/filters/renderer_impl.h"
 #include "media/mojo/services/mojo_demuxer_stream_adapter.h"
 #include "mojo/application/application_runner_chromium.h"
 #include "mojo/public/c/system/main.h"
@@ -37,9 +39,54 @@ static void LogMediaSourceError(const scoped_refptr<MediaLog>& media_log,
 }
 #endif
 
-static base::TimeDelta TimeUpdateInterval() {
-  return base::TimeDelta::FromMilliseconds(kTimeUpdateIntervalMs);
-}
+// Shim DemuxerStreamProvider wrapper for a single DemuxerStream.
+// TODO(dalecurtis): Once we support more than one DemuxerStream we'll need a
+// more complicated shim which can handle a mojo::Array<DemuxerStream>.
+class DemuxerStreamProviderShim : public DemuxerStreamProvider {
+ public:
+  DemuxerStreamProviderShim(scoped_ptr<MojoDemuxerStreamAdapter> stream)
+      : stream_(stream.Pass()) {}
+
+  ~DemuxerStreamProviderShim() override {}
+
+  DemuxerStream* GetStream(DemuxerStream::Type type) override {
+    DCHECK_EQ(type, stream_->type());
+    return stream_.get();
+  };
+
+  Liveness GetLiveness() const override {
+    return DemuxerStreamProvider::LIVENESS_UNKNOWN;
+  }
+
+ private:
+  scoped_ptr<MojoDemuxerStreamAdapter> stream_;
+
+  DISALLOW_COPY_AND_ASSIGN(DemuxerStreamProviderShim);
+};
+
+// Stub VideoRenderer, scoped_ptr<VideoRenderer> doesn't take a nullptr nicely.

[xhwang, 2014/10/28 17:13:33]

This is unfortunate...

Does this work for you?

scoped_ptr<VideoRenderer> video_renderer(nullptr);

new RendererImpl(task_runner, audio_renderer.Pass(), video_renderer.Pass());

It compiles for me at least.

[DaleCurtis, 2014/10/28 21:51:29]

\o/, yes :)

+class VideoRendererStub : public VideoRenderer {
+ public:
+  ~VideoRendererStub() override {}
+  void Initialize(DemuxerStream* stream,
+                  bool low_delay,
+                  const PipelineStatusCB& init_cb,
+                  const StatisticsCB& statistics_cb,
+                  const BufferingStateCB& buffering_state_cb,
+                  const base::Closure& ended_cb,
+                  const PipelineStatusCB& error_cb,
+                  const TimeDeltaCB& get_time_cb) override {
+    NOTREACHED();
+  }
+
+  void Flush(const base::Closure& callback) override {
+    NOTREACHED();
+  }
+
+  void StartPlayingFrom(base::TimeDelta timestamp) override {
+    NOTREACHED();
+  }
+};
 
 class MojoRendererApplication
     : public mojo::ApplicationDelegate,
@@ -59,32 +106,30 @@ class MojoRendererApplication
   }
 };
 
-// TODO(xhwang): This class looks insanely similar to RendererImpl. We should
-// really host a Renderer in this class instead of a AudioRenderer.
+static void MojoTrampoline(const mojo::Closure& closure) {

[xhwang, 2014/10/28 17:13:33]

I wonder whether we have some generic way to convert base::Callback to
mojo::Callback... I searched around and didn't find any though.

[DaleCurtis, 2014/10/28 21:51:29]

Yeah I think that'd be useful, but am too weak in variadic templates to craft
such a thing quickly myself. :|

[xhwang, 2014/10/28 22:43:28]

Agreed :)

+  closure.Run();
+}
 
 MojoRendererService::MojoRendererService(
     mojo::ApplicationConnection* connection)
     : state_(STATE_UNINITIALIZED),
-      time_source_(NULL),
-      time_ticking_(false),
-      ended_(false),
+      last_media_time_(0),
       // AudioManager() has been created by WebMediaPlayerFactory. This will
       // be problematic when MojoRendererService really runs in a separate
       // process.
       // TODO(xhwang/dalecurtis): Figure out what config we should use.
-      audio_manager_(
-          media::AudioManager::Get()
-              ? media::AudioManager::Get()
-              : media::AudioManager::Create(&fake_audio_log_factory_)),
+      audio_manager_(AudioManager::Get()
+                         ? AudioManager::Get()
+                         : AudioManager::Create(&fake_audio_log_factory_)),
       audio_hardware_config_(
           audio_manager_->GetInputStreamParameters(
-              media::AudioManagerBase::kDefaultDeviceId),
+              AudioManagerBase::kDefaultDeviceId),
           audio_manager_->GetDefaultOutputStreamParameters()),
       weak_factory_(this),
       weak_this_(weak_factory_.GetWeakPtr()) {
   DVLOG(1) << __FUNCTION__;
 
-  scoped_refptr<base::SingleThreadTaskRunner> runner(
+  scoped_refptr<base::SingleThreadTaskRunner> task_runner(
       base::MessageLoop::current()->task_runner());
   scoped_refptr<MediaLog> media_log(new MediaLog());
 
@@ -92,213 +137,141 @@ MojoRendererService::MojoRendererService(
   ScopedVector<AudioDecoder> audio_decoders;
 
 #if !defined(OS_ANDROID)
-  audio_decoders.push_back(new media::FFmpegAudioDecoder(
-      runner, base::Bind(&LogMediaSourceError, media_log)));
-  audio_decoders.push_back(new media::OpusAudioDecoder(runner));
+  audio_decoders.push_back(new FFmpegAudioDecoder(
+      task_runner, base::Bind(&LogMediaSourceError, media_log)));
+  audio_decoders.push_back(new OpusAudioDecoder(task_runner));
 #endif
 
-  audio_renderer_.reset(new AudioRendererImpl(
-      runner,
+  scoped_ptr<AudioRenderer> audio_renderer(new AudioRendererImpl(
+      task_runner,
       // TODO(tim): We should use |connection| passed to MojoRendererService
       // to connect to a MojoAudioRendererSink implementation that we would
       // wrap in an AudioRendererSink and pass in here.
-      new NullAudioSink(runner),
+      new NullAudioSink(task_runner),
       audio_decoders.Pass(),
       // TODO(tim): Not needed for now?
       SetDecryptorReadyCB(),
       audio_hardware_config_,
       media_log));
+
+  // Create renderer.
+  renderer_.reset(new RendererImpl(task_runner,
+                                   audio_renderer.Pass(),
+                                   make_scoped_ptr(new VideoRendererStub())));
 }
 
 MojoRendererService::~MojoRendererService() {
 }
 
 void MojoRendererService::Initialize(mojo::DemuxerStreamPtr stream,
-                                     const mojo::Callback<void()>& callback) {
+                                     const mojo::Closure& callback) {
   DVLOG(1) << __FUNCTION__;
-  DCHECK_EQ(state_, STATE_UNINITIALIZED) << state_;
+  DCHECK_EQ(state_, STATE_UNINITIALIZED);
   DCHECK(client());
 
-  init_cb_ = callback;
   state_ = STATE_INITIALIZING;
-  stream_.reset(new MojoDemuxerStreamAdapter(
-      stream.Pass(),
-      base::Bind(&MojoRendererService::OnStreamReady, weak_this_)));
+  stream_provider_.reset(new DemuxerStreamProviderShim(
+      make_scoped_ptr(new MojoDemuxerStreamAdapter(
+                          stream.Pass(),
+                          base::Bind(&MojoRendererService::OnStreamReady,
+                                     weak_this_,
+                                     callback))).Pass()));

[xhwang, 2014/10/28 17:13:33]

Imagine when we have two streams, how will this call look like in terms of
callback passing?

[DaleCurtis, 2014/10/28 21:51:29]

I was thinking the DemuxerStreamProviderShim would handle creation of the
MojoDemuxerStreamAdapter using a SerialRunner before finally calling a provided
OnStreamReadyCB.

Otherwise it would work like any other provider, it would just route calls based
on which DemuxerStream::Type is given.  I can do that in this CL if you prefer. 
I thought it might be nicer to do so once we add support for video streams;
though that ended up sooner than expected.

[xhwang, 2014/10/28 22:43:28]

I think we are dropping SerialRunner...

Route calls based on Type sgtm. Let's do it in the next CL.

 }
 
-void MojoRendererService::Flush(const mojo::Callback<void()>& callback) {
+void MojoRendererService::Flush(const mojo::Closure& callback) {
   DVLOG(2) << __FUNCTION__;
-  DCHECK_EQ(state_, STATE_PLAYING) << state_;
+  DCHECK_EQ(state_, STATE_PLAYING);
 
   state_ = STATE_FLUSHING;
-  if (time_ticking_)
-    PausePlayback();
-
-  // TODO(xhwang): This is not completed. Finish the flushing path.
-  NOTIMPLEMENTED();
+  renderer_->Flush(base::Bind(&MojoTrampoline, callback));
 }
 
 void MojoRendererService::StartPlayingFrom(int64_t time_delta_usec) {
   DVLOG(2) << __FUNCTION__ << ": " << time_delta_usec;
-  base::TimeDelta time = base::TimeDelta::FromMicroseconds(time_delta_usec);
-  time_source_->SetMediaTime(time);
-  audio_renderer_->StartPlaying();
+  renderer_->StartPlayingFrom(
+      base::TimeDelta::FromMicroseconds(time_delta_usec));
+
+  // TODO(dalecurtis): Time updates shouldn't always tick, but without adding
+  // a bunch of additional state to this class plus RendererImpl, how to tell?

[xhwang, 2014/10/28 17:13:33]

hmm, I thought after StartPlayingFrom() (i.e. as long as we are playing) the
time will advance. So what do you mean by "time updates shouldn't always tick"?
I think I am missing something...

[DaleCurtis, 2014/10/28 21:51:29]

Well this class can't determine if time is actually moving when nominally
"playing" since underflows, sink startup time and such aren't really moving the
clock forward.

There's also the matter of when time updates should stop. I.e. what about
playback_rate=0?  Do we want time to always tick until OnRendererEnded? That's
what I'm doing now... but I'm not convinced it's the right thing.  Ideally these
updates should only tick when time is actually moving.

We could also let clients register with the RendererImpl for MediaTime updates
or something similar using an observer pattern.

[xhwang, 2014/10/28 22:43:28]

I see. Underflow is indeed a legit case.

I was suggesting dropping |only_if_changed| and decide whether we should update
the media time solely based on the current media time and last_media_time_. That
requires us to update last_media_time_ during StartPlayingFrom() and
PausePlayback(). But I feel that's cleaner. WDYT?

[DaleCurtis, 2014/10/29 00:11:19]

There is no PausePlayback() method, do you mean when SetPlaybackRate() changes?

[DaleCurtis, 2014/10/29 00:13:03]

Also note, that if the first media time is 0 then we won't send an update if we
rely only on last_media_time != media_time (which is why I have the only if
changed flag)

+  SchedulePeriodicMediaTimeUpdates();
 }
 
 void MojoRendererService::SetPlaybackRate(float playback_rate) {
   DVLOG(2) << __FUNCTION__ << ": " << playback_rate;
-
-  // Playback rate changes are only carried out while playing.
-  if (state_ != STATE_PLAYING)
-    return;
-
-  time_source_->SetPlaybackRate(playback_rate);
+  DCHECK_EQ(state_, STATE_PLAYING);
+  renderer_->SetPlaybackRate(playback_rate);
 }
 
 void MojoRendererService::SetVolume(float volume) {
-  if (audio_renderer_)
-    audio_renderer_->SetVolume(volume);
+  renderer_->SetVolume(volume);
 }
 
-void MojoRendererService::OnStreamReady() {
-  DCHECK_EQ(state_, STATE_INITIALIZING) << state_;
-  audio_renderer_->Initialize(
-      stream_.get(),
-      base::Bind(&MojoRendererService::OnAudioRendererInitializeDone,
-                 weak_this_),
+void MojoRendererService::OnStreamReady(const mojo::Closure& callback) {
+  DCHECK_EQ(state_, STATE_INITIALIZING);
+
+  renderer_->Initialize(
+      stream_provider_.get(),
+      base::Bind(
+          &MojoRendererService::OnRendererInitializeDone, weak_this_, callback),
       base::Bind(&MojoRendererService::OnUpdateStatistics, weak_this_),
-      base::Bind(&MojoRendererService::OnBufferingStateChanged, weak_this_),
-      base::Bind(&MojoRendererService::OnAudioRendererEnded, weak_this_),
-      base::Bind(&MojoRendererService::OnError, weak_this_));
+      base::Bind(&MojoRendererService::OnRendererEnded, weak_this_),
+      base::Bind(&MojoRendererService::OnError, weak_this_),
+      base::Bind(&MojoRendererService::OnBufferingStateChanged, weak_this_));
 }
 
-void MojoRendererService::OnAudioRendererInitializeDone(PipelineStatus status) {
-  DVLOG(1) << __FUNCTION__ << ": " << status;
-  DCHECK_EQ(state_, STATE_INITIALIZING) << state_;
+void MojoRendererService::OnRendererInitializeDone(
+    const mojo::Closure& callback) {
+  DVLOG(1) << __FUNCTION__;
 
-  if (status != PIPELINE_OK) {
-    state_ = STATE_ERROR;
-    audio_renderer_.reset();
-    client()->OnError();
-    init_cb_.Run();
-    init_cb_.reset();
-    return;
+  if (state_ != STATE_INITIALIZING) {
+    DCHECK_EQ(state_, STATE_ERROR);
+    renderer_.reset();
+  } else {
+    DCHECK_EQ(state_, STATE_INITIALIZING);
+    state_ = STATE_PLAYING;
   }

[xhwang, 2014/10/28 17:13:33]

How about

if (state_ == STATE_ERROR) {
  renderer_.reset();  
} else {
  DCHECK_EQ(state_, STATE_INITIALIZING);
  state_ = STATE_PLAYING;
}

[DaleCurtis, 2014/10/28 21:51:29]

Done.

 
-  time_source_ = audio_renderer_->GetTimeSource();
-
-  state_ = STATE_PLAYING;
-  init_cb_.Run();
-  init_cb_.reset();
+  callback.Run();
 }
 
 void MojoRendererService::OnUpdateStatistics(const PipelineStatistics& stats) {
   NOTIMPLEMENTED();
 }
 
-void MojoRendererService::UpdateMediaTime() {
-  uint64_t media_time = time_source_->CurrentMediaTime().InMicroseconds();
+void MojoRendererService::UpdateMediaTime(bool only_if_changed) {
+  const uint64_t media_time = renderer_->GetMediaTime().InMicroseconds();
+  if (only_if_changed && media_time == last_media_time_)
+    return;

[xhwang, 2014/10/28 17:13:33]

See comments above... How often is media_time == last_media_time_ happening?

If it does happen, can we make |only_if_changed| an implementation detail of
UpdateMediaTime()? You can think it this way, only_if_changed is always true
implicitly. Then during StartPlayingFrom(time_delta_usec), we can either:
1, set last_media_time_ to time_delta_usec, or
2, set last_media_time_ to 0.

In (1), we won't update time if it is the same as time_delta_usec. This is okay
because MojoRendererImpl sets time_ = time in StartPlayingFrom() as well.

In (2), we always update time once after StartPlayingFrom().

I feel (2) is better. We probably shouldn't rely on how MojoRendererImpl is
implemented. On a second thought, we probably should clear last_media_time_ in
PausePlayback().

WDYT?

[DaleCurtis, 2014/10/28 21:51:29]

This is just to prevent unnecessary time update calls when the time hasn't
changed. I expect it will happen for ~hundred milliseconds or so around startup
and for an unknown amount of time if the renderer underflows.

2) is in fact what I'm doing right now unless I misunderstand you. Above,
SchedulePeriodicMediaTimeUpdates() is called by StartPlayingFrom() which forces
a media time update. 

[xhwang, 2014/10/28 22:43:28]

See my comment above about dropping |only_if_changed|.

+
   client()->OnTimeUpdate(media_time, media_time);
+  last_media_time_ = media_time;
 }
 
 void MojoRendererService::SchedulePeriodicMediaTimeUpdates() {
-  // Update media time immediately.
-  UpdateMediaTime();
-
-  // Then setup periodic time update.
+  UpdateMediaTime(false);
   time_update_timer_.Start(
       FROM_HERE,
-      TimeUpdateInterval(),
-      base::Bind(&MojoRendererService::UpdateMediaTime, weak_this_));
+      base::TimeDelta::FromMilliseconds(kTimeUpdateIntervalMs),
+      base::Bind(&MojoRendererService::UpdateMediaTime, weak_this_, true));
 }
 
 void MojoRendererService::OnBufferingStateChanged(
-    media::BufferingState new_buffering_state) {
-  DVLOG(2) << __FUNCTION__ << "(" << buffering_state_ << ", "
-           << new_buffering_state << ") ";
-  bool was_waiting_for_enough_data = WaitingForEnoughData();
-
-  buffering_state_ = new_buffering_state;
-
-  // Renderer underflowed.
-  if (!was_waiting_for_enough_data && WaitingForEnoughData()) {
-    PausePlayback();
-    // TODO(xhwang): Notify client of underflow condition.
-    return;
-  }
-
-  // Renderer prerolled.
-  if (was_waiting_for_enough_data && !WaitingForEnoughData()) {
-    StartPlayback();
-    client()->OnBufferingStateChange(
-        static_cast<mojo::BufferingState>(new_buffering_state));
-    return;
-  }
+    BufferingState new_buffering_state) {
+  DVLOG(2) << __FUNCTION__ << "(" << new_buffering_state << ") ";
+  client()->OnBufferingStateChange(
+      static_cast<mojo::BufferingState>(new_buffering_state));
 }
 
-void MojoRendererService::OnAudioRendererEnded() {
+void MojoRendererService::OnRendererEnded() {
   DVLOG(1) << __FUNCTION__;
-
-  if (state_ != STATE_PLAYING)
-    return;
-
-  DCHECK(!ended_);
-  ended_ = true;
-
-  if (time_ticking_)
-    PausePlayback();
-
   client()->OnEnded();
+  time_update_timer_.Reset();
 }
 
 void MojoRendererService::OnError(PipelineStatus error) {
-  client()->OnError();
-}
-
-bool MojoRendererService::WaitingForEnoughData() const {
-  DCHECK(audio_renderer_);
-
-  return state_ == STATE_PLAYING && buffering_state_ != BUFFERING_HAVE_ENOUGH;
-}
-
-void MojoRendererService::StartPlayback() {
-  DVLOG(1) << __FUNCTION__;
-  DCHECK_EQ(state_, STATE_PLAYING);
-  DCHECK(!time_ticking_);
-  DCHECK(!WaitingForEnoughData());
-
-  time_ticking_ = true;
-  time_source_->StartTicking();
-
-  SchedulePeriodicMediaTimeUpdates();
-}
-
-void MojoRendererService::PausePlayback() {
   DVLOG(1) << __FUNCTION__;
-  DCHECK(time_ticking_);
-  switch (state_) {
-    case STATE_PLAYING:
-      DCHECK(ended_ || WaitingForEnoughData())
-          << "Playback should only pause due to ending or underflowing";
-      break;
-
-    case STATE_FLUSHING:
-      // It's OK to pause playback when flushing.
-      break;
-
-    case STATE_UNINITIALIZED:
-    case STATE_INITIALIZING:
-    case STATE_ERROR:
-      NOTREACHED() << "Invalid state: " << state_;
-      break;
-  }
-
-  time_ticking_ = false;
-  time_source_->StopTicking();
-
-  // Cancel repeating time update timer and update the current media time.
-  time_update_timer_.Stop();
-  UpdateMediaTime();
+  state_ = STATE_ERROR;
+  client()->OnError();
 }
 
 }  // namespace media