Move Renderer permanent callbacks into RendererClient interface.

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 59a00bfaf97046b13eccd31f36153a84e53e4d85..0e066ba9710685b34f7d4c2bf1ec80a0f90960f1 100644
--- a/media/renderers/renderer_impl.cc
+++ b/media/renderers/renderer_impl.cc
@@ -19,6 +19,7 @@
 #include "media/base/bind_to_current_loop.h"
 #include "media/base/demuxer_stream_provider.h"
 #include "media/base/media_switches.h"
+#include "media/base/renderer_client.h"
 #include "media/base/time_source.h"
 #include "media/base/video_decoder_config.h"
 #include "media/base/video_renderer.h"
@@ -29,6 +30,28 @@ namespace media {
 // See |video_underflow_threshold_|.
 static const int kDefaultVideoUnderflowThresholdMs = 3000;
 
+class RendererImpl::AudioVideoRendererClient : public RendererClient {
+ public:
+  AudioVideoRendererClient(RendererImpl* renderer) : renderer_(renderer) {}
+
+  void OnError(PipelineStatus error) override {
+    renderer_->OnError(this, error);
+  }
+  void OnEnded() override { renderer_->OnRendererEnded(this); }
+  void OnStatisticsUpdate(const PipelineStatistics& stats) override {
+    renderer_->OnStatisticsUpdate(this, stats);
+  }
+  void OnBufferingStateChange(BufferingState state) override {
+    renderer_->OnBufferingStateChange(this, state);
+  }
+  void OnWaitingForDecryptionKey() override {
+    renderer_->OnWaitingForDecryptionKey(this);
+  }
+
+ private:
+  RendererImpl* renderer_;
+};
+
 RendererImpl::RendererImpl(
     const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
     std::unique_ptr<AudioRenderer> audio_renderer,
@@ -81,32 +104,20 @@ RendererImpl::~RendererImpl() {
   }
 }
 
-void RendererImpl::Initialize(
-    DemuxerStreamProvider* demuxer_stream_provider,
-    const PipelineStatusCB& init_cb,
-    const StatisticsCB& statistics_cb,
-    const BufferingStateCB& buffering_state_cb,
-    const base::Closure& ended_cb,
-    const PipelineStatusCB& error_cb,
-    const base::Closure& waiting_for_decryption_key_cb) {
+void RendererImpl::Initialize(RendererClient* client,
+                              DemuxerStreamProvider* demuxer_stream_provider,
+                              const PipelineStatusCB& init_cb) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_UNINITIALIZED);
   DCHECK(!init_cb.is_null());
-  DCHECK(!statistics_cb.is_null());
-  DCHECK(!buffering_state_cb.is_null());
-  DCHECK(!ended_cb.is_null());
-  DCHECK(!error_cb.is_null());
+  DCHECK(client);
   DCHECK(demuxer_stream_provider->GetStream(DemuxerStream::AUDIO) ||
          demuxer_stream_provider->GetStream(DemuxerStream::VIDEO));
 
+  client_ = client;
   demuxer_stream_provider_ = demuxer_stream_provider;
-  statistics_cb_ = statistics_cb;
-  buffering_state_cb_ = buffering_state_cb;
-  ended_cb_ = ended_cb;
-  error_cb_ = error_cb;
   init_cb_ = init_cb;
-  waiting_for_decryption_key_cb_ = waiting_for_decryption_key_cb;
 
   if (HasEncryptedStream() && !cdm_context_) {
     state_ = STATE_INIT_PENDING_CDM;
@@ -305,16 +316,13 @@ void RendererImpl::InitializeAudioRenderer() {
     return;
   }
 
+  audio_renderer_client_.reset(new AudioVideoRendererClient(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(
-      demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO), done_cb,
-      cdm_context_, base::Bind(&RendererImpl::OnUpdateStatistics, weak_this_),
-      base::Bind(&RendererImpl::OnBufferingStateChanged, weak_this_,
-                 &audio_buffering_state_),
-      base::Bind(&RendererImpl::OnAudioRendererEnded, weak_this_),
-      base::Bind(&RendererImpl::OnError, weak_this_),
-      waiting_for_decryption_key_cb_);
+      audio_renderer_client_.get(),
+      demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO), cdm_context_,
+      done_cb);
 }
 
 void RendererImpl::OnAudioRendererInitializeDone(PipelineStatus status) {
@@ -353,15 +361,12 @@ void RendererImpl::InitializeVideoRenderer() {
     return;
   }
 
+  video_renderer_client_.reset(new AudioVideoRendererClient(this));
   video_renderer_->Initialize(
-      demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO), done_cb,
-      cdm_context_, base::Bind(&RendererImpl::OnUpdateStatistics, weak_this_),
-      base::Bind(&RendererImpl::OnBufferingStateChanged, weak_this_,
-                 &video_buffering_state_),
-      base::Bind(&RendererImpl::OnVideoRendererEnded, weak_this_),
-      base::Bind(&RendererImpl::OnError, weak_this_),
+      video_renderer_client_.get(),
+      demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO), cdm_context_,
       base::Bind(&RendererImpl::GetWallClockTimes, base::Unretained(this)),
