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

media/base/pipeline.h

Index: media/base/pipeline.h
diff --git a/media/base/pipeline.h b/media/base/pipeline.h
index 9b0ebdf3f45da672935e6d18b5ee7a481bb59cda..d66ef4e98063ff996484a30ede0324e7cb8cf552 100644
--- a/media/base/pipeline.h
+++ b/media/base/pipeline.h
@@ -34,6 +34,18 @@ class TextRenderer;
 class TextTrackConfig;
 class VideoRenderer;
 
+// Metadata describing a pipeline once it has been initialized. Sent by the
+// buffering_state callback.

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

second sentence is incorrect -- heck I'd just remove it since it you cover the
timing aspect by saying "once it has been initialized"

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

Done.

+struct PipelineMetadata {
+  PipelineMetadata() : has_audio_(), has_video_(), natural_size_() {}

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

nit: instead of using the zero-initializer for POD types (e.g., bools) it's
better to be explicit and state false

for class types (e.g., gfx::Size) you can let the initializer list call the
default constructor

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

Done.

+
+  bool has_audio_;

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

structs don't use trailing _ for variables:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Variable_Names

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

Done.

+  bool has_video_;
+  gfx::Size natural_size_;
+};
+
+typedef base::Callback<void(PipelineMetadata)> HaveMetadataCB;

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

I'd model the naming after PipelineStatusCB:
  - call this PipelineMetadataCB
  - have the variable names metadata_cb_ (pithiness is nice where possible)

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

Done.

+
 // Pipeline runs the media pipeline.  Filters are created and called on the
 // task runner injected into this object. Pipeline works like a state
 // machine to perform asynchronous initialization, pausing, seeking and playing.
