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

media/base/pipeline.h

 // Copyright (c) 2012 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.
 
 #ifndef MEDIA_BASE_PIPELINE_H_
 #define MEDIA_BASE_PIPELINE_H_
 
 #include <string>
 
 #include "base/gtest_prod_util.h"
@@ skipping 16 matching lines @@
 
 namespace media {
 
 class Clock;
 class FilterCollection;
 class MediaLog;
 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.
 //
 // Here's a state diagram that describes the lifetime of this object.
 //
 //   [ *Created ]                       [ Any State ]
 //         | Start()                         | Stop() / SetError()
 //         V                                 V
 //   [ InitXXX (for each filter) ]      [ Stopping ]
@@ skipping 13 matching lines @@
 // completed, we are implicitly in a "Paused" state.  At that point we simulate
 // a Seek() to the beginning of the media to give filters a chance to preroll.
 // From then on the normal Seek() transitions are carried out and we start
 // playing the media.
 //
 // If any error ever happens, this object will transition to the "Error" state
 // from any state. If Stop() is ever called, this object will transition to
 // "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);
   virtual ~Pipeline();
 
   // Build a pipeline to using the given filter collection to construct a filter
   // chain, executing |seek_cb| when the initial seek/preroll has completed.
   //
   // |filter_collection| must be a complete collection containing a demuxer,
   // audio/video decoders, and audio/video renderers. Failing to do so will
   // result in a crash.
   //
   // 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
   // teardown has completed.
   //
   // Stop() must complete before destroying the pipeline. It it permissible to
   // call Stop() at any point during the lifetime of the pipeline.
   //
   // It is safe to delete the pipeline during the execution of |stop_cb|.
   void Stop(const base::Closure& stop_cb);
 
   // Attempt to seek to the position specified by time.  |seek_cb| will be
   // executed when the all filters in the pipeline have processed the seek.
   //
   // Clients are expected to call GetMediaTime() to check whether the seek
   // succeeded.
   //
   // It is an error to call this method if the pipeline has not started.
   void Seek(base::TimeDelta time, const PipelineStatusCB& seek_cb);
 
   // Returns true if the pipeline has been started via Start().  If IsRunning()
   // returns true, it is expected that Stop() will be called before destroying
   // 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
   // values for playback rate are >= 0.0f.
   float GetPlaybackRate() const;
 
   // Attempt to adjust the playback rate. Setting a playback rate of 0.0f pauses
   // all rendering of the media.  A rate of 1.0f indicates a normal playback
   // rate.  Values for the playback rate must be greater than or equal to 0.0f.
   //
@@ skipping 18 matching lines @@
   Ranges<base::TimeDelta> GetBufferedTimeRanges();
 
   // Get the duration of the media in microseconds.  If the duration has not
   // been determined yet, then returns 0.
   base::TimeDelta GetMediaDuration() const;
 
   // Get the total size of the media file.  If the size has not yet been
   // 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;
 
   // Gets the current pipeline statistics.
   PipelineStatistics GetStatistics() const;
 
   void SetClockForTesting(Clock* clock);
   void SetErrorForTesting(PipelineStatus status);
 
@@ skipping 60 matching lines @@
 
   // Callback executed by audio renderer to update clock time.
   void OnAudioTimeUpdate(base::TimeDelta time, base::TimeDelta max_time);
 
   // Callback executed by video renderer to update clock time.
   void OnVideoTimeUpdate(base::TimeDelta max_time);
 
   // 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);
 
   // Carries out stopping and destroying all filters, placing the pipeline in
   // the kStopped state.
   void ErrorChangedTask(PipelineStatus error);
 
   // Carries out notifying filters that the playback rate has changed.
   void PlaybackRateChangedTask(float playback_rate);
@@ skipping 82 matching lines @@
   Ranges<int64> buffered_byte_ranges_;
   Ranges<base::TimeDelta> buffered_time_ranges_;
 
   // True when AddBufferedByteRange() has been called more recently than
   // DidLoadingProgress().
   mutable bool did_loading_progress_;
 
   // 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.
   float volume_;
 
   // Current playback rate (>= 0.0f).  This value is set immediately via
   // SetPlaybackRate() and a task is dispatched on the task runner to notify
   // the filters.
   float playback_rate_;
 
   // base::TickClock used by |clock_|.
   base::DefaultTickClock default_tick_clock_;
 
   // Reference clock.  Keeps track of current playback time.  Uses system
   // clock and linear interpolation, but can have its time manually set
   // by filters.
   scoped_ptr<Clock> clock_;
 
   // If this value is set to true, then |clock_| is paused and we are waiting
   // for an update of the clock greater than or equal to the elapsed time to
   // start the clock.
   bool waiting_for_clock_update_;
 
   // Status of the pipeline.  Initialized to PIPELINE_OK which indicates that
   // the pipeline is operating correctly. Any other value indicates that the
   // pipeline is stopped or is stopping.  Clients can call the Stop() method to
   // 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_;
   bool text_ended_;
 
   // Set to true in DisableAudioRendererTask().
   bool audio_disabled_;
 
   // Temporary callback used for Start() and Seek().
   PipelineStatusCB seek_cb_;
 
   // Temporary callback used for Stop().
   base::Closure stop_cb_;
 
   // 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.
   scoped_ptr<FilterCollection> filter_collection_;
 
   // Holds the initialized demuxer. Used for seeking. Owned by client.
   Demuxer* demuxer_;
 
   // Holds the initialized renderers. Used for setting the volume,
   // playback rate, and determining when playback has finished.
@@ skipping 10 matching lines @@
   scoped_ptr<SerialRunner> pending_callbacks_;
 
   base::ThreadChecker thread_checker_;
 
   DISALLOW_COPY_AND_ASSIGN(Pipeline);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_PIPELINE_H_