Remove Pipeline::IsInitialized() and replace stop_pending_ with checks for stop_cb_.

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 65 matching lines @@
 //   [ Seeking (for each filter) ] <--------[ Flushing (for each filter) ]
 //         |                         if Seek              ^
 //         V                                              |
 //   [ Starting (for each filter) ]                       |
 //         |                                              |
 //         V      Seek()/Stop()                           |
 //   [ Started ] -------------------------> [ Pausing (for each filter) ]
 //                                                        ^  SetError()
 //                                                        |
 //                                         [ Any State Other Than InitXXX ]
-
 //
 // Initialization is a series of state transitions from "Created" through each
 // filter initialization state.  When all filter initialization states have
 // 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 base::RefCountedThreadSafe<Pipeline>,
       public DemuxerHost {
  public:
   // Constructs a media pipeline that will execute on |message_loop|.
   Pipeline(MessageLoop* message_loop, MediaLog* media_log);
 
   // Build a pipeline to using the given filter collection to construct a filter
-  // chain.
+  // chain, executing |start_cb| when initialization has completed.
   //
-  // 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).
+  // The following permanent callbacks will be executed as follows up until
+  // Stop() has completed:

[Ami GONE FROM CHROMIUM, 2012/08/09 20:46:23]

I thought error during Stop() doesn't fire error_cb.  IOW s/completed/has been
called/ above?

[scherkus (not reviewing), 2012/08/09 21:02:59]

You are correct but as discussed offline this is as accurate of a guarantee
we're willing to make (i.e., error/ended _could_ fire after Stop() is called
since Stop() has to queue a task on the pipeline msg loop)

   //   |ended_cb| will be executed whenever the media reaches the end.
-  //   |error_cb_| will be executed whenever an error occurs but hasn't
+  //   |error_cb| will be executed whenever an error occurs but hasn't
   //               been reported already through another callback.
   //
-  // These callbacks are only executed after Start() has been called and until
-  // 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,
              const PipelineStatusCB& start_cb);
 
   // Asynchronously stops the pipeline and resets it to an uninitialized state.
   //
   // If provided, |stop_cb| will be executed when the pipeline has been
   // completely torn down and reset to an uninitialized state.  It is acceptable
   // to call Start() again once the callback has finished executing.
@@ skipping 14 matching lines @@
   // 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 pipeline has been started and fully initialized to a
-  // 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;
 
   // 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.
@@ skipping 36 matching lines @@
   // be 0.
   void GetNaturalVideoSize(gfx::Size* out_size) 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;
 
+  // TODO(scherkus): Remove IsInitializedForTesting() after stop/error teardown
+  // paths are sane, see http://crbug.com/110228
+  bool IsInitializedForTesting();

[scherkus (not reviewing), 2012/08/09 19:27:10]

FYI this gets removed in follow-up CL http://codereview.chromium.org/10823261/

   void SetClockForTesting(Clock* clock);
   void SetErrorForTesting(PipelineStatus status);
 
  private:
   FRIEND_TEST_ALL_PREFIXES(PipelineTest, GetBufferedTimeRanges);
   FRIEND_TEST_ALL_PREFIXES(PipelineTest, DisableAudioRenderer);
   FRIEND_TEST_ALL_PREFIXES(PipelineTest, DisableAudioRendererDuringInit);
   FRIEND_TEST_ALL_PREFIXES(PipelineTest, EndedCallback);
   FRIEND_TEST_ALL_PREFIXES(PipelineTest, AudioStreamShorterThanVideo);
   friend class MediaLog;
@@ skipping 25 matching lines @@
 
   // 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
@@ skipping 163 matching lines @@
 
   // Lock used to serialize access for the following data members.
   mutable base::Lock lock_;
 
   // 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_;
 
   // Amount of available buffered data.  Set by filters.
@@ skipping 100 matching lines @@
   base::Time creation_time_;
 
   scoped_ptr<SerialRunner> pending_callbacks_;
 
   DISALLOW_COPY_AND_ASSIGN(Pipeline);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_PIPELINE_H_