Move Renderer permanent callbacks into RendererClient interface.

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"
 #include "base/callback_helpers.h"
 #include "base/command_line.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "media/base/audio_decoder_config.h"
 #include "media/base/audio_renderer.h"
 #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"
 #include "media/base/wall_clock_time_source.h"
 
 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,
     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)),
       time_source_(NULL),
       time_ticking_(false),
@@ skipping 32 matching lines @@
   video_renderer_.reset();
   audio_renderer_.reset();
 
   if (!init_cb_.is_null()) {
     FinishInitialization(PIPELINE_ERROR_ABORT);
   } else if (!flush_cb_.is_null()) {
     base::ResetAndReturn(&flush_cb_).Run();
   }
 }
 
-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;
     return;
   }
 
   state_ = STATE_INITIALIZING;
   InitializeAudioRenderer();
 }
 
@@ skipping 178 matching lines @@
 
   PipelineStatusCB done_cb =
       base::Bind(&RendererImpl::OnAudioRendererInitializeDone, weak_this_);
 
   if (!demuxer_stream_provider_->GetStream(DemuxerStream::AUDIO)) {
     audio_renderer_.reset();
     task_runner_->PostTask(FROM_HERE, base::Bind(done_cb, PIPELINE_OK));
     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) {
   DVLOG(1) << __FUNCTION__ << ": " << status;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // OnError() may be fired at any time by the renderers, even if they thought
   // they initialized successfully (due to delayed output device setup).
   if (state_ != STATE_INITIALIZING) {
     DCHECK(init_cb_.is_null());
@@ skipping 18 matching lines @@
 
   PipelineStatusCB done_cb =
       base::Bind(&RendererImpl::OnVideoRendererInitializeDone, weak_this_);
 
   if (!demuxer_stream_provider_->GetStream(DemuxerStream::VIDEO)) {
     video_renderer_.reset();
     task_runner_->PostTask(FROM_HERE, base::Bind(done_cb, PIPELINE_OK));
     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) {
   DVLOG(1) << __FUNCTION__ << ": " << status;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // OnError() may be fired at any time by the renderers, even if they thought
   // they initialized successfully (due to delayed output device setup).
   if (state_ != STATE_INITIALIZING) {
     DCHECK(init_cb_.is_null());
@@ skipping 85 matching lines @@
 
   DCHECK_EQ(state_, STATE_FLUSHING);
   DCHECK(!flush_cb_.is_null());
 
   DCHECK_EQ(video_buffering_state_, BUFFERING_HAVE_NOTHING);
   video_ended_ = false;
   state_ = STATE_PLAYING;
   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());
 
   bool was_waiting_for_enough_data = WaitingForEnoughData();
 
   // 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.
   if (audio_renderer_ && !is_audio && state_ == STATE_PLAYING) {
     if (video_buffering_state_ == BUFFERING_HAVE_ENOUGH &&
         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_);
       return;
     }
 
     deferred_underflow_cb_.Cancel();
   } else if (!deferred_underflow_cb_.IsCancelled() && is_audio &&
              new_buffering_state == BUFFERING_HAVE_NOTHING) {
     // If audio underflows while we have a deferred video underflow in progress
@@ skipping 16 matching lines @@
     PausePlayback();
 
     // TODO(scherkus): Fire BUFFERING_HAVE_NOTHING callback to alert clients of
     // underflow state http://crbug.com/144683
     return;
   }
 
   // Renderer prerolled.
   if (was_waiting_for_enough_data && !WaitingForEnoughData()) {
     StartPlayback();
-    buffering_state_cb_.Run(BUFFERING_HAVE_ENOUGH);
+    client_->OnBufferingStateChange(BUFFERING_HAVE_ENOUGH);
     return;
   }
 }
 
 bool RendererImpl::WaitingForEnoughData() const {
   DCHECK(task_runner_->BelongsToCurrentThread());
   if (state_ != STATE_PLAYING)
     return false;
   if (audio_renderer_ && audio_buffering_state_ != BUFFERING_HAVE_ENOUGH)
     return true;
@@ skipping 39 matching lines @@
   DCHECK_EQ(state_, STATE_PLAYING);
   DCHECK(!time_ticking_);
   DCHECK(!WaitingForEnoughData());
 
   time_ticking_ = true;
   time_source_->StartTicking();
   if (playback_rate_ > 0 && video_renderer_)
     video_renderer_->OnTimeStateChanged(true);
 }
 
-void RendererImpl::OnAudioRendererEnded() {
+void RendererImpl::OnRendererEnded(AudioVideoRendererClient* client) {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (state_ != STATE_PLAYING)
     return;
 
-  DCHECK(!audio_ended_);
-  audio_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();
 }
-
-void RendererImpl::OnVideoRendererEnded() {
-  DVLOG(1) << __FUNCTION__;
-  DCHECK(task_runner_->BelongsToCurrentThread());
-
-  if (state_ != STATE_PLAYING)
-    return;
-
-  DCHECK(!video_ended_);
-  video_ended_ = true;
-
-  RunEndedCallbackIfNeeded();
-}
 
 bool RendererImpl::PlaybackHasEnded() const {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (audio_renderer_ && !audio_ended_)
     return false;
 
   if (video_renderer_ && !video_ended_)
     return false;
 
   return true;
 }
 
 void RendererImpl::RunEndedCallbackIfNeeded() {
   DVLOG(1) << __FUNCTION__;
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   if (!PlaybackHasEnded())
     return;
 
   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!";
 
   // An error has already been delivered.
   if (state_ == STATE_ERROR)
     return;
 
   const State old_state = state_;
   state_ = STATE_ERROR;
 
   if (!init_cb_.is_null()) {
     DCHECK(old_state == STATE_INITIALIZING ||
            old_state == STATE_INIT_PENDING_CDM);
     FinishInitialization(error);
     return;
   }
 
   // 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