-      waiting_for_decryption_key_cb_);
+      done_cb);
 }
 
 void RendererImpl::OnVideoRendererInitializeDone(PipelineStatus status) {
@@ -467,14 +472,18 @@ void RendererImpl::OnVideoRendererFlushDone() {
   base::ResetAndReturn(&flush_cb_).Run();
 }
 
-void RendererImpl::OnUpdateStatistics(const PipelineStatistics& stats) {
+void RendererImpl::OnStatisticsUpdate(AudioVideoRendererClient* client,

[xhwang, 2016/05/09 18:13:23]

Why do you need |client| here?

+                                      const PipelineStatistics& stats) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  statistics_cb_.Run(stats);
+  client_->OnStatisticsUpdate(stats);
 }
 
-void RendererImpl::OnBufferingStateChanged(BufferingState* buffering_state,
-                                           BufferingState new_buffering_state) {
-  const bool is_audio = buffering_state == &audio_buffering_state_;
+void RendererImpl::OnBufferingStateChange(AudioVideoRendererClient* client,
+                                          BufferingState new_buffering_state) {
+  const bool is_audio = client == audio_renderer_client_.get();

[xhwang, 2016/05/09 18:13:23]

The old code here was a bit hacky (I blame legacy reasons and my laziness) and I
never liked the idea of comparing pointers to decide whether it's audio or
video.

Instead of changing to another way of comparing pointers. Can we take this
opportunity to fix it? 

I think we can at least add an extra parameter to
RendererImpl::OnBufferingStateChange to indicate whether it's audio or video,
and also pass in this parameter to AudioVideoRendererClient's constructor so
that it's super clear which client is for which.

[alokp, 2016/05/09 21:31:44]

SGTM. What do you think about using DemuxerStream::Type?

[alokp, 2016/05/11 18:21:50]

I have addressed all comments except this one. Xiaohan: Still waiting for you
thoughts about using DemuxerStream::Type instead of comparig pointers.

+  BufferingState* buffering_state =
+      is_audio ? &audio_buffering_state_ : &video_buffering_state_;
+
   DVLOG(1) << __FUNCTION__ << "(" << *buffering_state << ", "
            << new_buffering_state << ") " << (is_audio ? "audio" : "video");
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ -489,9 +498,9 @@ void RendererImpl::OnBufferingStateChanged(BufferingState* buffering_state,
         audio_buffering_state_ == BUFFERING_HAVE_ENOUGH &&
         new_buffering_state == BUFFERING_HAVE_NOTHING &&
         deferred_underflow_cb_.IsCancelled()) {
-      deferred_underflow_cb_.Reset(base::Bind(
-          &RendererImpl::OnBufferingStateChanged, weak_factory_.GetWeakPtr(),
-          buffering_state, new_buffering_state));
+      deferred_underflow_cb_.Reset(
+          base::Bind(&RendererImpl::OnBufferingStateChange,
+                     weak_factory_.GetWeakPtr(), client, new_buffering_state));
       task_runner_->PostDelayedTask(FROM_HERE,
                                     deferred_underflow_cb_.callback(),
                                     video_underflow_threshold_);
@@ -528,7 +537,7 @@ void RendererImpl::OnBufferingStateChanged(BufferingState* buffering_state,
   // Renderer prerolled.
   if (was_waiting_for_enough_data && !WaitingForEnoughData()) {
     StartPlayback();
-    buffering_state_cb_.Run(BUFFERING_HAVE_ENOUGH);
+    client_->OnBufferingStateChange(BUFFERING_HAVE_ENOUGH);
     return;
   }
 }
@@ -588,28 +597,21 @@ void RendererImpl::StartPlayback() {
     video_renderer_->OnTimeStateChanged(true);
 }
 
-void RendererImpl::OnAudioRendererEnded() {
-  DVLOG(1) << __FUNCTION__;
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  if (state_ != STATE_PLAYING)
-    return;
-
-  DCHECK(!audio_ended_);
-  audio_ended_ = true;
-
-  RunEndedCallbackIfNeeded();
-}
-
-void RendererImpl::OnVideoRendererEnded() {
+void RendererImpl::OnRendererEnded(AudioVideoRendererClient* client) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (state_ != STATE_PLAYING)
     return;
 
-  DCHECK(!video_ended_);
-  video_ended_ = true;
+  if (client == audio_renderer_client_.get()) {
+    DCHECK(!audio_ended_);
+    audio_ended_ = true;
+  } else {
+    DCHECK_EQ(client, video_renderer_client_.get());
+    DCHECK(!video_ended_);
+    video_ended_ = true;
+  }

[xhwang, 2016/05/09 18:13:23]

ditto about not relying on pointer comparison

 
   RunEndedCallbackIfNeeded();
 }
@@ -637,10 +639,11 @@ void RendererImpl::RunEndedCallbackIfNeeded() {
   if (time_ticking_)
     PausePlayback();
 
-  ended_cb_.Run();
+  client_->OnEnded();
 }
 
-void RendererImpl::OnError(PipelineStatus error) {
+void RendererImpl::OnError(AudioVideoRendererClient* client,

[xhwang, 2016/05/09 18:13:23]

|client| not needed?

+                           PipelineStatus error) {
   DVLOG(1) << __FUNCTION__ << "(" << error << ")";
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_NE(PIPELINE_OK, error) << "PIPELINE_OK isn't an error!";
@@ -660,10 +663,15 @@ void RendererImpl::OnError(PipelineStatus error) {
   }
 
   // After OnError() returns, the pipeline may destroy |this|.
-  base::ResetAndReturn(&error_cb_).Run(error);
+  client_->OnError(error);
 
   if (!flush_cb_.is_null())
     base::ResetAndReturn(&flush_cb_).Run();
 }
 
+void RendererImpl::OnWaitingForDecryptionKey(AudioVideoRendererClient* client) {

[xhwang, 2016/05/09 18:13:23]

|client| not needed?

+  DCHECK(task_runner_->BelongsToCurrentThread());
+  client_->OnWaitingForDecryptionKey();
+}
+
 }  // namespace media