Rewrite media::Pipeline state transition machinery.

media/base/pipeline.h

Index: media/base/pipeline.h
diff --git a/media/base/pipeline.h b/media/base/pipeline.h
index ce028be1be6b3f1102d4e126f68eaefa8a559d85..53a6a45e29166c37b2dfa976ad65126d08fdee93 100644
--- a/media/base/pipeline.h
+++ b/media/base/pipeline.h
@@ -64,28 +64,19 @@ class MEDIA_EXPORT PipelineStatusNotification {
 // message loop 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 ]                                    [ Stopped ]
-//         | Start()                                      ^
-//         V                       SetError()             |
-//   [ InitXXX (for each filter) ] -------->[ Stopping (for each filter) ]
-//         |                                              ^
-//         V                                              | if Stop
-//   [ Seeking (for each filter) ] <--------[ Flushing (for each filter) ]
-//         |                         if Seek              ^
-//         V                                              |
-//   [ Starting (for each filter) ]                       |
-//         |                                              |
-//         V      Seek()/Stop()                           |
-//   [ Started ] -------------------------> [ Pausing (for each filter) ]
-//         |                                              ^
-//         |   OnRendererEnded()             Seek()/Stop()    |
-//         `-------------> [ Ended ] ---------------------'
-//                                                        ^  SetError()
-//                                                        |
-//                                         [ Any State Other Than InitXXX ]
-
+//  kCreated                                (from any state)
+//     | Start()                                   | Stop()/SetError()
+//     V                                           V
+//  kInitXXX (for each component)              kStopping
+//     |                                           |
+//     V                                           V
+//  kSeeking <-------------.                   kStopped
+//     |                   |
+//     V                   |
+//  kStarting           kFlushing
+//     |                   ^
+//     V       Seek()      |
+//  kStarted ---------> kPausing

[Ami GONE FROM CHROMIUM, 2012/07/30 04:06:21]

The fact that most states have a single ingress edge means the names are
interesting.  For example, kStarting implies "pre-rolled/seeked", kFlushing &
kSeeking imply "Paused", etc.

All this is to say I think s/kStarted/kPlaying/ is an improvement to
consistency.  (either verbify everything or adjective everything; don't mix)

If you agree w/ this, then maybe also s/kCreated/kInitPipeline/

 //
 // Initialization is a series of state transitions from "Created" through each
 // filter initialization state.  When all filter initialization states have
@@ -94,9 +85,8 @@ class MEDIA_EXPORT PipelineStatusNotification {
 // 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.
+// If Stop() is invoked or an error occurs during any point in time, the
+// pipeline will transition to a stopped state.

[Ami GONE FROM CHROMIUM, 2012/07/30 04:06:21]

s/stopped/(terminal) stopped/

 class MEDIA_EXPORT Pipeline
     : public base::RefCountedThreadSafe<Pipeline>,
       public DemuxerHost {
@@ -107,10 +97,6 @@ class MEDIA_EXPORT Pipeline
   // Build a pipeline to using the given filter collection to construct a filter
   // chain.
   //
-  // Pipeline initialization is an inherently asynchronous process.  Clients can
-  // either poll the IsInitialized() method (discouraged) or optionally pass in
-  // |start_cb|, which will be executed when initialization completes.
-  //
   // The following permanent callbacks will be executed as follows:
   //   |start_cb_| will be executed when Start is done (successfully or not).
   //   |ended_cb| will be executed whenever the media reaches the end.
@@ -121,8 +107,6 @@ class MEDIA_EXPORT Pipeline
   // Stop() has completed.
   //
   // It is an error to call this method after the pipeline has already started.
-  //
-  // TODO(scherkus): remove IsInitialized() and force clients to use callbacks.
   void Start(scoped_ptr<FilterCollection> filter_collection,
              const PipelineStatusCB& ended_cb,
              const PipelineStatusCB& error_cb,
@@ -157,11 +141,6 @@ class MEDIA_EXPORT Pipeline
   // the pipeline.
   bool IsRunning() const;
 
-  // Returns true if the pipeline has been started and fully initialized to a

[Ami GONE FROM CHROMIUM, 2012/07/30 04:06:21]

hawt!

-  // point where playback controls will be respected.  Note that it is possible
-  // for a pipeline to be started but not initialized (i.e., an error occurred).
-  bool IsInitialized() const;
-
   // Returns true if the media has audio.
   bool HasAudio() const;
 
@@ -219,6 +198,7 @@ class MEDIA_EXPORT Pipeline
   PipelineStatistics GetStatistics() const;
 
   void SetClockForTesting(Clock* clock);
+  void SetErrorForTesting(PipelineStatus status);
 
  private:
   FRIEND_TEST_ALL_PREFIXES(PipelineTest, GetBufferedTimeRanges);
@@ -241,50 +221,28 @@ class MEDIA_EXPORT Pipeline
     kInitVideoDecoder,
     kInitVideoRenderer,
     kPausing,
-    kSeeking,
     kFlushing,
+    kSeeking,
     kStarting,
     kStarted,
-    kEnded,

[scherkus (not reviewing), 2012/07/28 02:26:07]

kEnded wasn't actually used so it's been folded into kStarted

     kStopping,
     kStopped,
-    kError,

[scherkus (not reviewing), 2012/07/28 02:26:07]

kError has been replaced by kStopped && status_ != PIPELINE_OK

   };
 
-  // Reset the state of the pipeline object to the initial state.  This method
-  // is used by the constructor, and the Stop() method.
-  void ResetState();

[scherkus (not reviewing), 2012/07/28 02:26:07]

This was only called via Stop() path but not SetError() path -- madness!

We now set stuff in ctor and be done with it -- no more resetting of variables

-
   // Updates |state_|. All state transitions should use this call.
   void SetState(State next_state);
 
   // Simple method used to make sure the pipeline is running normally.
   bool IsPipelineOk();
 
-  // Helper method to tell whether we are stopped or in error.
-  bool IsPipelineStopped();
-
-  // Helper method to tell whether we are in transition to stop state.
-  bool IsPipelineTearingDown();
-
-  // We could also be delayed by a transition during seek is performed.
-  bool IsPipelineStopPending();
-
-  // Helper method to tell whether we are in transition to seek state.
-  bool IsPipelineSeeking();
-
-  // Helper method to execute callback from Start() and reset
-  // |filter_collection_|. Called when initialization completes
-  // normally or when pipeline is stopped or error occurs during
-  // initialization.
-  void FinishInitialization();
-
-  // Returns true if the given state is one that transitions to a new state
-  // after iterating through each filter.
-  static bool TransientState(State state);
+  // Returns true if an asynchronous operation is under way that will result in
+  // calling DoStateTransition().
+  bool IsTransitioning();
 
-  // Given the current state, returns the next state.
-  State FindNextState(State current);
+  // Completes the state transition for the current state, taking into account
+  // any errors that may have occured.
+  void OnStateTransition(PipelineStatus status);
+  void DoStateTransition(PipelineStatus status);

[Ami GONE FROM CHROMIUM, 2012/07/30 04:06:21]

These names are not sufficiently self-explanatory that they don't need
descriptions.

 
   // DataSourceHost (by way of DemuxerHost) implementation.
   virtual void SetTotalBytes(int64 total_bytes) OVERRIDE;
@@ -307,18 +265,6 @@ class MEDIA_EXPORT Pipeline
   // Callback executed when either of the renderers have ended.
   void OnRendererEnded();
 
-  // Callbacks executed by filters upon completing initialization.
-  void OnFilterInitialize(PipelineStatus status);
-
-  // Callback executed by filters upon completing Play(), Pause(), or Stop().
-  void OnFilterStateTransition();
-
-  // Callback executed by filters upon completing Seek().
-  void OnFilterStateTransitionWithStatus(PipelineStatus status);
-
-  // Callback executed by filters when completing teardown operations.
-  void OnTeardownStateTransition();
-
   // Callback executed by filters to update statistics.
   void OnUpdateStatistics(const PipelineStatistics& stats);
 
@@ -368,47 +314,13 @@ class MEDIA_EXPORT Pipeline
   // Carries out disabling the audio renderer.
   void AudioDisabledTask();
 
-  // Carries out advancing to the next filter during Play()/Pause()/Seek().
-  void FilterStateTransitionTask();
-
-  // Carries out advancing to the next teardown operation.
-  void TeardownStateTransitionTask();
-
-  // Carries out stopping filter threads, deleting filters, running
-  // appropriate callbacks, and setting the appropriate pipeline state
-  // depending on whether we performing Stop() or SetError().
-  // Called after all filters have been stopped.
-  void FinishDestroyingFiltersTask();
-
-  // Internal methods used in the implementation of the pipeline thread.  All
-  // of these methods are only called on the pipeline thread.
-
-  // The following initialize methods are used to select a specific type of
-  // object from FilterCollection and initialize it asynchronously.
-  void InitializeDemuxer();
-  void OnDemuxerInitialized(PipelineStatus status);
-
-  // Returns true if the asynchronous action of creating decoder has started.
-  // Returns false if this method did nothing because the corresponding
-  // audio/video stream does not exist.
-  bool InitializeAudioDecoder(const scoped_refptr<Demuxer>& demuxer);
-  bool InitializeVideoDecoder(const scoped_refptr<Demuxer>& demuxer);
-
-  // Initializes a renderer and connects it with decoder. Returns true if the
-  // asynchronous action of creating renderer has started. Returns
-  // false if this method did nothing because the corresponding audio/video
-  // stream does not exist.
-  bool InitializeAudioRenderer(const scoped_refptr<AudioDecoder>& decoder);
-  bool InitializeVideoRenderer(const scoped_refptr<VideoDecoder>& decoder);
-
-  // Kicks off destroying filters. Called by StopTask() and ErrorChangedTask().
-  // When we start to tear down the pipeline, we will consider two cases:
-  // 1. when pipeline has not been initialized, we will transit to stopping
-  // state first.
-  // 2. when pipeline has been initialized, we will first transit to pausing
-  // => flushing => stopping => stopped state.
-  // This will remove the race condition during stop between filters.
-  void TearDownPipeline();
+  // Initiates asynchronous initialization for each media object, executing
+  // |done_cb| when completed.
+  void DoInitDemuxer(const PipelineStatusCB& done_cb);
+  void DoInitAudioDecoder(const PipelineStatusCB& done_cb);
+  void DoInitAudioRenderer(const PipelineStatusCB& done_cb);
+  void DoInitVideoDecoder(const PipelineStatusCB& done_cb);
+  void DoInitVideoRenderer(const PipelineStatusCB& done_cb);
 
   // Compute the time corresponding to a byte offset.
   base::TimeDelta TimeForByteOffset_Locked(int64 byte_offset) const;
@@ -424,11 +336,12 @@ class MEDIA_EXPORT Pipeline
   // |done_cb| with the final status when completed. If |skip_demuxer_seek| is
   // true then only renderers will attempt to preroll.
   //
+  // |seek_timestamp_| must be set prior to calling DoSeek().
+  //
   // TODO(scherkus): Prerolling should be separate from seeking so we can report
   // finer grained ready states (HAVE_CURRENT_DATA vs. HAVE_FUTURE_DATA)
   // indepentent from seeking.
-  void DoSeek(base::TimeDelta seek_timestamp, bool skip_demuxer_seek,
-              const PipelineStatusCB& done_cb);
+  void DoSeek(bool skip_demuxer_seek, const PipelineStatusCB& done_cb);
 
   void OnAudioUnderflow();
 
@@ -449,21 +362,6 @@ class MEDIA_EXPORT Pipeline
   // Whether or not the pipeline is running.
   bool running_;
 
-  // Whether or not the pipeline is in transition for a seek operation.
-  bool seek_pending_;
-
-  // Whether or not the pipeline is pending a stop operation.
-  bool stop_pending_;
-
-  // Whether or not the pipeline is perform a stop operation.
-  bool tearing_down_;
-
-  // Whether or not an error triggered the teardown.
-  bool error_caused_teardown_;
-
-  // Whether or not a playback rate change should be done once seeking is done.
-  bool playback_rate_change_pending_;

[scherkus (not reviewing), 2012/07/28 02:26:07]

I replaced this + set volume pending-ness with a simpler policy: before we begin
playback always set the rate + volume to the current value

-
   // Amount of available buffered data.  Set by filters.
   Ranges<int64> buffered_byte_ranges_;
   Ranges<base::TimeDelta> buffered_time_ranges_;
@@ -488,9 +386,6 @@ class MEDIA_EXPORT Pipeline
   // the filters.
   float playback_rate_;
 
-  // Playback rate to set when the current seek has finished.
-  float pending_playback_rate_;
-
   // Reference clock.  Keeps track of current playback time.  Uses system
   // clock and linear interpolation, but can have its time manually set
   // by filters.
@@ -531,6 +426,7 @@ class MEDIA_EXPORT Pipeline
   scoped_ptr<FilterCollection> filter_collection_;
 
   // Callbacks for various pipeline operations.
+  PipelineStatusCB start_cb_;
   PipelineStatusCB seek_cb_;
   base::Closure stop_cb_;
   PipelineStatusCB ended_cb_;