Remove HasAudio(), HasVideo(), GetInitialNaturalSize() from media::Pipeline.

media/base/pipeline.cc

Index: media/base/pipeline.cc
diff --git a/media/base/pipeline.cc b/media/base/pipeline.cc
index 824585f5d31589206c67109cd5d48ad47553381a..1a5e309a835f521cc652d5efc18f350867e9d20b 100644
--- a/media/base/pipeline.cc
+++ b/media/base/pipeline.cc
@@ -45,9 +45,8 @@ Pipeline::Pipeline(
       clock_(new Clock(&default_tick_clock_)),
       waiting_for_clock_update_(false),
       status_(PIPELINE_OK),
-      has_audio_(false),
-      has_video_(false),
       state_(kCreated),
+      initial_natural_size_(),
       audio_ended_(false),
       video_ended_(false),
       text_ended_(false),
@@ -74,16 +73,23 @@ void Pipeline::Start(scoped_ptr<FilterCollection> collection,
                      const base::Closure& ended_cb,
                      const PipelineStatusCB& error_cb,
                      const PipelineStatusCB& seek_cb,
-                     const BufferingStateCB& buffering_state_cb,
+                     const HaveMetadataCB& have_metadata_cb,
+                     const base::Closure& preroll_completed_cb,
                      const base::Closure& duration_change_cb) {
   base::AutoLock auto_lock(lock_);
   CHECK(!running_) << "Media pipeline is already running";
-  DCHECK(!buffering_state_cb.is_null());
-
   running_ = true;
-  task_runner_->PostTask(FROM_HERE, base::Bind(
-      &Pipeline::StartTask, base::Unretained(this), base::Passed(&collection),
-      ended_cb, error_cb, seek_cb, buffering_state_cb, duration_change_cb));
+
+  filter_collection_ = collection.Pass();
+  ended_cb_ = ended_cb;
+  error_cb_ = error_cb;
+  seek_cb_ = seek_cb;
+  have_metadata_cb_ = have_metadata_cb;
+  preroll_completed_cb_ = preroll_completed_cb;
+  duration_change_cb_ = duration_change_cb;
+
+  task_runner_->PostTask(
+      FROM_HERE, base::Bind(&Pipeline::StartTask, base::Unretained(this)));
 }
 
 void Pipeline::Stop(const base::Closure& stop_cb) {
@@ -108,16 +114,6 @@ bool Pipeline::IsRunning() const {
   return running_;
 }
 
-bool Pipeline::HasAudio() const {
-  base::AutoLock auto_lock(lock_);
-  return has_audio_;
-}
-
-bool Pipeline::HasVideo() const {
-  base::AutoLock auto_lock(lock_);
-  return has_video_;
-}
-
 float Pipeline::GetPlaybackRate() const {
   base::AutoLock auto_lock(lock_);
   return playback_rate_;
@@ -188,11 +184,6 @@ int64 Pipeline::GetTotalBytes() const {
   return total_bytes_;
 }
 
-gfx::Size Pipeline::GetInitialNaturalSize() const {
-  base::AutoLock auto_lock(lock_);
-  return initial_natural_size_;
-}
-
 bool Pipeline::DidLoadingProgress() const {
   base::AutoLock auto_lock(lock_);
   bool ret = did_loading_progress_;
@@ -333,8 +324,9 @@ void Pipeline::OnAudioTimeUpdate(TimeDelta time, TimeDelta max_time) {
   DCHECK(IsRunning());
   base::AutoLock auto_lock(lock_);
 
-  if (!has_audio_)
+  if (audio_renderer_ == NULL || audio_disabled_)

[scherkus (not reviewing), 2014/03/21 22:26:10]

audio_renderer_ should never be NULL in this case as we only register this
callback if it exists in InitializeAudioRenderer()

remove from if statement -- you can DCHECK(audio_renderer_) if you want

[sandersd (OOO until July 31), 2014/03/21 23:30:35]

Done.

     return;
+
   if (waiting_for_clock_update_ && time < clock_->Elapsed())
     return;
 
@@ -351,7 +343,7 @@ void Pipeline::OnVideoTimeUpdate(TimeDelta max_time) {
   DCHECK(IsRunning());
   base::AutoLock auto_lock(lock_);
 
-  if (has_audio_)
+  if (audio_renderer_ != NULL && !audio_disabled_)

[scherkus (not reviewing), 2014/03/21 22:26:10]

nit: hrmm.. can we ditch the direct comparison to NULL here + in the rest of
your CL?  AFAIK scoped_ptr<T> has bool conversion

(pithiness: treating pointers as bool is less verbose than a comparison to NULL,
so we like to do it whenever possible)

[sandersd (OOO until July 31), 2014/03/21 23:30:35]

Done.

     return;
 
   // TODO(scherkus): |state_| should only be accessed on pipeline thread, see
@@ -462,21 +454,20 @@ void Pipeline::StateTransitionTask(PipelineStatus status) {
         // We do not want to start the clock running. We only want to set the
         // base media time so our timestamp calculations will be correct.
         clock_->SetTime(demuxer_->GetStartTime(), demuxer_->GetStartTime());
-
-        // TODO(scherkus): |has_audio_| should be true no matter what --
-        // otherwise people with muted/disabled sound cards will make our
-        // default controls look as if every video doesn't contain an audio
-        // track.
-        has_audio_ = audio_renderer_ != NULL && !audio_disabled_;
-        has_video_ = video_renderer_ != NULL;
       }
+
       if (!audio_renderer_ && !video_renderer_) {
         done_cb.Run(PIPELINE_ERROR_COULD_NOT_RENDER);
         return;
       }
 
-      buffering_state_cb_.Run(kHaveMetadata);
-
+      if (!have_metadata_cb_.is_null()) {
+        PipelineMetadata metadata;

[scherkus (not reviewing), 2014/03/21 22:26:10]

what about adding an extra ctor that lets you set all values at once?

[sandersd (OOO until July 31), 2014/03/21 23:30:35]

That is what I did initially, but I didn't like that there was no natural
ordering for has_audio and has_video.

[scherkus (not reviewing), 2014/03/21 23:36:36]

out of curiosity can you clarify what you meant by "natural ordering"?

[sandersd (OOO until July 31), 2014/03/22 00:40:59]

I mean that there is no explicit ordering relation between audio and video, so a
quick glance may not spot them being out of order.

+        metadata.has_audio_ = audio_renderer_ != NULL;
+        metadata.has_video_ = video_renderer_ != NULL;
+        metadata.natural_size_ = initial_natural_size_;
+        have_metadata_cb_.Run(metadata);
+      }
       return DoInitialPreroll(done_cb);
 
     case kStarting:
@@ -488,7 +479,7 @@ void Pipeline::StateTransitionTask(PipelineStatus status) {
         // We use audio stream to update the clock. So if there is such a
         // stream, we pause the clock until we receive a valid timestamp.
         waiting_for_clock_update_ = true;
-        if (!has_audio_) {
+        if (audio_renderer_ == NULL || audio_disabled_) {
           clock_->SetMaxTime(clock_->Duration());
           StartClockIfWaitingForTimeUpdate_Locked();
         }
@@ -500,7 +491,8 @@ void Pipeline::StateTransitionTask(PipelineStatus status) {
       // Fire canplaythrough immediately after playback begins because of
       // crbug.com/106480.
       // TODO(vrk): set ready state to HaveFutureData when bug above is fixed.
-      buffering_state_cb_.Run(kPrerollCompleted);
+      if (!preroll_completed_cb_.is_null())
+        preroll_completed_cb_.Run();
       return base::ResetAndReturn(&seek_cb_).Run(PIPELINE_OK);
 
     case kStopping:
@@ -731,23 +723,11 @@ void Pipeline::OnUpdateStatistics(const PipelineStatistics& stats) {
   statistics_.video_frames_dropped += stats.video_frames_dropped;
 }
 
-void Pipeline::StartTask(scoped_ptr<FilterCollection> filter_collection,
-                         const base::Closure& ended_cb,
-                         const PipelineStatusCB& error_cb,
-                         const PipelineStatusCB& seek_cb,
-                         const BufferingStateCB& buffering_state_cb,
-                         const base::Closure& duration_change_cb) {
+void Pipeline::StartTask() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   CHECK_EQ(kCreated, state_)
       << "Media pipeline cannot be started more than once";
 
-  filter_collection_ = filter_collection.Pass();
-  ended_cb_ = ended_cb;
-  error_cb_ = error_cb;
-  seek_cb_ = seek_cb;
-  buffering_state_cb_ = buffering_state_cb;
-  duration_change_cb_ = duration_change_cb;
-
   text_renderer_ = filter_collection_->GetTextRenderer();
 
   if (text_renderer_) {
@@ -925,7 +905,6 @@ void Pipeline::AudioDisabledTask() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   base::AutoLock auto_lock(lock_);
-  has_audio_ = false;
   audio_disabled_ = true;
 
   // Notify our demuxer that we're no longer rendering audio.
@@ -975,18 +954,13 @@ void Pipeline::InitializeVideoRenderer(const PipelineStatusCB& done_cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   // Get an initial natural size so we have something when we signal
-  // the kHaveMetadata buffering state.
+  // HaveMetadata.
   //
   // TODO(acolwell): We have to query demuxer outside of the lock to prevent a
   // deadlock between ChunkDemuxer and Pipeline. See http://crbug.com/334325 for
   // ideas on removing locking from ChunkDemuxer.
   DemuxerStream* stream = demuxer_->GetStream(DemuxerStream::VIDEO);
-  gfx::Size initial_natural_size =
-      stream->video_decoder_config().natural_size();
-  {
-    base::AutoLock l(lock_);
-    initial_natural_size_ = initial_natural_size;
-  }
+  initial_natural_size_ = stream->video_decoder_config().natural_size();

[scherkus (not reviewing), 2014/03/21 22:26:10]

you can move this up to where we call have_metadata_cb_.Run()

the funny part is that now that we don't need to hold the lock to set
|initial_natural_size| (because your CL makes us use message passing, which is
good!) this whole comment + TODO can get nuked

[sandersd (OOO until July 31), 2014/03/21 23:30:35]

Done.

 
   video_renderer_ = filter_collection_->GetVideoRenderer();
   video_renderer_->Initialize(