@@ -67,21 +79,6 @@ class VideoRenderer;
 // "Stopped" state.
 class MEDIA_EXPORT Pipeline : public DemuxerHost {
  public:
-  // Buffering states the pipeline transitions between during playback.
-  // kHaveMetadata:
-  //   Indicates that the following things are known:
-  //   content duration, container video size, start time, and whether the
-  //   content has audio and/or video in supported formats.
-  // kPrerollCompleted:
-  //   All renderers have buffered enough data to satisfy preroll and are ready
-  //   to start playback.
-  enum BufferingState {
-    kHaveMetadata,
-    kPrerollCompleted,
-  };
-
-  typedef base::Callback<void(BufferingState)> BufferingStateCB;
-
   // Constructs a media pipeline that will execute on |task_runner|.
   Pipeline(const scoped_refptr<base::SingleThreadTaskRunner>& task_runner,
            MediaLog* media_log);
@@ -97,18 +94,25 @@ class MEDIA_EXPORT Pipeline : public DemuxerHost {
   // The following permanent callbacks will be executed as follows up until
   // Stop() has completed:
   //   |ended_cb| will be executed whenever the media reaches the end.
-  //   |error_cb| will be executed whenever an error occurs but hasn't
-  //              been reported already through another callback.
-  //   |buffering_state_cb| Optional callback that will be executed whenever the
-  //                    pipeline's buffering state changes.
-  //   |duration_change_cb| Optional callback that will be executed whenever the
+  //   |error_cb| optional callback that will be executed whenever an error

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

careful tweaking comments: error_cb is definitely non-optional!

(it might look like it since it's guarded by an if-statement, however that's
because we call error_cb_.Reset() to ensure we only call either stop_cb_ or
error_cb_)

also see my comments below regarding optional params

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

Done.

+  //              occurs but hasn't been reported already through another
+  //              callback.
+  //   |have_metadata_cb| optional callback that will be executed when the

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

I usually advocate for as little optional-ness as possible and to follow what's
needed in production code. It makes APIs stricter and the impl code simpler.

In our case, our production code is WebMediaPlayerImpl where it passes in
callbacks for all of these.


(I know the existing |duration_change_cb| is stated as optional, but the only
caller that passes in an empty callback is pipeline_integration_test.cc. It'd be
nicer to have it pass in something. We should fix that in a separate CL.)

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

Done.

+  //                      content duration, container video size, start time,
+  //                      and whether the content has audio and/or video in
+  //                      supported formats are known.
+  //   |preroll_completed_cb| optional callback that will be executed when all
+  //                          renderers have buffered enough data to satisfy
+  //                          preroll and are ready to start playback.
+  //   |duration_change_cb| optional callback that will be executed whenever the
   //                        presentation duration changes.
   // It is an error to call this method after the pipeline has already started.
   void Start(scoped_ptr<FilterCollection> filter_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);
 
   // Asynchronously stops the pipeline, executing |stop_cb| when the pipeline
@@ -134,12 +138,6 @@ class MEDIA_EXPORT Pipeline : public DemuxerHost {
   // the pipeline.
   bool IsRunning() const;
 
-  // Returns true if the media has audio.
-  bool HasAudio() const;
-
-  // Returns true if the media has video.
-  bool HasVideo() const;
-
   // Gets the current playback rate of the pipeline.  When the pipeline is
   // started, the playback rate will be 0.0f.  A rate of 1.0f indicates
   // that the pipeline is rendering the media at the standard rate.  Valid
@@ -178,10 +176,6 @@ class MEDIA_EXPORT Pipeline : public DemuxerHost {
   // determined or can not be determined, this value is 0.
   int64 GetTotalBytes() const;
 
-  // Get the video's initial natural size as reported by the container. Note
-  // that the natural size can change during playback.
-  gfx::Size GetInitialNaturalSize() const;
-
   // Return true if loading progress has been made since the last time this
   // method was called.
   bool DidLoadingProgress() const;
@@ -262,12 +256,7 @@ class MEDIA_EXPORT Pipeline : public DemuxerHost {
   // The following "task" methods correspond to the public methods, but these
   // methods are run as the result of posting a task to the Pipeline's
   // task runner.
-  void 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 StartTask();
 
   // Stops and destroys all filters, placing the pipeline in the kStopped state.
   void StopTask(const base::Closure& stop_cb);
@@ -370,9 +359,6 @@ class MEDIA_EXPORT Pipeline : public DemuxerHost {
   // Total size of the media.  Set by filters.
   int64 total_bytes_;
 
-  // The initial natural size of the video as reported by the container.
-  gfx::Size initial_natural_size_;
-
   // Current volume level (from 0.0f to 1.0f).  This value is set immediately
   // via SetVolume() and a task is dispatched on the task runner to notify the
   // filters.
@@ -402,19 +388,15 @@ class MEDIA_EXPORT Pipeline : public DemuxerHost {
   // reset the pipeline state, and restore this to PIPELINE_OK.
   PipelineStatus status_;
 
-  // Whether the media contains rendered audio or video streams.
-  // TODO(fischman,scherkus): replace these with checks for
-  // {audio,video}_decoder_ once extraction of {Audio,Video}Decoder from the
-  // Filter heirarchy is done.
-  bool has_audio_;
-  bool has_video_;
-
   // The following data members are only accessed by tasks posted to
   // |task_runner_|.
 
   // Member that tracks the current state.
   State state_;
 
+  // The initial natural size of the video as reported by the container.
+  gfx::Size initial_natural_size_;
+
   // Whether we've received the audio/video/text ended events.
   bool audio_ended_;
   bool video_ended_;
@@ -432,7 +414,8 @@ class MEDIA_EXPORT Pipeline : public DemuxerHost {
   // Permanent callbacks passed in via Start().
   base::Closure ended_cb_;
   PipelineStatusCB error_cb_;
-  BufferingStateCB buffering_state_cb_;
+  HaveMetadataCB have_metadata_cb_;
+  base::Closure preroll_completed_cb_;
   base::Closure duration_change_cb_;
 
   // Contains the demuxer and renderers to use when initializing.